-
Notifications
You must be signed in to change notification settings - Fork 0
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
Downloader prototype and CLI rework #7
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
- Coverage 33.33% 9.83% -23.50%
==========================================
Files 5 6 +1
Lines 18 61 +43
==========================================
Hits 6 6
- Misses 12 55 +43 ☔ View full report in Codecov by Sentry. |
src/fibad/__init__.py
Outdated
@@ -1,3 +1,3 @@ | |||
from .example_module import greetings, meaning | |||
|
|||
__all__ = ["greetings", "meaning"] | |||
__all__ = ["greetings", "meaning", "download"] |
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.
Interesting, does this just grab the module named download? I don't think I've seen this syntax before.
src/fibad/download.py
Outdated
Main entrypoint for downloading cutouts from HSC for use with fibad | ||
""" | ||
|
||
config = config["download"] |
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.
We can be pretty confident that "download" will be a key in the config at this point, but I think it would be good practice to try to be as defensive as possible with something like config.get('download', {})
.
If we adopt something that enforces more of the config structure like Pydantic or just using dataclasses that might let us be a bit less explicit.
parser.add_argument("--version", dest="version", action="store_true", help="Show version") | ||
parser.add_argument("-c", "--runtime-config", type=str, help="Full path to runtime config file") | ||
|
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.
As discussed, just want to confirm that both of the following will work on the command line still:
fibad download --runtime-config=<foo>
and
fibad --runtime-config=<foo> download
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.
Looks pretty good to me. I left a few comments on here, but no blockers for sure. After the comments are addressed, feel free to merge.
d7f8d6e
to
c3a4d50
Compare
Dutifully downloads HSC cutouts for 10 sources from a fits file using HSC's downloadCutout utility Modified CLI to use a single entrypoint with the idea to centralize argument and config parsing independent of what verb is used.
c3a4d50
to
e64841a
Compare
This is (like a lot of this project now) super preliminary. Comments encouraged, but may be addressed on followup for the sake of building out the scaffolding of this project quickly 🙂
Reworking the CLI to have a single entrypoint. The intent here is to centralize config parsing in the hopes that we can keep discipline about reproducibility via the config file mechanism, and keep the CLI small. There is a potential trade-off in CLI ergonomics which we're going to keep an eye on as development continues.
Downloader: This is a basic downloader that can process the
z_0_025.fits
catalog and download snapshots. It is currently hard-coded to only attempt the first 10 sources. It uses the downloadCutouts utility which has been imported from HSC's repository.