From 64ad5252ae3a0484628b0a743b8701cba2dbe0a2 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Sat, 19 Oct 2024 13:44:25 -0700 Subject: [PATCH 01/25] fix(array): thread order parameter through to array __init__ --- src/zarr/api/asynchronous.py | 10 +++------- src/zarr/core/array.py | 12 +++++------- tests/test_api.py | 16 ++++++++++++++++ tests/test_array.py | 17 +++++++++++++++++ 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index 2c423ff59..680433565 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -712,7 +712,7 @@ async def create( dtype: npt.DTypeLike | None = None, compressor: dict[str, JSON] | None = None, # TODO: default and type change fill_value: Any | None = 0, # TODO: need type - order: MemoryOrder | None = None, # TODO: default change + order: MemoryOrder | None = None, store: str | StoreLike | None = None, synchronizer: Any | None = None, overwrite: bool = False, @@ -761,6 +761,7 @@ async def create( Default value to use for uninitialized portions of the array. order : {'C', 'F'}, optional Memory layout to be used within each chunk. + Default is set in Zarr's config (`array.order`). store : Store or str Store or path to directory in file system or name of zip file. synchronizer : object, optional @@ -834,12 +835,6 @@ async def create( else: chunk_shape = shape - if order is not None: - warnings.warn( - "order is deprecated, use config `array.order` instead", - DeprecationWarning, - stacklevel=2, - ) if synchronizer is not None: warnings.warn("synchronizer is not yet implemented", RuntimeWarning, stacklevel=2) if chunk_store is not None: @@ -889,6 +884,7 @@ async def create( codecs=codecs, dimension_names=dimension_names, attributes=attributes, + order=order, **kwargs, ) diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index 6e3430c41..304cef016 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -471,10 +471,6 @@ async def create( raise ValueError( "dimension_separator cannot be used for arrays with version 3. Use chunk_key_encoding instead." ) - if order is not None: - raise ValueError( - "order cannot be used for arrays with version 3. Use a transpose codec instead." - ) if filters is not None: raise ValueError( "filters cannot be used for arrays with version 3. Use array-to-array codecs instead." @@ -494,6 +490,7 @@ async def create( dimension_names=dimension_names, attributes=attributes, exists_ok=exists_ok, + order=order, ) elif zarr_format == 2: if dtype is str or dtype == "str": @@ -545,6 +542,7 @@ async def _create_v3( dtype: npt.DTypeLike, chunk_shape: ChunkCoords, fill_value: Any | None = None, + order: Literal["C", "F"] | None = None, chunk_key_encoding: ( ChunkKeyEncoding | tuple[Literal["default"], Literal[".", "/"]] @@ -588,7 +586,7 @@ async def _create_v3( attributes=attributes or {}, ) - array = cls(metadata=metadata, store_path=store_path) + array = cls(metadata=metadata, store_path=store_path, order=order) await array._save_metadata(metadata, ensure_parents=True) return array @@ -611,7 +609,7 @@ async def _create_v2( if not exists_ok: await ensure_no_existing_node(store_path, zarr_format=2) if order is None: - order = "C" + order = config.get("array.order", "C") if dimension_separator is None: dimension_separator = "." @@ -627,7 +625,7 @@ async def _create_v2( filters=filters, attributes=attributes, ) - array = cls(metadata=metadata, store_path=store_path) + array = cls(metadata=metadata, store_path=store_path, order=order) await array._save_metadata(metadata, ensure_parents=True) return array diff --git a/tests/test_api.py b/tests/test_api.py index 9b7b4f8b9..ddd7051b1 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -206,6 +206,22 @@ def test_open_with_mode_w_minus(tmp_path: pathlib.Path) -> None: zarr.open(store=tmp_path, mode="w-") +@pytest.mark.parametrize("order", ["C", "F", None]) +@pytest.mark.parametrize("zarr_format", [2, 3]) +def test_array_order(order: str | None, zarr_format: int) -> None: + arr = zarr.ones(shape=(2, 2), order=order, zarr_format=zarr_format) + expected = order or zarr.config.get("array.order") + assert arr.order == expected + + vals = np.asarray(arr) + if expected == "C": + assert vals.flags.c_contiguous + elif expected == "F": + assert vals.flags.f_contiguous + else: + raise AssertionError + + # def test_lazy_loader(): # foo = np.arange(100) # bar = np.arange(100, 0, -1) diff --git a/tests/test_array.py b/tests/test_array.py index 829a04d30..1f79b8879 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -417,3 +417,20 @@ def test_update_attrs(zarr_format: int) -> None: arr2 = zarr.open_array(store=store, zarr_format=zarr_format) assert arr2.attrs["foo"] == "bar" + + +@pytest.mark.parametrize("order", ["C", "F", None]) +@pytest.mark.parametrize("zarr_format", [2, 3]) +@pytest.mark.parametrize("store", ["memory"], indirect=True) +def test_array_create_order(order: str | None, zarr_format: int, store: MemoryStore) -> None: + arr = Array.create(store=store, shape=(2, 2), order=order, zarr_format=zarr_format, dtype="i4") + expected = order or zarr.config.get("array.order") + assert arr.order == expected + + vals = np.asarray(arr) + if expected == "C": + assert vals.flags.c_contiguous + elif expected == "F": + assert vals.flags.f_contiguous + else: + raise AssertionError From 8b706f2863aa427d1b3cf3ec8392c4759a0d2d4e Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 21 Oct 2024 10:08:29 -0700 Subject: [PATCH 02/25] type fixes --- tests/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_api.py b/tests/test_api.py index ddd7051b1..7917c1ab5 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -208,7 +208,7 @@ def test_open_with_mode_w_minus(tmp_path: pathlib.Path) -> None: @pytest.mark.parametrize("order", ["C", "F", None]) @pytest.mark.parametrize("zarr_format", [2, 3]) -def test_array_order(order: str | None, zarr_format: int) -> None: +def test_array_order(order: Literal["C", "F"] | None, zarr_format: ZarrFormat) -> None: arr = zarr.ones(shape=(2, 2), order=order, zarr_format=zarr_format) expected = order or zarr.config.get("array.order") assert arr.order == expected From 59d7f7910765a14cdbd0bc56e310312069e0bdda Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 21 Oct 2024 10:46:32 -0700 Subject: [PATCH 03/25] move defaults --- src/zarr/core/array.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index 304cef016..bdafa33f6 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -33,6 +33,7 @@ ZARRAY_JSON, ZATTRS_JSON, ChunkCoords, + MemoryOrder, ShapeLike, ZarrFormat, concurrent_map, @@ -203,14 +204,14 @@ class AsyncArray(Generic[T_ArrayMetadata]): metadata: T_ArrayMetadata store_path: StorePath codec_pipeline: CodecPipeline = field(init=False) - order: Literal["C", "F"] + order: MemoryOrder @overload def __init__( self: AsyncArray[ArrayV2Metadata], metadata: ArrayV2Metadata | ArrayV2MetadataDict, store_path: StorePath, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, ) -> None: ... @overload @@ -218,14 +219,14 @@ def __init__( self: AsyncArray[ArrayV3Metadata], metadata: ArrayV3Metadata | ArrayV3MetadataDict, store_path: StorePath, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, ) -> None: ... def __init__( self, metadata: ArrayMetadata | ArrayMetadataDict, store_path: StorePath, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, ) -> None: if isinstance(metadata, dict): zarr_format = metadata["zarr_format"] @@ -261,7 +262,7 @@ async def create( attributes: dict[str, JSON] | None = None, chunks: ShapeLike | None = None, dimension_separator: Literal[".", "/"] | None = None, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, filters: list[dict[str, JSON]] | None = None, compressor: dict[str, JSON] | None = None, # runtime @@ -350,7 +351,7 @@ async def create( # v2 only chunks: ShapeLike | None = None, dimension_separator: Literal[".", "/"] | None = None, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, filters: list[dict[str, JSON]] | None = None, compressor: dict[str, JSON] | None = None, # runtime @@ -382,7 +383,7 @@ async def create( # v2 only chunks: ShapeLike | None = None, dimension_separator: Literal[".", "/"] | None = None, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, filters: list[dict[str, JSON]] | None = None, compressor: dict[str, JSON] | None = None, # runtime @@ -422,7 +423,6 @@ async def create( V2 only. V3 arrays cannot have a dimension separator. order : Literal["C", "F"], optional The order of the array (default is None). - V2 only. V3 arrays should not have 'order' parameter. filters : list[dict[str, JSON]], optional The filters used to compress the data (default is None). V2 only. V3 arrays should not have 'filters' parameter. @@ -542,7 +542,7 @@ async def _create_v3( dtype: npt.DTypeLike, chunk_shape: ChunkCoords, fill_value: Any | None = None, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, chunk_key_encoding: ( ChunkKeyEncoding | tuple[Literal["default"], Literal[".", "/"]] @@ -600,7 +600,7 @@ async def _create_v2( chunks: ChunkCoords, dimension_separator: Literal[".", "/"] | None = None, fill_value: None | float = None, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, filters: list[dict[str, JSON]] | None = None, compressor: dict[str, JSON] | None = None, attributes: dict[str, JSON] | None = None, @@ -608,8 +608,9 @@ async def _create_v2( ) -> AsyncArray[ArrayV2Metadata]: if not exists_ok: await ensure_no_existing_node(store_path, zarr_format=2) + if order is None: - order = config.get("array.order", "C") + order = parse_indexing_order(config.get("array.order")) if dimension_separator is None: dimension_separator = "." @@ -1177,7 +1178,7 @@ def create( # v2 only chunks: ChunkCoords | None = None, dimension_separator: Literal[".", "/"] | None = None, - order: Literal["C", "F"] | None = None, + order: MemoryOrder | None = None, filters: list[dict[str, JSON]] | None = None, compressor: dict[str, JSON] | None = None, # runtime @@ -1368,7 +1369,7 @@ def store_path(self) -> StorePath: return self._async_array.store_path @property - def order(self) -> Literal["C", "F"]: + def order(self) -> MemoryOrder: return self._async_array.order @property From 075b92987d07cc399b2023ea61d327572edc3d85 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 21 Oct 2024 10:49:16 -0700 Subject: [PATCH 04/25] apply MemoryOrder type to ArrayV2Metadata --- src/zarr/core/metadata/v2.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/zarr/core/metadata/v2.py b/src/zarr/core/metadata/v2.py index 2e1833605..f18f2e4e8 100644 --- a/src/zarr/core/metadata/v2.py +++ b/src/zarr/core/metadata/v2.py @@ -25,7 +25,7 @@ from zarr.core.array_spec import ArraySpec from zarr.core.chunk_grids import RegularChunkGrid from zarr.core.chunk_key_encodings import parse_separator -from zarr.core.common import ZARRAY_JSON, ZATTRS_JSON, parse_shapelike +from zarr.core.common import ZARRAY_JSON, ZATTRS_JSON, MemoryOrder, parse_shapelike from zarr.core.config import config, parse_indexing_order from zarr.core.metadata.common import parse_attributes @@ -45,7 +45,7 @@ class ArrayV2Metadata(Metadata): chunks: tuple[int, ...] dtype: np.dtype[Any] fill_value: None | int | float | str | bytes = 0 - order: Literal["C", "F"] = "C" + order: MemoryOrder = "C" filters: tuple[numcodecs.abc.Codec, ...] | None = None dimension_separator: Literal[".", "/"] = "." compressor: numcodecs.abc.Codec | None = None @@ -59,7 +59,7 @@ def __init__( dtype: npt.DTypeLike, chunks: ChunkCoords, fill_value: Any, - order: Literal["C", "F"], + order: MemoryOrder, dimension_separator: Literal[".", "/"] = ".", compressor: numcodecs.abc.Codec | dict[str, JSON] | None = None, filters: Iterable[numcodecs.abc.Codec | dict[str, JSON]] | None = None, @@ -185,7 +185,7 @@ def to_dict(self) -> dict[str, JSON]: return zarray_dict def get_chunk_spec( - self, _chunk_coords: ChunkCoords, order: Literal["C", "F"], prototype: BufferPrototype + self, _chunk_coords: ChunkCoords, order: MemoryOrder, prototype: BufferPrototype ) -> ArraySpec: return ArraySpec( shape=self.chunks, From b072545ef06ef27c060b204e94be6aac44451d93 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 21 Oct 2024 10:51:12 -0700 Subject: [PATCH 05/25] more more --- src/zarr/core/array_spec.py | 8 ++++---- src/zarr/core/metadata/v3.py | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/zarr/core/array_spec.py b/src/zarr/core/array_spec.py index e84a81cb0..c4d9c363f 100644 --- a/src/zarr/core/array_spec.py +++ b/src/zarr/core/array_spec.py @@ -1,11 +1,11 @@ from __future__ import annotations from dataclasses import dataclass -from typing import TYPE_CHECKING, Any, Literal +from typing import TYPE_CHECKING, Any import numpy as np -from zarr.core.common import parse_fill_value, parse_order, parse_shapelike +from zarr.core.common import MemoryOrder, parse_fill_value, parse_order, parse_shapelike if TYPE_CHECKING: from zarr.core.buffer import BufferPrototype @@ -17,7 +17,7 @@ class ArraySpec: shape: ChunkCoords dtype: np.dtype[Any] fill_value: Any - order: Literal["C", "F"] + order: MemoryOrder prototype: BufferPrototype def __init__( @@ -25,7 +25,7 @@ def __init__( shape: ChunkCoords, dtype: np.dtype[Any], fill_value: Any, - order: Literal["C", "F"], + order: MemoryOrder, prototype: BufferPrototype, ) -> None: shape_parsed = parse_shapelike(shape) diff --git a/src/zarr/core/metadata/v3.py b/src/zarr/core/metadata/v3.py index e9d2f92d8..6b6f28dd9 100644 --- a/src/zarr/core/metadata/v3.py +++ b/src/zarr/core/metadata/v3.py @@ -31,6 +31,7 @@ JSON, ZARR_JSON, ChunkCoords, + MemoryOrder, parse_named_configuration, parse_shapelike, ) @@ -289,7 +290,7 @@ def ndim(self) -> int: return len(self.shape) def get_chunk_spec( - self, _chunk_coords: ChunkCoords, order: Literal["C", "F"], prototype: BufferPrototype + self, _chunk_coords: ChunkCoords, order: MemoryOrder, prototype: BufferPrototype ) -> ArraySpec: assert isinstance( self.chunk_grid, RegularChunkGrid From 87267342f0bae40dbfd8ad30d237e7c3d6e92a10 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 21 Oct 2024 10:58:17 -0700 Subject: [PATCH 06/25] more more more --- tests/test_api.py | 4 ++-- tests/test_array.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 7917c1ab5..5b62e3a2f 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -21,7 +21,7 @@ save_array, save_group, ) -from zarr.core.common import ZarrFormat +from zarr.core.common import MemoryOrder, ZarrFormat from zarr.errors import MetadataValidationError from zarr.storage._utils import normalize_path from zarr.storage.memory import MemoryStore @@ -208,7 +208,7 @@ def test_open_with_mode_w_minus(tmp_path: pathlib.Path) -> None: @pytest.mark.parametrize("order", ["C", "F", None]) @pytest.mark.parametrize("zarr_format", [2, 3]) -def test_array_order(order: Literal["C", "F"] | None, zarr_format: ZarrFormat) -> None: +def test_array_order(order: MemoryOrder | None, zarr_format: ZarrFormat) -> None: arr = zarr.ones(shape=(2, 2), order=order, zarr_format=zarr_format) expected = order or zarr.config.get("array.order") assert arr.order == expected diff --git a/tests/test_array.py b/tests/test_array.py index 1f79b8879..f182cb1a1 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -10,7 +10,7 @@ from zarr.codecs import BytesCodec, VLenBytesCodec from zarr.core.array import chunks_initialized from zarr.core.buffer.cpu import NDBuffer -from zarr.core.common import JSON, ZarrFormat +from zarr.core.common import JSON, MemoryOrder, ZarrFormat from zarr.core.group import AsyncGroup from zarr.core.indexing import ceildiv from zarr.core.sync import sync @@ -422,7 +422,9 @@ def test_update_attrs(zarr_format: int) -> None: @pytest.mark.parametrize("order", ["C", "F", None]) @pytest.mark.parametrize("zarr_format", [2, 3]) @pytest.mark.parametrize("store", ["memory"], indirect=True) -def test_array_create_order(order: str | None, zarr_format: int, store: MemoryStore) -> None: +def test_array_create_order( + order: MemoryOrder | None, zarr_format: int, store: MemoryStore +) -> None: arr = Array.create(store=store, shape=(2, 2), order=order, zarr_format=zarr_format, dtype="i4") expected = order or zarr.config.get("array.order") assert arr.order == expected From 858d4fbde29b2e4697a49864187c47c8ec19bdd6 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Tue, 22 Oct 2024 06:34:21 -0700 Subject: [PATCH 07/25] feature(store,group,array): stores learn to delete prefixes when overwriting nodes - add Store.delete_dir and Store.delete_prefix - update array and group creation methods to call delete_dir - change list_prefix to return absolue keys --- src/zarr/abc/store.py | 30 ++++++++++++++++++++++++++++++ src/zarr/core/array.py | 14 ++++++++++++-- src/zarr/core/group.py | 21 ++++++++------------- src/zarr/storage/common.py | 12 ++++++++++++ src/zarr/storage/local.py | 4 +++- src/zarr/storage/logging.py | 3 +++ src/zarr/storage/memory.py | 11 ++++++++--- src/zarr/storage/remote.py | 7 ++++--- src/zarr/storage/zip.py | 2 +- src/zarr/testing/store.py | 5 ++--- tests/test_array.py | 3 +++ tests/test_group.py | 23 ++++++++++++++++++++++- tests/test_store/test_logging.py | 8 ++++++-- tests/test_store/test_zip.py | 2 +- 14 files changed, 115 insertions(+), 30 deletions(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index a995a6bf3..3927bfb9d 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -371,6 +371,36 @@ def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: """ ... + async def delete_dir(self, prefix: str, recursive: bool = True) -> None: + """ + Remove all keys and prefixes in the store that begin with a given prefix. + """ + if not self.supports_deletes: + raise NotImplementedError + if not self.supports_listing: + raise NotImplementedError + self._check_writable() + if recursive: + if not prefix.endswith("/"): + prefix += "/" + async for key in self.list_prefix(prefix): + await self.delete(f"{key}") + else: + async for key in self.list_dir(prefix): + await self.delete(f"{prefix}/{key}") + + async def delete_prefix(self, prefix: str) -> None: + """ + Remove all keys in the store that begin with a given prefix. + """ + if not self.supports_deletes: + raise NotImplementedError + if not self.supports_listing: + raise NotImplementedError + self._check_writable() + async for key in self.list_prefix(prefix): + await self.delete(f"{key}") + def close(self) -> None: """Close the store.""" self._is_open = False diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index bdafa33f6..4549cc3e7 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -554,7 +554,12 @@ async def _create_v3( attributes: dict[str, JSON] | None = None, exists_ok: bool = False, ) -> AsyncArray[ArrayV3Metadata]: - if not exists_ok: + if exists_ok: + if store_path.store.supports_deletes: + await store_path.delete_dir(recursive=True) + else: + await ensure_no_existing_node(store_path, zarr_format=3) + else: await ensure_no_existing_node(store_path, zarr_format=3) shape = parse_shapelike(shape) @@ -606,7 +611,12 @@ async def _create_v2( attributes: dict[str, JSON] | None = None, exists_ok: bool = False, ) -> AsyncArray[ArrayV2Metadata]: - if not exists_ok: + if exists_ok: + if store_path.store.supports_deletes: + await store_path.delete_dir(recursive=True) + else: + await ensure_no_existing_node(store_path, zarr_format=2) + else: await ensure_no_existing_node(store_path, zarr_format=2) if order is None: diff --git a/src/zarr/core/group.py b/src/zarr/core/group.py index 6e54b7ec9..914f001cc 100644 --- a/src/zarr/core/group.py +++ b/src/zarr/core/group.py @@ -404,7 +404,13 @@ async def from_store( zarr_format: ZarrFormat = 3, ) -> AsyncGroup: store_path = await make_store_path(store) - if not exists_ok: + + if exists_ok: + if store_path.store.supports_deletes: + await store_path.delete_dir(recursive=True) + else: + await ensure_no_existing_node(store_path, zarr_format=zarr_format) + else: await ensure_no_existing_node(store_path, zarr_format=zarr_format) attributes = attributes or {} group = cls( @@ -710,19 +716,8 @@ def _getitem_consolidated( async def delitem(self, key: str) -> None: store_path = self.store_path / key - if self.metadata.zarr_format == 3: - await (store_path / ZARR_JSON).delete() - - elif self.metadata.zarr_format == 2: - await asyncio.gather( - (store_path / ZGROUP_JSON).delete(), # TODO: missing_ok=False - (store_path / ZARRAY_JSON).delete(), # TODO: missing_ok=False - (store_path / ZATTRS_JSON).delete(), # TODO: missing_ok=True - ) - - else: - raise ValueError(f"unexpected zarr_format: {self.metadata.zarr_format}") + await store_path.delete_dir(recursive=True) if self.metadata.consolidated_metadata: self.metadata.consolidated_metadata.metadata.pop(key, None) await self._save_metadata() diff --git a/src/zarr/storage/common.py b/src/zarr/storage/common.py index 9ed8c274d..af434ecd3 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -101,6 +101,18 @@ async def delete(self) -> None: """ await self.store.delete(self.path) + async def delete_dir(self, recursive: bool = False) -> None: + """ + Delete all keys with the given prefix from the store. + """ + await self.store.delete_dir(self.path, recursive=recursive) + + async def delete_prefix(self) -> None: + """ + Delete all keys with the given prefix from the store. + """ + await self.store.delete_prefix(self.path) + async def set_if_not_exists(self, default: Buffer) -> None: """ Store a key to ``value`` if the key is not already present. diff --git a/src/zarr/storage/local.py b/src/zarr/storage/local.py index 5c03009a9..0ab2cc0be 100644 --- a/src/zarr/storage/local.py +++ b/src/zarr/storage/local.py @@ -226,7 +226,9 @@ async def list(self) -> AsyncGenerator[str, None]: async def list_prefix(self, prefix: str) -> AsyncGenerator[str, None]: # docstring inherited - to_strip = os.path.join(str(self.root / prefix)) + to_strip = str(self.root) + if prefix.endswith("/"): + prefix = prefix[:-1] for p in (self.root / prefix).rglob("*"): if p.is_file(): yield str(p.relative_to(to_strip)) diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index 66fd1687e..453a793cb 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -230,3 +230,6 @@ def with_mode(self, mode: AccessModeLiteral) -> Self: log_level=self.log_level, log_handler=self.log_handler, ) + + +# TODO: wrap delete methods here diff --git a/src/zarr/storage/memory.py b/src/zarr/storage/memory.py index f942d57b9..fa4ede2a8 100644 --- a/src/zarr/storage/memory.py +++ b/src/zarr/storage/memory.py @@ -1,5 +1,6 @@ from __future__ import annotations +from logging import getLogger from typing import TYPE_CHECKING, Self from zarr.abc.store import ByteRangeRequest, Store @@ -14,6 +15,9 @@ from zarr.core.common import AccessModeLiteral +logger = getLogger(__name__) + + class MemoryStore(Store): """ In-memory store for testing purposes. @@ -137,7 +141,7 @@ async def delete(self, key: str) -> None: try: del self._store_dict[key] except KeyError: - pass + logger.debug("Key %s does not exist.", key) async def set_partial_values(self, key_start_values: Iterable[tuple[str, int, bytes]]) -> None: # docstring inherited @@ -150,9 +154,10 @@ async def list(self) -> AsyncGenerator[str, None]: async def list_prefix(self, prefix: str) -> AsyncGenerator[str, None]: # docstring inherited - for key in self._store_dict: + # note: we materialize all dict keys into a list here so we can mutate the dict in-place (e.g. in delete_prefix) + for key in list(self._store_dict): if key.startswith(prefix): - yield key.removeprefix(prefix) + yield key async def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: # docstring inherited diff --git a/src/zarr/storage/remote.py b/src/zarr/storage/remote.py index 0a0ec7f7c..0eb72eb34 100644 --- a/src/zarr/storage/remote.py +++ b/src/zarr/storage/remote.py @@ -298,6 +298,7 @@ async def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: async def list_prefix(self, prefix: str) -> AsyncGenerator[str, None]: # docstring inherited - find_str = f"{self.path}/{prefix}" - for onefile in await self.fs._find(find_str, detail=False, maxdepth=None, withdirs=False): - yield onefile.removeprefix(find_str) + for onefile in await self.fs._find( + f"{self.path}/{prefix}", detail=False, maxdepth=None, withdirs=False + ): + yield onefile.removeprefix(f"{self.path}/") diff --git a/src/zarr/storage/zip.py b/src/zarr/storage/zip.py index 204a381bd..a7b8e1558 100644 --- a/src/zarr/storage/zip.py +++ b/src/zarr/storage/zip.py @@ -241,7 +241,7 @@ async def list_prefix(self, prefix: str) -> AsyncGenerator[str, None]: # docstring inherited async for key in self.list(): if key.startswith(prefix): - yield key.removeprefix(prefix) + yield key async def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: # docstring inherited diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index b4da75b06..fd9d6e691 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -249,8 +249,7 @@ async def test_list(self, store: S) -> None: async def test_list_prefix(self, store: S) -> None: """ Test that the `list_prefix` method works as intended. Given a prefix, it should return - all the keys in storage that start with this prefix. Keys should be returned with the shared - prefix removed. + all the keys in storage that start with this prefix. """ prefixes = ("", "a/", "a/b/", "a/b/c/") data = self.buffer_cls.from_bytes(b"") @@ -264,7 +263,7 @@ async def test_list_prefix(self, store: S) -> None: expected: tuple[str, ...] = () for key in store_dict: if key.startswith(prefix): - expected += (key.removeprefix(prefix),) + expected += (key,) expected = tuple(sorted(expected)) assert observed == expected diff --git a/tests/test_array.py b/tests/test_array.py index f182cb1a1..dac8ce859 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -48,6 +48,9 @@ def test_array_creation_existing_node( new_dtype = "float32" if exists_ok: + if not store.supports_deletes: + # TODO: confirm you get the expected error here + pytest.skip("store does not support deletes") arr_new = Array.create( spath / "extant", shape=new_shape, diff --git a/tests/test_group.py b/tests/test_group.py index 21e4ef4e5..13f412afa 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -290,6 +290,10 @@ def test_group_open(store: Store, zarr_format: ZarrFormat, exists_ok: bool) -> N with pytest.raises(ContainsGroupError): Group.from_store(store, attributes=attrs, zarr_format=zarr_format, exists_ok=exists_ok) else: + if not store.supports_deletes: + pytest.skip( + "Store does not support deletes but `exists_ok` is True, requiring deletes to override a group" + ) group_created_again = Group.from_store( store, attributes=new_attrs, zarr_format=zarr_format, exists_ok=exists_ok ) @@ -703,6 +707,8 @@ def test_group_creation_existing_node( new_attributes = {"new": True} if exists_ok: + if not store.supports_deletes: + pytest.skip("store does not support deletes but exists_ok is True") node_new = Group.from_store( spath / "extant", attributes=new_attributes, @@ -1075,7 +1081,9 @@ async def test_require_group(store: LocalStore | MemoryStore, zarr_format: ZarrF assert foo_group.attrs == {"foo": 100} # test that we can get the group using require_group and overwrite=True - foo_group = await root.require_group("foo", overwrite=True) + if store.supports_deletes: + foo_group = await root.require_group("foo", overwrite=True) + assert foo_group.attrs == {} _ = await foo_group.create_array( "bar", shape=(10,), dtype="uint8", chunk_shape=(2,), attributes={"foo": 100} @@ -1354,3 +1362,16 @@ def test_group_deprecated_positional_args(method: str) -> None: with pytest.warns(FutureWarning, match=r"Pass name=.*, data=.* as keyword args."): arr = getattr(root, method)("foo_like", data, **kwargs) assert arr.shape == data.shape + + +@pytest.mark.parametrize("store", ["local", "memory"], indirect=["store"]) +def test_delitem_removes_children(store: Store, zarr_format: ZarrFormat) -> None: + # https://github.com/zarr-developers/zarr-python/issues/2191 + g1 = zarr.group(store=store) + g1.create_group("0") + g1.create_group("0/0") + arr = g1.create_array("0/0/0", shape=(1,)) + arr[:] = 1 + del g1["0"] + with pytest.raises(KeyError): + g1["0/0"] diff --git a/tests/test_store/test_logging.py b/tests/test_store/test_logging.py index 0258244c5..976da6818 100644 --- a/tests/test_store/test_logging.py +++ b/tests/test_store/test_logging.py @@ -45,10 +45,14 @@ async def test_logging_store_counter(store: Store) -> None: arr[:] = 1 assert wrapped.counter["set"] == 2 - assert wrapped.counter["get"] == 0 # 1 if overwrite=False assert wrapped.counter["list"] == 0 assert wrapped.counter["list_dir"] == 0 - assert wrapped.counter["list_prefix"] == 0 + if store.supports_deletes: + assert wrapped.counter["get"] == 0 # 1 if overwrite=False + assert wrapped.counter["list_prefix"] == 1 + else: + assert wrapped.counter["get"] == 1 + assert wrapped.counter["list_prefix"] == 0 async def test_with_mode(): diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index d05422ecd..eb129a1b2 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -103,4 +103,4 @@ async def test_with_mode(self, store: ZipStore) -> None: @pytest.mark.parametrize("mode", ["a", "w"]) async def test_store_open_mode(self, store_kwargs: dict[str, Any], mode: str) -> None: - super().test_store_open_mode(store_kwargs, mode) + await super().test_store_open_mode(store_kwargs, mode) From 9c173c1220866d4ecdb221cb22daee5d8e28ec8c Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Wed, 23 Oct 2024 06:14:42 -0700 Subject: [PATCH 08/25] fixup --- src/zarr/abc/store.py | 2 +- src/zarr/storage/local.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 1ed098910..76a71c5a9 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -399,7 +399,7 @@ async def delete_prefix(self, prefix: str) -> None: raise NotImplementedError self._check_writable() async for key in self.list_prefix(prefix): - await self.delete(f"{key}") + await self.delete(key) def close(self) -> None: """Close the store.""" diff --git a/src/zarr/storage/local.py b/src/zarr/storage/local.py index 0ab2cc0be..c08b351b4 100644 --- a/src/zarr/storage/local.py +++ b/src/zarr/storage/local.py @@ -227,8 +227,7 @@ async def list(self) -> AsyncGenerator[str, None]: async def list_prefix(self, prefix: str) -> AsyncGenerator[str, None]: # docstring inherited to_strip = str(self.root) - if prefix.endswith("/"): - prefix = prefix[:-1] + prefix = prefix.rstrip("/") for p in (self.root / prefix).rglob("*"): if p.is_file(): yield str(p.relative_to(to_strip)) From d7a698284671df7c4de62e0ea3010328557cf86e Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Wed, 23 Oct 2024 16:13:24 -0700 Subject: [PATCH 09/25] fixup --- src/zarr/abc/store.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 76a71c5a9..03f42584f 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -384,7 +384,7 @@ async def delete_dir(self, prefix: str, recursive: bool = True) -> None: if not prefix.endswith("/"): prefix += "/" async for key in self.list_prefix(prefix): - await self.delete(f"{key}") + await self.delete(key) else: async for key in self.list_dir(prefix): await self.delete(f"{prefix}/{key}") From c38c2f7cd52dfa884e4f542de6aaed73a57835c7 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Thu, 24 Oct 2024 16:29:35 -0700 Subject: [PATCH 10/25] respond to review --- src/zarr/abc/store.py | 26 +++++--------------------- src/zarr/core/array.py | 4 ++-- src/zarr/core/group.py | 4 ++-- src/zarr/storage/common.py | 13 +++++-------- src/zarr/storage/logging.py | 5 +++++ src/zarr/testing/store.py | 13 +++++++++++++ tests/test_store/test_logging.py | 5 +++-- tests/test_store/test_zip.py | 4 ---- 8 files changed, 35 insertions(+), 39 deletions(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 03f42584f..c7f21df50 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -342,8 +342,8 @@ def list(self) -> AsyncGenerator[str, None]: @abstractmethod def list_prefix(self, prefix: str) -> AsyncGenerator[str, None]: """ - Retrieve all keys in the store that begin with a given prefix. Keys are returned with the - common leading prefix removed. + Retrieve all keys in the store that begin with a given prefix. Keys are returned as + absolute paths (i.e. including the prefix). Parameters ---------- @@ -371,7 +371,7 @@ def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: """ ... - async def delete_dir(self, prefix: str, recursive: bool = True) -> None: + async def delete_dir(self, prefix: str) -> None: """ Remove all keys and prefixes in the store that begin with a given prefix. """ @@ -380,24 +380,8 @@ async def delete_dir(self, prefix: str, recursive: bool = True) -> None: if not self.supports_listing: raise NotImplementedError self._check_writable() - if recursive: - if not prefix.endswith("/"): - prefix += "/" - async for key in self.list_prefix(prefix): - await self.delete(key) - else: - async for key in self.list_dir(prefix): - await self.delete(f"{prefix}/{key}") - - async def delete_prefix(self, prefix: str) -> None: - """ - Remove all keys in the store that begin with a given prefix. - """ - if not self.supports_deletes: - raise NotImplementedError - if not self.supports_listing: - raise NotImplementedError - self._check_writable() + if not prefix.endswith("/"): + prefix += "/" async for key in self.list_prefix(prefix): await self.delete(key) diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index 4549cc3e7..7b5733116 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -556,7 +556,7 @@ async def _create_v3( ) -> AsyncArray[ArrayV3Metadata]: if exists_ok: if store_path.store.supports_deletes: - await store_path.delete_dir(recursive=True) + await store_path.delete_dir() else: await ensure_no_existing_node(store_path, zarr_format=3) else: @@ -613,7 +613,7 @@ async def _create_v2( ) -> AsyncArray[ArrayV2Metadata]: if exists_ok: if store_path.store.supports_deletes: - await store_path.delete_dir(recursive=True) + await store_path.delete_dir() else: await ensure_no_existing_node(store_path, zarr_format=2) else: diff --git a/src/zarr/core/group.py b/src/zarr/core/group.py index 914f001cc..5e6803930 100644 --- a/src/zarr/core/group.py +++ b/src/zarr/core/group.py @@ -407,7 +407,7 @@ async def from_store( if exists_ok: if store_path.store.supports_deletes: - await store_path.delete_dir(recursive=True) + await store_path.delete_dir() else: await ensure_no_existing_node(store_path, zarr_format=zarr_format) else: @@ -717,7 +717,7 @@ def _getitem_consolidated( async def delitem(self, key: str) -> None: store_path = self.store_path / key - await store_path.delete_dir(recursive=True) + await store_path.delete_dir() if self.metadata.consolidated_metadata: self.metadata.consolidated_metadata.metadata.pop(key, None) await self._save_metadata() diff --git a/src/zarr/storage/common.py b/src/zarr/storage/common.py index af434ecd3..337fbc59a 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -101,17 +101,14 @@ async def delete(self) -> None: """ await self.store.delete(self.path) - async def delete_dir(self, recursive: bool = False) -> None: + async def delete_dir(self) -> None: """ Delete all keys with the given prefix from the store. """ - await self.store.delete_dir(self.path, recursive=recursive) - - async def delete_prefix(self) -> None: - """ - Delete all keys with the given prefix from the store. - """ - await self.store.delete_prefix(self.path) + path = self.path + if not path.endswith("/"): + path += "/" + await self.store.delete_dir(path) async def set_if_not_exists(self, default: Buffer) -> None: """ diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index 453a793cb..755d7a58e 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -222,6 +222,11 @@ async def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: async for key in self._store.list_dir(prefix=prefix): yield key + async def delete_dir(self, prefix: str) -> None: + # docstring inherited + with self.log(prefix): + await self._store.delete_dir(prefix=prefix) + def with_mode(self, mode: AccessModeLiteral) -> Self: # docstring inherited with self.log(mode): diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index fd9d6e691..b39c548fb 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -213,11 +213,24 @@ async def test_exists(self, store: S) -> None: assert await store.exists("foo/zarr.json") async def test_delete(self, store: S) -> None: + if not store.supports_deletes: + pytest.skip("store does not support deletes") await store.set("foo/zarr.json", self.buffer_cls.from_bytes(b"bar")) assert await store.exists("foo/zarr.json") await store.delete("foo/zarr.json") assert not await store.exists("foo/zarr.json") + async def test_delete_dir(self, store: S) -> None: + if not store.supports_deletes: + pytest.skip("store does not support deletes") + await store.set("zarr.json", self.buffer_cls.from_bytes(b"root")) + await store.set("foo/zarr.json", self.buffer_cls.from_bytes(b"bar")) + await store.set("foo/c/0", self.buffer_cls.from_bytes(b"chunk")) + await store.delete_dir("foo") + assert await store.exists("zarr.json") + assert not await store.exists("foo/zarr.json") + assert not await store.exists("foo/c/0") + async def test_empty(self, store: S) -> None: assert await store.empty() await self.set( diff --git a/tests/test_store/test_logging.py b/tests/test_store/test_logging.py index 976da6818..50db5c1c5 100644 --- a/tests/test_store/test_logging.py +++ b/tests/test_store/test_logging.py @@ -47,12 +47,13 @@ async def test_logging_store_counter(store: Store) -> None: assert wrapped.counter["set"] == 2 assert wrapped.counter["list"] == 0 assert wrapped.counter["list_dir"] == 0 + assert wrapped.counter["list_prefix"] == 0 if store.supports_deletes: assert wrapped.counter["get"] == 0 # 1 if overwrite=False - assert wrapped.counter["list_prefix"] == 1 + assert wrapped.counter["delete_dir"] == 1 else: assert wrapped.counter["get"] == 1 - assert wrapped.counter["list_prefix"] == 0 + assert wrapped.counter["delete_dir"] == 0 async def test_with_mode(): diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index eb129a1b2..8dee47449 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -14,7 +14,6 @@ from zarr.testing.store import StoreTests if TYPE_CHECKING: - from collections.abc import Coroutine from typing import Any @@ -65,9 +64,6 @@ def test_store_supports_partial_writes(self, store: ZipStore) -> None: def test_store_supports_listing(self, store: ZipStore) -> None: assert store.supports_listing - def test_delete(self, store: ZipStore) -> Coroutine[Any, Any, None]: - pass - def test_api_integration(self, store: ZipStore) -> None: root = zarr.open_group(store=store) From cafb46ad88335001e8180c39bb776c4f9310e8f5 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Thu, 24 Oct 2024 16:56:48 -0700 Subject: [PATCH 11/25] fixup --- src/zarr/storage/logging.py | 3 --- src/zarr/testing/store.py | 2 ++ tests/test_array.py | 1 - 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index 755d7a58e..be259579e 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -235,6 +235,3 @@ def with_mode(self, mode: AccessModeLiteral) -> Self: log_level=self.log_level, log_handler=self.log_handler, ) - - -# TODO: wrap delete methods here diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index b39c548fb..3aece0f4a 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -224,10 +224,12 @@ async def test_delete_dir(self, store: S) -> None: if not store.supports_deletes: pytest.skip("store does not support deletes") await store.set("zarr.json", self.buffer_cls.from_bytes(b"root")) + await store.set("foo-bar/zarr.json", self.buffer_cls.from_bytes(b"root")) await store.set("foo/zarr.json", self.buffer_cls.from_bytes(b"bar")) await store.set("foo/c/0", self.buffer_cls.from_bytes(b"chunk")) await store.delete_dir("foo") assert await store.exists("zarr.json") + assert await store.exists("foo-bar/zarr.json") assert not await store.exists("foo/zarr.json") assert not await store.exists("foo/c/0") diff --git a/tests/test_array.py b/tests/test_array.py index e827b07b0..96475d43e 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -52,7 +52,6 @@ def test_array_creation_existing_node( if exists_ok: if not store.supports_deletes: - # TODO: confirm you get the expected error here pytest.skip("store does not support deletes") arr_new = Array.create( spath / "extant", From a1e71f1ef723ceeb8640396f366c7888e2df8329 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Thu, 24 Oct 2024 17:03:23 -0700 Subject: [PATCH 12/25] fixup --- tests/test_group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_group.py b/tests/test_group.py index 7de5b629f..6bacca488 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -1384,7 +1384,7 @@ def test_group_deprecated_positional_args(method: str) -> None: @pytest.mark.parametrize("store", ["local", "memory"], indirect=["store"]) def test_delitem_removes_children(store: Store, zarr_format: ZarrFormat) -> None: # https://github.com/zarr-developers/zarr-python/issues/2191 - g1 = zarr.group(store=store) + g1 = zarr.group(store=store, zarr_format=zarr_format) g1.create_group("0") g1.create_group("0/0") arr = g1.create_array("0/0/0", shape=(1,)) From ee47265f3e12eb8942112b6805d7ed37ee77dc97 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 28 Oct 2024 09:03:12 -0700 Subject: [PATCH 13/25] fix(store): refactor store initialization to include only r or w permissions --- src/zarr/abc/store.py | 100 ++++++------------- src/zarr/api/asynchronous.py | 127 +++++++++++++++++------- src/zarr/api/synchronous.py | 6 +- src/zarr/core/array.py | 2 +- src/zarr/core/group.py | 2 +- src/zarr/storage/common.py | 55 ++++++++-- src/zarr/storage/local.py | 25 +---- src/zarr/storage/logging.py | 13 ++- src/zarr/storage/memory.py | 13 +-- src/zarr/storage/remote.py | 24 ++--- src/zarr/storage/zip.py | 16 +-- src/zarr/testing/store.py | 36 ++++--- src/zarr/testing/strategies.py | 12 ++- tests/test_api.py | 2 +- tests/test_metadata/test_v2.py | 2 +- tests/test_store/test_core.py | 13 +-- tests/test_store/test_local.py | 2 +- tests/test_store/test_logging.py | 2 +- tests/test_store/test_memory.py | 2 +- tests/test_store/test_remote.py | 2 +- tests/test_store/test_stateful_store.py | 12 ++- tests/test_store/test_zip.py | 28 ++++-- 22 files changed, 276 insertions(+), 220 deletions(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index c7f21df50..055fe1b3a 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -3,7 +3,7 @@ from abc import ABC, abstractmethod from asyncio import gather from itertools import starmap -from typing import TYPE_CHECKING, NamedTuple, Protocol, runtime_checkable +from typing import TYPE_CHECKING, Literal, Protocol, runtime_checkable if TYPE_CHECKING: from collections.abc import AsyncGenerator, Iterable @@ -11,51 +11,12 @@ from typing import Any, Self, TypeAlias from zarr.core.buffer import Buffer, BufferPrototype - from zarr.core.common import AccessModeLiteral, BytesLike + from zarr.core.common import BytesLike -__all__ = ["AccessMode", "ByteGetter", "ByteSetter", "Store", "set_or_delete"] +__all__ = ["StoreAccessMode", "ByteGetter", "ByteSetter", "Store", "set_or_delete"] ByteRangeRequest: TypeAlias = tuple[int | None, int | None] - - -class AccessMode(NamedTuple): - """Access mode flags.""" - - str: AccessModeLiteral - readonly: bool - overwrite: bool - create: bool - update: bool - - @classmethod - def from_literal(cls, mode: AccessModeLiteral) -> Self: - """ - Create an AccessMode instance from a literal. - - Parameters - ---------- - mode : AccessModeLiteral - One of 'r', 'r+', 'w', 'w-', 'a'. - - Returns - ------- - AccessMode - The created instance. - - Raises - ------ - ValueError - If mode is not one of 'r', 'r+', 'w', 'w-', 'a'. - """ - if mode in ("r", "r+", "a", "w", "w-"): - return cls( - str=mode, - readonly=mode == "r", - overwrite=mode == "w", - create=mode in ("a", "w", "w-"), - update=mode in ("r+", "a"), - ) - raise ValueError("mode must be one of 'r', 'r+', 'w', 'w-', 'a'") +StoreAccessMode = Literal["r", "w"] class Store(ABC): @@ -63,12 +24,13 @@ class Store(ABC): Abstract base class for Zarr stores. """ - _mode: AccessMode + _mode: StoreAccessMode _is_open: bool - def __init__(self, *args: Any, mode: AccessModeLiteral = "r", **kwargs: Any) -> None: + def __init__(self, *args: Any, mode: StoreAccessMode = "r", **kwargs: Any) -> None: self._is_open = False - self._mode = AccessMode.from_literal(mode) + assert mode in ("r", "w") + self._mode = mode @classmethod async def open(cls, *args: Any, **kwargs: Any) -> Self: @@ -112,19 +74,9 @@ async def _open(self) -> None: ------ ValueError If the store is already open. - FileExistsError - If ``mode='w-'`` and the store already exists. - - Notes - ----- - * When ``mode='w'`` and the store already exists, it will be cleared. """ if self._is_open: raise ValueError("store is already open") - if self.mode.str == "w": - await self.clear() - elif self.mode.str == "w-" and not await self.empty(): - raise FileExistsError("Store already exists") self._is_open = True async def _ensure_open(self) -> None: @@ -132,29 +84,38 @@ async def _ensure_open(self) -> None: if not self._is_open: await self._open() - @abstractmethod - async def empty(self) -> bool: + async def empty(self, prefix: str = "") -> bool: """ - Check if the store is empty. + Check if the directory is empty. + + Parameters + ---------- + prefix : str + Prefix of keys to check. Returns ------- bool True if the store is empty, False otherwise. """ - ... - @abstractmethod + if not prefix.endswith("/"): + prefix += "/" + async for _ in self.list_prefix(prefix): + return False + return True + async def clear(self) -> None: """ Clear the store. Remove all keys and values from the store. """ - ... + async for key in self.list(): + await self.delete(key) @abstractmethod - def with_mode(self, mode: AccessModeLiteral) -> Self: + def with_mode(self, mode: StoreAccessMode) -> Self: """ Return a new store of the same type pointing to the same location with a new mode. @@ -163,7 +124,7 @@ def with_mode(self, mode: AccessModeLiteral) -> Self: Parameters ---------- - mode : AccessModeLiteral + mode : StoreAccessMode The new mode to use. Returns @@ -179,14 +140,19 @@ def with_mode(self, mode: AccessModeLiteral) -> Self: ... @property - def mode(self) -> AccessMode: + def mode(self) -> StoreAccessMode: """Access mode of the store.""" return self._mode + @property + def readonly(self) -> bool: + """Is the store read-only?""" + return self.mode == "r" + def _check_writable(self) -> None: """Raise an exception if the store is not writable.""" - if self.mode.readonly: - raise ValueError("store mode does not support writing") + if self.mode != "w": + raise ValueError(f"store mode ({self.mode}) does not support writing") @abstractmethod def __eq__(self, value: object) -> bool: diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index cd8c3543c..69c44611c 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -8,7 +8,6 @@ import numpy as np import numpy.typing as npt -from zarr.abc.store import Store from zarr.core.array import Array, AsyncArray, get_array_metadata from zarr.core.common import ( JSON, @@ -23,7 +22,6 @@ from zarr.errors import NodeTypeValidationError from zarr.storage import ( StoreLike, - StorePath, make_store_path, ) @@ -31,6 +29,7 @@ from collections.abc import Iterable from zarr.abc.codec import Codec + from zarr.abc.store import StoreAccessMode from zarr.core.buffer import NDArrayLike from zarr.core.chunk_key_encodings import ChunkKeyEncoding @@ -66,6 +65,31 @@ "zeros_like", ] +# Persistence mode: 'r' means read only (must exist); 'r+' means +# read/write (must exist); 'a' means read/write (create if doesn't +# exist); 'w' means create (overwrite if exists); 'w-' means create +# (fail if exists). +persistence_to_store_modes: dict[AccessModeLiteral, StoreAccessMode] = { + "r": "r", + "r+": "w", + "a": "w", + "w": "w", + "w-": "w", +} + + +def _handle_store_mode(mode: AccessModeLiteral | None) -> StoreAccessMode: + if mode is None: + return "r" + else: + return persistence_to_store_modes[mode] + + +def _infer_exists_ok(mode: AccessModeLiteral) -> bool: + if mode in ("a", "r+", "w"): + return True + return False + def _get_shape_chunks(a: ArrayLike | Any) -> tuple[ChunkCoords | None, ChunkCoords | None]: """Helper function to get the shape and chunks from an array-like object""" @@ -252,7 +276,7 @@ async def load( async def open( *, store: StoreLike | None = None, - mode: AccessModeLiteral | None = None, # type and value changed + mode: AccessModeLiteral = "a", zarr_version: ZarrFormat | None = None, # deprecated zarr_format: ZarrFormat | None = None, path: str | None = None, @@ -288,9 +312,14 @@ async def open( """ zarr_format = _handle_zarr_version_or_format(zarr_version=zarr_version, zarr_format=zarr_format) - store_path = await make_store_path(store, mode=mode, path=path, storage_options=storage_options) + store_mode = _handle_store_mode(mode) + store_path = await make_store_path( + store, mode=store_mode, path=path, storage_options=storage_options + ) + await store_path._init(mode=mode) - if "shape" not in kwargs and mode in {"a", "w", "w-"}: + # TODO: the mode check below seems wrong! + if "shape" not in kwargs and mode in {"a", "r", "r+"}: try: metadata_dict = await get_array_metadata(store_path, zarr_format=zarr_format) # TODO: remove this cast when we fix typing for array metadata dicts @@ -300,17 +329,17 @@ async def open( is_v3_array = zarr_format == 3 and _metadata_dict.get("node_type") == "array" if is_v3_array or zarr_format == 2: return AsyncArray(store_path=store_path, metadata=_metadata_dict) - except (AssertionError, FileNotFoundError): + except (AssertionError, FileNotFoundError, NodeTypeValidationError): pass return await open_group(store=store_path, zarr_format=zarr_format, mode=mode, **kwargs) try: - return await open_array(store=store_path, zarr_format=zarr_format, **kwargs) + return await open_array(store=store_path, zarr_format=zarr_format, mode=mode, **kwargs) except (KeyError, NodeTypeValidationError): # KeyError for a missing key # NodeTypeValidationError for failing to parse node metadata as an array when it's # actually a group - return await open_group(store=store_path, zarr_format=zarr_format, **kwargs) + return await open_group(store=store_path, zarr_format=zarr_format, mode=mode, **kwargs) async def open_consolidated( @@ -394,8 +423,12 @@ async def save_array( or _default_zarr_version() ) - mode = kwargs.pop("mode", None) - store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options) + mode = kwargs.pop("mode", "a") + store_mode = _handle_store_mode(mode) + store_path = await make_store_path( + store, path=path, mode=store_mode, storage_options=storage_options + ) + await store_path._init(mode=mode) if np.isscalar(arr): arr = np.array(arr) shape = arr.shape @@ -406,6 +439,7 @@ async def save_array( shape=shape, dtype=arr.dtype, chunks=chunks, + exists_ok=kwargs.pop("exists_ok", None) or _infer_exists_ok(mode), **kwargs, ) await new.setitem(slice(None), arr) @@ -439,6 +473,9 @@ async def save_group( **kwargs NumPy arrays with data to save. """ + + store_path = await make_store_path(store, path=path, mode="w", storage_options=storage_options) + zarr_format = ( _handle_zarr_version_or_format( zarr_version=zarr_version, @@ -453,20 +490,14 @@ async def save_group( for i, arr in enumerate(args): aws.append( save_array( - store, + store_path, arr, zarr_format=zarr_format, - path=f"{path}/arr_{i}", - storage_options=storage_options, + path=f"arr_{i}", ) ) for k, arr in kwargs.items(): - _path = f"{path}/{k}" if path is not None else k - aws.append( - save_array( - store, arr, zarr_format=zarr_format, path=_path, storage_options=storage_options - ) - ) + aws.append(save_array(store_path, arr, zarr_format=zarr_format, path=k)) await asyncio.gather(*aws) @@ -576,9 +607,17 @@ async def group( zarr_format = _handle_zarr_version_or_format(zarr_version=zarr_version, zarr_format=zarr_format) - mode = None if isinstance(store, Store) else cast(AccessModeLiteral, "a") - - store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options) + mode: AccessModeLiteral + if overwrite: + mode = "w" + else: + mode = "r+" + store_mode = _handle_store_mode(mode) + print(f"store_mode: {store_mode}") + store_path = await make_store_path( + store, path=path, mode=store_mode, storage_options=storage_options + ) + await store_path._init(mode=mode) if chunk_store is not None: warnings.warn("chunk_store is not yet implemented", RuntimeWarning, stacklevel=2) @@ -606,7 +645,7 @@ async def group( async def open_group( store: StoreLike | None = None, *, # Note: this is a change from v2 - mode: AccessModeLiteral | None = None, + mode: AccessModeLiteral = "a", cache_attrs: bool | None = None, # not used, default changed synchronizer: Any = None, # not used path: str | None = None, @@ -691,22 +730,31 @@ async def open_group( if chunk_store is not None: warnings.warn("chunk_store is not yet implemented", RuntimeWarning, stacklevel=2) - store_path = await make_store_path(store, mode=mode, storage_options=storage_options, path=path) + store_mode = _handle_store_mode(mode) + exists_ok = _infer_exists_ok(mode) + store_path = await make_store_path( + store, mode=store_mode, storage_options=storage_options, path=path + ) + await store_path._init(mode=mode) if attributes is None: attributes = {} try: - return await AsyncGroup.open( - store_path, zarr_format=zarr_format, use_consolidated=use_consolidated - ) + if mode in {"r", "r+", "a"}: + return await AsyncGroup.open( + store_path, zarr_format=zarr_format, use_consolidated=use_consolidated + ) except (KeyError, FileNotFoundError): + pass + if mode in {"a", "w", "w-"}: return await AsyncGroup.from_store( store_path, zarr_format=zarr_format or _default_zarr_version(), - exists_ok=True, + exists_ok=exists_ok, attributes=attributes, ) + raise FileNotFoundError(f"Unable to find group: {store_path}") async def create( @@ -849,6 +897,8 @@ async def create( warnings.warn("cache_attrs is not yet implemented", RuntimeWarning, stacklevel=2) if object_codec is not None: warnings.warn("object_codec is not yet implemented", RuntimeWarning, stacklevel=2) + if read_only is not None: + warnings.warn("read_only is not yet implemented", RuntimeWarning, stacklevel=2) if dimension_separator is not None: if zarr_format == 3: raise ValueError( @@ -867,10 +917,12 @@ async def create( mode = kwargs.pop("mode", None) if mode is None: - if not isinstance(store, Store | StorePath): - mode = "a" - - store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options) + mode = "a" + store_mode = _handle_store_mode(mode) + store_path = await make_store_path( + store, path=path, mode=store_mode, storage_options=storage_options + ) + await store_path._init(mode) return await AsyncArray.create( store_path, @@ -1049,7 +1101,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 array constructor. + Any keyword arguments to pass to the ``create``. Returns ------- @@ -1058,18 +1110,21 @@ async def open_array( """ mode = kwargs.pop("mode", None) - store_path = await make_store_path(store, path=path, mode=mode) + store_mode = _handle_store_mode(mode) + store_path = await make_store_path( + store, path=path, mode=store_mode, storage_options=storage_options + ) zarr_format = _handle_zarr_version_or_format(zarr_version=zarr_version, zarr_format=zarr_format) try: return await AsyncArray.open(store_path, zarr_format=zarr_format) except FileNotFoundError: - if store_path.store.mode.create: + if not store_path.store.readonly: return await create( store=store_path, zarr_format=zarr_format or _default_zarr_version(), - overwrite=store_path.store.mode.overwrite, + overwrite=_infer_exists_ok(mode), **kwargs, ) raise diff --git a/src/zarr/api/synchronous.py b/src/zarr/api/synchronous.py index 9dcd6fe2d..9616c4135 100644 --- a/src/zarr/api/synchronous.py +++ b/src/zarr/api/synchronous.py @@ -68,7 +68,7 @@ def load( def open( store: StoreLike | None = None, *, - mode: AccessModeLiteral | None = None, # type and value changed + mode: AccessModeLiteral = "a", zarr_version: ZarrFormat | None = None, # deprecated zarr_format: ZarrFormat | None = None, path: str | None = None, @@ -199,8 +199,8 @@ def group( @_deprecate_positional_args def open_group( store: StoreLike | None = None, - *, # Note: this is a change from v2 - mode: AccessModeLiteral | None = None, # not used in async api + *, + mode: AccessModeLiteral = "a", cache_attrs: bool | None = None, # default changed, not used in async api synchronizer: Any = None, # not used in async api path: str | None = None, diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index 34d59bc3d..d8500cc1d 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -772,7 +772,7 @@ def read_only(self) -> bool: True if the array is read-only """ # Backwards compatibility for 2.x - return self.store_path.store.mode.readonly + return self.store_path.store.readonly @property def path(self) -> str: diff --git a/src/zarr/core/group.py b/src/zarr/core/group.py index 86cf191ca..a9985ef2e 100644 --- a/src/zarr/core/group.py +++ b/src/zarr/core/group.py @@ -815,7 +815,7 @@ def store(self) -> Store: @property def read_only(self) -> bool: # Backwards compatibility for 2.x - return self.store_path.store.mode.readonly + return self.store_path.store.readonly @property def synchronizer(self) -> None: diff --git a/src/zarr/storage/common.py b/src/zarr/storage/common.py index 337fbc59a..d00489c0e 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -4,9 +4,9 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Literal -from zarr.abc.store import ByteRangeRequest, Store +from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode from zarr.core.buffer import Buffer, default_buffer_prototype -from zarr.core.common import ZARR_JSON, ZARRAY_JSON, ZGROUP_JSON, ZarrFormat +from zarr.core.common import ZARR_JSON, ZARRAY_JSON, ZGROUP_JSON, AccessModeLiteral, ZarrFormat from zarr.errors import ContainsArrayAndGroupError, ContainsArrayError, ContainsGroupError from zarr.storage._utils import normalize_path from zarr.storage.local import LocalStore @@ -16,7 +16,6 @@ if TYPE_CHECKING: from zarr.core.buffer import BufferPrototype - from zarr.core.common import AccessModeLiteral def _dereference_path(root: str, path: str) -> str: @@ -46,6 +45,36 @@ def __init__(self, store: Store, path: str = "") -> None: self.store = store self.path = path + async def _init(self, mode: AccessModeLiteral) -> None: + """ + Initialize the StorePath based on the provided mode. + + * If the mode is 'w-' and the StorePath contains keys, raise a FileExistsError. + * If the mode is 'w', delete all keys nested within the StorePath + * If the mode is 'a', 'r', or 'r+', do nothing + + Parameters + ---------- + mode : AccessModeLiteral + The mode to use when initializing the store path. + + Raises + ------ + FileExistsError + If the mode is 'w-' and the store path already exists. + """ + match mode: + case "w-": + if not await self.empty(): + raise FileExistsError(f"{self} must be empty. Mode is 'w-'") + case "w": + await self.delete_dir() + case "a" | "r" | "r+": + # No init action + pass + case _: + raise ValueError(f"Invalid mode: {mode}") + async def get( self, prototype: BufferPrototype | None = None, @@ -132,6 +161,17 @@ async def exists(self) -> bool: """ return await self.store.exists(self.path) + async def empty(self) -> bool: + """ + Check if any keys exist in the store with the given prefix. + + Returns + ------- + bool + True if no keys exist in the store with the given prefix, False otherwise. + """ + return await self.store.empty(self.path) + def __truediv__(self, other: str) -> StorePath: """Combine this store path with another path""" return self.__class__(self.store, _dereference_path(self.path, other)) @@ -170,7 +210,7 @@ async def make_store_path( store_like: StoreLike | None, *, path: str | None = "", - mode: AccessModeLiteral | None = None, + mode: StoreAccessMode | None = None, storage_options: dict[str, Any] | None = None, ) -> StorePath: """ @@ -203,7 +243,7 @@ async def make_store_path( path : str | None, optional The path to use when creating the `StorePath` object. If None, the default path is the empty string. - mode : AccessModeLiteral | None, optional + mode : StoreAccessMode | None, optional The mode to use when creating the `StorePath` object. If None, the default mode is 'r'. storage_options : dict[str, Any] | None, optional @@ -224,14 +264,15 @@ async def make_store_path( used_storage_options = False path_normalized = normalize_path(path) + assert mode in (None, "r", "r+", "a", "w", "w-") if isinstance(store_like, StorePath): - if mode is not None and mode != store_like.store.mode.str: + if mode is not None and mode != store_like.store.mode: _store = store_like.store.with_mode(mode) await _store._ensure_open() store_like = StorePath(_store, path=store_like.path) result = store_like / path_normalized elif isinstance(store_like, Store): - if mode is not None and mode != store_like.mode.str: + if mode is not None and mode != store_like.mode: store_like = store_like.with_mode(mode) await store_like._ensure_open() result = StorePath(store_like, path=path_normalized) diff --git a/src/zarr/storage/local.py b/src/zarr/storage/local.py index 331c9857c..ff16a8941 100644 --- a/src/zarr/storage/local.py +++ b/src/zarr/storage/local.py @@ -2,12 +2,11 @@ import asyncio import io -import os import shutil from pathlib import Path from typing import TYPE_CHECKING, Self -from zarr.abc.store import ByteRangeRequest, Store +from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode from zarr.core.buffer import Buffer from zarr.core.common import concurrent_map @@ -15,7 +14,6 @@ from collections.abc import AsyncGenerator, Iterable from zarr.core.buffer import BufferPrototype - from zarr.core.common import AccessModeLiteral def _get( @@ -75,7 +73,7 @@ class LocalStore(Store): root : str or Path Directory to use as root of store. mode : str - Mode in which to open the store. Either 'r', 'r+', 'a', 'w', 'w-'. + Mode in which to open the store. Either 'r', or 'w'. Attributes ---------- @@ -93,7 +91,7 @@ class LocalStore(Store): root: Path - def __init__(self, root: Path | str, *, mode: AccessModeLiteral = "r") -> None: + def __init__(self, root: Path | str, *, mode: StoreAccessMode = "r") -> None: super().__init__(mode=mode) if isinstance(root, str): root = Path(root) @@ -104,7 +102,7 @@ def __init__(self, root: Path | str, *, mode: AccessModeLiteral = "r") -> None: self.root = root async def _open(self) -> None: - if not self.mode.readonly: + if self.mode == "w": self.root.mkdir(parents=True, exist_ok=True) return await super()._open() @@ -114,20 +112,7 @@ async def clear(self) -> None: shutil.rmtree(self.root) self.root.mkdir() - async def empty(self) -> bool: - # docstring inherited - try: - with os.scandir(self.root) as it: - for entry in it: - if entry.is_file(): - # stop once a file is found - return False - except FileNotFoundError: - return True - else: - return True - - def with_mode(self, mode: AccessModeLiteral) -> Self: + def with_mode(self, mode: StoreAccessMode) -> Self: # docstring inherited return type(self)(root=self.root, mode=mode) diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index be259579e..c07aff13d 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -7,13 +7,12 @@ from contextlib import contextmanager from typing import TYPE_CHECKING, Any, Self -from zarr.abc.store import AccessMode, ByteRangeRequest, Store +from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode if TYPE_CHECKING: from collections.abc import AsyncGenerator, Generator, Iterable from zarr.core.buffer import Buffer, BufferPrototype - from zarr.core.common import AccessModeLiteral class LoggingStore(Store): @@ -114,9 +113,9 @@ def supports_listing(self) -> bool: return self._store.supports_listing @property - def _mode(self) -> AccessMode: # type: ignore[override] + def mode(self) -> StoreAccessMode: with self.log(): - return self._store._mode + return self._store.mode @property def _is_open(self) -> bool: @@ -136,10 +135,10 @@ async def _ensure_open(self) -> None: with self.log(): return await self._store._ensure_open() - async def empty(self) -> bool: + async def empty(self, prefix: str = "") -> bool: # docstring inherited with self.log(): - return await self._store.empty() + return await self._store.empty(prefix=prefix) async def clear(self) -> None: # docstring inherited @@ -227,7 +226,7 @@ async def delete_dir(self, prefix: str) -> None: with self.log(prefix): await self._store.delete_dir(prefix=prefix) - def with_mode(self, mode: AccessModeLiteral) -> Self: + def with_mode(self, mode: StoreAccessMode) -> Self: # docstring inherited with self.log(mode): return type(self)( diff --git a/src/zarr/storage/memory.py b/src/zarr/storage/memory.py index fa4ede2a8..5164be0ca 100644 --- a/src/zarr/storage/memory.py +++ b/src/zarr/storage/memory.py @@ -3,7 +3,7 @@ from logging import getLogger from typing import TYPE_CHECKING, Self -from zarr.abc.store import ByteRangeRequest, Store +from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode from zarr.core.buffer import Buffer, gpu from zarr.core.common import concurrent_map from zarr.storage._utils import _normalize_interval_index @@ -12,7 +12,6 @@ from collections.abc import AsyncGenerator, Iterable, MutableMapping from zarr.core.buffer import BufferPrototype - from zarr.core.common import AccessModeLiteral logger = getLogger(__name__) @@ -48,22 +47,18 @@ def __init__( self, store_dict: MutableMapping[str, Buffer] | None = None, *, - mode: AccessModeLiteral = "r", + mode: StoreAccessMode = "r", ) -> None: super().__init__(mode=mode) if store_dict is None: store_dict = {} self._store_dict = store_dict - async def empty(self) -> bool: - # docstring inherited - return not self._store_dict - async def clear(self) -> None: # docstring inherited self._store_dict.clear() - def with_mode(self, mode: AccessModeLiteral) -> Self: + def with_mode(self, mode: StoreAccessMode) -> Self: # docstring inherited return type(self)(store_dict=self._store_dict, mode=mode) @@ -202,7 +197,7 @@ def __init__( self, store_dict: MutableMapping[str, gpu.Buffer] | None = None, *, - mode: AccessModeLiteral = "r", + mode: StoreAccessMode = "r", ) -> None: super().__init__(store_dict=store_dict, mode=mode) # type: ignore[arg-type] diff --git a/src/zarr/storage/remote.py b/src/zarr/storage/remote.py index ca7a010bd..8e8970bdf 100644 --- a/src/zarr/storage/remote.py +++ b/src/zarr/storage/remote.py @@ -3,7 +3,7 @@ import warnings from typing import TYPE_CHECKING, Any, Self -from zarr.abc.store import ByteRangeRequest, Store +from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode from zarr.storage.common import _dereference_path if TYPE_CHECKING: @@ -12,7 +12,7 @@ from fsspec.asyn import AsyncFileSystem from zarr.core.buffer import Buffer, BufferPrototype - from zarr.core.common import AccessModeLiteral, BytesLike + from zarr.core.common import BytesLike ALLOWED_EXCEPTIONS: tuple[type[Exception], ...] = ( @@ -30,7 +30,7 @@ class RemoteStore(Store): ---------- fs : AsyncFileSystem The Async FSSpec filesystem to use with this store. - mode : AccessModeLiteral + mode : StoreAccessMode The access mode to use. path : str The root path of the store. This should be a relative path and must not include the @@ -77,7 +77,7 @@ class RemoteStore(Store): def __init__( self, fs: AsyncFileSystem, - mode: AccessModeLiteral = "r", + mode: StoreAccessMode = "r", path: str = "/", allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> None: @@ -102,7 +102,7 @@ def __init__( def from_upath( cls, upath: Any, - mode: AccessModeLiteral = "r", + mode: StoreAccessMode = "r", allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> RemoteStore: """ @@ -134,7 +134,7 @@ def from_url( cls, url: str, storage_options: dict[str, Any] | None = None, - mode: AccessModeLiteral = "r", + mode: StoreAccessMode = "r", allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> RemoteStore: """ @@ -184,17 +184,7 @@ async def clear(self) -> None: except FileNotFoundError: pass - async def empty(self) -> bool: - # docstring inherited - - # TODO: it would be nice if we didn't have to list all keys here - # it should be possible to stop after the first key is discovered - try: - return not await self.fs._ls(self.path) - except FileNotFoundError: - return True - - def with_mode(self, mode: AccessModeLiteral) -> Self: + def with_mode(self, mode: StoreAccessMode) -> Self: # docstring inherited return type(self)( fs=self.fs, diff --git a/src/zarr/storage/zip.py b/src/zarr/storage/zip.py index cf9b338cf..85f6fe041 100644 --- a/src/zarr/storage/zip.py +++ b/src/zarr/storage/zip.py @@ -7,7 +7,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Literal, Self -from zarr.abc.store import ByteRangeRequest, Store +from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode from zarr.core.buffer import Buffer, BufferPrototype if TYPE_CHECKING: @@ -68,7 +68,12 @@ def __init__( compression: int = zipfile.ZIP_STORED, allowZip64: bool = True, ) -> None: - super().__init__(mode=mode) + _mode: StoreAccessMode + if mode in ("w", "a", "x"): + _mode = "w" + else: + _mode = "r" + super().__init__(mode=_mode) if isinstance(path, str): path = Path(path) @@ -121,12 +126,7 @@ async def clear(self) -> None: self.path, mode="w", compression=self.compression, allowZip64=self.allowZip64 ) - async def empty(self) -> bool: - # docstring inherited - with self._lock: - return not self._zf.namelist() - - def with_mode(self, mode: ZipStoreAccessModeLiteral) -> Self: # type: ignore[override] + def with_mode(self, mode: StoreAccessMode) -> Self: # docstring inherited raise NotImplementedError("ZipStore cannot be reopened with a new mode.") diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index 3aece0f4a..ccb486bcf 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -1,11 +1,10 @@ import pickle -from typing import Any, Generic, TypeVar, cast +from typing import Any, Generic, TypeVar import pytest -from zarr.abc.store import AccessMode, Store +from zarr.abc.store import Store, StoreAccessMode from zarr.core.buffer import Buffer, default_buffer_prototype -from zarr.core.common import AccessModeLiteral from zarr.core.sync import _collect_aiterator from zarr.storage._utils import _normalize_interval_index from zarr.testing.utils import assert_bytes_equal @@ -40,7 +39,7 @@ async def get(self, store: S, key: str) -> Buffer: @pytest.fixture def store_kwargs(self) -> dict[str, Any]: - return {"mode": "r+"} + return {"mode": "w"} @pytest.fixture async def store(self, store_kwargs: dict[str, Any]) -> Store: @@ -63,27 +62,27 @@ def test_serializable_store(self, store: S) -> None: foo = pickle.dumps(store) assert pickle.loads(foo) == store - def test_store_mode(self, store: S, store_kwargs: dict[str, Any]) -> None: - assert store.mode == AccessMode.from_literal("r+") - assert not store.mode.readonly + def test_store_mode(self, store: S) -> None: + assert store.mode == "w" + assert not store.readonly with pytest.raises(AttributeError): - store.mode = AccessMode.from_literal("w") # type: ignore[misc] + store.mode = "w" # type: ignore[misc] - @pytest.mark.parametrize("mode", ["r", "r+", "a", "w", "w-"]) + @pytest.mark.parametrize("mode", ["r", "w"]) async def test_store_open_mode( - self, store_kwargs: dict[str, Any], mode: AccessModeLiteral + self, store_kwargs: dict[str, Any], mode: StoreAccessMode ) -> None: store_kwargs["mode"] = mode store = await self.store_cls.open(**store_kwargs) assert store._is_open - assert store.mode == AccessMode.from_literal(mode) + assert store.mode == mode async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> None: kwargs = {**store_kwargs, "mode": "r"} store = await self.store_cls.open(**kwargs) - assert store.mode == AccessMode.from_literal("r") - assert store.mode.readonly + assert store.mode == "r" + assert store.readonly # set with pytest.raises(ValueError): @@ -149,7 +148,7 @@ async def test_set(self, store: S, key: str, data: bytes) -> None: """ Ensure that data can be written to the store using the store.set method. """ - assert not store.mode.readonly + assert not store.readonly data_buf = self.buffer_cls.from_bytes(data) await store.set(key, data_buf) observed = await self.get(store, key) @@ -307,12 +306,11 @@ async def test_with_mode(self, store: S) -> None: await self.set(store, "key", self.buffer_cls.from_bytes(data)) assert (await self.get(store, "key")).to_bytes() == data - for mode in ["r", "a"]: - mode = cast(AccessModeLiteral, mode) + modes: list[StoreAccessMode] = ["r", "w"] + for mode in modes: clone = store.with_mode(mode) - # await store.close() await clone._ensure_open() - assert clone.mode == AccessMode.from_literal(mode) + assert clone.mode == mode assert isinstance(clone, type(store)) # earlier writes are visible @@ -326,7 +324,7 @@ async def test_with_mode(self, store: S) -> None: assert result is not None assert result.to_bytes() == data - if mode == "a": + if mode == "w": # writes to clone is visible in the original await clone.set("key-3", self.buffer_cls.from_bytes(data)) result = await clone.get("key-3", default_buffer_prototype()) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index c82e168cf..74b9551c3 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -6,8 +6,10 @@ from hypothesis import given, settings # noqa: F401 from hypothesis.strategies import SearchStrategy +import zarr from zarr.core.array import Array -from zarr.core.group import Group + +# from zarr.core.group import Group from zarr.storage import MemoryStore, StoreLike # Copied from Xarray @@ -134,7 +136,12 @@ def arrays( expected_attrs = {} if attributes is None else attributes array_path = path + ("/" if not path.endswith("/") else "") + name - root = Group.from_store(store, zarr_format=zarr_format) + root = zarr.open_group(store, mode="w") + + # try: + # del root[array_path] + # except KeyError: + # pass a = root.create_array( array_path, @@ -144,6 +151,7 @@ def arrays( attributes=attributes, # compressor=compressor, # FIXME fill_value=fill_value, + # exists_ok=True, # TODO: shouldn't need this! ) assert isinstance(a, Array) diff --git a/tests/test_api.py b/tests/test_api.py index 5b62e3a2f..bca816f4e 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -80,7 +80,7 @@ async def test_open_array(memory_store: MemoryStore) -> None: # open array, read-only store_cls = type(store) ro_store = await store_cls.open(store_dict=store._store_dict, mode="r") - z = open(store=ro_store) + z = open(store=ro_store, mode="r") assert isinstance(z, Array) assert z.shape == (200,) assert z.read_only diff --git a/tests/test_metadata/test_v2.py b/tests/test_metadata/test_v2.py index 089d5c98e..2ff65ef6f 100644 --- a/tests/test_metadata/test_v2.py +++ b/tests/test_metadata/test_v2.py @@ -141,7 +141,7 @@ async def v2_consolidated_metadata( "zarr_consolidated_format": 1, } store_dict = {} - store = zarr.storage.MemoryStore(store_dict=store_dict, mode="a") + store = zarr.storage.MemoryStore(store_dict=store_dict, mode="w") await store.set( ".zattrs", cpu.Buffer.from_bytes(json.dumps({"Conventions": "COARDS"}).encode()) ) diff --git a/tests/test_store/test_core.py b/tests/test_store/test_core.py index 38292e23c..5fc85cc54 100644 --- a/tests/test_store/test_core.py +++ b/tests/test_store/test_core.py @@ -6,6 +6,7 @@ from _pytest.compat import LEGACY_PATH from upath import UPath +from zarr.abc.store import StoreAccessMode from zarr.storage._utils import normalize_path from zarr.storage.common import StoreLike, StorePath, make_store_path from zarr.storage.local import LocalStore @@ -25,12 +26,12 @@ async def test_make_store_path_none(path: str) -> None: @pytest.mark.parametrize("path", [None, "", "bar"]) @pytest.mark.parametrize("store_type", [str, Path, LocalStore]) -@pytest.mark.parametrize("mode", ["r", "w", "a"]) +@pytest.mark.parametrize("mode", ["r", "w"]) async def test_make_store_path_local( tmpdir: LEGACY_PATH, store_type: type[str] | type[Path] | type[LocalStore], path: str, - mode: Literal["r", "w", "a"], + mode: StoreAccessMode, ) -> None: """ Test the various ways of invoking make_store_path that create a LocalStore @@ -40,13 +41,13 @@ async def test_make_store_path_local( assert isinstance(store_path.store, LocalStore) assert Path(store_path.store.root) == Path(tmpdir) assert store_path.path == normalize_path(path) - assert store_path.store.mode.str == mode + assert store_path.store.mode == mode @pytest.mark.parametrize("path", [None, "", "bar"]) -@pytest.mark.parametrize("mode", ["r", "w", "a"]) +@pytest.mark.parametrize("mode", ["r", "w"]) async def test_make_store_path_store_path( - tmpdir: LEGACY_PATH, path: str, mode: Literal["r", "w", "a"] + tmpdir: LEGACY_PATH, path: str, mode: Literal["r", "w"] ) -> None: """ Test invoking make_store_path when the input is another store_path. In particular we want to ensure @@ -59,7 +60,7 @@ async def test_make_store_path_store_path( path_normalized = normalize_path(path) assert store_path.path == (store_like / path_normalized).path - assert store_path.store.mode.str == mode + assert store_path.store.mode == mode async def test_make_store_path_invalid() -> None: diff --git a/tests/test_store/test_local.py b/tests/test_store/test_local.py index 5352e3520..1733ee2df 100644 --- a/tests/test_store/test_local.py +++ b/tests/test_store/test_local.py @@ -28,7 +28,7 @@ async def set(self, store: LocalStore, key: str, value: Buffer) -> None: @pytest.fixture def store_kwargs(self, tmpdir) -> dict[str, str]: - return {"root": str(tmpdir), "mode": "r+"} + return {"root": str(tmpdir), "mode": "w"} def test_store_repr(self, store: LocalStore) -> None: assert str(store) == f"file://{store.root!s}" diff --git a/tests/test_store/test_logging.py b/tests/test_store/test_logging.py index 50db5c1c5..9639b2602 100644 --- a/tests/test_store/test_logging.py +++ b/tests/test_store/test_logging.py @@ -59,5 +59,5 @@ async def test_logging_store_counter(store: Store) -> None: async def test_with_mode(): wrapped = LoggingStore(store=zarr.storage.MemoryStore(mode="w"), log_level="INFO") new = wrapped.with_mode(mode="r") - assert new.mode.str == "r" + assert new.mode == "r" assert new.log_level == "INFO" diff --git a/tests/test_store/test_memory.py b/tests/test_store/test_memory.py index bcd9fc444..ff69e7933 100644 --- a/tests/test_store/test_memory.py +++ b/tests/test_store/test_memory.py @@ -22,7 +22,7 @@ async def get(self, store: MemoryStore, key: str) -> Buffer: def store_kwargs( self, request: pytest.FixtureRequest ) -> dict[str, str | None | dict[str, Buffer]]: - kwargs = {"store_dict": None, "mode": "r+"} + kwargs = {"store_dict": None, "mode": "w"} if request.param is True: kwargs["store_dict"] = {} return kwargs diff --git a/tests/test_store/test_remote.py b/tests/test_store/test_remote.py index 2ad1fd787..7e0a9f1d3 100644 --- a/tests/test_store/test_remote.py +++ b/tests/test_store/test_remote.py @@ -113,7 +113,7 @@ def store_kwargs(self, request) -> dict[str, str | bool]: fs, path = fsspec.url_to_fs( f"s3://{test_bucket_name}", endpoint_url=endpoint_url, anon=False, asynchronous=True ) - return {"fs": fs, "path": path, "mode": "r+"} + return {"fs": fs, "path": path, "mode": "w"} @pytest.fixture def store(self, store_kwargs: dict[str, str | bool]) -> RemoteStore: diff --git a/tests/test_store/test_stateful_store.py b/tests/test_store/test_stateful_store.py index 9ac3bbc3f..af1bc37cc 100644 --- a/tests/test_store/test_stateful_store.py +++ b/tests/test_store/test_stateful_store.py @@ -14,7 +14,7 @@ from hypothesis.strategies import DataObject import zarr -from zarr.abc.store import AccessMode, Store +from zarr.abc.store import Store, StoreAccessMode from zarr.core.buffer import BufferPrototype, cpu, default_buffer_prototype from zarr.storage import LocalStore, ZipStore from zarr.testing.strategies import key_ranges @@ -35,9 +35,13 @@ def __init__(self, store: Store) -> None: self.store = store @property - def mode(self) -> AccessMode: + def mode(self) -> StoreAccessMode: return self.store.mode + @property + def readonly(self) -> bool: + return self.store.readonly + def set(self, key: str, data_buffer: zarr.core.buffer.Buffer) -> None: return self._sync(self.store.set(key, data_buffer)) @@ -117,7 +121,7 @@ def init_store(self): @rule(key=zarr_keys, data=st.binary(min_size=0, max_size=MAX_BINARY_SIZE)) def set(self, key: str, data: DataObject) -> None: note(f"(set) Setting {key!r} with {data}") - assert not self.store.mode.readonly + assert not self.store.readonly data_buf = cpu.Buffer.from_bytes(data) self.store.set(key, data_buf) self.model[key] = data_buf @@ -179,7 +183,7 @@ def delete(self, data: DataObject) -> None: @rule() def clear(self) -> None: - assert not self.store.mode.readonly + assert not self.store.readonly note("(clear)") self.store.clear() self.model.clear() diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 8dee47449..cba80165f 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -2,13 +2,13 @@ import os import tempfile +import zipfile from typing import TYPE_CHECKING import numpy as np import pytest import zarr -from zarr.abc.store import AccessMode from zarr.core.buffer import Buffer, cpu, default_buffer_prototype from zarr.storage.zip import ZipStore from zarr.testing.store import StoreTests @@ -25,6 +25,7 @@ class TestZipStore(StoreTests[ZipStore, cpu.Buffer]): def store_kwargs(self, request) -> dict[str, str | bool]: fd, temp_path = tempfile.mkstemp() os.close(fd) + os.unlink(temp_path) return {"path": temp_path, "mode": "w"} @@ -35,8 +36,8 @@ async def set(self, store: ZipStore, key: str, value: Buffer) -> None: return store._set(key, value) def test_store_mode(self, store: ZipStore, store_kwargs: dict[str, Any]) -> None: - assert store.mode == AccessMode.from_literal(store_kwargs["mode"]) - assert not store.mode.readonly + assert store.mode == "w" + assert not store.readonly async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> None: # we need to create the zipfile in write mode before switching to read mode @@ -45,8 +46,8 @@ async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> kwargs = {**store_kwargs, "mode": "r"} store = await self.store_cls.open(**kwargs) - assert store.mode == AccessMode.from_literal("r") - assert store.mode.readonly + assert store.mode == "r" + assert store.readonly # set with pytest.raises(ValueError): @@ -65,7 +66,7 @@ def test_store_supports_listing(self, store: ZipStore) -> None: assert store.supports_listing def test_api_integration(self, store: ZipStore) -> None: - root = zarr.open_group(store=store) + root = zarr.open_group(store=store, mode="a") data = np.arange(10000, dtype=np.uint16).reshape(100, 100) z = root.create_array( @@ -97,6 +98,19 @@ async def test_with_mode(self, store: ZipStore) -> None: with pytest.raises(NotImplementedError, match="new mode"): await super().test_with_mode(store) - @pytest.mark.parametrize("mode", ["a", "w"]) + @pytest.mark.parametrize("mode", ["r", "w"]) async def test_store_open_mode(self, store_kwargs: dict[str, Any], mode: str) -> None: + if mode == "r": + # create an empty zipfile + with zipfile.ZipFile(store_kwargs["path"], mode="w"): + pass + await super().test_store_open_mode(store_kwargs, mode) + + @pytest.mark.parametrize(("zip_mode", "store_mode"), [("w", "w"), ("a", "w"), ("x", "w")]) + async def test_zip_open_mode_translation( + self, store_kwargs: dict[str, Any], zip_mode: str, store_mode: str + ) -> None: + kws = {**store_kwargs, "mode": zip_mode} + store = await self.store_cls.open(**kws) + assert store.mode == store_mode From 89e853934f3c52945cd02cfff3279d92e8a1db40 Mon Sep 17 00:00:00 2001 From: Joe Hamman Date: Fri, 8 Nov 2024 06:17:30 -0800 Subject: [PATCH 14/25] Apply suggestions from code review Co-authored-by: Davis Bennett --- src/zarr/api/asynchronous.py | 1 - src/zarr/storage/common.py | 8 +++++++- src/zarr/testing/strategies.py | 4 ---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index 9f6721e86..1ebb7566c 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -627,7 +627,6 @@ async def group( else: mode = "r+" store_mode = _handle_store_mode(mode) - print(f"store_mode: {store_mode}") store_path = await make_store_path( store, path=path, mode=store_mode, storage_options=storage_options ) diff --git a/src/zarr/storage/common.py b/src/zarr/storage/common.py index d00489c0e..df8bac6db 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -66,7 +66,13 @@ async def _init(self, mode: AccessModeLiteral) -> None: match mode: case "w-": if not await self.empty(): - raise FileExistsError(f"{self} must be empty. Mode is 'w-'") + msg = ( + f"{self} is not empty, but `mode` is set to 'w-'." + "Either remove the existing objects in storage," + "or set `mode` to a value that handles pre-existing objects" + "in storage, like `a` or `w`." + ) + raise FileExistsError(msg) case "w": await self.delete_dir() case "a" | "r" | "r+": diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 74b9551c3..4a909035c 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -138,10 +138,6 @@ def arrays( array_path = path + ("/" if not path.endswith("/") else "") + name root = zarr.open_group(store, mode="w") - # try: - # del root[array_path] - # except KeyError: - # pass a = root.create_array( array_path, From cb470d466d578d15d25e62e6e2352afb0d5aa249 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Fri, 8 Nov 2024 20:11:41 -0800 Subject: [PATCH 15/25] store -> readonly --- docs/guide/storage.rst | 12 +-- src/zarr/abc/store.py | 64 +++------------- src/zarr/api/asynchronous.py | 60 ++------------- src/zarr/storage/common.py | 98 +++++++++++++----------- src/zarr/storage/local.py | 18 ++--- src/zarr/storage/logging.py | 17 +--- src/zarr/storage/memory.py | 22 +++--- src/zarr/storage/remote.py | 39 ++++------ src/zarr/storage/zip.py | 22 +++--- src/zarr/testing/store.py | 88 ++++++++++----------- src/zarr/testing/strategies.py | 3 +- tests/conftest.py | 18 ++--- tests/test_api.py | 22 +++--- tests/test_array.py | 30 ++++---- tests/test_attributes.py | 4 +- tests/test_buffer.py | 8 +- tests/test_config.py | 6 +- tests/test_group.py | 2 +- tests/test_indexing.py | 4 +- tests/test_metadata/test_consolidated.py | 6 +- tests/test_metadata/test_v2.py | 2 +- tests/test_store/test_core.py | 20 ++--- tests/test_store/test_local.py | 4 +- tests/test_store/test_logging.py | 7 -- tests/test_store/test_memory.py | 2 +- tests/test_store/test_remote.py | 6 +- tests/test_store/test_stateful_store.py | 6 +- tests/test_store/test_zip.py | 27 +++---- tests/test_sync.py | 2 +- tests/test_v2.py | 6 +- 30 files changed, 244 insertions(+), 381 deletions(-) diff --git a/docs/guide/storage.rst b/docs/guide/storage.rst index 0019f993a..64827fdcf 100644 --- a/docs/guide/storage.rst +++ b/docs/guide/storage.rst @@ -2,7 +2,7 @@ Storage ======= Zarr-Python supports multiple storage backends, including: local file systems, -Zip files, remote stores via ``fspec`` (S3, HTTP, etc.), and in-memory stores. In +Zip files, remote stores via ``fsspec`` (S3, HTTP, etc.), and in-memory stores. In Zarr-Python 3, stores must implement the abstract store API from :class:`zarr.abc.store.Store`. @@ -19,9 +19,9 @@ to Zarr's top level API will result in the store being created automatically. .. code-block:: python >>> import zarr - >>> zarr.open("data/foo/bar", mode="r") # implicitly creates a LocalStore + >>> zarr.open("data/foo/bar", mode="r") # implicitly creates a read-only LocalStore - >>> zarr.open("s3://foo/bar", mode="r") # implicitly creates a RemoteStore + >>> zarr.open("s3://foo/bar", mode="r") # implicitly creates a read-only RemoteStore >>> data = {} >>> zarr.open(data, mode="w") # implicitly creates a MemoryStore @@ -43,7 +43,7 @@ filesystem. .. code-block:: python >>> import zarr - >>> store = zarr.storage.LocalStore("data/foo/bar", mode="r") + >>> store = zarr.storage.LocalStore("data/foo/bar", readonly=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", mode="r") + >>> store = zarr.storage.RemoteStore.from_url("gs://foo/bar", readonly=True) >>> zarr.open(store=store) shape=(10, 20) dtype=float32> @@ -86,7 +86,7 @@ Zarr data (metadata and chunks) to a dictionary. >>> import zarr >>> data = {} - >>> store = zarr.storage.MemoryStore(data, mode="w") + >>> store = zarr.storage.MemoryStore(data) >>> zarr.open(store=store, shape=(2, )) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 424214986..191daa698 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -3,7 +3,7 @@ from abc import ABC, abstractmethod from asyncio import gather from itertools import starmap -from typing import TYPE_CHECKING, Literal, Protocol, runtime_checkable +from typing import TYPE_CHECKING, Protocol, runtime_checkable if TYPE_CHECKING: from collections.abc import AsyncGenerator, Iterable @@ -13,10 +13,9 @@ from zarr.core.buffer import Buffer, BufferPrototype from zarr.core.common import BytesLike -__all__ = ["StoreAccessMode", "ByteGetter", "ByteSetter", "Store", "set_or_delete"] +__all__ = ["ByteGetter", "ByteSetter", "Store", "set_or_delete"] ByteRangeRequest: TypeAlias = tuple[int | None, int | None] -StoreAccessMode = Literal["r", "w"] class Store(ABC): @@ -24,13 +23,12 @@ class Store(ABC): Abstract base class for Zarr stores. """ - _mode: StoreAccessMode + _readonly: bool _is_open: bool - def __init__(self, *args: Any, mode: StoreAccessMode = "r", **kwargs: Any) -> None: + def __init__(self, *, readonly: bool = False) -> None: self._is_open = False - assert mode in ("r", "w") - self._mode = mode + self._readonly = readonly @classmethod async def open(cls, *args: Any, **kwargs: Any) -> Self: @@ -99,7 +97,7 @@ async def empty(self, prefix: str = "") -> bool: True if the store is empty, False otherwise. """ - if not prefix.endswith("/"): + if prefix and not prefix.endswith("/"): prefix += "/" async for _ in self.list_prefix(prefix): return False @@ -114,45 +112,15 @@ async def clear(self) -> None: async for key in self.list(): await self.delete(key) - @abstractmethod - def with_mode(self, mode: StoreAccessMode) -> Self: - """ - Return a new store of the same type pointing to the same location with a new mode. - - The returned Store is not automatically opened. Call :meth:`Store.open` before - using. - - Parameters - ---------- - mode : StoreAccessMode - The new mode to use. - - Returns - ------- - store - A new store of the same type with the new mode. - - Examples - -------- - >>> writer = zarr.store.MemoryStore(mode="w") - >>> reader = writer.with_mode("r") - """ - ... - - @property - def mode(self) -> StoreAccessMode: - """Access mode of the store.""" - return self._mode - @property def readonly(self) -> bool: """Is the store read-only?""" - return self.mode == "r" + return self._readonly def _check_writable(self) -> None: """Raise an exception if the store is not writable.""" - if self.mode != "w": - raise ValueError(f"store mode ({self.mode}) does not support writing") + if self.readonly: + raise ValueError("store was opened in read-only mode and does not support writing") @abstractmethod def __eq__(self, value: object) -> bool: @@ -347,20 +315,6 @@ async def delete_dir(self, prefix: str) -> None: async for key in self.list_prefix(prefix): await self.delete(key) - async def delete_dir(self, prefix: str) -> None: - """ - Remove all keys and prefixes in the store that begin with a given prefix. - """ - if not self.supports_deletes: - raise NotImplementedError - if not self.supports_listing: - raise NotImplementedError - self._check_writable() - if not prefix.endswith("/"): - prefix += "/" - async for key in self.list_prefix(prefix): - await self.delete(key) - def close(self) -> None: """Close the store.""" self._is_open = False diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index 1ebb7566c..4c345397e 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -30,8 +30,6 @@ from collections.abc import Iterable from zarr.abc.codec import Codec - from zarr.abc.store import StoreAccessMode - from zarr.core.buffer import NDArrayLike from zarr.core.chunk_key_encodings import ChunkKeyEncoding # TODO: this type could use some more thought @@ -66,30 +64,9 @@ "zeros_like", ] -# Persistence mode: 'r' means read only (must exist); 'r+' means -# read/write (must exist); 'a' means read/write (create if doesn't -# exist); 'w' means create (overwrite if exists); 'w-' means create -# (fail if exists). -persistence_to_store_modes: dict[AccessModeLiteral, StoreAccessMode] = { - "r": "r", - "r+": "w", - "a": "w", - "w": "w", - "w-": "w", -} - - -def _handle_store_mode(mode: AccessModeLiteral | None) -> StoreAccessMode: - if mode is None: - return "r" - else: - return persistence_to_store_modes[mode] - def _infer_exists_ok(mode: AccessModeLiteral) -> bool: - if mode in ("a", "r+", "w"): - return True - return False + return mode in ("a", "r+", "w") def _get_shape_chunks(a: ArrayLike | Any) -> tuple[ChunkCoords | None, ChunkCoords | None]: @@ -313,11 +290,7 @@ async def open( """ zarr_format = _handle_zarr_version_or_format(zarr_version=zarr_version, zarr_format=zarr_format) - store_mode = _handle_store_mode(mode) - store_path = await make_store_path( - store, mode=store_mode, path=path, storage_options=storage_options - ) - await store_path._init(mode=mode) + store_path = await make_store_path(store, mode=mode, path=path, storage_options=storage_options) # TODO: the mode check below seems wrong! if "shape" not in kwargs and mode in {"a", "r", "r+"}: @@ -427,11 +400,7 @@ async def save_array( raise TypeError("arr argument must be numpy or other NDArrayLike array") mode = kwargs.pop("mode", "a") - store_mode = _handle_store_mode(mode) - store_path = await make_store_path( - store, path=path, mode=store_mode, storage_options=storage_options - ) - await store_path._init(mode=mode) + store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options) if np.isscalar(arr): arr = np.array(arr) shape = arr.shape @@ -626,11 +595,7 @@ async def group( mode = "w" else: mode = "r+" - store_mode = _handle_store_mode(mode) - store_path = await make_store_path( - store, path=path, mode=store_mode, storage_options=storage_options - ) - await store_path._init(mode=mode) + store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options) if chunk_store is not None: warnings.warn("chunk_store is not yet implemented", RuntimeWarning, stacklevel=2) @@ -743,12 +708,8 @@ async def open_group( if chunk_store is not None: warnings.warn("chunk_store is not yet implemented", RuntimeWarning, stacklevel=2) - store_mode = _handle_store_mode(mode) exists_ok = _infer_exists_ok(mode) - store_path = await make_store_path( - store, mode=store_mode, storage_options=storage_options, path=path - ) - await store_path._init(mode=mode) + store_path = await make_store_path(store, mode=mode, storage_options=storage_options, path=path) if attributes is None: attributes = {} @@ -931,11 +892,7 @@ async def create( mode = kwargs.pop("mode", None) if mode is None: mode = "a" - store_mode = _handle_store_mode(mode) - store_path = await make_store_path( - store, path=path, mode=store_mode, storage_options=storage_options - ) - await store_path._init(mode) + store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options) return await AsyncArray.create( store_path, shape=shape, @@ -1122,10 +1079,7 @@ async def open_array( """ mode = kwargs.pop("mode", None) - store_mode = _handle_store_mode(mode) - store_path = await make_store_path( - store, path=path, mode=store_mode, storage_options=storage_options - ) + store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options) zarr_format = _handle_zarr_version_or_format(zarr_version=zarr_version, zarr_format=zarr_format) diff --git a/src/zarr/storage/common.py b/src/zarr/storage/common.py index df8bac6db..394b9d63f 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -4,7 +4,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Literal -from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode +from zarr.abc.store import ByteRangeRequest, Store from zarr.core.buffer import Buffer, default_buffer_prototype from zarr.core.common import ZARR_JSON, ZARRAY_JSON, ZGROUP_JSON, AccessModeLiteral, ZarrFormat from zarr.errors import ContainsArrayAndGroupError, ContainsArrayError, ContainsGroupError @@ -45,9 +45,12 @@ def __init__(self, store: Store, path: str = "") -> None: self.store = store self.path = path - async def _init(self, mode: AccessModeLiteral) -> None: + @classmethod + async def open( + cls, store: Store, path: str, mode: AccessModeLiteral | None = None + ) -> StorePath: """ - Initialize the StorePath based on the provided mode. + Open StorePath based on the provided mode. * If the mode is 'w-' and the StorePath contains keys, raise a FileExistsError. * If the mode is 'w', delete all keys nested within the StorePath @@ -63,15 +66,26 @@ async def _init(self, mode: AccessModeLiteral) -> None: FileExistsError If the mode is 'w-' and the store path already exists. """ + + await store._ensure_open() + self = cls(store, path) + + # fastpath if mode is None + if mode is None: + return self + + if store.readonly and mode != "r": + raise ValueError(f"Store is read-only but mode is '{mode}'") + match mode: case "w-": if not await self.empty(): msg = ( - f"{self} is not empty, but `mode` is set to 'w-'." - "Either remove the existing objects in storage," - "or set `mode` to a value that handles pre-existing objects" - "in storage, like `a` or `w`." - ) + f"{self} is not empty, but `mode` is set to 'w-'." + "Either remove the existing objects in storage," + "or set `mode` to a value that handles pre-existing objects" + "in storage, like `a` or `w`." + ) raise FileExistsError(msg) case "w": await self.delete_dir() @@ -81,6 +95,8 @@ async def _init(self, mode: AccessModeLiteral) -> None: case _: raise ValueError(f"Invalid mode: {mode}") + return self + async def get( self, prototype: BufferPrototype | None = None, @@ -216,7 +232,7 @@ async def make_store_path( store_like: StoreLike | None, *, path: str | None = "", - mode: StoreAccessMode | None = None, + mode: AccessModeLiteral | None = None, storage_options: dict[str, Any] | None = None, ) -> StorePath: """ @@ -272,45 +288,35 @@ async def make_store_path( path_normalized = normalize_path(path) assert mode in (None, "r", "r+", "a", "w", "w-") if isinstance(store_like, StorePath): - if mode is not None and mode != store_like.store.mode: - _store = store_like.store.with_mode(mode) - await _store._ensure_open() - store_like = StorePath(_store, path=store_like.path) result = store_like / path_normalized - elif isinstance(store_like, Store): - if mode is not None and mode != store_like.mode: - store_like = store_like.with_mode(mode) - await store_like._ensure_open() - result = StorePath(store_like, path=path_normalized) - elif store_like is None: - # mode = "w" is an exception to the default mode = 'r' - result = StorePath(await MemoryStore.open(mode=mode or "w"), path=path_normalized) - elif isinstance(store_like, Path): - result = StorePath( - await LocalStore.open(root=store_like, mode=mode or "r"), path=path_normalized - ) - elif isinstance(store_like, str): - storage_options = storage_options or {} - - if _is_fsspec_uri(store_like): - used_storage_options = True - result = StorePath( - RemoteStore.from_url(store_like, storage_options=storage_options, mode=mode or "r"), - path=path_normalized, - ) - else: - result = StorePath( - await LocalStore.open(root=Path(store_like), mode=mode or "r"), path=path_normalized - ) - elif isinstance(store_like, dict): - # We deliberate only consider dict[str, Buffer] here, and not arbitrary mutable mappings. - # By only allowing dictionaries, which are in-memory, we know that MemoryStore appropriate. - result = StorePath( - await MemoryStore.open(store_dict=store_like, mode=mode or "r"), path=path_normalized - ) else: - msg = f"Unsupported type for store_like: '{type(store_like).__name__}'" # type: ignore[unreachable] - raise TypeError(msg) + # if mode 'r' was provided, we'll open any new stores as read-only + _readonly = mode == "r" + if isinstance(store_like, Store): + store = store_like + elif store_like is None: + store = await MemoryStore.open(readonly=_readonly) + elif isinstance(store_like, Path): + store = await LocalStore.open(root=store_like, readonly=_readonly) + elif isinstance(store_like, str): + storage_options = storage_options or {} + + if _is_fsspec_uri(store_like): + used_storage_options = True + store = RemoteStore.from_url( + store_like, storage_options=storage_options, readonly=_readonly + ) + else: + store = await LocalStore.open(root=Path(store_like), readonly=_readonly) + elif isinstance(store_like, dict): + # We deliberate only consider dict[str, Buffer] here, and not arbitrary mutable mappings. + # By only allowing dictionaries, which are in-memory, we know that MemoryStore appropriate. + store = await MemoryStore.open(store_dict=store_like, readonly=_readonly) + else: + msg = f"Unsupported type for store_like: '{type(store_like).__name__}'" # type: ignore[unreachable] + raise TypeError(msg) + + result = await StorePath.open(store, path=path_normalized, mode=mode) if storage_options and not used_storage_options: msg = "'storage_options' was provided but unused. 'storage_options' is only used for fsspec filesystem stores." diff --git a/src/zarr/storage/local.py b/src/zarr/storage/local.py index 87c368a9a..f42935ddc 100644 --- a/src/zarr/storage/local.py +++ b/src/zarr/storage/local.py @@ -4,9 +4,9 @@ import io import shutil from pathlib import Path -from typing import TYPE_CHECKING, Self +from typing import TYPE_CHECKING -from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode +from zarr.abc.store import ByteRangeRequest, Store from zarr.core.buffer import Buffer from zarr.core.common import concurrent_map @@ -72,8 +72,8 @@ class LocalStore(Store): ---------- root : str or Path Directory to use as root of store. - mode : str - Mode in which to open the store. Either 'r', or 'w'. + readonly : bool + Whether the store is read-only Attributes ---------- @@ -91,8 +91,8 @@ class LocalStore(Store): root: Path - def __init__(self, root: Path | str, *, mode: StoreAccessMode = "r") -> None: - super().__init__(mode=mode) + def __init__(self, root: Path | str, *, readonly: bool = False) -> None: + super().__init__(readonly=readonly) if isinstance(root, str): root = Path(root) if not isinstance(root, Path): @@ -102,7 +102,7 @@ def __init__(self, root: Path | str, *, mode: StoreAccessMode = "r") -> None: self.root = root async def _open(self) -> None: - if self.mode == "w": + if not self.readonly: self.root.mkdir(parents=True, exist_ok=True) return await super()._open() @@ -112,10 +112,6 @@ async def clear(self) -> None: shutil.rmtree(self.root) self.root.mkdir() - def with_mode(self, mode: StoreAccessMode) -> Self: - # docstring inherited - return type(self)(root=self.root, mode=mode) - def __str__(self) -> str: return f"file://{self.root.as_posix()}" diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index 7ec4cbf66..326ccb2ad 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -5,9 +5,9 @@ import time from collections import defaultdict from contextlib import contextmanager -from typing import TYPE_CHECKING, Any, Self +from typing import TYPE_CHECKING, Any -from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode +from zarr.abc.store import ByteRangeRequest, Store if TYPE_CHECKING: from collections.abc import AsyncGenerator, Generator, Iterable @@ -113,9 +113,9 @@ def supports_listing(self) -> bool: return self._store.supports_listing @property - def mode(self) -> StoreAccessMode: + def readonly(self) -> bool: with self.log(): - return self._store.mode + return self._store.readonly @property def _is_open(self) -> bool: @@ -225,12 +225,3 @@ async def delete_dir(self, prefix: str) -> None: # docstring inherited with self.log(prefix): await self._store.delete_dir(prefix=prefix) - - def with_mode(self, mode: StoreAccessMode) -> Self: - # docstring inherited - with self.log(mode): - return type(self)( - self._store.with_mode(mode), - log_level=self.log_level, - log_handler=self.log_handler, - ) diff --git a/src/zarr/storage/memory.py b/src/zarr/storage/memory.py index b33a2140b..2820eda7c 100644 --- a/src/zarr/storage/memory.py +++ b/src/zarr/storage/memory.py @@ -3,7 +3,7 @@ from logging import getLogger from typing import TYPE_CHECKING, Self -from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode +from zarr.abc.store import ByteRangeRequest, Store from zarr.core.buffer import Buffer, gpu from zarr.core.common import concurrent_map from zarr.storage._utils import _normalize_interval_index @@ -28,8 +28,8 @@ class MemoryStore(Store): ---------- store_dict : dict Initial data - mode : str - Access mode + readonly : readonly + Whether the store is read-only Attributes ---------- @@ -50,9 +50,9 @@ def __init__( self, store_dict: MutableMapping[str, Buffer] | None = None, *, - mode: StoreAccessMode = "r", + readonly: bool = False, ) -> None: - super().__init__(mode=mode) + super().__init__(readonly=readonly) if store_dict is None: store_dict = {} self._store_dict = store_dict @@ -61,10 +61,6 @@ async def clear(self) -> None: # docstring inherited self._store_dict.clear() - def with_mode(self, mode: StoreAccessMode) -> Self: - # docstring inherited - return type(self)(store_dict=self._store_dict, mode=mode) - def __str__(self) -> str: return f"memory://{id(self._store_dict)}" @@ -75,7 +71,7 @@ def __eq__(self, other: object) -> bool: return ( isinstance(other, type(self)) and self._store_dict == other._store_dict - and self.mode == other.mode + and self.readonly == other.readonly ) async def get( @@ -192,6 +188,8 @@ class GpuMemoryStore(MemoryStore): store_dict : MutableMapping, optional A mutable mapping with string keys and :class:`zarr.core.buffer.gpu.Buffer` values. + readonly : bool, optional + Whether to open the store in read-only mode. """ _store_dict: MutableMapping[str, gpu.Buffer] # type: ignore[assignment] @@ -200,9 +198,9 @@ def __init__( self, store_dict: MutableMapping[str, gpu.Buffer] | None = None, *, - mode: StoreAccessMode = "r", + readonly: bool = False, ) -> None: - super().__init__(store_dict=store_dict, mode=mode) # type: ignore[arg-type] + super().__init__(store_dict=store_dict, readonly=readonly) # type: ignore[arg-type] def __str__(self) -> str: return f"gpumemory://{id(self._store_dict)}" diff --git a/src/zarr/storage/remote.py b/src/zarr/storage/remote.py index 9c050f1ff..d0ebbbc94 100644 --- a/src/zarr/storage/remote.py +++ b/src/zarr/storage/remote.py @@ -1,9 +1,9 @@ from __future__ import annotations import warnings -from typing import TYPE_CHECKING, Any, Self +from typing import TYPE_CHECKING, Any -from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode +from zarr.abc.store import ByteRangeRequest, Store from zarr.storage.common import _dereference_path if TYPE_CHECKING: @@ -30,8 +30,8 @@ class RemoteStore(Store): ---------- fs : AsyncFileSystem The Async FSSpec filesystem to use with this store. - mode : StoreAccessMode - The access mode to use. + readonly : bool + Whether the store is read-only path : str The root path of the store. This should be a relative path and must not include the filesystem scheme. @@ -77,11 +77,11 @@ class RemoteStore(Store): def __init__( self, fs: AsyncFileSystem, - mode: StoreAccessMode = "r", + readonly: bool = False, path: str = "/", allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> None: - super().__init__(mode=mode) + super().__init__(readonly=readonly) self.fs = fs self.path = path self.allowed_exceptions = allowed_exceptions @@ -102,7 +102,7 @@ def __init__( def from_upath( cls, upath: Any, - mode: StoreAccessMode = "r", + readonly: bool = False, allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> RemoteStore: """ @@ -112,8 +112,8 @@ def from_upath( ---------- upath : UPath The upath to the root of the store. - mode : str, optional - The mode of the store. Defaults to "r". + readonly : bool + Whether the store is read-only, defaults to False. allowed_exceptions : tuple, optional The exceptions that are allowed to be raised when accessing the store. Defaults to ALLOWED_EXCEPTIONS. @@ -125,7 +125,7 @@ def from_upath( return cls( fs=upath.fs, path=upath.path.rstrip("/"), - mode=mode, + readonly=readonly, allowed_exceptions=allowed_exceptions, ) @@ -134,7 +134,7 @@ def from_url( cls, url: str, storage_options: dict[str, Any] | None = None, - mode: StoreAccessMode = "r", + readonly: bool = False, allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> RemoteStore: """ @@ -146,8 +146,8 @@ def from_url( The URL to the root of the store. storage_options : dict, optional The options to pass to fsspec when creating the filesystem. - mode : str, optional - The mode of the store. Defaults to "r". + readonly : bool + Whether the store is read-only, defaults to False. allowed_exceptions : tuple, optional The exceptions that are allowed to be raised when accessing the store. Defaults to ALLOWED_EXCEPTIONS. @@ -173,7 +173,7 @@ def from_url( # `not path.startswith("http")` is a special case for the http filesystem (¯\_(ツ)_/¯) path = fs._strip_protocol(path) - return cls(fs=fs, path=path, mode=mode, allowed_exceptions=allowed_exceptions) + return cls(fs=fs, path=path, readonly=readonly, allowed_exceptions=allowed_exceptions) async def clear(self) -> None: # docstring inherited @@ -184,15 +184,6 @@ async def clear(self) -> None: except FileNotFoundError: pass - def with_mode(self, mode: StoreAccessMode) -> Self: - # docstring inherited - return type(self)( - fs=self.fs, - mode=mode, - path=self.path, - allowed_exceptions=self.allowed_exceptions, - ) - def __repr__(self) -> str: return f"" @@ -200,7 +191,7 @@ def __eq__(self, other: object) -> bool: return ( isinstance(other, type(self)) and self.path == other.path - and self.mode == other.mode + and self.readonly == other.readonly and self.fs == other.fs ) diff --git a/src/zarr/storage/zip.py b/src/zarr/storage/zip.py index 7a95f857d..ae56a57c8 100644 --- a/src/zarr/storage/zip.py +++ b/src/zarr/storage/zip.py @@ -5,9 +5,9 @@ import time import zipfile from pathlib import Path -from typing import TYPE_CHECKING, Any, Literal, Self +from typing import TYPE_CHECKING, Any, Literal -from zarr.abc.store import ByteRangeRequest, Store, StoreAccessMode +from zarr.abc.store import ByteRangeRequest, Store from zarr.core.buffer import Buffer, BufferPrototype if TYPE_CHECKING: @@ -65,15 +65,11 @@ def __init__( path: Path | str, *, mode: ZipStoreAccessModeLiteral = "r", + readonly: bool = True, compression: int = zipfile.ZIP_STORED, allowZip64: bool = True, ) -> None: - _mode: StoreAccessMode - if mode in ("w", "a", "x"): - _mode = "w" - else: - _mode = "r" - super().__init__(mode=_mode) + super().__init__(readonly=readonly) if isinstance(path, str): path = Path(path) @@ -126,10 +122,6 @@ async def clear(self) -> None: self.path, mode="w", compression=self.compression, allowZip64=self.allowZip64 ) - def with_mode(self, mode: StoreAccessMode) -> Self: - # docstring inherited - raise NotImplementedError("ZipStore cannot be reopened with a new mode.") - def __str__(self) -> str: return f"zip://{self.path}" @@ -217,6 +209,12 @@ async def set_if_not_exists(self, key: str, value: Buffer) -> None: if key not in members: self._set(key, value) + async def delete_dir(self, prefix: str) -> None: + if not prefix.endswith("/"): + prefix += "/" + async for _ in self.list_prefix(prefix): + raise NotImplementedError + async def delete(self, key: str) -> None: # docstring inherited # we choose to only raise NotImplementedError here if the key exists diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index ccb486bcf..83ffd776d 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -3,7 +3,7 @@ import pytest -from zarr.abc.store import Store, StoreAccessMode +from zarr.abc.store import Store from zarr.core.buffer import Buffer, default_buffer_prototype from zarr.core.sync import _collect_aiterator from zarr.storage._utils import _normalize_interval_index @@ -39,7 +39,7 @@ async def get(self, store: S, key: str) -> Buffer: @pytest.fixture def store_kwargs(self) -> dict[str, Any]: - return {"mode": "w"} + return {"readonly": False} @pytest.fixture async def store(self, store_kwargs: dict[str, Any]) -> Store: @@ -62,26 +62,22 @@ def test_serializable_store(self, store: S) -> None: foo = pickle.dumps(store) assert pickle.loads(foo) == store - def test_store_mode(self, store: S) -> None: - assert store.mode == "w" + def test_store_readonly(self, store: S) -> None: assert not store.readonly with pytest.raises(AttributeError): - store.mode = "w" # type: ignore[misc] + store.readonly = False # type: ignore[misc] - @pytest.mark.parametrize("mode", ["r", "w"]) - async def test_store_open_mode( - self, store_kwargs: dict[str, Any], mode: StoreAccessMode - ) -> None: - store_kwargs["mode"] = mode + @pytest.mark.parametrize("readonly", [True, False]) + async def test_store_open_readonly(self, store_kwargs: dict[str, Any], readonly: bool) -> None: + store_kwargs["readonly"] = readonly store = await self.store_cls.open(**store_kwargs) assert store._is_open - assert store.mode == mode + assert store.readonly == readonly async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> None: - kwargs = {**store_kwargs, "mode": "r"} + kwargs = {**store_kwargs, "readonly": True} store = await self.store_cls.open(**kwargs) - assert store.mode == "r" assert store.readonly # set @@ -301,39 +297,39 @@ async def test_list_dir(self, store: S) -> None: keys_observed = await _collect_aiterator(store.list_dir(root + "/")) assert sorted(keys_expected) == sorted(keys_observed) - async def test_with_mode(self, store: S) -> None: - data = b"0000" - await self.set(store, "key", self.buffer_cls.from_bytes(data)) - assert (await self.get(store, "key")).to_bytes() == data - - modes: list[StoreAccessMode] = ["r", "w"] - for mode in modes: - clone = store.with_mode(mode) - await clone._ensure_open() - assert clone.mode == mode - assert isinstance(clone, type(store)) - - # earlier writes are visible - result = await clone.get("key", default_buffer_prototype()) - assert result is not None - assert result.to_bytes() == data - - # writes to original after with_mode is visible - await self.set(store, "key-2", self.buffer_cls.from_bytes(data)) - result = await clone.get("key-2", default_buffer_prototype()) - assert result is not None - assert result.to_bytes() == data - - if mode == "w": - # writes to clone is visible in the original - await clone.set("key-3", self.buffer_cls.from_bytes(data)) - result = await clone.get("key-3", default_buffer_prototype()) - assert result is not None - assert result.to_bytes() == data - - else: - with pytest.raises(ValueError, match="store mode"): - await clone.set("key-3", self.buffer_cls.from_bytes(data)) + # async def test_with_mode(self, store: S) -> None: + # data = b"0000" + # await self.set(store, "key", self.buffer_cls.from_bytes(data)) + # assert (await self.get(store, "key")).to_bytes() == data + + # modes: list[StoreAccessMode] = ["r", "w"] + # for mode in modes: + # clone = store.with_mode(mode) + # await clone._ensure_open() + # assert clone.mode == mode + # assert isinstance(clone, type(store)) + + # # earlier writes are visible + # result = await clone.get("key", default_buffer_prototype()) + # assert result is not None + # assert result.to_bytes() == data + + # # writes to original after with_mode is visible + # await self.set(store, "key-2", self.buffer_cls.from_bytes(data)) + # result = await clone.get("key-2", default_buffer_prototype()) + # assert result is not None + # assert result.to_bytes() == data + + # if mode == "w": + # # writes to clone is visible in the original + # await clone.set("key-3", self.buffer_cls.from_bytes(data)) + # result = await clone.get("key-3", default_buffer_prototype()) + # assert result is not None + # assert result.to_bytes() == data + + # else: + # with pytest.raises(ValueError, match="store mode"): + # await clone.set("key-3", self.buffer_cls.from_bytes(data)) async def test_set_if_not_exists(self, store: S) -> None: key = "k" diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 4a909035c..ac7ce5501 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -64,7 +64,7 @@ def v2_dtypes() -> st.SearchStrategy[np.dtype]: attrs = st.none() | st.dictionaries(_attr_keys, _attr_values) keys = st.lists(node_names, min_size=1).map("/".join) paths = st.just("/") | keys -stores = st.builds(MemoryStore, st.just({}), mode=st.just("w")) +stores = st.builds(MemoryStore, st.just({})) compressors = st.sampled_from([None, "default"]) zarr_formats: st.SearchStrategy[Literal[2, 3]] = st.sampled_from([2, 3]) array_shapes = npst.array_shapes(max_dims=4, min_side=0) @@ -138,7 +138,6 @@ def arrays( array_path = path + ("/" if not path.endswith("/") else "") + name root = zarr.open_group(store, mode="w") - a = root.create_array( array_path, shape=nparray.shape, diff --git a/tests/conftest.py b/tests/conftest.py index 8c66406c9..ec3afeb4d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -28,13 +28,13 @@ async def parse_store( store: Literal["local", "memory", "remote", "zip"], path: str ) -> LocalStore | MemoryStore | RemoteStore | ZipStore: if store == "local": - return await LocalStore.open(path, mode="w") + return await LocalStore.open(path, readonly=False) if store == "memory": - return await MemoryStore.open(mode="w") + return await MemoryStore.open(readonly=False) if store == "remote": - return await RemoteStore.open(url=path, mode="w") + return await RemoteStore.open(url=path, readonly=False) if store == "zip": - return await ZipStore.open(path + "/zarr.zip", mode="w") + return await ZipStore.open(path + "/zarr.zip", readonly=False, mode="w") raise AssertionError @@ -46,28 +46,28 @@ def path_type(request: pytest.FixtureRequest) -> Any: # todo: harmonize this with local_store fixture @pytest.fixture async def store_path(tmpdir: LEGACY_PATH) -> StorePath: - store = await LocalStore.open(str(tmpdir), mode="w") + store = await LocalStore.open(str(tmpdir), readonly=False) return StorePath(store) @pytest.fixture async def local_store(tmpdir: LEGACY_PATH) -> LocalStore: - return await LocalStore.open(str(tmpdir), mode="w") + return await LocalStore.open(str(tmpdir), readonly=False) @pytest.fixture async def remote_store(url: str) -> RemoteStore: - return await RemoteStore.open(url, mode="w") + return await RemoteStore.open(url, readonly=False) @pytest.fixture async def memory_store() -> MemoryStore: - return await MemoryStore.open(mode="w") + return await MemoryStore.open(readonly=False) @pytest.fixture async def zip_store(tmpdir: LEGACY_PATH) -> ZipStore: - return await ZipStore.open(str(tmpdir / "zarr.zip"), mode="w") + return await ZipStore.open(str(tmpdir / "zarr.zip"), mode="w", readonly=False) @pytest.fixture diff --git a/tests/test_api.py b/tests/test_api.py index 0436469f9..66857c4ff 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -72,14 +72,14 @@ async def test_open_array(memory_store: MemoryStore) -> None: # open array, overwrite # store._store_dict = {} - store = MemoryStore(mode="w") + store = MemoryStore() z = open(store=store, shape=200) assert isinstance(z, Array) assert z.shape == (200,) # open array, read-only store_cls = type(store) - ro_store = await store_cls.open(store_dict=store._store_dict, mode="r") + ro_store = await store_cls.open(store_dict=store._store_dict, readonly=True) z = open(store=ro_store, mode="r") assert isinstance(z, Array) assert z.shape == (200,) @@ -106,10 +106,10 @@ async def test_open_group(memory_store: MemoryStore) -> None: # open group, read-only store_cls = type(store) - ro_store = await store_cls.open(store_dict=store._store_dict, mode="r") - g = open_group(store=ro_store) + ro_store = await store_cls.open(store_dict=store._store_dict, readonly=True) + g = open_group(store=ro_store, mode="r") assert isinstance(g, Group) - # assert g.read_only + assert g.read_only @pytest.mark.parametrize("zarr_format", [None, 2, 3]) @@ -965,13 +965,13 @@ def test_tree() -> None: def test_open_positional_args_deprecated() -> None: - store = MemoryStore({}, mode="w") + store = MemoryStore() with pytest.warns(FutureWarning, match="pass"): open(store, "w", shape=(1,)) def test_save_array_positional_args_deprecated() -> None: - store = MemoryStore({}, mode="w") + store = MemoryStore() with warnings.catch_warnings(): warnings.filterwarnings( "ignore", message="zarr_version is deprecated", category=DeprecationWarning @@ -987,20 +987,20 @@ def test_save_array_positional_args_deprecated() -> None: def test_group_positional_args_deprecated() -> None: - store = MemoryStore({}, mode="w") + store = MemoryStore() with pytest.warns(FutureWarning, match="pass"): group(store, True) def test_open_group_positional_args_deprecated() -> None: - store = MemoryStore({}, mode="w") + store = MemoryStore() with pytest.warns(FutureWarning, match="pass"): open_group(store, "w") def test_open_falls_back_to_open_group() -> None: # https://github.com/zarr-developers/zarr-python/issues/2309 - store = MemoryStore(mode="w") + store = MemoryStore() zarr.open_group(store, attributes={"key": "value"}) group = zarr.open(store) @@ -1010,7 +1010,7 @@ def test_open_falls_back_to_open_group() -> None: async def test_open_falls_back_to_open_group_async() -> None: # https://github.com/zarr-developers/zarr-python/issues/2309 - store = MemoryStore(mode="w") + store = MemoryStore() await zarr.api.asynchronous.open_group(store, attributes={"key": "value"}) group = await zarr.api.asynchronous.open(store=store) diff --git a/tests/test_array.py b/tests/test_array.py index b8af26133..e4f4df635 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -194,13 +194,13 @@ def test_array_v3_fill_value(store: MemoryStore, fill_value: int, dtype_str: str def test_create_positional_args_deprecated() -> None: - store = MemoryStore({}, mode="w") + store = MemoryStore() with pytest.warns(FutureWarning, match="Pass"): Array.create(store, (2, 2), dtype="f8") def test_selection_positional_args_deprecated() -> None: - store = MemoryStore({}, mode="w") + store = MemoryStore() arr = Array.create(store, shape=(2, 2), dtype="f8") with pytest.warns(FutureWarning, match="Pass out"): @@ -313,7 +313,7 @@ def test_nchunks(test_cls: type[Array] | type[AsyncArray[Any]], nchunks: int) -> """ Test that nchunks returns the number of chunks defined for the array. """ - store = MemoryStore({}, mode="w") + store = MemoryStore() shape = 100 arr = Array.create(store, shape=(shape,), chunks=(ceildiv(shape, nchunks),), dtype="i4") expected = nchunks @@ -329,7 +329,7 @@ async def test_nchunks_initialized(test_cls: type[Array] | type[AsyncArray[Any]] """ Test that nchunks_initialized accurately returns the number of stored chunks. """ - store = MemoryStore({}, mode="w") + store = MemoryStore() arr = Array.create(store, shape=(100,), chunks=(10,), dtype="i4") # write chunks one at a time @@ -357,7 +357,7 @@ async def test_chunks_initialized() -> None: """ Test that chunks_initialized accurately returns the keys of stored chunks. """ - store = MemoryStore({}, mode="w") + store = MemoryStore() arr = Array.create(store, shape=(100,), chunks=(10,), dtype="i4") chunks_accumulated = tuple( @@ -371,34 +371,32 @@ async def test_chunks_initialized() -> None: def test_default_fill_values() -> None: - a = Array.create(MemoryStore({}, mode="w"), shape=5, chunk_shape=5, dtype=" None: with pytest.raises(ValueError, match="At least one ArrayBytesCodec is required."): - Array.create(MemoryStore({}, mode="w"), shape=5, chunk_shape=5, dtype=" None: @pytest.mark.parametrize("zarr_format", [2, 3]) def test_update_attrs(zarr_format: int) -> None: # regression test for https://github.com/zarr-developers/zarr-python/issues/2328 - store = MemoryStore({}, mode="w") + store = MemoryStore() arr = Array.create(store=store, shape=5, chunk_shape=5, dtype="f8", zarr_format=zarr_format) arr.attrs["foo"] = "bar" assert arr.attrs["foo"] == "bar" @@ -637,7 +635,7 @@ def test_array_create_order( ], ) async def test_special_complex_fill_values_roundtrip(fill_value: Any, expected: list[Any]) -> None: - store = MemoryStore({}, mode="w") + store = MemoryStore() Array.create(store=store, shape=(1,), dtype=np.complex64, fill_value=fill_value) content = await store.get("zarr.json", prototype=default_buffer_prototype()) assert content is not None diff --git a/tests/test_attributes.py b/tests/test_attributes.py index 12097eb2b..b26db5df8 100644 --- a/tests/test_attributes.py +++ b/tests/test_attributes.py @@ -4,7 +4,7 @@ def test_put() -> None: - store = zarr.storage.MemoryStore({}, mode="w") + store = zarr.storage.MemoryStore() attrs = zarr.core.attributes.Attributes( zarr.Group.from_store(store, attributes={"a": 1, "b": 2}) ) @@ -14,7 +14,7 @@ def test_put() -> None: def test_asdict() -> None: - store = zarr.storage.MemoryStore({}, mode="w") + store = zarr.storage.MemoryStore() attrs = zarr.core.attributes.Attributes( zarr.Group.from_store(store, attributes={"a": 1, "b": 2}) ) diff --git a/tests/test_buffer.py b/tests/test_buffer.py index 60816d764..7a275516c 100644 --- a/tests/test_buffer.py +++ b/tests/test_buffer.py @@ -48,7 +48,7 @@ async def test_async_array_prototype() -> None: expect = np.zeros((9, 9), dtype="uint16", order="F") a = await AsyncArray.create( - StorePath(StoreExpectingTestBuffer(mode="w")) / "test_async_array_prototype", + StorePath(StoreExpectingTestBuffer()) / "test_async_array_prototype", shape=expect.shape, chunk_shape=(5, 5), dtype=expect.dtype, @@ -77,7 +77,7 @@ async def test_async_array_gpu_prototype() -> None: expect = cp.zeros((9, 9), dtype="uint16", order="F") a = await AsyncArray.create( - StorePath(MemoryStore(mode="w")) / "test_async_array_gpu_prototype", + StorePath(MemoryStore()) / "test_async_array_gpu_prototype", shape=expect.shape, chunk_shape=(5, 5), dtype=expect.dtype, @@ -99,7 +99,7 @@ async def test_async_array_gpu_prototype() -> None: async def test_codecs_use_of_prototype() -> None: expect = np.zeros((10, 10), dtype="uint16", order="F") a = await AsyncArray.create( - StorePath(StoreExpectingTestBuffer(mode="w")) / "test_codecs_use_of_prototype", + StorePath(StoreExpectingTestBuffer()) / "test_codecs_use_of_prototype", shape=expect.shape, chunk_shape=(5, 5), dtype=expect.dtype, @@ -134,7 +134,7 @@ async def test_codecs_use_of_prototype() -> None: async def test_codecs_use_of_gpu_prototype() -> None: expect = cp.zeros((10, 10), dtype="uint16", order="F") a = await AsyncArray.create( - StorePath(MemoryStore(mode="w")) / "test_codecs_use_of_gpu_prototype", + StorePath(MemoryStore()) / "test_codecs_use_of_gpu_prototype", shape=expect.shape, chunk_shape=(5, 5), dtype=expect.dtype, diff --git a/tests/test_config.py b/tests/test_config.py index ddabffb46..2e919a0ad 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -201,7 +201,7 @@ def test_config_buffer_implementation() -> None: # has default value assert fully_qualified_name(get_buffer_class()) == config.defaults[0]["buffer"] - arr = zeros(shape=(100), store=StoreExpectingTestBuffer(mode="w")) + arr = zeros(shape=(100), store=StoreExpectingTestBuffer()) # AssertionError of StoreExpectingTestBuffer when not using my buffer with pytest.raises(AssertionError): @@ -219,7 +219,7 @@ def test_config_buffer_implementation() -> None: data2d = np.arange(1000).reshape(100, 10) arr_sharding = zeros( shape=(100, 10), - store=StoreExpectingTestBuffer(mode="w"), + store=StoreExpectingTestBuffer(), codecs=[ShardingCodec(chunk_shape=(10, 10))], ) arr_sharding[:] = data2d @@ -227,7 +227,7 @@ def test_config_buffer_implementation() -> None: arr_Crc32c = zeros( shape=(100, 10), - store=StoreExpectingTestBuffer(mode="w"), + store=StoreExpectingTestBuffer(), codecs=[BytesCodec(), Crc32cCodec()], ) arr_Crc32c[:] = data2d diff --git a/tests/test_group.py b/tests/test_group.py index 6bacca488..06a596436 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -1355,7 +1355,7 @@ def test_from_dict_extra_fields(self): def test_update_attrs() -> None: # regression test for https://github.com/zarr-developers/zarr-python/issues/2328 root = Group.from_store( - MemoryStore({}, mode="w"), + MemoryStore(), ) root.attrs["foo"] = "bar" assert root.attrs["foo"] == "bar" diff --git a/tests/test_indexing.py b/tests/test_indexing.py index 2c51f3da3..6f80705d9 100644 --- a/tests/test_indexing.py +++ b/tests/test_indexing.py @@ -38,7 +38,7 @@ @pytest.fixture async def store() -> AsyncGenerator[StorePath]: - return StorePath(await MemoryStore.open(mode="w")) + return StorePath(await MemoryStore.open()) def zarr_array_from_numpy_array( @@ -62,7 +62,7 @@ class CountingDict(MemoryStore): @classmethod async def open(cls) -> CountingDict: - store = await super().open(mode="w") + store = await super().open() store.counter = Counter() return store diff --git a/tests/test_metadata/test_consolidated.py b/tests/test_metadata/test_consolidated.py index d9143d09d..f4ac6086d 100644 --- a/tests/test_metadata/test_consolidated.py +++ b/tests/test_metadata/test_consolidated.py @@ -281,7 +281,7 @@ def test_consolidated_sync(self, memory_store): async def test_not_writable_raises(self, memory_store: zarr.storage.MemoryStore) -> None: await group(store=memory_store, attributes={"foo": "bar"}) - read_store = zarr.storage.MemoryStore(store_dict=memory_store._store_dict) + read_store = zarr.storage.MemoryStore(store_dict=memory_store._store_dict, readonly=True) with pytest.raises(ValueError, match="does not support writing"): await consolidate_metadata(read_store) @@ -457,7 +457,7 @@ def test_to_dict_empty(self): @pytest.mark.parametrize("zarr_format", [2, 3]) async def test_open_consolidated_raises_async(self, zarr_format: ZarrFormat): - store = zarr.storage.MemoryStore(mode="w") + store = zarr.storage.MemoryStore() await AsyncGroup.from_store(store, zarr_format=zarr_format) with pytest.raises(ValueError): await zarr.api.asynchronous.open_consolidated(store, zarr_format=zarr_format) @@ -466,7 +466,7 @@ async def test_open_consolidated_raises_async(self, zarr_format: ZarrFormat): await zarr.api.asynchronous.open_consolidated(store, zarr_format=None) async def test_consolidated_metadata_v2(self): - store = zarr.storage.MemoryStore(mode="w") + store = zarr.storage.MemoryStore() g = await AsyncGroup.from_store(store, attributes={"key": "root"}, zarr_format=2) await g.create_array(name="a", shape=(1,), attributes={"key": "a"}) g1 = await g.create_group(name="g1", attributes={"key": "g1"}) diff --git a/tests/test_metadata/test_v2.py b/tests/test_metadata/test_v2.py index 2ff65ef6f..003aef331 100644 --- a/tests/test_metadata/test_v2.py +++ b/tests/test_metadata/test_v2.py @@ -141,7 +141,7 @@ async def v2_consolidated_metadata( "zarr_consolidated_format": 1, } store_dict = {} - store = zarr.storage.MemoryStore(store_dict=store_dict, mode="w") + store = zarr.storage.MemoryStore(store_dict=store_dict) await store.set( ".zattrs", cpu.Buffer.from_bytes(json.dumps({"Conventions": "COARDS"}).encode()) ) diff --git a/tests/test_store/test_core.py b/tests/test_store/test_core.py index 5fc85cc54..e350d648d 100644 --- a/tests/test_store/test_core.py +++ b/tests/test_store/test_core.py @@ -1,12 +1,11 @@ import tempfile from pathlib import Path -from typing import Literal import pytest from _pytest.compat import LEGACY_PATH from upath import UPath -from zarr.abc.store import StoreAccessMode +from zarr.core.common import AccessModeLiteral from zarr.storage._utils import normalize_path from zarr.storage.common import StoreLike, StorePath, make_store_path from zarr.storage.local import LocalStore @@ -25,13 +24,13 @@ async def test_make_store_path_none(path: str) -> None: @pytest.mark.parametrize("path", [None, "", "bar"]) -@pytest.mark.parametrize("store_type", [str, Path, LocalStore]) +@pytest.mark.parametrize("store_type", [str, Path]) @pytest.mark.parametrize("mode", ["r", "w"]) async def test_make_store_path_local( tmpdir: LEGACY_PATH, store_type: type[str] | type[Path] | type[LocalStore], path: str, - mode: StoreAccessMode, + mode: AccessModeLiteral, ) -> None: """ Test the various ways of invoking make_store_path that create a LocalStore @@ -41,26 +40,27 @@ async def test_make_store_path_local( assert isinstance(store_path.store, LocalStore) assert Path(store_path.store.root) == Path(tmpdir) assert store_path.path == normalize_path(path) - assert store_path.store.mode == mode + assert store_path.store.readonly == (mode == "r") @pytest.mark.parametrize("path", [None, "", "bar"]) @pytest.mark.parametrize("mode", ["r", "w"]) async def test_make_store_path_store_path( - tmpdir: LEGACY_PATH, path: str, mode: Literal["r", "w"] + tmpdir: LEGACY_PATH, path: str, mode: AccessModeLiteral ) -> None: """ Test invoking make_store_path when the input is another store_path. In particular we want to ensure that a new path is handled correctly. """ - store_like = StorePath(LocalStore(str(tmpdir)), path="root") + ro = mode == "r" + store_like = await StorePath.open(LocalStore(str(tmpdir), readonly=ro), path="root", mode=mode) store_path = await make_store_path(store_like, path=path, mode=mode) assert isinstance(store_path.store, LocalStore) assert Path(store_path.store.root) == Path(tmpdir) path_normalized = normalize_path(path) assert store_path.path == (store_like / path_normalized).path - assert store_path.store.mode == mode + assert store_path.store.readonly == (mode == "r") async def test_make_store_path_invalid() -> None: @@ -82,8 +82,8 @@ async def test_make_store_path_fsspec(monkeypatch) -> None: None, str(tempfile.TemporaryDirectory()), Path(tempfile.TemporaryDirectory().name), - StorePath(store=MemoryStore(store_dict={}, mode="w"), path="/"), - MemoryStore(store_dict={}, mode="w"), + StorePath(store=MemoryStore(store_dict={}), path="/"), + MemoryStore(store_dict={}), {}, ], ) diff --git a/tests/test_store/test_local.py b/tests/test_store/test_local.py index 42cb6d63a..95cdfd019 100644 --- a/tests/test_store/test_local.py +++ b/tests/test_store/test_local.py @@ -28,7 +28,7 @@ async def set(self, store: LocalStore, key: str, value: Buffer) -> None: @pytest.fixture def store_kwargs(self, tmpdir) -> dict[str, str]: - return {"root": str(tmpdir), "mode": "w"} + return {"root": str(tmpdir)} def test_store_repr(self, store: LocalStore) -> None: assert str(store) == f"file://{store.root.as_posix()}" @@ -51,5 +51,5 @@ def test_creates_new_directory(self, tmp_path: pathlib.Path): target = tmp_path.joinpath("a", "b", "c") assert not target.exists() - store = self.store_cls(root=target, mode="w") + store = self.store_cls(root=target) zarr.group(store=store) diff --git a/tests/test_store/test_logging.py b/tests/test_store/test_logging.py index 9639b2602..aed1f06fa 100644 --- a/tests/test_store/test_logging.py +++ b/tests/test_store/test_logging.py @@ -54,10 +54,3 @@ async def test_logging_store_counter(store: Store) -> None: else: assert wrapped.counter["get"] == 1 assert wrapped.counter["delete_dir"] == 0 - - -async def test_with_mode(): - wrapped = LoggingStore(store=zarr.storage.MemoryStore(mode="w"), log_level="INFO") - new = wrapped.with_mode(mode="r") - assert new.mode == "r" - assert new.log_level == "INFO" diff --git a/tests/test_store/test_memory.py b/tests/test_store/test_memory.py index ff69e7933..f1aa804d3 100644 --- a/tests/test_store/test_memory.py +++ b/tests/test_store/test_memory.py @@ -22,7 +22,7 @@ async def get(self, store: MemoryStore, key: str) -> Buffer: def store_kwargs( self, request: pytest.FixtureRequest ) -> dict[str, str | None | dict[str, Buffer]]: - kwargs = {"store_dict": None, "mode": "w"} + kwargs = {"store_dict": None} if request.param is True: kwargs["store_dict"] = {} return kwargs diff --git a/tests/test_store/test_remote.py b/tests/test_store/test_remote.py index 7e0a9f1d3..82956afb9 100644 --- a/tests/test_store/test_remote.py +++ b/tests/test_store/test_remote.py @@ -87,7 +87,6 @@ def s3(s3_base: None) -> Generator[s3fs.S3FileSystem, None, None]: async def test_basic() -> None: store = RemoteStore.from_url( f"s3://{test_bucket_name}/foo/spam/", - mode="w", storage_options={"endpoint_url": endpoint_url, "anon": False}, ) assert store.fs.asynchronous @@ -113,7 +112,7 @@ def store_kwargs(self, request) -> dict[str, str | bool]: fs, path = fsspec.url_to_fs( f"s3://{test_bucket_name}", endpoint_url=endpoint_url, anon=False, asynchronous=True ) - return {"fs": fs, "path": path, "mode": "w"} + return {"fs": fs, "path": path} @pytest.fixture def store(self, store_kwargs: dict[str, str | bool]) -> RemoteStore: @@ -206,13 +205,12 @@ def test_init_warns_if_fs_asynchronous_is_false(self) -> None: fs, path = fsspec.url_to_fs( f"s3://{test_bucket_name}", endpoint_url=endpoint_url, anon=False, asynchronous=False ) - store_kwargs = {"fs": fs, "path": path, "mode": "r+"} + store_kwargs = {"fs": fs, "path": path} with pytest.warns(UserWarning, match=r".* was not created with `asynchronous=True`.*"): self.store_cls(**store_kwargs) async def test_empty_nonexistent_path(self, store_kwargs) -> None: # regression test for https://github.com/zarr-developers/zarr-python/pull/2343 - store_kwargs["mode"] = "w-" store_kwargs["path"] += "/abc" store = await self.store_cls.open(**store_kwargs) assert await store.empty() diff --git a/tests/test_store/test_stateful_store.py b/tests/test_store/test_stateful_store.py index af1bc37cc..67d9e88d5 100644 --- a/tests/test_store/test_stateful_store.py +++ b/tests/test_store/test_stateful_store.py @@ -14,7 +14,7 @@ from hypothesis.strategies import DataObject import zarr -from zarr.abc.store import Store, StoreAccessMode +from zarr.abc.store import Store from zarr.core.buffer import BufferPrototype, cpu, default_buffer_prototype from zarr.storage import LocalStore, ZipStore from zarr.testing.strategies import key_ranges @@ -34,10 +34,6 @@ def __init__(self, store: Store) -> None: """ self.store = store - @property - def mode(self) -> StoreAccessMode: - return self.store.mode - @property def readonly(self) -> bool: return self.store.readonly diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 30f478ac4..ceb54258c 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -27,7 +27,7 @@ def store_kwargs(self, request) -> dict[str, str | bool]: os.close(fd) os.unlink(temp_path) - return {"path": temp_path, "mode": "w"} + return {"path": temp_path, "mode": "w", "readonly": False} async def get(self, store: ZipStore, key: str) -> Buffer: return store._get(key, prototype=default_buffer_prototype()) @@ -35,8 +35,7 @@ async def get(self, store: ZipStore, key: str) -> Buffer: async def set(self, store: ZipStore, key: str, value: Buffer) -> None: return store._set(key, value) - def test_store_mode(self, store: ZipStore, store_kwargs: dict[str, Any]) -> None: - assert store.mode == "w" + def test_store_readonly(self, store: ZipStore, store_kwargs: dict[str, Any]) -> None: assert not store.readonly async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> None: @@ -44,9 +43,9 @@ async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> store = await self.store_cls.open(**store_kwargs) store.close() - kwargs = {**store_kwargs, "mode": "r"} + kwargs = {**store_kwargs, "mode": "a", "readonly": True} store = await self.store_cls.open(**kwargs) - assert store.mode == "r" + assert store._zmode == "a" assert store.readonly # set @@ -94,23 +93,19 @@ def test_api_integration(self, store: ZipStore) -> None: store.close() - async def test_with_mode(self, store: ZipStore) -> None: - with pytest.raises(NotImplementedError, match="new mode"): - await super().test_with_mode(store) - - @pytest.mark.parametrize("mode", ["r", "w"]) - async def test_store_open_mode(self, store_kwargs: dict[str, Any], mode: str) -> None: - if mode == "r": + @pytest.mark.parametrize("readonly", [True, False]) + async def test_store_open_readonly(self, store_kwargs: dict[str, Any], readonly: bool) -> None: + if readonly == "r": # create an empty zipfile with zipfile.ZipFile(store_kwargs["path"], mode="w"): pass - await super().test_store_open_mode(store_kwargs, mode) + await super().test_store_open_readonly(store_kwargs, readonly) - @pytest.mark.parametrize(("zip_mode", "store_mode"), [("w", "w"), ("a", "w"), ("x", "w")]) + @pytest.mark.parametrize(("zip_mode", "readonly"), [("w", False), ("a", False), ("x", False)]) async def test_zip_open_mode_translation( - self, store_kwargs: dict[str, Any], zip_mode: str, store_mode: str + self, store_kwargs: dict[str, Any], zip_mode: str, readonly: bool ) -> None: kws = {**store_kwargs, "mode": zip_mode} store = await self.store_cls.open(**kws) - assert store.mode == store_mode + assert store.readonly == readonly diff --git a/tests/test_sync.py b/tests/test_sync.py index bff3837e2..02b3b594f 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -142,7 +142,7 @@ def bar(self) -> list[int]: def test_open_positional_args_deprecate(): - store = MemoryStore({}, mode="w") + store = MemoryStore() with pytest.warns(FutureWarning, match="pass"): zarr.open(store, "w", shape=(1,)) diff --git a/tests/test_v2.py b/tests/test_v2.py index 3dd17848f..94aa513ce 100644 --- a/tests/test_v2.py +++ b/tests/test_v2.py @@ -16,7 +16,7 @@ @pytest.fixture async def store() -> Iterator[StorePath]: - return StorePath(await MemoryStore.open(mode="w")) + return StorePath(await MemoryStore.open()) def test_simple(store: StorePath) -> None: @@ -63,7 +63,7 @@ def test_implicit_fill_value(store: StorePath, dtype: str, fill_value: Any) -> N def test_codec_pipeline() -> None: # https://github.com/zarr-developers/zarr-python/issues/2243 - store = MemoryStore(mode="w") + store = MemoryStore() array = zarr.create( store=store, shape=(1,), @@ -80,7 +80,7 @@ def test_codec_pipeline() -> None: @pytest.mark.parametrize("dtype", ["|S", "|V"]) async def test_v2_encode_decode(dtype): - store = zarr.storage.MemoryStore(mode="w") + store = zarr.storage.MemoryStore() g = zarr.group(store=store, zarr_format=2) g.create_array( name="foo", From 10f0d971e78ad498f5dbc1bc6b645d8805496132 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Fri, 8 Nov 2024 22:26:12 -0800 Subject: [PATCH 16/25] fixups --- src/zarr/abc/store.py | 4 +-- src/zarr/storage/common.py | 10 ++++-- src/zarr/storage/logging.py | 4 +-- src/zarr/storage/memory.py | 3 -- src/zarr/storage/zip.py | 8 ++++- src/zarr/testing/store.py | 48 +++++-------------------- src/zarr/testing/strategies.py | 2 -- tests/conftest.py | 14 ++++---- tests/test_store/test_core.py | 4 +-- tests/test_store/test_local.py | 4 +-- tests/test_store/test_remote.py | 2 +- tests/test_store/test_stateful_store.py | 16 ++++----- 12 files changed, 46 insertions(+), 73 deletions(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 191daa698..5d280cd4a 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(self, prefix: str = "") -> bool: + async def empty_dir(self, prefix: str = "") -> bool: """ Check if the directory is empty. @@ -310,7 +310,7 @@ async def delete_dir(self, prefix: str) -> None: if not self.supports_listing: raise NotImplementedError self._check_writable() - if 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/storage/common.py b/src/zarr/storage/common.py index 394b9d63f..922fed339 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -45,6 +45,10 @@ def __init__(self, store: Store, path: str = "") -> None: self.store = store self.path = path + @property + def readonly(self) -> bool: + return self.store.readonly + @classmethod async def open( cls, store: Store, path: str, mode: AccessModeLiteral | None = None @@ -79,7 +83,7 @@ async def open( match mode: case "w-": - if not await self.empty(): + if not await self.empty_dir(): msg = ( f"{self} is not empty, but `mode` is set to 'w-'." "Either remove the existing objects in storage," @@ -183,7 +187,7 @@ async def exists(self) -> bool: """ return await self.store.exists(self.path) - async def empty(self) -> bool: + async def empty_dir(self) -> bool: """ Check if any keys exist in the store with the given prefix. @@ -192,7 +196,7 @@ async def empty(self) -> bool: bool True if no keys exist in the store with the given prefix, False otherwise. """ - return await self.store.empty(self.path) + return await self.store.empty_dir(self.path) def __truediv__(self, other: str) -> StorePath: """Combine this store path with another path""" diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index 326ccb2ad..59c0215cd 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -135,10 +135,10 @@ async def _ensure_open(self) -> None: with self.log(): return await self._store._ensure_open() - async def empty(self, prefix: str = "") -> bool: + async def empty_dir(self, prefix: str = "") -> bool: # docstring inherited with self.log(): - return await self._store.empty(prefix=prefix) + return await self._store.empty_dir(prefix=prefix) async def clear(self) -> None: # docstring inherited diff --git a/src/zarr/storage/memory.py b/src/zarr/storage/memory.py index 2820eda7c..cb959b1ac 100644 --- a/src/zarr/storage/memory.py +++ b/src/zarr/storage/memory.py @@ -17,9 +17,6 @@ logger = getLogger(__name__) -logger = getLogger(__name__) - - class MemoryStore(Store): """ In-memory store for testing purposes. diff --git a/src/zarr/storage/zip.py b/src/zarr/storage/zip.py index ae56a57c8..e56b48ccb 100644 --- a/src/zarr/storage/zip.py +++ b/src/zarr/storage/zip.py @@ -65,10 +65,13 @@ def __init__( path: Path | str, *, mode: ZipStoreAccessModeLiteral = "r", - readonly: bool = True, + readonly: bool | None = None, compression: int = zipfile.ZIP_STORED, allowZip64: bool = True, ) -> None: + if readonly is None: + readonly = mode == "r" + super().__init__(readonly=readonly) if isinstance(path, str): @@ -210,6 +213,8 @@ async def set_if_not_exists(self, key: str, value: Buffer) -> None: self._set(key, value) async def delete_dir(self, prefix: str) -> None: + # only raise NotImplementedError if any keys are found + self._check_writable() if not prefix.endswith("/"): prefix += "/" async for _ in self.list_prefix(prefix): @@ -219,6 +224,7 @@ async def delete(self, key: str) -> None: # docstring inherited # we choose to only raise NotImplementedError here if the key exists # this allows the array/group APIs to avoid the overhead of existence checks + self._check_writable() if await self.exists(key): raise NotImplementedError diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index 83ffd776d..fae9ea7d2 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -75,7 +75,7 @@ async def test_store_open_readonly(self, store_kwargs: dict[str, Any], readonly: assert store._is_open assert store.readonly == readonly - async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> None: + async def test_readonly_store_raises(self, store_kwargs: dict[str, Any]) -> None: kwargs = {**store_kwargs, "readonly": True} store = await self.store_cls.open(**kwargs) assert store.readonly @@ -228,19 +228,21 @@ async def test_delete_dir(self, store: S) -> None: assert not await store.exists("foo/zarr.json") assert not await store.exists("foo/c/0") - async def test_empty(self, store: S) -> None: - assert await store.empty() + async def test_empty_dir(self, store: S) -> None: + assert await store.empty_dir() await self.set( - store, "key", self.buffer_cls.from_bytes(bytes("something", encoding="utf-8")) + store, "foo/bar", self.buffer_cls.from_bytes(bytes("something", encoding="utf-8")) ) - assert not await store.empty() + assert not await store.empty_dir() + assert not await store.empty_dir("foo") + assert await store.empty_dir("spam/") async def test_clear(self, store: S) -> None: await self.set( store, "key", self.buffer_cls.from_bytes(bytes("something", encoding="utf-8")) ) await store.clear() - assert await store.empty() + assert await store.empty_dir() async def test_list(self, store: S) -> None: assert await _collect_aiterator(store.list()) == () @@ -297,40 +299,6 @@ async def test_list_dir(self, store: S) -> None: keys_observed = await _collect_aiterator(store.list_dir(root + "/")) assert sorted(keys_expected) == sorted(keys_observed) - # async def test_with_mode(self, store: S) -> None: - # data = b"0000" - # await self.set(store, "key", self.buffer_cls.from_bytes(data)) - # assert (await self.get(store, "key")).to_bytes() == data - - # modes: list[StoreAccessMode] = ["r", "w"] - # for mode in modes: - # clone = store.with_mode(mode) - # await clone._ensure_open() - # assert clone.mode == mode - # assert isinstance(clone, type(store)) - - # # earlier writes are visible - # result = await clone.get("key", default_buffer_prototype()) - # assert result is not None - # assert result.to_bytes() == data - - # # writes to original after with_mode is visible - # await self.set(store, "key-2", self.buffer_cls.from_bytes(data)) - # result = await clone.get("key-2", default_buffer_prototype()) - # assert result is not None - # assert result.to_bytes() == data - - # if mode == "w": - # # writes to clone is visible in the original - # await clone.set("key-3", self.buffer_cls.from_bytes(data)) - # result = await clone.get("key-3", default_buffer_prototype()) - # assert result is not None - # assert result.to_bytes() == data - - # else: - # with pytest.raises(ValueError, match="store mode"): - # await clone.set("key-3", self.buffer_cls.from_bytes(data)) - async def test_set_if_not_exists(self, store: S) -> None: key = "k" data_buf = self.buffer_cls.from_bytes(b"0000") diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index ac7ce5501..c74fe1862 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -8,8 +8,6 @@ import zarr from zarr.core.array import Array - -# from zarr.core.group import Group from zarr.storage import MemoryStore, StoreLike # Copied from Xarray diff --git a/tests/conftest.py b/tests/conftest.py index ec3afeb4d..3d9ba2ea4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -28,13 +28,13 @@ async def parse_store( store: Literal["local", "memory", "remote", "zip"], path: str ) -> LocalStore | MemoryStore | RemoteStore | ZipStore: if store == "local": - return await LocalStore.open(path, readonly=False) + return await LocalStore.open(path) if store == "memory": return await MemoryStore.open(readonly=False) if store == "remote": - return await RemoteStore.open(url=path, readonly=False) + return await RemoteStore.open(url=path) if store == "zip": - return await ZipStore.open(path + "/zarr.zip", readonly=False, mode="w") + return await ZipStore.open(path + "/zarr.zip", mode="w") raise AssertionError @@ -46,18 +46,18 @@ def path_type(request: pytest.FixtureRequest) -> Any: # todo: harmonize this with local_store fixture @pytest.fixture async def store_path(tmpdir: LEGACY_PATH) -> StorePath: - store = await LocalStore.open(str(tmpdir), readonly=False) + store = await LocalStore.open(str(tmpdir)) return StorePath(store) @pytest.fixture async def local_store(tmpdir: LEGACY_PATH) -> LocalStore: - return await LocalStore.open(str(tmpdir), readonly=False) + return await LocalStore.open(str(tmpdir)) @pytest.fixture async def remote_store(url: str) -> RemoteStore: - return await RemoteStore.open(url, readonly=False) + return await RemoteStore.open(url) @pytest.fixture @@ -67,7 +67,7 @@ async def memory_store() -> MemoryStore: @pytest.fixture async def zip_store(tmpdir: LEGACY_PATH) -> ZipStore: - return await ZipStore.open(str(tmpdir / "zarr.zip"), mode="w", readonly=False) + return await ZipStore.open(str(tmpdir / "zarr.zip"), mode="w") @pytest.fixture diff --git a/tests/test_store/test_core.py b/tests/test_store/test_core.py index e350d648d..5e8cdfd5e 100644 --- a/tests/test_store/test_core.py +++ b/tests/test_store/test_core.py @@ -40,7 +40,7 @@ async def test_make_store_path_local( assert isinstance(store_path.store, LocalStore) assert Path(store_path.store.root) == Path(tmpdir) assert store_path.path == normalize_path(path) - assert store_path.store.readonly == (mode == "r") + assert store_path.readonly == (mode == "r") @pytest.mark.parametrize("path", [None, "", "bar"]) @@ -60,7 +60,7 @@ async def test_make_store_path_store_path( path_normalized = normalize_path(path) assert store_path.path == (store_like / path_normalized).path - assert store_path.store.readonly == (mode == "r") + assert store_path.readonly == ro async def test_make_store_path_invalid() -> None: diff --git a/tests/test_store/test_local.py b/tests/test_store/test_local.py index 95cdfd019..bdb8098c9 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() + assert await store.empty_dir() (store.root / "foo/bar").mkdir(parents=True) - assert await store.empty() + 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 82956afb9..907f7f361 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() + assert await store.empty_dir() diff --git a/tests/test_store/test_stateful_store.py b/tests/test_store/test_stateful_store.py index 67d9e88d5..6ab352ded 100644 --- a/tests/test_store/test_stateful_store.py +++ b/tests/test_store/test_stateful_store.py @@ -55,8 +55,8 @@ def get_partial_values( def delete(self, path: str) -> None: return self._sync(self.store.delete(path)) - def empty(self) -> bool: - return self._sync(self.store.empty()) + def empty_dir(self, prefix: str = "") -> bool: + return self._sync(self.store.empty_dir(prefix=prefix)) def clear(self) -> None: return self._sync(self.store.clear()) @@ -184,18 +184,18 @@ def clear(self) -> None: self.store.clear() self.model.clear() - assert self.store.empty() + assert self.store.empty_dir() assert len(self.model.keys()) == len(list(self.store.list())) == 0 @rule() # Local store can be non-empty when there are subdirectories but no files @precondition(lambda self: not isinstance(self.store.store, LocalStore)) - def empty(self) -> None: - note("(empty)") + def empty_dir(self) -> None: + note("(empty_dir)") # make sure they either both are or both aren't empty (same state) - assert self.store.empty() == (not self.model) + assert self.store.empty_dir() == (not self.model) @rule(key=zarr_keys) def exists(self, key: str) -> None: @@ -228,10 +228,10 @@ def check_zarr_keys(self) -> None: keys = list(self.store.list()) if not keys: - assert self.store.empty() is True + assert self.store.empty_dir() is True else: - assert self.store.empty() is False + assert self.store.empty_dir() is False for key in keys: assert self.store.exists(key) is True From 542da8e57d61ab634f4e8d631e53a27598bd8218 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Fri, 8 Nov 2024 22:36:16 -0800 Subject: [PATCH 17/25] fixups --- src/zarr/api/asynchronous.py | 14 +++++++++----- src/zarr/testing/strategies.py | 1 - tests/conftest.py | 2 +- tests/test_store/test_zip.py | 2 +- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index 4c345397e..acae566fc 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -65,8 +65,13 @@ ] +_READ_MODES: tuple[AccessModeLiteral, ...] = ("r", "r+", "a") +_CREATE_MODES: tuple[AccessModeLiteral, ...] = ("a", "w", "w-") +_OVERWRITE_MODES: tuple[AccessModeLiteral, ...] = ("a", "r+", "w") + + def _infer_exists_ok(mode: AccessModeLiteral) -> bool: - return mode in ("a", "r+", "w") + return mode in _OVERWRITE_MODES def _get_shape_chunks(a: ArrayLike | Any) -> tuple[ChunkCoords | None, ChunkCoords | None]: @@ -708,24 +713,23 @@ async def open_group( if chunk_store is not None: warnings.warn("chunk_store is not yet implemented", RuntimeWarning, stacklevel=2) - exists_ok = _infer_exists_ok(mode) store_path = await make_store_path(store, mode=mode, storage_options=storage_options, path=path) if attributes is None: attributes = {} try: - if mode in {"r", "r+", "a"}: + if mode in _READ_MODES: return await AsyncGroup.open( store_path, zarr_format=zarr_format, use_consolidated=use_consolidated ) except (KeyError, FileNotFoundError): pass - if mode in {"a", "w", "w-"}: + if mode in _CREATE_MODES: return await AsyncGroup.from_store( store_path, zarr_format=zarr_format or _default_zarr_version(), - exists_ok=exists_ok, + exists_ok=_infer_exists_ok(mode), attributes=attributes, ) raise FileNotFoundError(f"Unable to find group: {store_path}") diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index c74fe1862..769d1c2c5 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -144,7 +144,6 @@ def arrays( attributes=attributes, # compressor=compressor, # FIXME fill_value=fill_value, - # exists_ok=True, # TODO: shouldn't need this! ) assert isinstance(a, Array) diff --git a/tests/conftest.py b/tests/conftest.py index 3d9ba2ea4..923c61f0e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -62,7 +62,7 @@ async def remote_store(url: str) -> RemoteStore: @pytest.fixture async def memory_store() -> MemoryStore: - return await MemoryStore.open(readonly=False) + return await MemoryStore.open() @pytest.fixture diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index ceb54258c..e958bed88 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -38,7 +38,7 @@ async def set(self, store: ZipStore, key: str, value: Buffer) -> None: def test_store_readonly(self, store: ZipStore, store_kwargs: dict[str, Any]) -> None: assert not store.readonly - async def test_not_writable_store_raises(self, store_kwargs: dict[str, Any]) -> None: + async def test_readonly_store_raises(self, store_kwargs: dict[str, Any]) -> None: # we need to create the zipfile in write mode before switching to read mode store = await self.store_cls.open(**store_kwargs) store.close() From ed8a8ecdd8c6b0716267e8ff5ab76892a82a4d37 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Fri, 8 Nov 2024 22:37:49 -0800 Subject: [PATCH 18/25] fixups --- src/zarr/api/asynchronous.py | 2 +- src/zarr/core/array.py | 2 +- src/zarr/core/group.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index acae566fc..f3841be08 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -1090,7 +1090,7 @@ async def open_array( try: return await AsyncArray.open(store_path, zarr_format=zarr_format) except FileNotFoundError: - if not store_path.store.readonly: + if not store_path.readonly: return await create( store=store_path, zarr_format=zarr_format or _default_zarr_version(), diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index 72c22611b..870d902c8 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -772,7 +772,7 @@ def read_only(self) -> bool: True if the array is read-only """ # Backwards compatibility for 2.x - return self.store_path.store.readonly + return self.store_path.readonly @property def path(self) -> str: diff --git a/src/zarr/core/group.py b/src/zarr/core/group.py index 683f7b936..62eebe0f0 100644 --- a/src/zarr/core/group.py +++ b/src/zarr/core/group.py @@ -815,7 +815,7 @@ def store(self) -> Store: @property def read_only(self) -> bool: # Backwards compatibility for 2.x - return self.store_path.store.readonly + return self.store_path.readonly @property def synchronizer(self) -> None: From 62939b642457c5f98d4ed487cea29a21962bcb9b Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Sun, 10 Nov 2024 20:08:52 -0800 Subject: [PATCH 19/25] fixup --- tests/test_store/test_memory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_store/test_memory.py b/tests/test_store/test_memory.py index f1aa804d3..56ccb4d3b 100644 --- a/tests/test_store/test_memory.py +++ b/tests/test_store/test_memory.py @@ -62,7 +62,7 @@ async def get(self, store: MemoryStore, key: str) -> Buffer: def store_kwargs( self, request: pytest.FixtureRequest ) -> dict[str, str | None | dict[str, Buffer]]: - kwargs = {"store_dict": None, "mode": "r+"} + kwargs = {"store_dict": None} if request.param is True: kwargs["store_dict"] = {} return kwargs From 980444645bf4030f4d9c990f0f2ca8c33e9cee5a Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 11 Nov 2024 13:33:36 -0800 Subject: [PATCH 20/25] readonly -> read_only --- src/zarr/abc/store.py | 12 ++++----- src/zarr/api/asynchronous.py | 2 +- src/zarr/core/array.py | 2 +- src/zarr/core/group.py | 2 +- src/zarr/storage/common.py | 20 +++++++-------- src/zarr/storage/local.py | 8 +++--- src/zarr/storage/logging.py | 4 +-- src/zarr/storage/memory.py | 14 +++++------ src/zarr/storage/remote.py | 20 +++++++-------- src/zarr/storage/zip.py | 8 +++--- src/zarr/testing/store.py | 26 ++++++++++--------- tests/conftest.py | 2 +- tests/test_api.py | 10 ++++---- tests/test_group.py | 10 +++++--- tests/test_metadata/test_consolidated.py | 2 +- tests/test_store/test_core.py | 6 ++--- tests/test_store/test_stateful_hierarchy.py | 2 +- tests/test_store/test_stateful_store.py | 8 +++--- tests/test_store/test_zip.py | 28 +++++++++++---------- 19 files changed, 97 insertions(+), 89 deletions(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 78b697164..70f67398f 100644 --- a/src/zarr/abc/store.py +++ b/src/zarr/abc/store.py @@ -23,12 +23,12 @@ class Store(ABC): Abstract base class for Zarr stores. """ - _readonly: bool + _read_only: bool _is_open: bool - def __init__(self, *, readonly: bool = False) -> None: + def __init__(self, *, read_only: bool = False) -> None: self._is_open = False - self._readonly = readonly + self._read_only = read_only @classmethod async def open(cls, *args: Any, **kwargs: Any) -> Self: @@ -113,13 +113,13 @@ async def clear(self) -> None: await self.delete(key) @property - def readonly(self) -> bool: + def read_only(self) -> bool: """Is the store read-only?""" - return self._readonly + return self._read_only def _check_writable(self) -> None: """Raise an exception if the store is not writable.""" - if self.readonly: + if self.read_only: raise ValueError("store was opened in read-only mode and does not support writing") @abstractmethod diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py index f3841be08..445f13df9 100644 --- a/src/zarr/api/asynchronous.py +++ b/src/zarr/api/asynchronous.py @@ -1090,7 +1090,7 @@ async def open_array( try: return await AsyncArray.open(store_path, zarr_format=zarr_format) except FileNotFoundError: - if not store_path.readonly: + if not store_path.read_only: return await create( store=store_path, zarr_format=zarr_format or _default_zarr_version(), diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index 5bb3088c3..43caf1653 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -796,7 +796,7 @@ def read_only(self) -> bool: True if the array is read-only """ # Backwards compatibility for 2.x - return self.store_path.readonly + return self.store_path.read_only @property def path(self) -> str: diff --git a/src/zarr/core/group.py b/src/zarr/core/group.py index eec8eaabc..86d5d4794 100644 --- a/src/zarr/core/group.py +++ b/src/zarr/core/group.py @@ -880,7 +880,7 @@ def store(self) -> Store: @property def read_only(self) -> bool: # Backwards compatibility for 2.x - return self.store_path.readonly + return self.store_path.read_only @property def synchronizer(self) -> None: diff --git a/src/zarr/storage/common.py b/src/zarr/storage/common.py index 922fed339..8a97a2be7 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -46,8 +46,8 @@ def __init__(self, store: Store, path: str = "") -> None: self.path = path @property - def readonly(self) -> bool: - return self.store.readonly + def read_only(self) -> bool: + return self.store.read_only @classmethod async def open( @@ -78,7 +78,7 @@ async def open( if mode is None: return self - if store.readonly and mode != "r": + if store.read_only and mode != "r": raise ValueError(f"Store is read-only but mode is '{mode}'") match mode: @@ -290,32 +290,32 @@ async def make_store_path( used_storage_options = False path_normalized = normalize_path(path) - assert mode in (None, "r", "r+", "a", "w", "w-") if isinstance(store_like, StorePath): result = store_like / path_normalized else: + assert mode in (None, "r", "r+", "a", "w", "w-") # if mode 'r' was provided, we'll open any new stores as read-only - _readonly = mode == "r" + _read_only = mode == "r" if isinstance(store_like, Store): store = store_like elif store_like is None: - store = await MemoryStore.open(readonly=_readonly) + store = await MemoryStore.open(read_only=_read_only) elif isinstance(store_like, Path): - store = await LocalStore.open(root=store_like, readonly=_readonly) + store = await LocalStore.open(root=store_like, read_only=_read_only) elif isinstance(store_like, str): storage_options = storage_options or {} if _is_fsspec_uri(store_like): used_storage_options = True store = RemoteStore.from_url( - store_like, storage_options=storage_options, readonly=_readonly + store_like, storage_options=storage_options, read_only=_read_only ) else: - store = await LocalStore.open(root=Path(store_like), readonly=_readonly) + store = await LocalStore.open(root=Path(store_like), read_only=_read_only) elif isinstance(store_like, dict): # We deliberate only consider dict[str, Buffer] here, and not arbitrary mutable mappings. # By only allowing dictionaries, which are in-memory, we know that MemoryStore appropriate. - store = await MemoryStore.open(store_dict=store_like, readonly=_readonly) + store = await MemoryStore.open(store_dict=store_like, read_only=_read_only) else: msg = f"Unsupported type for store_like: '{type(store_like).__name__}'" # type: ignore[unreachable] raise TypeError(msg) diff --git a/src/zarr/storage/local.py b/src/zarr/storage/local.py index 8c302b42b..139ce42ad 100644 --- a/src/zarr/storage/local.py +++ b/src/zarr/storage/local.py @@ -72,7 +72,7 @@ class LocalStore(Store): ---------- root : str or Path Directory to use as root of store. - readonly : bool + read_only : bool Whether the store is read-only Attributes @@ -91,8 +91,8 @@ class LocalStore(Store): root: Path - def __init__(self, root: Path | str, *, readonly: bool = False) -> None: - super().__init__(readonly=readonly) + def __init__(self, root: Path | str, *, read_only: bool = False) -> None: + super().__init__(read_only=read_only) if isinstance(root, str): root = Path(root) if not isinstance(root, Path): @@ -102,7 +102,7 @@ def __init__(self, root: Path | str, *, readonly: bool = False) -> None: self.root = root async def _open(self) -> None: - if not self.readonly: + if not self.read_only: self.root.mkdir(parents=True, exist_ok=True) return await super()._open() diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index 1f94dbde7..7888ebede 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -113,9 +113,9 @@ def supports_listing(self) -> bool: return self._store.supports_listing @property - def readonly(self) -> bool: + def read_only(self) -> bool: with self.log(): - return self._store.readonly + return self._store.read_only @property def _is_open(self) -> bool: diff --git a/src/zarr/storage/memory.py b/src/zarr/storage/memory.py index 34516fc06..e3290cfd4 100644 --- a/src/zarr/storage/memory.py +++ b/src/zarr/storage/memory.py @@ -25,7 +25,7 @@ class MemoryStore(Store): ---------- store_dict : dict Initial data - readonly : readonly + read_only : bool Whether the store is read-only Attributes @@ -47,9 +47,9 @@ def __init__( self, store_dict: MutableMapping[str, Buffer] | None = None, *, - readonly: bool = False, + read_only: bool = False, ) -> None: - super().__init__(readonly=readonly) + super().__init__(read_only=read_only) if store_dict is None: store_dict = {} self._store_dict = store_dict @@ -68,7 +68,7 @@ def __eq__(self, other: object) -> bool: return ( isinstance(other, type(self)) and self._store_dict == other._store_dict - and self.readonly == other.readonly + and self.read_only == other.read_only ) async def get( @@ -185,7 +185,7 @@ class GpuMemoryStore(MemoryStore): store_dict : MutableMapping, optional A mutable mapping with string keys and :class:`zarr.core.buffer.gpu.Buffer` values. - readonly : bool, optional + read_only : bool Whether to open the store in read-only mode. """ @@ -195,9 +195,9 @@ def __init__( self, store_dict: MutableMapping[str, gpu.Buffer] | None = None, *, - readonly: bool = False, + read_only: bool = False, ) -> None: - super().__init__(store_dict=store_dict, readonly=readonly) # type: ignore[arg-type] + super().__init__(store_dict=store_dict, read_only=read_only) # type: ignore[arg-type] def __str__(self) -> str: return f"gpumemory://{id(self._store_dict)}" diff --git a/src/zarr/storage/remote.py b/src/zarr/storage/remote.py index 2b06b6022..c08963bfa 100644 --- a/src/zarr/storage/remote.py +++ b/src/zarr/storage/remote.py @@ -30,7 +30,7 @@ class RemoteStore(Store): ---------- fs : AsyncFileSystem The Async FSSpec filesystem to use with this store. - readonly : bool + read_only : bool Whether the store is read-only path : str The root path of the store. This should be a relative path and must not include the @@ -77,11 +77,11 @@ class RemoteStore(Store): def __init__( self, fs: AsyncFileSystem, - readonly: bool = False, + read_only: bool = False, path: str = "/", allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> None: - super().__init__(readonly=readonly) + super().__init__(read_only=read_only) self.fs = fs self.path = path self.allowed_exceptions = allowed_exceptions @@ -102,7 +102,7 @@ def __init__( def from_upath( cls, upath: Any, - readonly: bool = False, + read_only: bool = False, allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> RemoteStore: """ @@ -112,7 +112,7 @@ def from_upath( ---------- upath : UPath The upath to the root of the store. - readonly : bool + read_only : bool Whether the store is read-only, defaults to False. allowed_exceptions : tuple, optional The exceptions that are allowed to be raised when accessing the @@ -125,7 +125,7 @@ def from_upath( return cls( fs=upath.fs, path=upath.path.rstrip("/"), - readonly=readonly, + read_only=read_only, allowed_exceptions=allowed_exceptions, ) @@ -134,7 +134,7 @@ def from_url( cls, url: str, storage_options: dict[str, Any] | None = None, - readonly: bool = False, + read_only: bool = False, allowed_exceptions: tuple[type[Exception], ...] = ALLOWED_EXCEPTIONS, ) -> RemoteStore: """ @@ -146,7 +146,7 @@ def from_url( The URL to the root of the store. storage_options : dict, optional The options to pass to fsspec when creating the filesystem. - readonly : bool + read_only : bool Whether the store is read-only, defaults to False. allowed_exceptions : tuple, optional The exceptions that are allowed to be raised when accessing the @@ -173,7 +173,7 @@ def from_url( # `not path.startswith("http")` is a special case for the http filesystem (¯\_(ツ)_/¯) path = fs._strip_protocol(path) - return cls(fs=fs, path=path, readonly=readonly, allowed_exceptions=allowed_exceptions) + return cls(fs=fs, path=path, read_only=read_only, allowed_exceptions=allowed_exceptions) async def clear(self) -> None: # docstring inherited @@ -191,7 +191,7 @@ def __eq__(self, other: object) -> bool: return ( isinstance(other, type(self)) and self.path == other.path - and self.readonly == other.readonly + and self.read_only == other.read_only and self.fs == other.fs ) diff --git a/src/zarr/storage/zip.py b/src/zarr/storage/zip.py index 09868962c..2cebab1b3 100644 --- a/src/zarr/storage/zip.py +++ b/src/zarr/storage/zip.py @@ -65,14 +65,14 @@ def __init__( path: Path | str, *, mode: ZipStoreAccessModeLiteral = "r", - readonly: bool | None = None, + read_only: bool | None = None, compression: int = zipfile.ZIP_STORED, allowZip64: bool = True, ) -> None: - if readonly is None: - readonly = mode == "r" + if read_only is None: + read_only = mode == "r" - super().__init__(readonly=readonly) + super().__init__(read_only=read_only) if isinstance(path, str): path = Path(path) diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index fae9ea7d2..c83f71af0 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -39,7 +39,7 @@ async def get(self, store: S, key: str) -> Buffer: @pytest.fixture def store_kwargs(self) -> dict[str, Any]: - return {"readonly": False} + return {"read_only": False} @pytest.fixture async def store(self, store_kwargs: dict[str, Any]) -> Store: @@ -62,23 +62,25 @@ def test_serializable_store(self, store: S) -> None: foo = pickle.dumps(store) assert pickle.loads(foo) == store - def test_store_readonly(self, store: S) -> None: - assert not store.readonly + def test_store_read_only(self, store: S) -> None: + assert not store.read_only with pytest.raises(AttributeError): - store.readonly = False # type: ignore[misc] + store.read_only = False # type: ignore[misc] - @pytest.mark.parametrize("readonly", [True, False]) - async def test_store_open_readonly(self, store_kwargs: dict[str, Any], readonly: bool) -> None: - store_kwargs["readonly"] = readonly + @pytest.mark.parametrize("read_only", [True, False]) + async def test_store_open_read_only( + self, store_kwargs: dict[str, Any], read_only: bool + ) -> None: + store_kwargs["read_only"] = read_only store = await self.store_cls.open(**store_kwargs) assert store._is_open - assert store.readonly == readonly + assert store.read_only == read_only - async def test_readonly_store_raises(self, store_kwargs: dict[str, Any]) -> None: - kwargs = {**store_kwargs, "readonly": True} + async def test_read_only_store_raises(self, store_kwargs: dict[str, Any]) -> None: + kwargs = {**store_kwargs, "read_only": True} store = await self.store_cls.open(**kwargs) - assert store.readonly + assert store.read_only # set with pytest.raises(ValueError): @@ -144,7 +146,7 @@ async def test_set(self, store: S, key: str, data: bytes) -> None: """ Ensure that data can be written to the store using the store.set method. """ - assert not store.readonly + assert not store.read_only data_buf = self.buffer_cls.from_bytes(data) await store.set(key, data_buf) observed = await self.get(store, key) diff --git a/tests/conftest.py b/tests/conftest.py index 923c61f0e..35f31d39b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -30,7 +30,7 @@ async def parse_store( if store == "local": return await LocalStore.open(path) if store == "memory": - return await MemoryStore.open(readonly=False) + return await MemoryStore.open() if store == "remote": return await RemoteStore.open(url=path) if store == "zip": diff --git a/tests/test_api.py b/tests/test_api.py index 66857c4ff..47a1c6e39 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -79,7 +79,7 @@ async def test_open_array(memory_store: MemoryStore) -> None: # open array, read-only store_cls = type(store) - ro_store = await store_cls.open(store_dict=store._store_dict, readonly=True) + ro_store = await store_cls.open(store_dict=store._store_dict, read_only=True) z = open(store=ro_store, mode="r") assert isinstance(z, Array) assert z.shape == (200,) @@ -106,7 +106,7 @@ async def test_open_group(memory_store: MemoryStore) -> None: # open group, read-only store_cls = type(store) - ro_store = await store_cls.open(store_dict=store._store_dict, readonly=True) + ro_store = await store_cls.open(store_dict=store._store_dict, read_only=True) g = open_group(store=ro_store, mode="r") assert isinstance(g, Group) assert g.read_only @@ -149,7 +149,7 @@ def test_save(store: Store, n_args: int, n_kwargs: int) -> None: assert isinstance(array, Array) assert_array_equal(array[:], data) else: - save(store, *args, **kwargs) # type: ignore[arg-type] + save(store, *args, **kwargs) group = open(store) assert isinstance(group, Group) for array in group.array_values(): @@ -1023,10 +1023,10 @@ async def test_metadata_validation_error() -> None: MetadataValidationError, match="Invalid value for 'zarr_format'. Expected '2, 3, or None'. Got '3.0'.", ): - await zarr.api.asynchronous.open_group(zarr_format="3.0") # type: ignore[arg-type] + await zarr.api.asynchronous.open_group(zarr_format="3.0") with pytest.raises( MetadataValidationError, match="Invalid value for 'zarr_format'. Expected '2, 3, or None'. Got '3.0'.", ): - await zarr.api.asynchronous.open_array(shape=(1,), zarr_format="3.0") # type: ignore[arg-type] + await zarr.api.asynchronous.open_array(shape=(1,), zarr_format="3.0") diff --git a/tests/test_group.py b/tests/test_group.py index cfa4ccc46..afa290207 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -1196,12 +1196,16 @@ async def test_members_name(store: Store, consolidate: bool, zarr_format: ZarrFo async def test_open_mutable_mapping(): - group = await zarr.api.asynchronous.open_group(store={}, mode="w") + group = await zarr.api.asynchronous.open_group( + store={}, + ) assert isinstance(group.store_path.store, MemoryStore) def test_open_mutable_mapping_sync(): - group = zarr.open_group(store={}, mode="w") + group = zarr.open_group( + store={}, + ) assert isinstance(group.store_path.store, MemoryStore) @@ -1347,7 +1351,7 @@ def test_from_dict_extra_fields(self): class TestInfo: def test_info(self): - store = zarr.storage.MemoryStore(mode="w") + store = zarr.storage.MemoryStore() A = zarr.group(store=store, path="A") B = A.create_group(name="B") diff --git a/tests/test_metadata/test_consolidated.py b/tests/test_metadata/test_consolidated.py index f4ac6086d..85e32f5d6 100644 --- a/tests/test_metadata/test_consolidated.py +++ b/tests/test_metadata/test_consolidated.py @@ -281,7 +281,7 @@ def test_consolidated_sync(self, memory_store): async def test_not_writable_raises(self, memory_store: zarr.storage.MemoryStore) -> None: await group(store=memory_store, attributes={"foo": "bar"}) - read_store = zarr.storage.MemoryStore(store_dict=memory_store._store_dict, readonly=True) + read_store = zarr.storage.MemoryStore(store_dict=memory_store._store_dict, read_only=True) with pytest.raises(ValueError, match="does not support writing"): await consolidate_metadata(read_store) diff --git a/tests/test_store/test_core.py b/tests/test_store/test_core.py index 5e8cdfd5e..2363048ab 100644 --- a/tests/test_store/test_core.py +++ b/tests/test_store/test_core.py @@ -40,7 +40,7 @@ async def test_make_store_path_local( assert isinstance(store_path.store, LocalStore) assert Path(store_path.store.root) == Path(tmpdir) assert store_path.path == normalize_path(path) - assert store_path.readonly == (mode == "r") + assert store_path.read_only == (mode == "r") @pytest.mark.parametrize("path", [None, "", "bar"]) @@ -53,14 +53,14 @@ async def test_make_store_path_store_path( that a new path is handled correctly. """ ro = mode == "r" - store_like = await StorePath.open(LocalStore(str(tmpdir), readonly=ro), path="root", mode=mode) + store_like = await StorePath.open(LocalStore(str(tmpdir), read_only=ro), path="root", mode=mode) store_path = await make_store_path(store_like, path=path, mode=mode) assert isinstance(store_path.store, LocalStore) assert Path(store_path.store.root) == Path(tmpdir) path_normalized = normalize_path(path) assert store_path.path == (store_like / path_normalized).path - assert store_path.readonly == ro + assert store_path.read_only == ro async def test_make_store_path_invalid() -> None: diff --git a/tests/test_store/test_stateful_hierarchy.py b/tests/test_store/test_stateful_hierarchy.py index 40c8f4499..844e1227d 100644 --- a/tests/test_store/test_stateful_hierarchy.py +++ b/tests/test_store/test_stateful_hierarchy.py @@ -43,7 +43,7 @@ def __init__(self, store) -> None: self.store = store - self.model = MemoryStore(mode="w") + self.model = MemoryStore() zarr.group(store=self.model) # Track state of the hierarchy, these should contain fully qualified paths diff --git a/tests/test_store/test_stateful_store.py b/tests/test_store/test_stateful_store.py index 6ab352ded..4514d630e 100644 --- a/tests/test_store/test_stateful_store.py +++ b/tests/test_store/test_stateful_store.py @@ -35,8 +35,8 @@ def __init__(self, store: Store) -> None: self.store = store @property - def readonly(self) -> bool: - return self.store.readonly + def read_only(self) -> bool: + return self.store.read_only def set(self, key: str, data_buffer: zarr.core.buffer.Buffer) -> None: return self._sync(self.store.set(key, data_buffer)) @@ -117,7 +117,7 @@ def init_store(self): @rule(key=zarr_keys, data=st.binary(min_size=0, max_size=MAX_BINARY_SIZE)) def set(self, key: str, data: DataObject) -> None: note(f"(set) Setting {key!r} with {data}") - assert not self.store.readonly + assert not self.store.read_only data_buf = cpu.Buffer.from_bytes(data) self.store.set(key, data_buf) self.model[key] = data_buf @@ -179,7 +179,7 @@ def delete(self, data: DataObject) -> None: @rule() def clear(self) -> None: - assert not self.store.readonly + assert not self.store.read_only note("(clear)") self.store.clear() self.model.clear() diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index e958bed88..df22b76e1 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -27,7 +27,7 @@ def store_kwargs(self, request) -> dict[str, str | bool]: os.close(fd) os.unlink(temp_path) - return {"path": temp_path, "mode": "w", "readonly": False} + return {"path": temp_path, "mode": "w", "read_only": False} async def get(self, store: ZipStore, key: str) -> Buffer: return store._get(key, prototype=default_buffer_prototype()) @@ -35,18 +35,18 @@ async def get(self, store: ZipStore, key: str) -> Buffer: async def set(self, store: ZipStore, key: str, value: Buffer) -> None: return store._set(key, value) - def test_store_readonly(self, store: ZipStore, store_kwargs: dict[str, Any]) -> None: - assert not store.readonly + def test_store_read_only(self, store: ZipStore, store_kwargs: dict[str, Any]) -> None: + assert not store.read_only - async def test_readonly_store_raises(self, store_kwargs: dict[str, Any]) -> None: + async def test_read_only_store_raises(self, store_kwargs: dict[str, Any]) -> None: # we need to create the zipfile in write mode before switching to read mode store = await self.store_cls.open(**store_kwargs) store.close() - kwargs = {**store_kwargs, "mode": "a", "readonly": True} + kwargs = {**store_kwargs, "mode": "a", "read_only": True} store = await self.store_cls.open(**kwargs) assert store._zmode == "a" - assert store.readonly + assert store.read_only # set with pytest.raises(ValueError): @@ -93,19 +93,21 @@ def test_api_integration(self, store: ZipStore) -> None: store.close() - @pytest.mark.parametrize("readonly", [True, False]) - async def test_store_open_readonly(self, store_kwargs: dict[str, Any], readonly: bool) -> None: - if readonly == "r": + @pytest.mark.parametrize("read_only", [True, False]) + async def test_store_open_read_only( + self, store_kwargs: dict[str, Any], read_only: bool + ) -> None: + if read_only == "r": # create an empty zipfile with zipfile.ZipFile(store_kwargs["path"], mode="w"): pass - await super().test_store_open_readonly(store_kwargs, readonly) + await super().test_store_open_read_only(store_kwargs, read_only) - @pytest.mark.parametrize(("zip_mode", "readonly"), [("w", False), ("a", False), ("x", False)]) + @pytest.mark.parametrize(("zip_mode", "read_only"), [("w", False), ("a", False), ("x", False)]) async def test_zip_open_mode_translation( - self, store_kwargs: dict[str, Any], zip_mode: str, readonly: bool + self, store_kwargs: dict[str, Any], zip_mode: str, read_only: bool ) -> None: kws = {**store_kwargs, "mode": zip_mode} store = await self.store_cls.open(**kws) - assert store.readonly == readonly + assert store.read_only == read_only From f5f71fa10041a7ce3b45d7bb3848056248a96d90 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 11 Nov 2024 13:51:21 -0800 Subject: [PATCH 21/25] fixup --- tests/test_api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 47a1c6e39..fa3d80014 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -149,7 +149,7 @@ def test_save(store: Store, n_args: int, n_kwargs: int) -> None: assert isinstance(array, Array) assert_array_equal(array[:], data) else: - save(store, *args, **kwargs) + save(store, *args, **kwargs) # type: ignore[arg-type] group = open(store) assert isinstance(group, Group) for array in group.array_values(): @@ -1023,10 +1023,10 @@ async def test_metadata_validation_error() -> None: MetadataValidationError, match="Invalid value for 'zarr_format'. Expected '2, 3, or None'. Got '3.0'.", ): - await zarr.api.asynchronous.open_group(zarr_format="3.0") + await zarr.api.asynchronous.open_group(zarr_format="3.0") # type: ignore[arg-type] with pytest.raises( MetadataValidationError, match="Invalid value for 'zarr_format'. Expected '2, 3, or None'. Got '3.0'.", ): - await zarr.api.asynchronous.open_array(shape=(1,), zarr_format="3.0") + await zarr.api.asynchronous.open_array(shape=(1,), zarr_format="3.0") # type: ignore[arg-type] From a1c281892ccd9844604d5a730104cfc0874795fe Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Mon, 11 Nov 2024 22:22:58 -0700 Subject: [PATCH 22/25] Fix hypothesis --- src/zarr/testing/strategies.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 332683f00..aed5b82e5 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -138,7 +138,7 @@ def arrays( expected_attrs = {} if attributes is None else attributes array_path = path + ("/" if not path.endswith("/") else "") + name - root = zarr.open_group(store, mode="w") + root = zarr.open_group(store, mode="w", zarr_format=zarr_format) a = root.create_array( array_path, From a0c86c514825cd7d911991552acaff583d6602ac Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Mon, 11 Nov 2024 22:06:18 -0800 Subject: [PATCH 23/25] fixup for windows --- tests/test_store/test_core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_store/test_core.py b/tests/test_store/test_core.py index 2363048ab..4d3f305e5 100644 --- a/tests/test_store/test_core.py +++ b/tests/test_store/test_core.py @@ -80,7 +80,7 @@ async def test_make_store_path_fsspec(monkeypatch) -> None: "store_like", [ None, - str(tempfile.TemporaryDirectory()), + tempfile.TemporaryDirectory().name, Path(tempfile.TemporaryDirectory().name), StorePath(store=MemoryStore(store_dict={}), path="/"), MemoryStore(store_dict={}), From f9dbd5b7044ea47434abb7e70e193ab19b1302a4 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Tue, 12 Nov 2024 13:25:29 -0800 Subject: [PATCH 24/25] 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 64827fdcf..69de796b3 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 70f67398f..0b3cc4002 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 445f13df9..94ecfee4d 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 c83f71af0..b8a2016c6 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 bdb8098c9..4b85708da 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 907f7f361..5702e63bf 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("") From 44f20ff25621fc6543f1d63cfb4022bbc4afd123 Mon Sep 17 00:00:00 2001 From: Joseph Hamman Date: Tue, 12 Nov 2024 15:28:15 -0800 Subject: [PATCH 25/25] is_empty --- src/zarr/abc/store.py | 2 +- src/zarr/storage/common.py | 6 +++--- src/zarr/storage/logging.py | 4 ++-- src/zarr/testing/store.py | 16 ++++++++-------- tests/test_store/test_local.py | 4 ++-- tests/test_store/test_remote.py | 2 +- tests/test_store/test_stateful_store.py | 16 ++++++++-------- 7 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/zarr/abc/store.py b/src/zarr/abc/store.py index 0b3cc4002..fa3c7f3bd 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 is_empty(self, prefix: str) -> bool: """ Check if the directory is empty. diff --git a/src/zarr/storage/common.py b/src/zarr/storage/common.py index ecb664798..0485a3357 100644 --- a/src/zarr/storage/common.py +++ b/src/zarr/storage/common.py @@ -83,7 +83,7 @@ async def open( match mode: case "w-": - if not await self.empty_dir(): + if not await self.is_empty(): msg = ( f"{self} is not empty, but `mode` is set to 'w-'." "Either remove the existing objects in storage," @@ -187,7 +187,7 @@ async def exists(self) -> bool: """ return await self.store.exists(self.path) - async def empty_dir(self) -> bool: + async def is_empty(self) -> bool: """ Check if any keys exist in the store with the given prefix. @@ -196,7 +196,7 @@ async def empty_dir(self) -> bool: bool True if no keys exist in the store with the given prefix, False otherwise. """ - return await self.store.empty_dir(self.path) + return await self.store.is_empty(self.path) def __truediv__(self, other: str) -> StorePath: """Combine this store path with another path""" diff --git a/src/zarr/storage/logging.py b/src/zarr/storage/logging.py index 7888ebede..3b11ddbba 100644 --- a/src/zarr/storage/logging.py +++ b/src/zarr/storage/logging.py @@ -135,10 +135,10 @@ async def _ensure_open(self) -> None: with self.log(): return await self._store._ensure_open() - async def empty_dir(self, prefix: str = "") -> bool: + async def is_empty(self, prefix: str = "") -> bool: # docstring inherited with self.log(): - return await self._store.empty_dir(prefix=prefix) + return await self._store.is_empty(prefix=prefix) async def clear(self) -> None: # docstring inherited diff --git a/src/zarr/testing/store.py b/src/zarr/testing/store.py index b8a2016c6..b544bf87e 100644 --- a/src/zarr/testing/store.py +++ b/src/zarr/testing/store.py @@ -230,23 +230,23 @@ async def test_delete_dir(self, store: S) -> None: assert not await store.exists("foo/zarr.json") assert not await store.exists("foo/c/0") - async def test_empty_dir(self, store: S) -> None: - assert await store.empty_dir("") + async def test_is_empty(self, store: S) -> None: + assert await store.is_empty("") await self.set( store, "foo/bar", self.buffer_cls.from_bytes(bytes("something", encoding="utf-8")) ) - 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/") + assert not await store.is_empty("") + assert await store.is_empty("fo") + assert not await store.is_empty("foo/") + assert not await store.is_empty("foo") + assert await store.is_empty("spam/") async def test_clear(self, store: S) -> None: await self.set( store, "key", self.buffer_cls.from_bytes(bytes("something", encoding="utf-8")) ) await store.clear() - assert await store.empty_dir("") + assert await store.is_empty("") 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 4b85708da..c614d32c2 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.is_empty("") (store.root / "foo/bar").mkdir(parents=True) - assert await store.empty_dir("") + assert await store.is_empty("") 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 5702e63bf..aee620796 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.is_empty("") diff --git a/tests/test_store/test_stateful_store.py b/tests/test_store/test_stateful_store.py index 4514d630e..751c1ac74 100644 --- a/tests/test_store/test_stateful_store.py +++ b/tests/test_store/test_stateful_store.py @@ -55,8 +55,8 @@ def get_partial_values( def delete(self, path: str) -> None: return self._sync(self.store.delete(path)) - def empty_dir(self, prefix: str = "") -> bool: - return self._sync(self.store.empty_dir(prefix=prefix)) + def is_empty(self, prefix: str) -> bool: + return self._sync(self.store.is_empty(prefix=prefix)) def clear(self) -> None: return self._sync(self.store.clear()) @@ -184,18 +184,18 @@ def clear(self) -> None: self.store.clear() self.model.clear() - assert self.store.empty_dir() + assert self.store.is_empty("") assert len(self.model.keys()) == len(list(self.store.list())) == 0 @rule() # Local store can be non-empty when there are subdirectories but no files @precondition(lambda self: not isinstance(self.store.store, LocalStore)) - def empty_dir(self) -> None: - note("(empty_dir)") + def is_empty(self) -> None: + note("(is_empty)") # make sure they either both are or both aren't empty (same state) - assert self.store.empty_dir() == (not self.model) + assert self.store.is_empty("") == (not self.model) @rule(key=zarr_keys) def exists(self, key: str) -> None: @@ -228,10 +228,10 @@ def check_zarr_keys(self) -> None: keys = list(self.store.list()) if not keys: - assert self.store.empty_dir() is True + assert self.store.is_empty("") is True else: - assert self.store.empty_dir() is False + assert self.store.is_empty("") is False for key in keys: assert self.store.exists(key) is True