MAST: Enable cloud dataset by default#3534
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3534 +/- ##
==========================================
+ Coverage 73.09% 73.13% +0.04%
==========================================
Files 219 219
Lines 20592 20640 +48
==========================================
+ Hits 15052 15096 +44
- Misses 5540 5544 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bsipocz
left a comment
There was a problem hiding this comment.
Overall this looks good, but it still needs a rebase to resolve the conflicts.
Also, I have one minor comment, not a blocker but maybe we can cleanup a little bit based on the answer.
astroquery/mast/tests/test_mast.py
Outdated
| try: | ||
| from botocore.exceptions import ClientError | ||
| except ImportError: | ||
| ClientError = BotoCoreError = () |
There was a problem hiding this comment.
Is this version dependent? If yes, do you know when the changes were added?
There was a problem hiding this comment.
I'm not sure what you mean. I do see that I left BotoCoreError in here by mistake, so I'll push a fix for that.
There was a problem hiding this comment.
I mean why do we need this? We use the mock that already has the importskip, so I think the try/except should be enough for making the dependency optional (and maybe leave a comment that we do that try/except as this is optional dependency).
Also, in the code itself I think we should not override these errors but patch around the missing boto packages better; e.g. raise a warning sooner or maybe even an exception if the user wants to use cloud but doesn't have the optional dependencies installed.
There was a problem hiding this comment.
Ah, I see what you're saying. I went back into the code and refactored things a bit. The functions that require cloud access error out if the initialization fails and now give a more descriptive error message. Functions that don't require cloud access to work (downloads) will warn if cloud access can't be established, but fall back to an on-prem download by default.
ff73a75 to
0570107
Compare
0570107 to
aad5436
Compare
Current Behavior
enable_cloud_datasetfunction before they use any cloud-related functions.Proposed Behavior
enable_cloud_dataset.disable_cloud_datasetfunction.Advantages
What Changes for Users?
enable_cloud_datasetexplicitlyboto3,botocore)Other Things to Note
download_file,download_products,get_cloud_uri(s))botocore, an optional package, is imported without a guard. Now, bothboto3andbotocoreare imported with try blocks andImportErroris properly handled.Observationsinternally keeps track of whether the cloud dataset is explicitly enabled or disabled. If not explicitly enabled by the user (they callenable_cloud_dataset), warning messages are not logged when a product cannot be found in the cloud and the download falls back to on-prem. This is to avoid clogging up the console and introducing warning messages where there were none before.Close #3546