Skip to content

Conversation

@astrofrog
Copy link
Member

@astrofrog astrofrog commented Jun 14, 2017

SSL doens't currently work reliably in astropy.vo.samp, and as @mbtaylor pointed out in #6187 (comment), 'there are serious problems with mixing SAMP and HTTPS'. Therefore, I suggest that we simply remove all HTTPS/SSL-related code (without deprecation, since it's currently broken) which simplifies the astropy.vo.samp code significantly.

@mbtaylor - do you agree with this?

Since astropy.vo.samp is now the only package in astropy.vo, and since many users might like to use SAMP without realizing it's a VO standard (but not a VO server/client!), I would suggest (in a separate PR) moving the package to either astropy.io.samp or simply astropy.samp. Any opinions on this?

Closes #2064
Closes #5460

@astrofrog astrofrog added the samp label Jun 14, 2017
@astrofrog astrofrog requested a review from pllim June 14, 2017 17:07
@astrofrog astrofrog added this to the v2.0.0 milestone Jun 14, 2017
@astrofrog astrofrog requested a review from mhvk June 14, 2017 17:13
@astrofrog
Copy link
Member Author

@mhvk @pllim - I also re-arranged the docs to give a better high-level overview at the start.

@astrofrog astrofrog force-pushed the remove-samp-https branch from fd6412f to 86f6c7f Compare June 14, 2017 17:21
@pllim
Copy link
Member

pllim commented Jun 14, 2017

I can't comment on the code itself but I don't have any objection on the goal it is trying to accomplish.

As for moving it, I originally had a thought similar to your astropy.io.samp suggestion, but I have a question: Must something in astropy.io be part of the unified I/O stuff? For example, io.ascii, io.fits, etc. io.samp obviously will not fit in that category... As for astropy.samp, that might work but does the clarity justify the trouble of moving it?

Last but not least, does this PR close or fix the following? If so, please add the "fix ..." or "close ..." magic incantations in your original post:
#5460
#2321
#2126
#2064

@astrofrog
Copy link
Member Author

Maybe we don't need to move it, though what we could do is now make SAMP a top-level section in the docs and skip the astropy.vo top-level docs altogether.

@mbtaylor
Copy link
Contributor

Removing the HTTPS-related code is fine by me. Regarding the namespace, I don't think it needs to be in the astropy.vo package, or otherwise advertised as VO-related functionality. Although the SAMP protocol has been developed in the VO context, IMO the connection is mainly historical - you don't need to be talking to remote services to use SAMP. I wonder if it might be confusing to change the namespace of an existing package, but that's definitely a call for astropy people.

I will mention parenthetically that we are experimenting with HTTPS in SAMP in relation to some other requirements (web samp from an HTTPS page), and it's possible that it might be desirable to look again at this code at some point in the future, though that's not definite. However, I still think it makes sense to remove this code now, since it's not currently part of the SAMP standard, and it's causing problems.

@astrofrog
Copy link
Member Author

@mbtaylor - thank you for the comments!

Regarding moving the package, it would actually be quite simple to move it and maintain backward compatibility, so I think it's something we should seriously consider. I agree with @mbtaylor that having it under the vo namespace is actually misleading since it can be used without any actual VO infrastructure. I quite like astropy.samp...

@mhvk
Copy link
Contributor

mhvk commented Jun 14, 2017

👍 to just having it be astropy.samp - If I recall correctly, the idea generally was having a fairly flat structure. In the process, maybe on the main documentation page change the i/o header to something like "File Input, Output, and Transmission" or "File I/O and Interfaces".

@astrofrog
Copy link
Member Author

@eteq @taldcroft @kelle - as coordinators, any objections to moving astropy.vo.samp to astropy.samp and deprecating astropy.vo altogether? (since there would be nothing left in it).

…dard), remove support for HTTPS/SSL from astropy.vo.samp, since this is not meant to be supported and HTTPS/SSL support in astropy.vo.samp is currently broken with recent versions of Python.
@astrofrog astrofrog force-pushed the remove-samp-https branch from 86f6c7f to 61f9576 Compare June 15, 2017 11:06
@astrofrog
Copy link
Member Author

I removed the doc changes here and included @mbtaylor's changes in #6209

@taldcroft
Copy link
Member

@astrofrog - I have no objections to your plan.

@astrofrog
Copy link
Member Author

I'm going to go ahead and merge this and close old SSL-related issues. I've opened a new issue #6210 to remind us in future to add back support if SAMP does go that way (@mbtaylor - feel free to comment in that issue if that happens at some point).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants