ENH: Make regtap service aware #386

msdemlei · 2022-11-09T13:15:12Z

This PR contains a set of changes addressing the fixable but not yet
fixed issues raised by Tom in
https://wiki.ivoa.net/internal/IVOA/InterOpOct2022Reg/PyVOandNAVORegTAP.pdf

PyVO now properly checks whether the server can do something not in
RegTAP 1.0 before trying it. This concerns the optimisation of the
Freetext constraint as well as the STC constraints.

To make this reasonably easy to write, this retrofits some methods to
non-registry code:

get_tap_cap to TAPService
get_adql to tr.TableAccesss
get_feature_list, get_feature, and get_udf to tr.Language

bsipocz · 2022-12-22T00:31:38Z

docs/registry/index.rst

+There are several RegTAP services in the VO.  PyVO by default uses the
+one at the TAP access URL http://reg.g-vo.org/tap.  You can use
+alternative ones, for instance, because they are nearer to you or
+because the default endpoint is down.
+
+You can pre-select the URL by setting the ``IVOA_REGISTRY`` environment
+variable to the TAP access URL of the service you would like to use.  In
+a bash-like shell, you would say::
+
+  export IVOA_REGISTRY="http://vao.stsci.edu/RegTAP/TapService.aspx"


I know that you don't yet want review, so I leave only one comment:

we should be able to do this with a more programatic way, from inside the python interpreter.
Whether to use the astropy config system, or also provide ways to override the defaults in a session, or be able to do both is a detail, but I very strongly think it should be doable without shell variables.

msdemlei · 2022-12-22T07:23:38Z

On Wed, Dec 21, 2022 at 04:31:50PM -0800, Brigitta Sipőcz wrote: we should be able to do this with a more programatic way, from inside the python interpreter.

You can (see registry.tests.test_regtap.test_endpoint_switching; it's regtap.choose_RegTAP_service). But the env var method is IMHO still important in the medium run -- I am hoping to set up RegTAP endpoints in less well-connected regions, and then people there can pre-configure their desired endpoints, which does make a noticable difference (I have first-hand experience on this from South America and China). By the way, I'm also trying to talk Mark Taylor into interpreting that environment variable.

bsipocz · 2023-03-01T18:49:44Z

I milestoned this for 1.5, with the mindset that this is rather an enhancement rather than a bugfix.

bsipocz · 2023-05-08T14:09:55Z

I think this is the only thing that holds up 1.5, if we want to maybe do a release soonish? I recall using this branch when testing the SIA2 registry stuff, so overall not sure how much more is missing for this to be ready to be merged. Maybe it's only a rebase from Markus and a review from Tom?

bsipocz · 2023-05-08T15:03:30Z

(I'm mostly pushing for the release as it would be nice to get that SIA2 registry search compatibility out of the door)

msdemlei · 2023-05-08T17:03:20Z

On Mon, May 08, 2023 at 07:10:06AM -0700, Brigitta Sipőcz wrote: I think this is the only thing that holds up 1.5, if we want to maybe do a release soonish? I recall using this branch when testing the SIA2 registry stuff, so overall not sure how much more is missing for this to be ready to be merged. Maybe it's only a rebase from Markus and a review from Tom?

I'd love to see this in RSN -- it's real progress towards fulfilling the promise of interoperating publishing registries. I've hence done the rebase (oh boy, never rebase over a whitespace-everywhere commit) and I hope I've not messed things up too much. Tom, a review would be most appreciated. While we're all here, I'd actually be happy to do that in presence. I expect the test suite will throw two errors: One is about a QTable (I'll investigate tomorrow -- alternatively, advice appreciated), the other is a failing query on the StScI service. I *think* that's just a matter of rolling out a fix on their side. Tom?

bsipocz · 2023-05-09T07:55:26Z

I've hence done
the rebase (oh boy, never rebase over a whitespace-everywhere commit)
and I hope I've not messed things up too much.

Could you push this back to the branch here?

bsipocz · 2023-05-10T07:44:01Z

I made some CI improvements, so will go ahead and rebase again and flip the switch to move this out from the draft PR status.

codecov · 2023-05-10T08:12:46Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (3c8c67f) 80.11% compared to head (1bbc88f) 80.27%.

Files	Patch %	Lines
pyvo/registry/regtap.py	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
+ Coverage   80.11%   80.27%   +0.16%     
==========================================
  Files          52       52              
  Lines        6085     6150      +65     
==========================================
+ Hits         4875     4937      +62     
- Misses       1210     1213       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bsipocz · 2023-05-10T08:13:13Z

@msdemlei - ~~I wonder why CI doesn't see those failures you mention before, are they covered by tests or it's about use cases you haven't covered in the test suite yet?~~ Sorry for the noise, apparently it's indeed failing. But the QTAble one went away, I suppose that's good news.

@tomdonaldson - this is all yours for review.

msdemlei · 2023-05-10T10:08:18Z

On Wed, May 10, 2023 at 01:13:24AM -0700, Brigitta Sipőcz wrote: @msdemlei - I wonder why CI doesn't see those failures you mention before, are they covered by tests or it's about use cases you haven't covered in the test suite yet?

So much the better if the CI doesn't see them. Note that I'm running bleeding-edge pyvo against astropy and all the pytest packages as in Debian stable, so it's totally possible we're just seeing legacy behaviour blowing up. [Side rant: Given today's standards of software engineering, I'm actually rather impressed I can develop pyvo in that configuration. You see, I can't for astropy, which needs a bleeding-edge venv (which I'm unhappy about given the perils of pulling executable code from only very lightly curated repos). Thanks to everyone for keeping pyvo Debian stable-friedly!]

bsipocz · 2023-05-10T10:15:07Z

bleeding-edge venv

I would certainly raise this issue upstream. IMO it should all work in a stable environment and when it doesn't it certainly should be looked at as a bug rather than a feature. Getting the latest and greatest cool tools shouldn't be a requirement to do dev work.

msdemlei · 2023-07-07T07:16:21Z

Um... I just noticed this still lingers in the doldrums where I'd say it's a big improvement over what we have in terms of letting people switch registries.

So... @tomdonaldson – could you spare an eye? Or perhaps @theresadower?

msdemlei · 2023-12-01T10:49:39Z

So, I've rebased and force-pushed here. Locally, I get three test failures at the moment:

test_auth.test_cookies_auth -- that's unrelated and probably an upstream failure
test_regtap.test_endpoint_switching -- that's a new bug and due to a parser bug at the MAST TAP server. I'd be willing to work around it (actually, I thought I already had), but I'd have to know what to do. It'd be lovely if this could be fixed before this branch gets stale again.
[doctest] index.rst: there's an UnknownElementWarning about nrows, probably in a tables element. Again unrelated to this, and it'd be lovely if someone could fix it (nrows was introduced with VODataService in Nov 2021, so I'm a bit surprised this only shows now and here; DaCHS has been handing out nrows forever, so this warning must be present in many other tests, too).

So... yeah, it would be nice to get the test_endpoint_switching test
to run before merging. MAST, any chance for that?

bsipocz · 2023-12-01T18:57:46Z

Again unrelated to this, and it'd be lovely if someone could fix it

I'm afraid this might have been rebased on an outdated branch. I started to look into a rebase on main, and some of the test failures are gone, but I see new ones. I expect some more cleaning would fix the unrelated ones of those. I'm happy to push back to this branch (but that won't be before ~middle/late next week)), or have you do another rebase yourself?

bsipocz · 2023-12-05T04:55:54Z

I went ahead and pushed the rebased version back to this branch as I managed to clear up the test failures that were due to the conflicts. I still see the endpoint switching issue @msdemlei mentioned above as well as an unrelated looking TestXSIType.test_bad_type (I'm about to open a separate cleanup PR, if I see this one in there, I'll try to fix it there).
And there is a doctest one that I'll comment/suggest in the review I'm about to submit.

So, bottom line, would be nice to fix that one related test and make this work and merge and then finally release v1.5.

bsipocz

A lot of nitpicky comments with suggestions that would either help with fixing the links in the docsbuild (they could be part of #416, but I feel it's better to get things fixed when they pop-up), or suggestions for the failing doctest.

As none of these are critical (except fixing the failing doctest), that take them or leave them, they should not block merging this PR.

Also, could you please add a changelog entry?

bsipocz · 2023-12-05T03:19:59Z

docs/registry/index.rst

+  >>> print("\n".join(sorted(r.get_interface("tap").access_url
+  ...   for r in res)))


nitpick: we can have longer lines

Suggested change

>>> print("\n".join(sorted(r.get_interface("tap").access_url

... for r in res)))

>>> print("\n".join(sorted(r.get_interface("tap").access_url for r in res)))

Mhhh... I actually prefer broken lines and the loop expression on an extra line...

bsipocz · 2023-12-05T03:24:46Z

pyvo/dal/tap.py

+
+        Returns
+        -------
+        A `pyvo.io.vosi.TableAccess` instance.


I'm afraid a lot of these will cause docs build failures once we turn on nitpicky-mode, e.g. the API is not documented at this namespace but one layer down at https://pyvo.readthedocs.io/en/latest/api/pyvo.io.vosi.tapregext.TableAccess.html

So, while we don't have this nitpicky mode enabled, this practice this means that there are no functional links to the API docs in this docstring: https://pyvo--386.org.readthedocs.build/en/386/api/pyvo.dal.TAPService.html#pyvo.dal.TAPService.get_tap_cap

This is not merge blocker, just a heads up that we need to use the API path to the actual namespace we put things in.

Some of these I commented for in this PR, but I very likely didn't catch all of them

Ah, can I be lazy and just ask: What's the right way that will be nitpicky-safe?

Wrapping up #416 and putting it into CI. I was doing a little bit of work on that PR, but the main issue is that we need to make some decisions about what's in the public API (and thus documented) and what's not. Currently I went ahead and ignored a couple of classes that we said is not for public consumption, yet we try to link to them as e.g. they are used as base classes.

(btw, wrapping up #416 is a bit of a hustle as there is an underlying sphinx-osx issue that in theory has a workaround but I never managed to make it work here, so I have to rely on CI for the iterations)

bsipocz · 2023-12-05T03:24:48Z

pyvo/dal/tap.py

@@ -128,6 +128,20 @@ def __init__(self, baseurl, session=None):
        if hasattr(self._session, 'update_from_capabilities'):
            self._session.update_from_capabilities(self.capabilities)

+    def get_tap_cap(self):


can we spell out cap here?

Absolutely, yes.

bsipocz · 2023-12-05T03:27:17Z

pyvo/io/vosi/tapregext.py

+
+    def get_feature(self, ivoid, form):
+        """
+        returns the `LanguageFeature` with ivoid and form if present.


Suggested change

returns the `LanguageFeature` with ivoid and form if present.

returns the `~pyvo.io.vosi.tapregext.LanguageFeature` with ivoid and form if present.

bsipocz · 2023-12-05T03:28:30Z

pyvo/dal/tap.py

+
+        Returns
+        -------
+        A `pyvo.io.vosi.TableAccess` instance.


Suggested change

A `pyvo.io.vosi.TableAccess` instance.

A `~pyvo.io.vosi.tapregext.TableAccess` instance.

bsipocz · 2023-12-05T03:32:00Z

pyvo/registry/rtcons.py

+
+        Parameters
+        ----------
+        service : `dal.tap.TAPService`


Suggested change

service : `dal.tap.TAPService`

service : `~pyvo.dal.tap.TAPService`

bsipocz · 2023-12-05T03:33:34Z

pyvo/registry/rtcons.py

+    to write that constraint, or because it is missing a table or column.
+
+    To recover, choose another RegTAP service. Search constraining
+    ``datamodel="regtap"``, and then use `pyvo.registry.switch_RegTAP_service`


switch_RegTAP_service doesn't seem to exist anywhere in the API. Is it meant to be choose_RegTAP_service here?

Yes... this is what happens when you change your naming preferences in between edits. Fixed.

bsipocz · 2023-12-05T03:33:51Z

pyvo/registry/regtap.py

+        Returns
+        -------
+
+        ~`pyvo.registry.regtap.Interface`


Suggested change

~`pyvo.registry.regtap.Interface`

`~pyvo.registry.regtap.Interface`

bsipocz · 2023-12-05T03:36:22Z

pyvo/registry/regtap.py

+    tables, etc.
+
+    To switch to a different RegTAP service, use
+    :py:func:`change_RegTAP_service`.


there doesn't seem to be a change_RegTAP_service. Is it meant to be choose_RegTAP_service here?

Please hand me a brown bag. Fixed in the next commit as well.

bsipocz · 2023-12-05T04:54:46Z

docs/registry/index.rst

+  ...   for r in res)))
+  http://dc.zah.uni-heidelberg.de/tap
+  http://gavo.aip.de/tap
+  http://voparis-rr.obspm.fr:80/tap


This causes one of the test failures

Suggested change

http://voparis-rr.obspm.fr:80/tap

http://voparis-rr.obspm.fr/tap

bsipocz · 2023-12-07T16:42:51Z

Perhaps we should have an integration test suite that contains the
more expensive of the tests requiring external services. This could
then be run on "major" occasions like, say, merge of PRs, without
making everyday test runs more tedious;

Good point, and exactly the reason we don't run astroquery remote tests on PRs, but on commits. Which means extra care has to be made prior to merging (in practice I run the given module locally), but ultimately we may figure out a way that does it more automaticly (e.g. only trigger remote tests of the given module, but even that would be too much for astroquery where we have diverse contributions).

Here, I'll try to remember to look into whether the PR status can be a trigger for github actions (e.g. we don't run the full test suite for draft PRs), or we may just want to use a label to trigger more resourceful remote tests?

TLDR, I'll need to think about how to make it more robust without adding a large burden for the remote services.

tomdonaldson · 2023-12-07T18:47:20Z

Curse optional features. Let's not do them any more unless there is absolutely no alternative. Supporting multiple code paths is a pain we shouldn't inflict on our clients.

Yes, I enthusiastically agree!

bsipocz · 2023-12-13T16:52:17Z

gentle ping that once this one gets wrapped up we can do the long-awaited 1.5 release 😄

msdemlei · 2023-12-14T09:58:20Z

On Wed, Dec 13, 2023 at 08:52:28AM -0800, Brigitta Sipőcz wrote: gentle ping that once this one gets wrapped up we can do the long-awaited 1.5 release 😄

Oh, I'm sorry. If forgot to push my latest changes. And I hadn't put in my changelog entry, either, as I just noticed. I'll pay more attention to the CI this time, promised. Thanks for your patience!

That's regtap.choose_RegTAP_service. While I'm at it (not really related): cleanup of get_interface's docstring

More specifically, it will use UNION only if available, falling back to the much-harder-to-plan OR otherwise. This commit also fixes a bug in the UNION-based condition where multiple words were joined (in effect) with an OR rather than and AND as documented. To make this more convenient, this adds a get_tap_cap method to TAPServices; this returns the (first) tr:TableAccess capability on the service. The IVOA are not really saying what multiple such capabilities would mean, so for now I'd say we're good on just returning the first one (and it's in line with what we've been doing so far with maxrec and hardlimit; upload_methods would now change behaviour if there were upload methods distributed over multiple TableAccess-es; but the previous behaviour would have been bad anyway). We are also adding convenience methods for accessing the tap capability to the TAPService, for accessing the ADQL language in there, and for pulling out language features and UDFs. We probably want to document these somewhere visible, as advanced users (cross-server) can do smart things with them. But then perhaps that's too esoteric for now. It is an interesting question whether at least get_adql (or something more aptly named) should sit on TAPService itself...

...when the current TAP server doesn't support MOC or doesn't have stc_spatial.

Regrettably, this currently fails on both alternative endpoints because of validity problems. I'll try to work it out with the operators.

(and a pdb tracing call removed)

This helps certain SQLServer-based implementations while not impacting compliant ones; consider this defensive programming.

This enables checking for table presence doing table_name in tableset.

This was missing the declaration of the res_subject table that it needs (and that's not neccessary in the union branch because the table is in a subquery there.

msdemlei · 2023-12-14T11:21:49Z

Ok, and I've rebased to current main. @tomdonaldson, can you perhaps add your placet?

tomdonaldson

Thanks @msdemlei ! This all look good to me now. The _extra_tables mechanism seems like a nice way to address the need for the extra join.

bsipocz · 2023-12-15T19:37:20Z

CHANGES.rst

+- registry.search now introspects the TAP service's capabilities and
+  only offers extended functionality or optimisations if the required
+  features are present [#386]


As this is user facing, I would think the information that now it's possible to use other registries is important to list, or to list as well.

That being said, I would go ahead and merge this as is so I can go ahead and do more tests and cleanups, but please do advise a rephrasing and I'll fix up the changelog before the release.

bsipocz · 2023-12-15T19:39:09Z

Thank you so much @msdemlei!

msdemlei · 2023-12-18T08:44:25Z

On Fri, Dec 15, 2023 at 11:37:31AM -0800, Brigitta Sipőcz wrote: @bsipocz commented on this pull request. > +- registry.search now introspects the TAP service's capabilities and + only offers extended functionality or optimisations if the required + features are present [#386] That being said, I would go ahead and merge this as is so I can go ahead and do more tests and cleanups, but please do advise a rephrasing and I'll fix up the changelog before the release.

Is it legal to have two Changelog items for one PR? If so, I'd make an extra item and say something like: You can now change the registry endpoint that registry.search uses at runtime using the choose_RegTAP_service function. [#386] I think that's enough for a Changelog; whoever wants to do that kind of thing will find in the docstring of the function both info on global selection using IVOA_REGISTRY and how you can find alternative endpoints.

bsipocz · 2023-12-19T06:26:23Z

Is it legal to have two Changelog items for one PR?

Of course it is, thanks for the extra text!

tomdonaldson marked this pull request as draft November 9, 2022 13:45

bsipocz reviewed Dec 22, 2022

View reviewed changes

msdemlei mentioned this pull request Dec 22, 2022

BUG: hardlimit should not be mandatory #389

Closed

msdemlei force-pushed the make-regtap-service-aware branch 2 times, most recently from e8bb39b to 2caee9d Compare January 13, 2023 10:55

bsipocz added this to the v1.5 milestone Mar 1, 2023

bsipocz mentioned this pull request Mar 7, 2023

Sane behaviour for servicetype="image" #429

Open

msdemlei force-pushed the make-regtap-service-aware branch from 2caee9d to dec257d Compare May 9, 2023 15:32

bsipocz force-pushed the make-regtap-service-aware branch from dec257d to 9e01762 Compare May 10, 2023 08:08

bsipocz marked this pull request as ready for review May 10, 2023 08:09

bsipocz changed the title ~~Make regtap service aware [draft, don't review just yet]~~ Make regtap service aware May 10, 2023

bsipocz added the component: registry label May 10, 2023

bsipocz changed the title ~~Make regtap service aware~~ ENH: Make regtap service aware May 10, 2023

bsipocz force-pushed the make-regtap-service-aware branch from 9e01762 to e9a0e82 Compare December 5, 2023 04:46

bsipocz reviewed Dec 5, 2023

View reviewed changes

msdemlei added a commit to msdemlei/pyvo that referenced this pull request Dec 14, 2023

+ Changelog entry for PR astropy#386

29958e0

msdemlei and others added 15 commits December 14, 2023 12:20

Adding a facility to change the RegTAP endpoint.

ad302d9

That's regtap.choose_RegTAP_service. While I'm at it (not really related): cleanup of get_interface's docstring

Meaningful errors on the spatial constraint...

da72a06

...when the current TAP server doesn't support MOC or doesn't have stc_spatial.

Adding table conditions to Time and Spectral constraints

66e3c34

Adding an actual query to the RegTAP endpoint switching test.

7015144

Regrettably, this currently fails on both alternative endpoints because of validity problems. I'll try to work it out with the operators.

Adding a bit of documentation on endpoint switching

fd230e1

Documentation fixes

cd52fb3

(and a pdb tracing call removed)

The Freetext constraint now qualifies res_subject.

112b8a4

This helps certain SQLServer-based implementations while not impacting compliant ones; consider this defensive programming.

MAINT: fixing flake issues

2da23d9

TST: updating tests after rebase

b5715fa

MAINT: using pytest to catch and assert warnings

8ca5285

Minor fixes after Brigitta's 2023-12-05 review

23a76ef

Adding a __contains__ method to VOSITables.

170a79b

This enables checking for table presence doing table_name in tableset.

Fixing the or branch of the regtap Freetext constraint.

e0f997f

This was missing the declaration of the res_subject table that it needs (and that's not neccessary in the union branch because the table is in a subquery there.

+ Changelog entry for PR #386

1bbc88f

msdemlei force-pushed the make-regtap-service-aware branch from 29958e0 to 1bbc88f Compare December 14, 2023 11:22

tomdonaldson approved these changes Dec 14, 2023

View reviewed changes

bsipocz reviewed Dec 15, 2023

View reviewed changes

bsipocz merged commit c49a408 into main Dec 15, 2023
14 checks passed

bsipocz deleted the make-regtap-service-aware branch December 15, 2023 23:07

ManonMarchand mentioned this pull request Dec 19, 2023

Feature: add possibility to return full list of services #505

Merged

		>>> print("\n".join(sorted(r.get_interface("tap").access_url
		... for r in res)))

	returns the `LanguageFeature` with ivoid and form if present.
	returns the `~pyvo.io.vosi.tapregext.LanguageFeature` with ivoid and form if present.

	A `pyvo.io.vosi.TableAccess` instance.
	A `~pyvo.io.vosi.tapregext.TableAccess` instance.

	service : `dal.tap.TAPService`
	service : `~pyvo.dal.tap.TAPService`

	~`pyvo.registry.regtap.Interface`
	`~pyvo.registry.regtap.Interface`

	http://voparis-rr.obspm.fr:80/tap
	http://voparis-rr.obspm.fr/tap

ENH: Make regtap service aware #386

ENH: Make regtap service aware #386

Conversation

msdemlei commented Nov 9, 2022

Choose a reason for hiding this comment

msdemlei commented Dec 22, 2022 via email

bsipocz commented Mar 1, 2023

bsipocz commented May 8, 2023

bsipocz commented May 8, 2023

msdemlei commented May 8, 2023 via email

bsipocz commented May 9, 2023

bsipocz commented May 10, 2023

codecov bot commented May 10, 2023 • edited Loading

Codecov Report

bsipocz commented May 10, 2023 • edited Loading

msdemlei commented May 10, 2023 via email

bsipocz commented May 10, 2023

msdemlei commented Jul 7, 2023

msdemlei commented Dec 1, 2023

bsipocz commented Dec 1, 2023

bsipocz commented Dec 5, 2023

bsipocz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Dec 7, 2023

tomdonaldson commented Dec 7, 2023

bsipocz commented Dec 13, 2023

msdemlei commented Dec 14, 2023 via email

msdemlei commented Dec 14, 2023

tomdonaldson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Dec 15, 2023

msdemlei commented Dec 18, 2023 via email

bsipocz commented Dec 19, 2023

codecov bot commented May 10, 2023 •

edited

Loading

bsipocz commented May 10, 2023 •

edited

Loading

bsipocz left a comment •

edited

Loading