-
Notifications
You must be signed in to change notification settings - Fork 362
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
Ensure reference FS wraps any sync filesystems #1755
base: master
Are you sure you want to change the base?
Conversation
You are thinking of a referenceFS called from an async context? |
The issue arises here, I believe: https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/remote.py#L219-L225 Here, the |
Test failures here are pretty strange. Don't think these changes should have any effect on |
Those tests seem to be counting the number of coroutines, but now you are wrapping the coroutines inside coroutines? Maybe there's a more rigorous way to count. |
OK, interesting. Perhaps I'm missing something crucial about how that might be happening, because the failures in |
Actually this is the error. The stuff about async is in the cleanup stage for those coroutines (which ought to be awaited in the test even when they fail, but this doesn't cause a failure)
|
To fix the async thing (if you wish to try), probably in this block from
(or maybe try/except at |
This PR guards against the possibility (and likelihood, as
LocalFileSystem
is the default value here) that non-async filesystems are used inReferenceFileSystem
. This is important, asReferenceFileSystem
is, itself, anAsyncFileSystem
and it is currently failing to provide functional methods required by that interface (cf. https://github.com/fsspec/filesystem_spec/blob/master/fsspec/implementations/reference.py#L809)