RegTAP search: add a flag for including standard auxiliary capabilities

[theresadower]

Proposed search flag includeaux : boolean, default false.

This flag controls whether to include auxiliary capabilities (with standard interfaces) in registry search results. This may result in duplicate capabilities being returned, especially if the servicetype is not specified, and therefore remains default off.

This behavior is discussed in #238 . The issue originally arose in a NAVO-run pyvo workshop at AAS, where a workflow of performing cone searches and then moving to a related TAP service for more fine-grained queries and additional filtering didn't work as expected. @trjaffe and @tomdonaldson can comment further from the workshop experience.

[codecov]

Codecov Report

Merging #258 (841807b) into master (fba1ebc) will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##           master     #258   +/-   ##
=======================================
  Coverage   74.94%   74.95%           
=======================================
  Files          44       44           
  Lines        5057     5063    +6     
=======================================
+ Hits         3790     3795    +5     
- Misses       1267     1268    +1     

Impacted Files	Coverage Δ
pyvo/registry/regtap.py	51.33% <90.00%> (+1.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fba1ebc...841807b. Read the comment docs.

[cbanek]

I'm not a registry expert, but this looks pretty good. I'm not sure what's up with the test failures, it looks like maybe those are related to some kind of mock issue, but once the tests pass I'm good to approve!

Thanks for the PR!

[theresadower]

I'm not a registry expert, but this looks pretty good. I'm not sure what's up with the test failures, it looks like maybe those are related to some kind of mock issue, but once the tests pass I'm good to approve!

Thanks for the PR!

Thanks! I'm not very familiar with fixtures or mocker myself. I'll spend some time working on this and possibly ask around for help, then un-mark it WIP for further review once they pass.

[theresadower]

@cbanek Tests fixed. It looks like one of the remaining failures is needing a milestone; I don't understand the codestyle: commands failed error, caused by the former or otherwise. Please let me know what I should do next, if anything, and thank you for your time!

[cbanek]

LGTM. Milestone set. Leaving for anyone else open to comment for a while. While I think the where clauses that are strings could be maybe combined or something, it's probably better to not complicate that part, since it's really readable the way it is here (and there's all sorts of issues with parens and other things like that).

[msdemlei]

As commented over in issue #238, this isn't my preferred way to address the underlying use case.

But if we do it this way, we shouldn't blindly match anything that ends in aux; that will give far too many false positives (try servicetype='sia' includeaux=True). If we go the way proposed here, I'd say we ought to have something like

# where perhaps keep this in a central location
KNOWN_SERVICE_TYPES = {"conesearch", "sia", "ssa", "slap", "tap"}

match_caps = None  # not used with this logic, but for robustness
if servicetype:
  match_caps = set([servicetype])
else:
  match_caps = KNOWN_SERVICE_TYPES

if includeaux:
  match_caps |= {s+"#aux" for s in match_caps}

if match_caps:
  wheres.append("standard_id IN ({})".format(
    ",".join(
      tap.as_adql_literal("ivo://ivoa.net/std/"+s)
      for s in match_caps)))

tap.as_adql_literal would have to be defined (but that's a useful function anyway) -- it would return ADQL literals for python values, e.g., 'that''s' for a python string that's.

(I'd like that better than current main's code whether or not we merge this PR)

[msdemlei]

Incidentally: Whatever we do, I suppose we need to touch the service property, too, when we deal with auxiliary capabilities. For now, a startwith approach as in describe would look about ok, but I have to admit I've not really looked at this particular part of the API and frankly suspect this needs to be re-thought a bit because of course there are now many resources that are both, say, SCS and TAP.

[theresadower]

As commented over in issue #238, this isn't my preferred way to address the underlying use case.

But if we do it this way, we shouldn't blindly match anything that ends in aux; that will give far too many false positives (try servicetype='sia' includeaux=True). If we go the way proposed here, I'd say we ought to have something like

# where perhaps keep this in a central location
KNOWN_SERVICE_TYPES = {"conesearch", "sia", "ssa", "slap", "tap"}

match_caps = None  # not used with this logic, but for robustness
if servicetype:
  match_caps = set([servicetype])
else:
  match_caps = KNOWN_SERVICE_TYPES

if includeaux:
  match_caps |= {s+"#aux" for s in match_caps}

if match_caps:
  wheres.append("standard_id IN ({})".format(
    ",".join(
      tap.as_adql_literal("ivo://ivoa.net/std/"+s)
      for s in match_caps)))

tap.as_adql_literal would have to be defined (but that's a useful function anyway) -- it would return ADQL literals for python values, e.g., 'that''s' for a python string that's.

(I'd like that better than current main's code whether or not we merge this PR)

OK, double checking just from a read of the code without having run it, the intention is:
if aux is included and servicetype is specified, only return auxiliary capabilities of that servicetype (that also match the rest of keyword WHEREs, etc) and if aux is included but no servicetype, return all standard capabilities and aux capabilities (that also match the rest of the keyword WHEREs, etc) and (while they would be arguably wrong from the registry record creation and organization standpoint) non-standard aux capabilities, if they ever did exist, would not be returned.

Is this right?
I think I like this actually, @trjaffe, is this closer to what we wanted out of the workshop workflow?

[theresadower]

As commented over in issue #238, this isn't my preferred way to address the underlying use case.

But if we do it this way, we shouldn't blindly match anything that ends in aux; that will give far too many false positives (try servicetype='sia' includeaux=True). If we go the way proposed here, I'd say we ought to have something like

# where perhaps keep this in a central location
KNOWN_SERVICE_TYPES = {"conesearch", "sia", "ssa", "slap", "tap"}

match_caps = None  # not used with this logic, but for robustness
if servicetype:
  match_caps = set([servicetype])
else:
  match_caps = KNOWN_SERVICE_TYPES

if includeaux:
  match_caps |= {s+"#aux" for s in match_caps}

if match_caps:
  wheres.append("standard_id IN ({})".format(
    ",".join(
      tap.as_adql_literal("ivo://ivoa.net/std/"+s)
      for s in match_caps)))

tap.as_adql_literal would have to be defined (but that's a useful function anyway) -- it would return ADQL literals for python values, e.g., 'that''s' for a python string that's.

(I'd like that better than current main's code whether or not we merge this PR)

Adapted this slightly - we already had a map of accepted servicetypes to standards-names, where users input 'scs' rather than 'conesearch', and used that rather than break it. Given the additional string wrangling around that, the new adql_as_literal does not seem to be needed.

If we want to change around this behaviour on a larger scale we can do a separate branch and MR for it.

Additionally, added a check for bad servicetype input, added to pytest.

[theresadower]

@cbanek This is currently failing due to lack of changelog, forgive me for not knowing process or being able to find it, should I just edit CHANGES.rst or is there some other process that will pick it up?

[andamian]

@cbanek This is currently failing due to lack of changelog, forgive me for not knowing process or being able to find it, should I just edit CHANGES.rst or is there some other process that will pick it up?

@theresadower - you need to update the CHANGES.rst and push it. Thanks

[cbanek]

This looks good to me, is everyone happy with this and ready to go?

[tomdonaldson]

This looks good to me, is everyone happy with this and ready to go?

Looks good to me too.