-
-
Notifications
You must be signed in to change notification settings - Fork 302
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
Wrap sync fs for xarray.to_zarr #2533
base: main
Are you sure you want to change the base?
Wrap sync fs for xarray.to_zarr #2533
Conversation
8cd084d
to
38a841c
Compare
This fix is implicated in the issue here: #2554. Opening for review. Merge (and CI) is blocked pending a release of fsspec with async wrapper and it may be sensible to wait on that release for a bit to ensure compatibility with other projects as there are a lot of moving pieces across the various libraries being updated |
@moradology - is there interest in finishing this up? The proposed changes look good but we'll want a test to make sure this is working as expected. |
Yeah, definite interest in getting back to this - just at the tail end of a long period of vacation/travel and will have time in the coming week! |
563e99d
to
c7e0f59
Compare
src/zarr/storage/_fsspec.py
Outdated
@@ -3,6 +3,8 @@ | |||
import warnings | |||
from typing import TYPE_CHECKING, Any | |||
|
|||
from fsspec.implementations.asyn_wrapper import AsyncFileSystemWrapper |
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.
This makes the whole module depend on fsspec. I'm OK with that.
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, this is definitely unfortunate but a bit difficult to avoid
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.
why can't this import be done in or around line 162?
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.
Happy to do that (and add a bit of caution around the possibly failing import depending on fsspec versions)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2533 +/- ##
==========================================
- Coverage 99.98% 90.27% -9.72%
==========================================
Files 38 59 +21
Lines 14718 6468 -8250
==========================================
- Hits 14716 5839 -8877
- Misses 2 629 +627
|
4b8338f
to
b77e1c2
Compare
This PR automatically wraps synchronous filesystems in the
RemoteStore.from_url
function. This is necessary unless we provide aforce_async
argument onfsspec
surl_to_fs
function.I'm not sure adding more flags (even if we sugar them with default values) is the best move. Another option might be to implement a
url_to_asyncfs
method that simply wrapsurl_to_fs
and wraps any synchronous filesystems that come out automatically but that, too, would require changes here.This PR is in service of downstream kerchunk compatibility: fsspec/kerchunk#516
It depends on: fsspec/filesystem_spec#1755