-
-
Notifications
You must be signed in to change notification settings - Fork 282
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
Fs multiget #606
Fs multiget #606
Conversation
Thoughts? |
requirements_dev_optional.txt
Outdated
@@ -18,6 +18,7 @@ pytest-cov==2.7.1 | |||
pytest-doctestplus==0.4.0 | |||
pytest-remotedata==0.3.2 | |||
h5py==2.10.0 | |||
s3fs==0.5.0; python_version > '3.6' | |||
s3fs==0.5.1; python_version > '3.6' | |||
git+https://github.com/intake/filesystem_spec; python_version > '3.6' |
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 there a package on PyPI or conda-forge that we could install?
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.
Not yet, no.
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 have a rough sense when these might be created?
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.
When everything is good here, I can be sure to make a release before merging, so that this line can be reverted
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.
Sounds good @martindurant.
We're just reviewing this now in the zarr v3 dev call. Looks good to me :-) |
@alimanfoo , can I get your +1 on this? |
@jakirkham @joshmoore , any more thoughts? |
Just catching up on this PR. Fantastic work @martindurant! Are we planning to eventually do |
Yes, of course. I have local code that is partway there. |
Sorry if this was already answered and I just missed it, was there an fsspec release? Asking as we are planning to do a Zarr release soonish. cc @Carreau |
Yes, the (test) requirement that went in with this PR is for the latest release fsspec. I don't know if we want to say anything about this functionality's dependency in the release notes. |
Good point. If there is some wording you'd like to see in the release notes, would suggest proposing it in PR ( #616 ) |
Hm, not sure whether to say use fsspec>=0.8.0 or to fallback to sequential |
Is the |
Thank you. I don't use fsspec but try to implement
|
That looks OK to me, but that's what the current implementation is effectively doing anyway, with the loop outside of the storage class. |
I think we would still be open to a PR if someone wants to implement that directly in Zarr. |
It would may be sensible to make a fallback implementation of getitems in the storage class, doing the same loop currently higher up; but that would require a strict class hierarchy and not actually make anything simpler overall. |
False alarm, |
I don't think that is strictly true. We already implement functions that have fallbacks (like That said, I think having a class hierarchy would be a nice improvement as long as we still have a way to coerce generic |
For async IO.
Read-only for now, and needs fsspec master
TODO: