Deprecating image and spectrum as registry servicetype-s

[msdemlei]

I'm also promising all-VO-searches in some other way in the docs (which perhaps I shouldn't).

This is supposed to address bug #429.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.10%. Comparing base (ded4360) to head (8c9c011). Report is 14 commits behind head on main.

:exclamation: Current head 8c9c011 differs from pull request most recent head e4e8c9f. Consider uploading reports for the commit e4e8c9f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #449      +/-   ##
==========================================
- Coverage   80.38%   80.10%   -0.28%     
==========================================
  Files          52       52              
  Lines        6189     6027     -162     
==========================================
- Hits         4975     4828     -147     
+ Misses       1214     1199      -15     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[msdemlei]

On Fri, Aug 04, 2023 at 11:37:56AM -0700, Brigitta Sipőcz wrote:

E.g. I strongly feel instead of this, one should be able to do one image search, that returns the ~90 services.

import pyvo
image_services_v1 = pyvo.regsearch(servicetype='sia')
image_services_v2 = pyvo.regsearch(servicetype='sia2')
v2_irsa = [s for s in image_services_v2 if 'irsa' in s.ivoid]
v1_irsa = [s for s in image_services_v1 if 'irsa' in s.ivoid]

[for the record: I'd much rather do the ivoid constraint on the server side; if what you're doing here is something people want to do, let's add a partial option to the registry.Ivoid constraint]

Well, the trouble with this is: What would you do with such a resource list? I think almost always people would do this kind of thing to query all these services. This then would look like this:

for rec in pyvo.regsearch(servicetype='image-meaning-any-sia'): svc = reg.get_service() # trouble if there's two sia caps: which # one do we return here? res = svc.query(????) # depending on what SIA version we have, # users have to pass different parameters for rec in res: ???? # depending on the SIA main version, you get different # sorts of records you need to treat in totally different # ways

Frankly, I simply don't see a use case where this image-meaning-any-sia constraint would yield behaviour useful to any user – unless we abstracted the various versions away with a pyVO-specific ImageService class.

And even with that, you still haven't started with obscore -- which can also contain images.

My conclusion is that way the VO has worked out, normal astronomers have an almost zero chance of successfully doing an "All-VO image search" just using the pyvo.registry and pyvo.dal modules themselves.

I'm now more convinced of that than ever, because I've just written a bit of code that I hope will evolve into an All-VO querier. See PR#470 to get an idea what's involved (and for a number of items where I'd be grateful for good ideas).

And so I'd still like to warn people already in the next release that neither 'image' nor 'spectrum' does not do what they could think it does and that it likely never will. And I'd appreciate contributions to the my global-discovery branch.

[trjaffe]

The NAVO Python working group has discussed this PR internally and come up with a review. (TJ is just speaking for the team.)

Basically, we agree that while we work on the alternative (we also have a review of #470), the user needs to be warned that they are not getting global discovery results. We just feel that a deprecation that involves removing mention of "image" and "spectrum" from the docs is not yet appropriate, so we would suggest two things. Firstly, can the short warning link to the docs? I.e., something like:

“Warning: services of type “image” are not complete. See https://pyvo.readthedocs.io/en/latest/api/pyvo.registry.Servicetype.html#pyvo.registry.Servicetype.”

Then in the updated doc page, we suggest not yet removing mention of the "image" and "spectrum" options but adding something like:

“Contrary to appearances, querying “image” or "spectrum" services will not get you all resources serving those data products through the VO. Ensuring you get a more-or-less complete list of what’s available would require you to become a bit more familiar with the complexities of the VO. You would, for instance, have to search for images served three different ways: SIA, SIAv2, and ObsTAP, each of which has a different interface. This “image” search currently only searches for the first of those types, and "spectrum" similarly omits ObsTAP. A global discovery module is under active development to provide this functionality. When it is ready, these two misleading options will be deprecated.”

This gets the warning out there, gives a pointer to the intrepid user who wants to dig in, and promises a better way.

[msdemlei]

Thanks for the review and the suggestions!

On Tue, Jul 16, 2024 at 06:44:59AM -0700, trjaffe wrote:

deprecation that involves removing mention of "image" and "spectrum" from the docs is not yet appropriate, so we would

I was not quite sure whether it is the deprecation as such that you do not like or just the lack of an explanation; since you say:

suggest two things. Firstly, can the short warning link to the docs? I.e., something like:

and hence would raise a warning anyway, it would seem you are ok with the DeprecationWarning, though; I also can't think of a more appropriate warning, because what we tell people effectively is: Don't use that, we don't like it, and we hope that one day we can drop it.

So... I've reworked the message and explain the rationale of the deprecation in a few more words, and I have folded things into one commit, 17681b608.

Can you live with that?

[msdemlei]

Ummm... it's actually commit 3bb54e57. Ahhh... Can we have a plan to change the SIA2 tests so running the test suite locally before a commit becomes non-tedious again?

[trjaffe]

This is fine with me. But it's @bsipocz who has the opinions on how to use astropy's deprecation warning. Brigitta, is there a different type of warning you prefer this uses?

[bsipocz]

I have issues with the alternative as it requires the users to know way more about SIA, SSA, etc.

So let me phrase it differently: are we ready to remove all usage of 'image' in the Navo material? If it's just a heads up warning (e.g. userwarning type) then it's fine not to and say something about it during the session, but we definitely should use something that raises a deprecation as that already marks the functionality for removal.

[trjaffe]

I have issues with the alternative as it requires the users to know way more about SIA, SSA, etc.

So let me phrase it differently: are we ready to remove all usage of 'image' in the Navo material? If it's just a heads up warning (e.g. userwarning type) then it's fine not to and say something about it during the session, but we definitely should use something that raises a deprecation as that already marks the functionality for removal.

We are not ready yet. So right now, it's a heads up. But we will be working on getting rid of our usage of it.

[msdemlei]

On Fri, Jul 19, 2024 at 09:26:14AM -0700, Brigitta Sipőcz wrote:

I have issues with the alternative as it requires the users to know way more about SIA, SSA, etc.

Let me say again that I don't think the servicetype constraint ought be be considered API that normal users would routinely use. It does make sense for certain maintenance jobs on the side of "VO engineers". That is leaked out to "normal" users was more of a kludge that somewhat worked in the early days of the VO. These days, "normal" people should do data discovery (i.e., without constraining the access mode) and then use the most capable interface they see in access_modes().

So let me phrase it differently: are we ready to remove all usage of 'image' in the Navo material? If it's just a heads up warning

You should be: As I said, there is no way I can see to make this work in a way that is not totally confusing.

If there is stuff you do with servicetype constraints that does not work without the servicetype constraint (and I suspect that is the case until we get productTypeServed with the next VODataService), I am all for honesty and sprinkling in sia or sia2 with an appropriate warning. That warning you need anyway when saying "image" right now, as it raises entirely unrealistic expectations.

that raises a deprecation as that already marks the functionality for removal.

I am convinced this is exactly what we should do: There is no non-confusing functionality we could give "image" or "spectrum" in servicetype. What these words promise is positively complex and beyond anything a normal user could write on top of registry alone; see add-global-discovery, building a special API for what these should do, for why I'm saying that.

[msdemlei]

On Fri, Jul 19, 2024 at 09:31:48AM -0700, trjaffe wrote:

So let me phrase it differently: are we ready to remove all usage of 'image' in the Navo material? If it's just a heads up warning

We are not ready yet. So right now, it's a heads up. But we will be working on getting rid of our usage of it.

Hm... is this a blocker for merging the Deprecation into 1.6?

If so, I'd plead again that blindly search-replacing "sia" where there is "image" now would actually be an improvement.

It even, I claim, requires fewer words of explanation: "'sia' is one of the image discovery protocols that the VO define, and here we assume we know which one to pick." vs. "Regrettably, writing 'image' here does not mean we will be finding all image services. For historical reasons, it actually means 'find all SIA 1 services', which..." I am happy I don't have to fill in the ellipis :-)

[bsipocz]

While I was looking into updating the navo material to switch out all the image and spectrum examples, I notice that we do have sia2 but don't have sia1 (only sia). I see that we cannot get rid of sia as is to mean sia1, but nevertheless I think it would be better to add the more explicit sia1 and use that when teaching.

Do you think it fits into this PR, or should I rather open a follow-up to this one?

[msdemlei]

On Tue, Jul 30, 2024 at 12:13:10PM -0700, Brigitta Sipőcz wrote:

Do you think it fits into this PR, or should I rather open a follow-up to this one?

I agree, and I think that's exactly on-topic for this PR. I'm on holiday until next Wednesday, though. If anyone else wants to push the relevant changes (I think there are not many, except that we should decide on what we intend to do with "sia" [my hunch: Keep it as a synonym for sia1; we will have similar cases in the future, when there are new major versions of standards]), that's fine, otherwise I'll propose some changes in a week.

[bsipocz]

Looking at the SIA classes, we kept the without number ones as SIAv1, so keeping sia is fine. In fact, we haven't introduced SIA1... classnames, but here I still think it would be useful for removing ambiguity.

[msdemlei]

On Thu, Aug 01, 2024 at 11:11:52AM -0700, Brigitta Sipőcz wrote:

Looking at the SIA classes, we kept the without number ones as SIAv1, so keeping sia is fine. In fact, we haven't introduced SIA1... classnames, but here I still think it would be useful for removing ambiguity.

My take: Let's not. I'm extremely skeptical about breaking changes for reasons of good taste (so let's not rename the classes), and having multiple names for the same thing requires a lot of explanation and is still confusing, so it's probably not a win from an asthetic point of view (so let's not add aliases).

Anyway, with the latest push you can say "sia1", and the failing tests are unrelated to this code. What's not overly pretty is that und access modes, the thing will still be called "sia" (because that's the IVOA short name). How bad do we think this is? Me, I'd rather strongly prefer to not add any cosmetic hacks in the describe method, and even less in access_modes and friends.

[bsipocz]

OK, I'm totally fine with leaving the classes, and only have this servicetype 'sia1' alias. We may still want to add more explicit docstrings, but that is way out of scope for this PR.

[msdemlei]

On Thu, Aug 08, 2024 at 09:48:40AM -0700, Brigitta Sipőcz wrote:

(I'm happy to do this all as well as fixing the changelog conflict github complains about)

Thanks!

[bsipocz]

Thank you!