astroquery icon indicating copy to clipboard operation
astroquery copied to clipboard

ALMA DataLink service changes

Open at88mph opened this issue 3 years ago • 6 comments

  • Add registry lookup per ARC
  • Correct DataLink semantic layout to match new layout

at88mph avatar Jun 10 '22 20:06 at88mph

Codecov Report

Merging #2438 (aef76fd) into main (be0938f) will increase coverage by 0.08%. The diff coverage is 74.50%.

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

@@            Coverage Diff             @@
##             main    #2438      +/-   ##
==========================================
+ Coverage   62.97%   63.05%   +0.08%     
==========================================
  Files         133      133              
  Lines       17290    17315      +25     
==========================================
+ Hits        10888    10918      +30     
+ Misses       6402     6397       -5     
Impacted Files Coverage Δ
...oplanet_orbit_database/exoplanet_orbit_database.py 93.02% <ø> (-0.16%) :arrow_down:
...roquery/ipac/nexsci/nasa_exoplanet_archive/core.py 57.26% <ø> (-3.51%) :arrow_down:
astroquery/query.py 58.54% <ø> (-0.18%) :arrow_down:
astroquery/sdss/core.py 85.91% <ø> (-0.05%) :arrow_down:
astroquery/sdss/field_names.py 92.68% <ø> (-0.18%) :arrow_down:
astroquery/alma/core.py 47.70% <72.91%> (+4.90%) :arrow_up:
astroquery/oac/core.py 75.53% <100.00%> (+0.35%) :arrow_up:
astroquery/jplhorizons/core.py 65.29% <0.00%> (-1.05%) :arrow_down:
astroquery/gaia/core.py 71.47% <0.00%> (-0.47%) :arrow_down:
... and 2 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Jun 10 '22 20:06 codecov[bot]

Hello @at88mph! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-08-15 20:53:04 UTC

pep8speaks avatar Jun 13 '22 23:06 pep8speaks

Originally this ticket involved switching the client to use the IVOA Registry deployed at the ALMA Regional Centres (ARCs) to lookup the various services. After deliberation at the Canadian Astronomy Data Centre, it was agreed to rely on the existing known service endpoints and let the main proxy service (https://almascience.org) to direct the client to one of the ARCs and cache the URL to that ARC as being the user's "session".

I would like to open a discussion on whether using the Registry is the most appropriate way for clients to issue requests to services as it would involve them treating their registry as a stable and unchanging location. Is that best done with a new GitHub Issue?

at88mph avatar Jun 24 '22 19:06 at88mph

@at88mph here is OK, or in a new issue. Can you clarify what you mean by "the Registry"? Is that what's implemented in this PR?

I often have to change ALMA servers, ignoring the auto-redirect from almascience.org, because NRAO or ESO are down (or they haven't yet synced), so the user should definitely be able to override any 'automated' choice.

keflavich avatar Jun 24 '22 20:06 keflavich

Thanks @keflavich, that's really good input. I've been wondering how this library is often used, but I suspect it's from users scripting something.

The Registry is a web service running at each ALMA Regional Centre that is used to locate the endpoint of any deployed service (TAP, SIA, etc.) at that ARC. It's basically a canned query of URIs that map to URLs to use a service the caller is looking for.

One could make the argument that clients ought to rely on the Registry to obtain service locations rather than relying on a known endpoint (i.e. /tap, /sia2, etc.) that could potentially change.

at88mph avatar Jun 24 '22 20:06 at88mph

@at88mph - First of all, thanks for reworking this to be contained within the module while we see how to proceed with a more general API.

As for e.g. the registry question: I think it's fine to use registry calls from under the hood rather than hard-wired endpoints. My understanding is that astroquery users don't necessarily know or want to dive into the VO details and jumping through hoops of registry and then direct SIA/TAP/etc calls themselves, but as long as it works out of the box it's not an issue whether we get the access through a registry entry or by maintaining URLs ourselves.

bsipocz avatar Aug 12 '22 18:08 bsipocz

I seem to have picked up a lot of other commits as well and my git is giving warnings it never has before. I'm going to close this and reopen it.

at88mph avatar Aug 15 '22 20:08 at88mph

@at88mph - Yes, the complications of working on the default branch rather than on feature branches. However, I checked this out last Friday for review, and am happy to push back that version here and finish the review.

bsipocz avatar Aug 15 '22 21:08 bsipocz