Move functionality from gaia/core.py to utils/tap/core.py. #2311
Conversation
…ionality prevent users to have problems opening the downloaded file if they select a format which results are returned in zip and the name for the file selected by the user does not contain the right extension for the compress format.
Codecov Report
@@ Coverage Diff @@
## main #2311 +/- ##
==========================================
- Coverage 63.08% 62.97% -0.12%
==========================================
Files 131 131
Lines 17005 17073 +68
==========================================
+ Hits 10728 10752 +24
- Misses 6277 6321 +44
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Minor comments only. No changelog is needed as the new method is not in the public API.
And thank you for the cleanups, too.
Some tests would be useful though, as I see only the output_file=None case is covered in the test suite.
Either as unit tests for the new method (then do iterate over all output format scenarios), or a remote test for querying the gaia server. An example in the gaia docs would be helpful, too, I see/grep no mention of output_file anywhere there. The examples in the docs now run in remote tests, so they do offer code coverage, too.
| output_file_with_extension = output_file | ||
|
|
||
| if output_file is not None: | ||
| if output_format in format_with_results_compressed: |
There was a problem hiding this comment.
I only wondering out loud: is there any way for a user to force the output to be a non-compressed fits?
There was a problem hiding this comment.
As far as I know the answer is not. However, I will bring this question to the team and will suggest if it would be that ca be implemented in a near future.
There was a problem hiding this comment.
I really just wondered, I think you and the team know better the use cases. Either case, this should not hold up this PR.
|
@mhsarmiento - I'm planning to cut a release tomorrow, and will include this. I'll do some minimal fixes to get this merged in the morning (morning PDT), and would suggest adding the new tests we talked about above in a separate PR. Based on your comments above it seems that you already made some changes/commits, please do push them to this branch, so I can include those in the merge. |
Thank you very much @bsipocz and sorry because I haven't had the time yet to include those unit test for this part. However next week we will start a new sprint and we have a few improvements to include in the Gaia/Tap+ modules. I will include this unit test together with those changes. Once again, thank you very much for your support. Have a nice weekend |
|
Something went very wrong with the gaia module, but it's separate from this PR, so I go ahead with the merge and open an issue for the new failures (and treat that as a release blocker, at least until we understand the reason, which maybe just a temporary glitch server side) |
|
Thank you @mhsarmiento! |
Hi @bsipocz. Gaia archive is on maintenance. The downtime started last Friday at 17h (CET) and will last until today EOB (CET). Sorry for seeing this only now (I was on traveling this weekend). |
|
I'm afraid this had an effect on other ESA modules that relied on some default file extensions, etc. I think fixing the tests is relatively straightforward, but I would love to hear the go ahead from the other ESA devs, too that they are OK with the assumptions here, e.g. the results for votable output formats are not I think it's more than reasonable to have all the modules behave the same way, and I suspect gaia may have most of the usage, so following what is sensible for gaia sounds logical. |
|
Hi @bsipocz, I agree with these assumptions. As ESA TAP services are relying on the same architecture, we are offering the files the same way in all the Archives (same formats, compressed...). For this reason, all the modules will benefit from this PR, as the correct format will be set when users try to download the tables. Thanks for merging this! |
|
@jespinosaar - failing remote tests, that are affected are tracked e.g. here: #2203 Specifically for all the esa modules you can run It would be extremely useful if the ESA team could have a look at the failures. There is no time pressure on this though, I'm more interested in figuring out a way that gets the relevant person to know about these failures sooner. |
Dear all, this is mhsarmiento, from esdc/gaia team.
As agreed with @eerovaher, we have moved a functionality that was only available for the Gaia package to the tap package so all packages can get benefit from it. (Please check discussion #2077 (comment))
This functionality prevents users to have problems when opening the downloaded file if they select a format which results are returned in zip and the name for the file selected by the user does not contain the right extension for the compress format.
Thank you very much in advance!
Maria