BUG: Fix for broken alma downloader

[keflavich]

This should fix #2489. The failure is already covered by tests.

[codecov]

Codecov Report

Merging #2490 (0828aa2) into main (0883e92) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2490      +/-   ##
==========================================
- Coverage   62.99%   62.98%   -0.02%     
==========================================
  Files         133      133              
  Lines       17291    17297       +6     
==========================================
+ Hits        10892    10894       +2     
- Misses       6399     6403       +4     

Impacted Files	Coverage Δ
astroquery/alma/core.py	47.83% <0.00%> (-0.37%)	⬇️
astroquery/mast/observations.py	76.62% <0.00%> (+0.20%)	⬆️

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

[ceb8]

LGTM

[keflavich]

Thanks. I'd like to have @andamian 's eyes on this before merging in case there is a reason to take a different approach (e.g., there are some other metadata we should be inspecting to determine what action to take in these blank-line cases)

[bsipocz]

@keflavich - I don't see any of the tests to fail even without this PR, so they'll need to be adjusted to make sure they catch the pre-PR regression.

Also, I haven't looked into the details, but recall that #2438 might be in play here?

[bsipocz]

Tests should be added/refactored for this. Also it needs a changelog.

[at88mph]

This is already addressed in #2438 .

[keflavich]

OK, then I'll defer to that PR, but it's not at all obvious to me how this is addressed there. The underlying infrastructure is opaque to me.

The test that needs to fail is indeed marked xfail because it's awaiting #2438.

So I guess the new datalink service was deployed in the last ~48 hours? I have a daily cron job that started failing today, but was fine yeserday.

[bsipocz]

I'll try to get back to that PR this afternoon, as I recall my reservations were all about the modifications to the generic tooling, and nothing about the specifics in the alma module.

[bsipocz]

Closing this in favour of #2493

[bsipocz]

Can you rebase and add a changelog entry?

[bsipocz]

Are those datalinks copies of the same products that are already been listed with non-blank access_url?

For merging, this will need the review and approval of someone knowing the details of the ALMA API, @at88mph, or @andamian.

[keflavich]

Are those datalinks copies of the same products that are already been listed with non-blank access_url?

afaict, yes, the blank lines correspond to the tarballs that are now excluded from the download b/c they've been expanded.

There is likely to be a more rational way to handle this, but this is the bugfix I need now because the currently deployed version is broken. So, if other users stumble across this before we resolve the issue, this PR is an acceptable temporary workaround.

[keflavich]

@at88mph @andamian will either of you be able to review this PR this week?

If not, I recommend we merge it and request post-merge review to see if there is a better solution, but we need this as a patch now.

[at88mph]

Good catch @keflavich, thanks for this.

[bsipocz]

I take the comment of @at88mph from above as an approval, so am going ahead and merging this now. Thanks, @keflavich!