Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 451
Create fsstore from filesystem#911
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
2865d5305cf7a3a5acfbf67568621b0c084585e16c5b073b43ec6855c2ed221d1d8e390599d4e36c05edc045d520cd11c7c1c9255File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1262,8 +1262,9 @@ class FSStore(Store): | ||
| Parameters | ||
| ---------- | ||
| url : str | ||
| The destination to map. Should include protocol and path, | ||
| like "s3://bucket/root" | ||
| The destination to map. If no fs is provided, should include protocol | ||
martindurant marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| and path, like "s3://bucket/root". If an fs is provided, can be a path | ||
| within that filesystem, like "bucket/root" | ||
| normalize_keys : bool | ||
| key_separator : str | ||
| public API for accessing dimension_separator. Never `None` | ||
| @@ -1275,7 +1276,19 @@ class FSStore(Store): | ||
| as a missing key | ||
| dimension_separator : {'.', '/'}, optional | ||
| Separator placed between the dimensions of a chunk. | ||
| storage_options : passed to the fsspec implementation | ||
| fs : fsspec.spec.AbstractFileSystem, optional | ||
| An existing filesystem to use for the store. | ||
| check : bool, optional | ||
| If True, performs a touch at the root location, to check for write access. | ||
| Passed to `fsspec.mapping.FSMap` constructor. | ||
| create : bool, optional | ||
| If True, performs a mkdir at the rool location. | ||
| Passed to `fsspec.mapping.FSMap` constructor. | ||
| missing_exceptions : sequence of Exceptions, optional | ||
| Exceptions classes to associate with missing files. | ||
| Passed to `fsspec.mapping.FSMap` constructor. | ||
| storage_options : passed to the fsspec implementation. Cannot be used | ||
martindurant marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| together with fs. | ||
| """ | ||
| _array_meta_key = array_meta_key | ||
| _group_meta_key = group_meta_key | ||
| @@ -1285,18 +1298,37 @@ def __init__(self, url, normalize_keys=False, key_separator=None, | ||
| mode='w', | ||
| exceptions=(KeyError, PermissionError, IOError), | ||
| dimension_separator=None, | ||
| fs=None, | ||
| check=False, | ||
| create=False, | ||
| missing_exceptions=None, | ||
| **storage_options): | ||
| import fsspec | ||
| self.normalize_keys = normalize_keys | ||
| protocol, _ = fsspec.core.split_protocol(url) | ||
| # set auto_mkdir to True for local file system | ||
| if protocol in (None, "file") and not storage_options.get("auto_mkdir"): | ||
| storage_options["auto_mkdir"] = True | ||
| mapper_options = {"check": check, "create": create} | ||
| # https://github.com/zarr-developers/zarr-python/pull/911#discussion_r841926292 | ||
| # Some fsspec implementations don't accept missing_exceptions. | ||
martindurant marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| # This is a workaround to avoid passing it in the most common scenarios. | ||
| # Remove this and add missing_exceptions to mapper_options when fsspec is released. | ||
| if missing_exceptions is not None: | ||
| mapper_options["missing_exceptions"] = missing_exceptions # pragma: no cover | ||
| if fs is None: | ||
| protocol, _ = fsspec.core.split_protocol(url) | ||
| # set auto_mkdir to True for local file system | ||
| if protocol in (None, "file") and not storage_options.get("auto_mkdir"): | ||
| storage_options["auto_mkdir"] = True | ||
| self.map = fsspec.get_mapper(url, **{**mapper_options, **storage_options}) | ||
| self.fs = self.map.fs # for direct operations | ||
| self.path = self.fs._strip_protocol(url) | ||
| else: | ||
| if storage_options: | ||
| raise ValueError("Cannot specify both fs and storage_options") | ||
| self.fs = fs | ||
| self.path = self.fs._strip_protocol(url) | ||
| self.map = self.fs.get_mapper(self.path, **mapper_options) | ||
| self.map = fsspec.get_mapper(url, **storage_options) | ||
| self.fs = self.map.fs # for direct operations | ||
| self.path = self.fs._strip_protocol(url) | ||
| self.normalize_keys = normalize_keys | ||
| self.mode = mode | ||
| self.exceptions = exceptions | ||
| # For backwards compatibility. Guaranteed to be non-None | ||
| @@ -1308,8 +1340,6 @@ def __init__(self, url, normalize_keys=False, key_separator=None, | ||
| # Pass attributes to array creation | ||
| self._dimension_separator = dimension_separator | ||
| if self.fs.exists(self.path) and not self.fs.isdir(self.path): | ||
| raise FSPathExistNotDir(url) | ||
Comment on lines
-1311
to
-1312
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To fix #993 I have simply removed this check... | ||
| def _default_key_separator(self): | ||
| if self.key_separator is None: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -26,6 +26,7 @@ | ||
| from zarr.hierarchy import Group, group | ||
| from zarr.storage import ( | ||
| ConsolidatedMetadataStore, | ||
| FSStore, | ||
| KVStore, | ||
| MemoryStore, | ||
| atexit_rmtree, | ||
| @@ -205,9 +206,18 @@ def test_tree(zarr_version): | ||
| @pytest.mark.parametrize('zarr_version', [2, 3]) | ||
| @pytest.mark.parametrize('with_chunk_store', [False, True], ids=['default', 'with_chunk_store']) | ||
| @pytest.mark.parametrize('stores_from_path', [False, True]) | ||
| def test_consolidate_metadata(with_chunk_store, zarr_version, stores_from_path): | ||
| @pytest.mark.parametrize( | ||
| 'with_chunk_store,listable', | ||
| [(False, True), (True, True), (False, False)], | ||
| ids=['default-listable', 'with_chunk_store-listable', 'default-unlistable'] | ||
| ) | ||
| def test_consolidate_metadata(with_chunk_store, | ||
| zarr_version, | ||
| listable, | ||
| monkeypatch, | ||
| stores_from_path): | ||
| # setup initial data | ||
| if stores_from_path: | ||
| store = tempfile.mkdtemp() | ||
| @@ -228,6 +238,10 @@ def test_consolidate_metadata(with_chunk_store, zarr_version, stores_from_path): | ||
| version_kwarg = {} | ||
| path = 'dataset' if zarr_version == 3 else None | ||
| z = group(store, chunk_store=chunk_store, path=path, **version_kwarg) | ||
| # Reload the actual store implementation in case str | ||
| store_to_copy = z.store | ||
| z.create_group('g1') | ||
| g2 = z.create_group('g2') | ||
| g2.attrs['hello'] = 'world' | ||
| @@ -278,14 +292,36 @@ def test_consolidate_metadata(with_chunk_store, zarr_version, stores_from_path): | ||
| for key in meta_keys: | ||
| del store[key] | ||
| # https://github.com/zarr-developers/zarr-python/issues/993 | ||
| # Make sure we can still open consolidated on an unlistable store: | ||
| if not listable: | ||
| fs_memory = pytest.importorskip("fsspec.implementations.memory") | ||
| monkeypatch.setattr(fs_memory.MemoryFileSystem, "isdir", lambda x, y: False) | ||
martindurant marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| monkeypatch.delattr(fs_memory.MemoryFileSystem, "ls") | ||
| fs = fs_memory.MemoryFileSystem() | ||
| if zarr_version == 2: | ||
| store_to_open = FSStore("", fs=fs) | ||
| else: | ||
| store_to_open = FSStoreV3("", fs=fs) | ||
| # copy original store to new unlistable store | ||
| store_to_open.update(store_to_copy) | ||
| else: | ||
| store_to_open = store | ||
| # open consolidated | ||
| z2 = open_consolidated(store, chunk_store=chunk_store, path=path, **version_kwarg) | ||
| z2 = open_consolidated(store_to_open, chunk_store=chunk_store, path=path, **version_kwarg) | ||
| assert ['g1', 'g2'] == list(z2) | ||
| assert 'world' == z2.g2.attrs['hello'] | ||
| assert 1 == z2.g2.arr.attrs['data'] | ||
| assert (z2.g2.arr[:] == 1.0).all() | ||
| assert 16 == z2.g2.arr.nchunks | ||
| assert 16 == z2.g2.arr.nchunks_initialized | ||
| if listable: | ||
| assert 16 == z2.g2.arr.nchunks_initialized | ||
| else: | ||
| with pytest.raises(NotImplementedError): | ||
| _ = z2.g2.arr.nchunks_initialized | ||
rabernat marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| if stores_from_path: | ||
| # path string is note a BaseStore subclass so cannot be used to | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although 2.11.3 has been released, I noted that the docs had not been updated, so I added it here.
It seems we have skipped release 2.11.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those releases are coming from the
2_11branch. (See the related #898 (comment))I assume you intend this for release as a 2.11.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I have clearly not been able to follow that discussion. After reading, I now understand the situation better. Let's discuss at today's SC meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ping this thread with any decision on which branch to merge into
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martindurant : @rabernat seemed to think it was ok to hold off on @grlee77's upcoming v3 configuration fix, i.e. this can stay on the branch it is and be released as something
>2.11.x. If it looks like we want it in a quick~=2.11.xrelease, I can attempt the backport.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do whatever the other devs recommend here (either backport to 2.11.x or put this in 2.12.0)
I am confused about how we will maintain a consistent changelog for these two branches. So I am standing by for advice.