-
-
Notifications
You must be signed in to change notification settings - Fork 314
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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: