Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow fsspec/zarr/mfdataset #4461
Allow fsspec/zarr/mfdataset #4461
Changes from 11 commits
a1a794f
4bc674e
ca029ea
67a86f7
3fe984d
ee48ae2
53ced82
46e068a
65d3862
0596088
d34423b
29305aa
0b8e25d
13855ff
2da9b4d
db4a84e
cce42e2
7516d3e
bb4d2a9
857077a
3ad1448
c5f6249
bd26f79
64a6150
ef85740
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Rather than adding the
fs
keyword argument, why not just encourage passing in an appropriate mapping forfilename_or_obj
?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.
That works already, and will continue to work. However, the whole point. of this PR is to allow working out those details in a single call to open_dataset, which turns out very convenient for encoding in an Intake catalog, for instance, or indeed for the open_mfdataset implementation in here.
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.
Note that we are working on refactor of the backend API that among other things aims at removing all knowledge of what backends can or can't do from
open_dataset
. Adding logic insideif engine == "zarr"
will probably result in merge conflicts.I would suggest to move the call to
fs.get_mapper(filename_or_obj)
inside the zarr backend.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.
Thanks for the heads up. I already did one slightly complex conflict resolve.
It isn't totally clear, though, that the logic can be buried in the zarr engine for two reasons:
Happy to go whichever way is most convenient for the library.
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 need to resolve this discussion in order to decide what to do about this PR. Any more thoughts from other devs.
In my view, some of the fsspec logic introduced in the PR should eventually move to the generic
open_mfdataset
function, as it is not really specific to Zarr. However, I don't see a strong downside to adding it toopen_zarr
right now. Eventuallyopen_zarr
will be deprecated. But the pattern used here could be copied and incorporated into the backend refactor.