-
-
Notifications
You must be signed in to change notification settings - Fork 401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend Gemini to include getting file content and logging out of GOA #2851
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2851 +/- ##
==========================================
+ Coverage 67.49% 67.53% +0.03%
==========================================
Files 233 233
Lines 18420 18444 +24
==========================================
+ Hits 12433 12456 +23
- Misses 5987 5988 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of test failures, those need to be fixed before this can go ahead. Also, will need a changelog entry.
(run pytest -P gemini -R
to run the failing remote tests)
Big picture thing, that may not be known outside of a small group is that we're working on moving adding some download utilities to pyvo and plan to use that in astroquery. So it maybe useful for this PR/work, too (but given it's not yet merged, let alone released, I would not wait for it with this PR)
Besides I left a couple of smaller review comments.
if length == 0: | ||
log.warn(f'URL {url} has length=0') | ||
|
||
blocksize = astropy.utils.data.conf.download_block_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to explicitly import this, importing astropy
above is not enough.
@@ -433,6 +434,97 @@ def get_file(self, filename, *, download_dir='.', timeout=None): | |||
local_filepath = os.path.join(download_dir, filename) | |||
self._download_file(url=url, local_filepath=local_filepath, timeout=timeout) | |||
|
|||
def _download_file_content(self, url, timeout=None, auth=None, method="GET", **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make optional arguments kwarg only. Also, what are the possible keys in kwargs, explicitly list them if possible.
def _download_file_content(self, url, timeout=None, auth=None, method="GET", **kwargs): | |
def _download_file_content(self, url, *, timeout=None, auth=None, method="GET", **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kwargs could be any accepted keys in the underlying Session.request
. I can make a note of them.
self._authenticated = False | ||
|
||
def get_file_content(self, filename, timeout=None, auth=None, method="GET", **kwargs): | ||
"""Wrapper around `_download_file_content`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use double backticks for everything that is not sphinx linkable.
str | ||
The URL where the file can be downloaded. | ||
""" | ||
return f"https://archive.gemini.edu/file/{filename}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This url is hardwired at a lot of places, couldn't ve instead use the conf value, or store it as an instance attribute and use it here and elsewhere, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I have refactored the gemini
module to use conf
more and extend the module in a planned future PR.
I did not want to make much changes on this first PR so I could first confirm that the documentation and coding styling matched astroquery
.
@bsipocz Thanks for the review. I am working on addressing your comments. I have comments below:
|
ba2a654
to
348ef14
Compare
This PR extends the
Gemini
module.get_file_content
- Grabs the content of a file, does not save to disk.logout
- Deletes the Gemini Observatory Archive cookie.get_file_url
- helper method to get the URL for direct file download.I have updated the documentation to include the new functions and included tests.