search

fsspec / filesystem_spec

A specification that python filesystems should adhere to.
BSD 3-Clause "New" or "Revised" License
1.05k stars 362 forks source link

Add async wrapper for sync FS #1745

Closed moradology closed 1 week ago

moradology commented 2 weeks ago

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 commented 2 weeks ago

cf https://github.com/fsspec/kerchunk/pull/524

moradology commented 2 weeks ago

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

martindurant commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

martindurant commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

OK, thank you. Let me know.

moradology commented 1 week ago

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 commented 1 week ago

how best to handle fsid

I think what you've done is fine.

martindurant commented 1 week ago

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

moradology commented 1 week ago

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