Deprecating image and spectrum as registry servicetype-s #449
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 ☔ View full report in Codecov by Sentry. |
|
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.
|
msdemlei
force-pushed
the
deprecate-image-and-spectrum
branch
from
8c9c011
to
7cabd98
Compare
msdemlei
force-pushed
the
deprecate-image-and-spectrum
branch
2 times, most recently
from
e4e8c9f
to
723bd0a
Compare
|
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:
Then in the updated doc page, we suggest not yet removing mention of the "image" and "spectrum" options but adding something like:
This gets the warning out there, gives a pointer to the intrepid user who wants to dig in, and promises a better way. |
In an attempt to accomodate the concerns brought up in astropy#449 (comment), we also explain a bit why image and spectrum are being deprecated. we still raise deprecation warnings (rather than something else), as that is, I think, what we are doing (and should be doing): We are saying that we discourage people to use these two strings and reserve the right to drop them one day.
msdemlei
force-pushed
the
deprecate-image-and-spectrum
branch
from
723bd0a
to
17681b6
Compare
|
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, 17681b6.
Can you live with that?
|
In an attempt to accomodate the concerns brought up in astropy#449 (comment), we also explain a bit why image and spectrum are being deprecated. we still raise deprecation warnings (rather than something else), as that is, I think, what we are doing (and should be doing): We are saying that we discourage people to use these two strings and reserve the right to drop them one day.
msdemlei
force-pushed
the
deprecate-image-and-spectrum
branch
from
17681b6
to
afbce15
Compare
In an attempt to accomodate the concerns brought up in astropy#449 (comment), we also explain a bit why image and spectrum are being deprecated. we still raise deprecation warnings (rather than something else), as that is, I think, what we are doing (and should be doing): We are saying that we discourage people to use these two strings and reserve the right to drop them one day.
msdemlei
force-pushed
the
deprecate-image-and-spectrum
branch
from
afbce15
to
3bb54e5
Compare
|
Ummm... it's actually commit 3bb54e5. Ahhh... Can we have a plan to change the SIA2 tests so running the test suite locally before a commit becomes non-tedious again? |
|
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? |
|
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. |
|
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.
|
|
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 :-)
|
|
While I was looking into updating the navo material to switch out all the Do you think it fits into this PR, or should I rather open a follow-up to this one? |
|
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.
|
|
Looking at the SIA classes, we kept the without number ones as SIAv1, so keeping |
msdemlei
force-pushed
the
deprecate-image-and-spectrum
branch
from
5344176
to
9c50df4
Compare
|
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.
|
|
OK, I'm totally fine with leaving the classes, and only have this |
There was a problem hiding this comment.
I think this can go in now once I have @tomdonaldson or @andamian gives a thumbs up, too.
However, I would like to cut it up into two PRs, will cherry-pick you second commit into a separate PR to backport for a 1.5.3 release, and keep the deprecation for 1.6.
(I'm happy to do this all as well as fixing the changelog conflict github complains about)
|
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!
|
In an attempt to accomodate the concerns brought up in astropy#449 (comment), we also explain a bit why image and spectrum are being deprecated. we still raise deprecation warnings (rather than something else), as that is, I think, what we are doing (and should be doing): We are saying that we discourage people to use these two strings and reserve the right to drop them one day.
bsipocz
force-pushed
the
deprecate-image-and-spectrum
branch
from
9c50df4
to
2ddb7d2
Compare
|
Thank you! |
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.