-
Notifications
You must be signed in to change notification settings - Fork 22
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
Gb/s3docs #189
Gb/s3docs #189
Conversation
…te file handler including fsspec for zero setup s3 access
9139acf
to
984befd
Compare
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.
Nice! The code changes look fairly straightforward and robust. Really nice updates to the docs as well. Left a few minor comments that can be ignored
- os: ubuntu-latest | ||
python-version: 3.9 | ||
- os: ubuntu-latest | ||
python-version: 3.8 |
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.
Do we need to have Python 3.8 and 3.9 tests for this? Probably Python 3.10 across the three OS is enough, but I'll leave that up to you
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.
Probably not but it's free testing... I think leave it for now but i don't feel that strongly. We might also want to add new main tests for python 3.11-3.13 but we can leave that for future PRs.
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.
Well, it's definitely not free - there are usage limits on GitHub, but you're probably right that we wouldn't hit them unless we do extensive development on rex
years : list, optional | ||
List of integer years to access, by default None | ||
res_cls : obj | ||
Resource class to use to open and access resource data | ||
hsds : bool | ||
Boolean flag to use h5pyd to handle .h5 'files' hosted on AWS | ||
behind HSDS | ||
behind HSDS, by default False. This is now redundant; file paths |
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.
Is the idea to drop this option in the future sometime? Or should we leave it in case someone wants to use HSDS for a non /nrel
domain? If the latter, we can probably say something to the extent of "This is redundant for the /nrel
domain"
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.
Yeah i'm not 100% sure. My thinking was basically to not deprecate a kwarg right now until we've built experience with the new automatic parsing features. Trying not to make breaking changes in this PR, just new features. I would be open to dropping it in the future, maybe in an intermediate version release?
Co-authored-by: Paul Pinchuk <[email protected]>
Co-authored-by: Paul Pinchuk <[email protected]>
Automatic parsing of hsds/s3/local file paths with automatic fsspec access of s3 files without setup