From f9dbd5b7044ea47434abb7e70e193ab19b1302a4 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Tue, 12 Nov 2024 13:25:29 -0800 Subject: [PATCH] respond to reviews --- docs/guide/storage.rst | 4 ++-- src/zarr/abc/store.py | 17 +++++++++++------ src/zarr/api/asynchronous.py | 25 +++++++++++++++++-------- src/zarr/testing/store.py | 8 +++++--- tests/test_store/test_local.py | 4 ++-- tests/test_store/test_remote.py | 2 +- 6 files changed, 38 insertions(+), 22 deletions(-) diff --git a/docs/guide/storage.rst b/docs/guide/storage.rst index 64827fdcf5..69de796b3d 100644 --- a/docs/guide/storage.rst +++ b/docs/guide/storage.rst @@ -43,7 +43,7 @@ filesystem. .. code-block:: python >>> import zarr - >>> store = zarr.storage.LocalStore("data/foo/bar", readonly=True) + >>> store = zarr.storage.LocalStore("data/foo/bar", read_only=True) >>> zarr.open(store=store) @@ -72,7 +72,7 @@ that implements the `AbstractFileSystem` API, .. code-block:: python >>> import zarr - >>> store = zarr.storage.RemoteStore.from_url("gs://foo/bar", readonly=True) + >>> store = zarr.storage.RemoteStore.from_url("gs://foo/bar", read_only=True) >>> zarr.open(store=store) shape=(10, 20) dtype=float32> diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 70f67398fd..0b3cc40021 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -82,7 +82,7 @@ async def _ensure_open(self) -> None: if not self._is_open: await self._open() - async def empty_dir(self, prefix: str = "") -> bool: + async def empty_dir(self, prefix: str) -> bool: """ Check if the directory is empty. @@ -96,8 +96,9 @@ async def empty_dir(self, prefix: str = "") -> bool: bool True if the store is empty, False otherwise. """ - - if prefix and not prefix.endswith("/"): + if not self.supports_listing: + raise NotImplementedError + if prefix != "" and not prefix.endswith("/"): prefix += "/" async for _ in self.list_prefix(prefix): return False @@ -109,8 +110,12 @@ async def clear(self) -> None: Remove all keys and values from the store. """ - async for key in self.list(): - await self.delete(key) + if not self.supports_deletes: + raise NotImplementedError + if not self.supports_listing: + raise NotImplementedError + self._check_writable() + await self.delete_dir("") @property def read_only(self) -> bool: @@ -319,7 +324,7 @@ async def delete_dir(self, prefix: str) -> None: if not self.supports_listing: raise NotImplementedError self._check_writable() - if prefix and not prefix.endswith("/"): + if prefix != "" and not prefix.endswith("/"): prefix += "/" async for key in self.list_prefix(prefix): await self.delete(key) diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index 445f13df95..94ecfee4d6 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -71,6 +71,9 @@ def _infer_exists_ok(mode: AccessModeLiteral) -> bool: + """ + Check that an ``AccessModeLiteral`` is compatible with overwriting an existing Zarr node. + """ return mode in _OVERWRITE_MODES @@ -410,13 +413,14 @@ async def save_array( arr = np.array(arr) shape = arr.shape chunks = getattr(arr, "chunks", None) # for array-likes with chunks attribute + exists_ok = kwargs.pop("exists_ok", None) or _infer_exists_ok(mode) new = await AsyncArray.create( store_path, zarr_format=zarr_format, shape=shape, dtype=arr.dtype, chunks=chunks, - exists_ok=kwargs.pop("exists_ok", None) or _infer_exists_ok(mode), + exists_ok=exists_ok, **kwargs, ) await new.setitem(slice(None), arr) @@ -617,9 +621,10 @@ async def group( try: return await AsyncGroup.open(store=store_path, zarr_format=zarr_format) except (KeyError, FileNotFoundError): + _zarr_format = zarr_format or _default_zarr_version() return await AsyncGroup.from_store( store=store_path, - zarr_format=zarr_format or _default_zarr_version(), + zarr_format=_zarr_format, exists_ok=overwrite, attributes=attributes, ) @@ -726,10 +731,12 @@ async def open_group( except (KeyError, FileNotFoundError): pass if mode in _CREATE_MODES: + exists_ok = _infer_exists_ok(mode) + _zarr_format = zarr_format or _default_zarr_version() return await AsyncGroup.from_store( store_path, - zarr_format=zarr_format or _default_zarr_version(), - exists_ok=_infer_exists_ok(mode), + zarr_format=_zarr_format, + exists_ok=exists_ok, attributes=attributes, ) raise FileNotFoundError(f"Unable to find group: {store_path}") @@ -904,7 +911,7 @@ async def create( dtype=dtype, compressor=compressor, fill_value=fill_value, - exists_ok=overwrite, # TODO: name change + exists_ok=overwrite, filters=filters, dimension_separator=dimension_separator, zarr_format=zarr_format, @@ -1074,7 +1081,7 @@ async def open_array( If using an fsspec URL to create the store, these will be passed to the backend implementation. Ignored otherwise. **kwargs - Any keyword arguments to pass to the ``create``. + Any keyword arguments to pass to ``create``. Returns ------- @@ -1091,10 +1098,12 @@ async def open_array( return await AsyncArray.open(store_path, zarr_format=zarr_format) except FileNotFoundError: if not store_path.read_only: + exists_ok = _infer_exists_ok(mode) + _zarr_format = zarr_format or _default_zarr_version() return await create( store=store_path, - zarr_format=zarr_format or _default_zarr_version(), - overwrite=_infer_exists_ok(mode), + zarr_format=_zarr_format, + overwrite=exists_ok, **kwargs, ) raise diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index c83f71af0e..b8a2016c6d 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -231,11 +231,13 @@ async def test_delete_dir(self, store: S) -> None: assert not await store.exists("foo/c/0") async def test_empty_dir(self, store: S) -> None: - assert await store.empty_dir() + assert await store.empty_dir("") await self.set( store, "foo/bar", self.buffer_cls.from_bytes(bytes("something", encoding="utf-8")) ) - assert not await store.empty_dir() + assert not await store.empty_dir("") + assert await store.empty_dir("fo") + assert not await store.empty_dir("foo/") assert not await store.empty_dir("foo") assert await store.empty_dir("spam/") @@ -244,7 +246,7 @@ async def test_clear(self, store: S) -> None: store, "key", self.buffer_cls.from_bytes(bytes("something", encoding="utf-8")) ) await store.clear() - assert await store.empty_dir() + assert await store.empty_dir("") async def test_list(self, store: S) -> None: assert await _collect_aiterator(store.list()) == () diff --git a/tests/test_store/test_local.py b/tests/test_store/test_local.py index bdb8098c98..4b85708dab 100644 --- a/tests/test_store/test_local.py +++ b/tests/test_store/test_local.py @@ -43,9 +43,9 @@ def test_store_supports_listing(self, store: LocalStore) -> None: assert store.supports_listing async def test_empty_with_empty_subdir(self, store: LocalStore) -> None: - assert await store.empty_dir() + assert await store.empty_dir("") (store.root / "foo/bar").mkdir(parents=True) - assert await store.empty_dir() + assert await store.empty_dir("") def test_creates_new_directory(self, tmp_path: pathlib.Path): target = tmp_path.joinpath("a", "b", "c") diff --git a/tests/test_store/test_remote.py b/tests/test_store/test_remote.py index 907f7f361a..5702e63bfd 100644 --- a/tests/test_store/test_remote.py +++ b/tests/test_store/test_remote.py @@ -213,4 +213,4 @@ async def test_empty_nonexistent_path(self, store_kwargs) -> None: # regression test for https://github.com/zarr-developers/zarr-python/pull/2343 store_kwargs["path"] += "/abc" store = await self.store_cls.open(**store_kwargs) - assert await store.empty_dir() + assert await store.empty_dir("")