Handle fsspec.FSMap using FSStore store

[ravwojdyla]

Re: #1296

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[codecov]

Codecov Report

Merging #1304 (3a9fab9) into main (482dfe5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1304   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           35        35           
  Lines        14130     14148   +18     
=========================================
+ Hits         14130     14148   +18     

Impacted Files	Coverage Δ
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)

[martindurant]

In principle this can be OK, but please see my major comment.

I wonder why this is necessary? Currently the FSMap will be wrapped by KVStore and everything works fine.

[martindurant]

At this point, we switch from the already configured Map and internal FS instance we were given to making new instances with these storage_options, which will in general be different. This is missed in the test, which uses the local filesystem and needs no kwargs.

(same for the other copy of this call below)

[ravwojdyla]

At this point, we switch from the already configured Map and internal FS instance we were given to making new instances with these storage_options, which will in general be different. This is missed in the test, which uses the local filesystem and needs no kwargs.

@martindurant what's your recommendation?

[ravwojdyla]

I wonder why this is necessary? Currently the FSMap will be wrapped by KVStore and everything works fine.

@martindurant see #1296. afaiu an alternative to this could be to add set/get items to KVStore.

[martindurant]

Ah I see, so previous store.getitems existed, but not when wrapped. So doing what this PR does is worthwhile, but my comment still needs to be solved. Can we have FSStore accept an FSMap instance as an alternative to creating one?

[ravwojdyla]

Can we have FSStore accept an FSMap instance as an alternative to creating one?

@martindurant I don't have a strong opinion about this, but want to point out that right now FSStore has a single required argument url of type str. There's optional fs argument. If we add optional fs_map argument, that fs_map will already include an "url" (root), so it can be somewhat confusing API (why would the user have an option to pass both url and fs_map, and what happens when there's discrepancies between these two). wdyt?

[martindurant]

OK, so we should pass (mapper.root, fs=mapper.fs), right?

[martindurant]

I mean the point is, not to pass the storage_options. Maybe it makes no difference, so sorry if I am just confusing things!

[martindurant]

OK, I guess you would hit https://github.com/zarr-developers/zarr-python/pull/1304/files#diff-565e487a2f60258b6baa2e4db8ef175cc16b8a949651834bd43d0a9f21e07358R1342 if you tried, never mind...

[ravwojdyla]

@martindurant sorry, that link above doesn't resolve for me, you probably mean this:

zarr-python/zarr/storage.py

Lines 1329 to 1331 in 1af77b6

	else:
	if storage_options:
	raise ValueError("Cannot specify both fs and storage_options")

So it's not possible to pass both fs and storage_options ^

[martindurant]

Yes that's the one +1

[ravwojdyla]

Actually just noticed that need to update this:

Should be FSStoreV3 ^

[rabernat]

The use case here is that many users (e.g. @ravwojdyla, others in the Pangeosphere) are used to constructing stores via fsspec.get_mapper() -> FSMap and have existing production code that does this; but the FSMap feature set has diverged from FSStore and is therefore less performant.

I support this PR in general because it seems like a lightweight way of upgrading an FSSMap to an FSStore.

An alternative would be to bring FSMap into feature parity with FSStore.

[rabernat]

Thanks a lot for this @ravwojdyla! I see no major issues, just one small suggestion regarding coverage.

[rabernat]

I think it's actually fine to pass storage_options through like this. If it is not empty, it will trigger an error via

zarr-python/zarr/storage.py

Lines 1329 to 1331 in 1af77b6

	else:
	if storage_options:
	raise ValueError("Cannot specify both fs and storage_options")

This is the desired behavior. Storage options will already be configured on the FSMap, so specifying them twice would not make sense.

[joshmoore]

@martindurant, are all your concerns addressed?

From my side, I do start to wonder just looking at the changes needed if fsspec isn't starting to become more than an optional dependency.

[martindurant]

To be sure, fsspec is a very small dep and one that has none of its own. I cannot make that decision, however.

Yes, I am happy otherwise.

[rabernat]

👍 to making fsspec a required dependency. However, let's address that in a separate PR. This is @ravwojdyla's first contribution to Zarr, so let's keep it simple.

@ravwojdyla - I invite you to give yourself appropriate credit for this bug fix in docs/release.rst. Once that is done we can merge.

[ravwojdyla]

@rabernat added the release note in 3a9fab9

[joshmoore]

🎉 Thanks, @ravwojdyla 🙏🏽