Skip to content
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

Add async wrapper for sync FS #1745

Merged
merged 5 commits into from
Nov 12, 2024

Conversation

moradology
Copy link
Contributor

This PR adds support for arbitrarily wrapping synchronous filesystems to make them compatible with interfaces which might expect AsynchronousFileSystem instances. Both instance and class-level wrapping behavior is provided and tested. This is a feature that will be of use downstream to enable kerchunk/zarr-python3 compatibility and is related to the efforts here

@martindurant
Copy link
Member

cf fsspec/kerchunk#524

@moradology
Copy link
Contributor Author

Hey @martindurant - would you prefer this (or some version thereof) implemented over in kerchunk rather than here?

@moradology moradology marked this pull request as draft November 5, 2024 19:04
@martindurant
Copy link
Member

The only reason to have it in kerchunk, is to indicate a level of experimentation and allow for releasing whenever is convenient. I suppose ice/zarr has no problem depending on it. I don't think it really matters.

@moradology moradology marked this pull request as ready for review November 5, 2024 19:31
@moradology
Copy link
Contributor Author

moradology commented Nov 5, 2024

OK, digging into things a little bit, I think there's a decent argument for pushing this behavior upstream to fsspec itself for the sake of not requiring a kerchunk dependency in zarr-python. In particular, this method will need to have access to the wrapper: https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/remote.py#L159-L168

The alternative might be to provide it as a util in zarr-python itself, as kerchunk also depends on zarr-python.
image

I find myself without as much context on prior decisions regarding project organization as might be useful, so please feel free to correct any apparent misunderstandings!

@martindurant
Copy link
Member

It can stay here, perhaps it'll come in useful for some other uses besides zarr.

@martindurant
Copy link
Member

Py3.8 has reached end-of-life, so you could either remove its CI, or protect your code with a condition sys.version_info > (3, 9).

@martindurant
Copy link
Member

Is there anything more you need from me to finish this off? I am happy to have it supersede my attempt (but you might hear about future issues! :) ).

@moradology
Copy link
Contributor Author

I still need to get the docs updated here. Other than that, I think I think this should be good. I've been chasing other test failures for zarr3 integration, but will plan to get docs updated + CI fixed ready for potential merger tomorrow

@martindurant
Copy link
Member

OK, thank you. Let me know.

@moradology
Copy link
Contributor Author

moradology commented Nov 12, 2024

KK, I think this is ready. Wasn't entirely sure how best to handle fsid, so that might deserve some critical attention (I just created a new one like: "async_{wrapped_class.fsid}") but otherwise things look OK

@martindurant
Copy link
Member

how best to handle fsid

I think what you've done is fine.

@martindurant
Copy link
Member

I did the lint, but please check this - is there a chance that self gets lost?

@moradology
Copy link
Contributor Author

Just looked things over again. It appears to me that line was kept in error and should be ditched

@martindurant martindurant merged commit 4cb98ab into fsspec:master Nov 12, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants