-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
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
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
The kerchunk test suite makes extensive use of xr.to_zarr("memory://..."), which currently doesn't work without this change.
OK to proceed? cc @jhamman |
OK, I have no idea what labeler is or why it's not happy. I added one label ("bug") to this PR to be certain. |
@martindurant I think I figured the labeler's strange behavior out. Should be fixed once I get eyes+approval on #2747 |
I wouldn't worry too much about the broken labeller - this (and other PRs) can definitely be merged before it's fixed (although we should definitely fix it!) |
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