Support async FSMap objects in zarr.open

[maxrjones]

Addresses #2706

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/user-guide/*.rst
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

I recall there being issues with fsmap before, but I confess I don't really know what an fsmap is -- can someone explain what an fsmap is, how it differs from an fsspec filesystem, and why people would use one over the other?

cc @martindurant

I have a vague feeling that it could be useful to have a Store class that wraps generic MutableMapping instances, and maybe fsmaps could go there, but that requires me knowing more about the user context for fsmap.

[martindurant]

FSMap was created specifically for the needs of Zarr, and it could have been essentially the same as the v2 FSStore, but was much quicker to get out and working within dask/fsspec.

FSMap is a dict-compatible interface (mutable-mapping) on top of a FS instance, which zarr worked with since forever and ignores some FS functionality like the file-like API.

To make it work with v3 might be complex, because zarr makes async calls, and the FSMap interface is blocking, even if the underlying FS is async. That means that there will be sync() within sync(), which might still work as zarr maintains its own event loop in a thread separate from fsspec's.

[martindurant]

To be clear: this PR does not use the mapper, but constructs a normal store from the mapper's attributes. I support this path.

[maxrjones]

thanks for the questions and answers, Davis and Martin!

IIUC there are three cases that we'd need to account for:

1. FSMap wraps an async instance of an async-compatible filesystem
   Solution - as implemented in this PR, extract the wrapped filesystem and use it to open an FsspecStore
2. FSMap wraps a non-async instance of a non-async-compatible filesystem
   Solution option 1 - extract the wrapped filesystem and wrap in AsyncFileSystemWrapper to open an FsspecStore as in Wrap sync fs for xarray.to_zarr #2533
   Solution option 2 - if it's a "file" protocol, extract the wrapped filesystem and open a LocalStore rather than FsspecStore, for other protocols, wrap in AsyncFileSystemWrapper to open an FsspecStore
3. FSMap wraps a non-async instance of an async-compatibile filesystem
   Solution - this is the case I'm not sure about. Is there a way to convert from a sync instance to an async instance without needing to wrap it in AsyncFileSystemWrapper? @martindurant could you please offer guidance here?

[martindurant]

Is there a way to convert from a sync instance to an async instance without needing to wrap it in AsyncFileSystemWrapper

The instance has all the arguments it was made with as attributes, so you can make a new instance with asynchronous=True from those.

if it's a "file" protocol, extract the wrapped filesystem and open a LocalStore rather than FsspecStore

Is there a reason to bother doing this?

[maxrjones]

if it's a "file" protocol, extract the wrapped filesystem and open a LocalStore rather than FsspecStore

Is there a reason to bother doing this?

I was thinking based on https://filesystem-spec.readthedocs.io/en/latest/async.html#limitations that LocalStore may be faster since it was designed around providing an async interface.

[d-v-b]

IMO it's simpler if the path is always fsmap -> fsspecstore

[martindurant]

LocalStore may be faster since it was designed around providing an async interface.

I doubt it. The disk is not really async (at least with the standard syscalls python uses), so none of the async code should make any difference for local reads at all.

[maxrjones]

I ran into #2808 when trying to wrap sync filesystems. My preference for this PR would be to bring the codecov up to 100% and request that this be considered for review, with the plan that the conversion of sync instances to async and the wrapping of sync filesystems could be handled separately. My reasoning is that some people could use this current implementation and I'm not sure how quickly I'll be able to find time to add the other missing features.

Update: I added conversion of sync instances to async

[maxrjones]

I think this is ready for a final review, as I addressed all the comments and increased the test coverage. Would you have time for a review before the next release @d-v-b @martindurant? Thanks for your support with the PR

[d-v-b]

thanks max, this looks great!

[maxrjones]

@d-v-b what was your fix the last time you ran into the upstream fsspec <-> s3fs incompatibility issue?

[d-v-b]

see #3091, in that pr @dstansby speculated that one of the git commits in fsspec was tagged, which prevented the version from being indicated as a dev version. Maybe we should re-open that PR...

[d-v-b]

note that the problem went away without me doing anything, presumably due to some activity over in fsspec?

[maxrjones]

note that the problem went away without me doing anything, presumably due to some activity over in fsspec?

Interesting, I don't even see the commit hash from the attempted install in the fsspec main branch's log. I also don't have permissions to rerun the failing job, so I'll just leave this PR as complete despite the failing test.

[maxrjones]

@martindurant thanks for your earlier review of this PR. I just wanted to check if you want to take another look before it get merged?

[d-v-b]

some minor questions about imports and re-assignment of object attributes, but otherwise this looks good

[d-v-b]

why do we import these here?

[d-v-b]

can we declare this import at the top of the file?

[d-v-b]

can we avoid mutating the fs_map object here? It looks like we only need the fs and root attributes, so lets pull those off into their own variables.