Repository navigation
Deprecate vo.samp and move it out to affiliated package #6187
Description
As suggested in #5460 (comment)
- v2.0 -- Deprecated
astropy.vo.samp. Move it to affiliated package. - v3.0 -- Remove
astropy.vofrom core.
c/c @astrofrog @mhvk @bsipocz
p.s. This was announced in astropy-dev mailing list on June 12, 2017.
p.p.s. Looks like Ginga uses this module, so will need to issue a PR to update it as appropriate if this goes forward.
Activity
mbtaylor commentedon Jun 12, 2017
I was not aware of this issue before now, and I haven't looked at the details of the failing test in question. But note that as usually configured, SAMP works over HTTP not HTTPS, and in fact there are serious problems with trying to mix SAMP and HTTPS. So just removing tests that attempt to run SAMP over HTTPS sounds like a reasonable thing to do, and in fact adding them in the first place may have been questionable. Would that be an acceptable alternative to the (somewhat drastic?) step of deprecating or removing SAMP code from astropy altogether?
bsipocz commentedon Jun 12, 2017
@mbtaylor - Fixing the test is definitely a welcomed addition, but I see much logic in the discussion that prompted this idea of deprecation from here, namely that this is probably too specialized to have in the core (#5460 (comment)).
astrofrog commentedon Jun 12, 2017
@mbtaylor - just to be clear, if we do this we'd just be moving it back to a standalone package, but it would still be maintained by the astropy community.
mbtaylor commentedon Jun 13, 2017
If the astropy community wants to split the SAMP library out to a separate package, it's not my place to try to prevent that happening. But I'd like to make sure the motivation for doing it is well understood. The comment of @mhvk referenced above by @bsipocz was "it seems running one's own VO server is a bit overly specialized for the core." It is true that under some (not all) circumstances, using SAMP from Python does involve running an HTTP server from within the python process. But it's misleading to call that "running one's own VO server"; this HTTP service is only used for inter-process communication on the localhost, it's not e.g. something that a data center would do to expose local data holdings to the internet at large.
astrofrog commentedon Jun 13, 2017
An alternative option would be that instead of splitting it out into a separate package, we simply remove all the HTTPS-related code. There's a lot of complication in astropy.vo.samp related to supporting HTTPS which @mbtaylor has said should not actually be supported. The non-HTTPS code works fine and is pretty stable.
I'm not sure yet what I actually think would be best, but just wanted to mention that option.
mhvk commentedon Jun 13, 2017
@mbtaylor - I really do not have a strong opinion, at some level my reasoning was simply that the module does not seem to be maintained very actively, which gave me the perhaps wrong impression that it was also not used very actively -- with this impression reinforced by documentation that, to me at least, doesn't immediately make clear what one would use the module for!
I guess the real question is whether this module is useful on its own, or whether anyone who wants to use it pretty much also has to use some particular VO affiliated package. If the latter, then it seems logical to just move it to that affiliated package.
As a particular example, after going through the documentation in a bit more detail, it does seem handy to be able to send images to
ds9, and I can see the rationale for having the basics of such communication be in astropy. (That said, looking at the current documentation, I see no reason to abandonpyds9...)Anyway, the fact alone that you think it is useful makes me less inclined to move the package. What would really help, though, is a better introduction in the documentation --- if you are up for that...
pllim commentedon Jun 14, 2017
I removed v2.0 milestone since this is looking like just a doc update.
mbtaylor commentedon Jun 15, 2017
I've had a go at rewording the module introduction at 6a47546 - if you think it looks more comprehensible you can use it, but if not that's fine. I'm not experienced with astropy procedures or documentation conventions, so if any aspect of this looks wrong then (a) sorry, and (b) feel free to adjust or reject it or tell me what I should have done differently.
Answering the question about whether it's useful on its own - this module is not linked to any other
python package, though you wouldn't use it strictly alone, you would normally expect to use it in conjunction with some external (quite likely non-python) SAMP-aware application.
mhvk commentedon Jun 15, 2017
That looks good, substantially clearer. One advantage of which is that it now also to me is quite obvious this is something that belongs in astropy!
astrofrog commentedon Jun 15, 2017
I've merged in @mbtaylor's changes in #6209, so closing this issue.
pllim commentedon Jun 15, 2017
Deprecator defeated... for now. 😝
bsipocz commentedon Jun 15, 2017
@pllim - You will have a great time in the next few months though when we get rid of py2 :)
pllim commentedon Jun 15, 2017
@bsipocz , I can't wait. 😂
bsipocz commentedon Jun 15, 2017
We should do it during the sprint, so more of us can be part of the fun :)