Add global discovery #470
Add global discovery #470
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #470 +/- ##
==========================================
- Coverage 81.24% 79.13% -2.11%
==========================================
Files 69 54 -15
Lines 7129 6155 -974
==========================================
- Hits 5792 4871 -921
+ Misses 1337 1284 -53 ☔ View full report in Codecov by Sentry. |
This attempts to address astropy#470 (review)
service. Also, improving discovery logging a bit Also, adding an admonition to not use utils.testing outside of pyVO. (which attemtps to address astropy#470 (review)) Also, removing stale test inputs for global discovery.
msdemlei
force-pushed
the
add-global-discovery
branch
from
4735346
to
159aa5c
Compare
msdemlei
changed the title
service. Also, improving discovery logging a bit Also, adding an admonition to not use utils.testing outside of pyVO. (which attemtps to address astropy#470 (review)) Also, removing stale test inputs for global discovery.
msdemlei
force-pushed
the
add-global-discovery
branch
2 times, most recently
from
2142430
to
e6ca7bc
Compare
|
So... I think this could now be worth a look. See also my blog post on this: https://blog.g-vo.org/global-dataset-discovery-in-pyvo.html. The CI failures look interesting – I half fear they are related to implementation details changing the request hashes that utils.testing generates. Perhaps we'll have to properly scaffold these things after all? Anyway, I'll look at this some other day; it shouldn't impact discovery as such. Meanwhile, on the usual codestyle failures I have to say I'm fairly unhappy that I'll now have to say foo / 2 rather than foo/2 – does that really help someone? And do people here in pyVO actually prefer the former? If we have to have these checks in place: Can someone recommend a formatter/pretty-printer that I can run halfway blindly on the sources? Frankly, I don't think I'd like to get used to typing foo / 2 or 1 + 2, so if this kind of thing can happen in a pre-commit hook, that'd make my life easier... |
msdemlei
force-pushed
the
add-global-discovery
branch
from
e6ca7bc
to
bba06d9
Compare
even old basic tools could fix this, autopep8, or I have flymake set up on my emacs to check on the codestyle (and it should pick up the setting from the repo) |
|
So... I think modulo review points and perhaps a few style matters, this could be merged, with a caveat that the API is probably not stable yet. It does something interesting, and although data providers still can do quite a bit to make this work better, it's a lot more likely they will do that if there is merged code in pyVO. From my perspective, the main problem is testing. Since there is so much network activity going on here, my plan has been to ship frozen network answers as part of the source (see utils.testing for the general scheme). It turns out that that is nowhere near as simple as I had hoped. The test failures with old versions of python are almost certainly due to some subtle difference in the serialisation of query parameters; they might be fixable by normalising these a bit more aggressively, but perhaps they are not; I'd have to pull up systems with these old versions to look. But: Is this the right way to go about this? Perhaps the instrumentation should sit in TAPService, where the parameters are much less dependent on versions than where requests.requests has written its payload? Or should we, especially after the xz disaster, try harder to avoid what looks like binary blobs in the VCS in the first place? But then how do we do the scaffolding for these tests without going insane? Thoughts and/or ideas? |
|
Hi Markus, |
|
On Tue, Apr 16, 2024 at 06:46:34AM -0700, Manon Marchand wrote:
With which python version do the tests pass without access to
remote data? Maybe there was a change in the way hashs are
calculated that we could identify in python changelogs?
I'm rather sure it's not related to hash computations as such; these
are fairly stable.
In the current checks, things already fail for python 3.9. But good
thing you ask, because it made me realise that for python 3.8, things
already fail because there is no functools.cache there. At least
that I can fix. Perhaps we can learn something from the emerging
pattern.
|
|
Ah well... failures up and down. I'll dig a bit deeper tomorrow, but this does look as if we're going to need a giant scaffold. Oh dang. |
|
Oh yikes. I've tried this on two different Debian relases (so, I'm not reproducing github's environment). The first instance for differing hashes is because astropy includes something like "Produced with astropy.io.votable version 5.2.1" in its VOTables (which get uploaded in the course of the tests). Updating the cached files depending on the underlying astropy release obviously is not an option. I think the testing plan with intercepting and caching https requests and responses and recovering the responses based on hashes of the requests is fundamentally broken for something as complex as the global discovery. I suppose we'll have to mock at a higher level. If someone has good ideas how to go about this: I'd be most grateful. |
I don't have any suggestions without diving into this properly, but will try to find time to do so next week. |
service. Also, improving discovery logging a bit Also, adding an admonition to not use utils.testing outside of pyVO. (which attemtps to address astropy#470 (review)) Also, removing stale test inputs for global discovery.
msdemlei
force-pushed
the
add-global-discovery
branch
2 times, most recently
from
5f89f41
to
d143384
Compare
service. Also, improving discovery logging a bit Also, adding an admonition to not use utils.testing outside of pyVO. (which attemtps to address astropy#470 (review)) Also, removing stale test inputs for global discovery.
msdemlei
force-pushed
the
add-global-discovery
branch
from
d143384
to
a2f7d8a
Compare
This will keep records that don't have STC coverage declared. We need this for global dataset discovery.
Also, new method set_services on _ImageDiscoverer to pass in a custom resource list (for now, for testability) Also, exposing RegistryResource and RegistryResults in the registry API; they're really central in using the stuff, and we'll need them in type annotations a lot.
service. Also, improving discovery logging a bit Also, adding an admonition to not use utils.testing outside of pyVO. (which attemtps to address astropy#470 (review)) Also, removing stale test inputs for global discovery.
Also, some changes to make registry requests more predictable (i.e., sorting the extra tables to have a deterministic order).
This is a temporary measure until we have moved the VO to a saner obscore registration (i.e., obscore_new). When the Registry shows everyone has moved, obscore_new should become obscore, and the current obscore probably just dropped.
In global image discovery, using this to elide TAP services. Updating test data.
Basically, you can now clear the queue of services to query. Once the currently running query finishes, that will also end query_services. I've thought about canceling the currently running query, too, but that seems to be hard.
Also, fixing a crash in relationship discovery when there are no records to discover relationships for.
This is relevant when, for one reason or other, multiple capabilities of a type are present in a registry record. For our purposes here (sia, ssap, tap...), it is highly unlikely that these different capabilities correspond to different data (that's as different from VizieR SCS, where different capabilities correspond to different tables; but that's a pain and we don't want anything like that here).
SIA1 has all the various bandpass definitions optional (whether we like it or not). This commit makes our result construction resilient against services that don't provide it.
Also, we're sneaking in a change in SIA1 bandpass behaviour: We now accept the standard strings for bandpass units and assume m if the bandpass declaration is missing (that's against the standard, but in line with acutal practice).
This is mainly about using 1=contains rather than distance in the obscore queries; we also had to update the RegTAP capability.xml in the registry test to bring it in line with what the discover tests do. A better isolation of these test suites would be desirable; but then we don't want forsake capabilities caching altogether either... Also, sneaking in a few formatting problems.
It turns out that hashing actual network requests is too fragile -- due to version strings in headers and payloads, it is probably impossible to pull that off across python versions. I'm also dropping utils.testing; I think it might be useful to retain this in pyvo's history -- perhaps it will be useful as a basis for something comparable. Meanwhile, I have made the discovery tests remote_data. More unit tests ought to mock on a higher level, presumably by monkeypatching registry.search and the dal classes involved (yikes!).
When resetting the services and hence emptying the queue, the iterator currently active would keep returning the queriables that were in there before. That's no longer the case now. Also, adding a few tests to improve coverage.
msdemlei
force-pushed
the
add-global-discovery
branch
from
1ced6d5
to
d2769e4
Compare
msdemlei
force-pushed
the
add-global-discovery
branch
from
d2769e4
to
5acf4b0
Compare
|
After some deliberation I have decided that doing local tests properly for many of the features is really hard (and would hide a lot of interesting bugs). Hence, I have, with a solid helping of regret, made the more interesting tests remote. They're still relatively quick. So... Volunteers for reviewing this? It'd be lovely to have it merged into main for improved visibility, even though there will be quite a bit of refactoring once we also do global spectrum discovery. The codecov analysis is way off (I suspect they don't do remote tests). In my local pytest --cov, I only have 14 uncovered lines, and most of these are odd-ish (well, the obscore vs. tap-obscore thing should eventually be covered, but that's not easy and I hope this code will be obsolete rather soon anyway). The doctest failures... I think that's a separate matter independent of this PR. We really should think about dropping a few of them or at least figure out why the doctests take so long in the first place. Documentation building has some fairly odd messages, some of which may be introduced with the PR. I'd appreciate if someone with more automodapi-fu than me could have a look at that and figure out how to silence/fix these diagnostics. |
I know that there are some disagreements about this approach, maybe we should clear that up first next week, then I can put this on my list to review.
I'll check this once we do the code review, but recently there were some codecov issues elsewhere, too, so it may very well be a red herring.
separate, I'm opening the fix for it.
I volunteer for fixing these, once we go ahead with the code review. |
This PR should eventually add a facility for global dataset discovery to pyVO. Specifically, people should be able to say "give me all images in the VO showing M42 around 42nm on MJD 54125."
Realistically, we ought to allow intervals on input and probably multiple positions (though we should refuse SIA1 for multiple objects).
At the moment, none of this will work; I'm missing two very basic building
blocks that I won't have time to locate before I'm going on holiday, both related to the way pyvo treats query results:
If there is no better solution for both, we probably need to turn things into astropy tables, manipulate them, and then turn them back to VOTables. It'd be great if there were a less convoluted way to do this. Is there?
Even if that's there, this won't be ready for prime time; we will at least need defined timeouts on all services involved, because there's always some services in the VO hanging clients for almost arbitrary times.
Once we have the image case, a similar modules for spectrum discovery (in SSA and Obscore for now) should be simple.
We probably should talk to the IVOA to make throwing out capabilities leading to the same data possible/more robust; I think we ought to suggest that SIAv2 services over data that's also there in Obscore services should have an IsDerivedFrom set to the respective TAP service (or Obscore resource).
So, there's a metric ton of TODOs – I'm grateful for commits fixing any of them.
Ah – and before you ask: Testing code like this that talks to services all over the internet is of course a particular challenge; I think I have a plan, so don't worry about that particular issue too much at this point.