diff --git a/changes/4375.bugfix.md b/changes/4375.bugfix.md new file mode 100644 index 0000000000..689d8e3be7 --- /dev/null +++ b/changes/4375.bugfix.md @@ -0,0 +1,3 @@ +Arrays whose stored `regular` chunk grid mixes chunk sizes with lists of chunk edge lengths, such as `"chunk_shape": [2, [5, 10, 5]]` (written by zarr 3.2.0 and 3.2.1 for `chunks=(2, (5, 10, 5))`, and as `[2, [5.0, 10.0, 5.0]]` for float edges), can be read again, without enabling `array.rectilinear_chunks`. The grid is read as the rectilinear chunk grid it describes, with a `ZarrUserWarning`; re-saving the array's metadata (or writing chunks to it) stores that rectilinear grid, which requires the flag. A group's consolidated metadata keeps such an array's metadata as it was stored. + +The `array.rectilinear_chunks` flag now gates reading and storing array metadata that declares a rectilinear chunk grid, including in a group's consolidated metadata, instead of constructing `RectilinearChunkGridMetadata`. An operation that would store such metadata with the flag off (creating an array, `Array.resize`, deleting a member of a group with consolidated metadata, `create_hierarchy`) raises before it deletes or writes anything, and the error names the array, also when it is a member of a group's consolidated metadata. diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index 9788f60743..eb2b55c65f 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -120,10 +120,12 @@ ) from zarr.core.metadata.io import ( ARRAY_DOCUMENTS, + encode_documents, parse_stored_array, read_documents, save_metadata, save_new_metadata, + store_documents, upsert_metadata, ) from zarr.core.metadata.v2 import ( @@ -646,7 +648,6 @@ async def _create_v3( dimension_names=dimension_names, attributes=attributes, ) - array = cls(metadata=metadata, store_path=store_path, config=config) await save_new_metadata(store_path, metadata, overwrite=overwrite, ensure_parents=True) return array @@ -727,7 +728,6 @@ async def _create_v2( compressor=compressor_parsed, attributes=attributes, ) - array = cls(metadata=metadata, store_path=store_path, config=config) await save_new_metadata(store_path, metadata, overwrite=overwrite, ensure_parents=True) return array @@ -1607,7 +1607,9 @@ async def get_coordinate_selection( async def _save_metadata(self, metadata: ArrayMetadata, ensure_parents: bool = False) -> None: """Store `metadata` as this array's own documents, then clear the - `_stored_document` mark (see `_stored_document_replaced`).""" + `_stored_document` mark (see `_stored_document_replaced`). `_resize` stores the + documents it encoded before deleting chunks directly, then clears the mark the + same way.""" await save_metadata(self.store_path, metadata, ensure_parents=ensure_parents) self._stored_document_replaced() @@ -1642,14 +1644,14 @@ async def _store_repaired_document(self) -> None: zarr_format = self.metadata.zarr_format documents = await read_documents(self.store_path, ARRAY_DOCUMENTS[zarr_format]) try: - current = parse_stored_array(documents, zarr_format) + current = parse_stored_array(documents, zarr_format, str(self.store_path)) except ArrayNotFoundError: pass else: if _chunk_layout(current) != _chunk_layout(self.metadata): raise ValueError( - f"The metadata stored for the array at {str(self.store_path)!r} has " - "changed since this array was opened: reopen the array to write to it." + f"Array {str(self.store_path)!r}: the metadata stored has changed since " + "this array was opened; reopen the array to write to it. Nothing was stored." ) if current._stored_document is not None: await upsert_metadata(self.store_path, current, documents) @@ -5904,6 +5906,10 @@ async def _resize( # ensure deletion is only run if array is shrinking as the delete_outside_chunks path is unbounded in memory only_growing = all(new >= old for new, old in zip(new_shape, array.metadata.shape, strict=True)) + # Encode the new metadata before deleting any chunk: metadata that cannot be stored + # then fails with the store untouched. + documents = encode_documents(array.store_path, new_metadata) + if delete_outside_chunks and not only_growing: # Remove all chunks outside of the new shape old_chunk_coords = set(array._chunk_grid.all_chunk_coords()) @@ -5922,7 +5928,8 @@ async def _delete_key(key: str) -> None: ) # Write new metadata - await array._save_metadata(new_metadata) + await store_documents(array.store_path, documents) + array._stored_document_replaced() # Update metadata and chunk_grid (in place) object.__setattr__(array, "metadata", new_metadata) diff --git a/src/zarr/core/group.py b/src/zarr/core/group.py index 7d4e26a1dd..7792fa58de 100644 --- a/src/zarr/core/group.py +++ b/src/zarr/core/group.py @@ -49,8 +49,13 @@ from zarr.core.dtype import parse_data_type from zarr.core.json_parse import parse_field from zarr.core.metadata import ArrayV2Metadata, ArrayV3Metadata -from zarr.core.metadata.io import save_metadata, save_new_metadata -from zarr.core.metadata.v3 import AllowedExtraField, parse_extra_fields +from zarr.core.metadata.io import ( + encode_documents, + save_metadata, + save_new_metadata, + store_documents, +) +from zarr.core.metadata.v3 import AllowedExtraField, check_storable, parse_extra_fields from zarr.core.sync import SyncMixin, sync from zarr.errors import ( ArrayNotFoundError, @@ -380,6 +385,16 @@ class GroupMetadata(Metadata): extra_fields: dict[str, AllowedExtraField] = field(default_factory=dict) def to_buffer_dict(self, prototype: BufferPrototype) -> dict[str, Buffer]: + if self.consolidated_metadata is not None: + for path, member in self.consolidated_metadata.flattened_metadata.items(): + # A member read from a document that had to be repaired is stored as it + # was stored (see `ConsolidatedMetadata.to_dict`). + if isinstance(member, ArrayV3Metadata) and member._stored_document is None: + try: + check_storable(member) + except ValueError as e: + e.add_note(f"Array {path!r} in the consolidated metadata.") + raise indent = config.get("json_indent") if self.zarr_format == 3: return {ZARR_JSON: json_to_buffer(self.to_dict(), prototype=prototype, indent=indent)} @@ -834,11 +849,24 @@ async def delitem(self, key: str) -> None: Array or group name """ store_path = self.store_path / key - + consolidated = self.metadata.consolidated_metadata + if consolidated is None: + await store_path.delete_dir() + return + # Encode the group metadata without the member before deleting it: metadata that + # cannot be stored then fails with the store and this group untouched. What is + # stored is encoded after the deletion, from the metadata as it then is, so + # concurrent deletions each store the deletions made before them. + members = {name: node for name, node in consolidated.metadata.items() if name != key} + encode_documents( + self.store_path, + replace(self.metadata, consolidated_metadata=replace(consolidated, metadata=members)), + ) await store_path.delete_dir() - if self.metadata.consolidated_metadata: - self.metadata.consolidated_metadata.metadata.pop(key, None) - await self._save_metadata() + # In place, so every handle sharing this consolidated metadata (a parent's or a + # subgroup's) sees the deletion. + consolidated.metadata.pop(key, None) + await store_documents(self.store_path, encode_documents(self.store_path, self.metadata)) async def get[DefaultT]( self, key: str, default: DefaultT | None = None @@ -3307,11 +3335,11 @@ async def create_hierarchy( else: nodes_explicit[k] = v - # Build every node before deleting or storing anything: metadata that no array or - # group can be built from then fails with the store untouched. - built = _build_nodes(store, nodes_explicit) + # Build and encode every node before deleting anything: a node that cannot be built + # or whose metadata cannot be stored then fails with the store untouched. + built, documents = _prepare_nodes(store, nodes_explicit) await asyncio.gather(*(store.delete_dir(key) for key in to_delete_keys)) - async for key, node in _store_nodes(store, nodes_explicit, built): + async for key, node in _store_nodes(store, nodes_explicit, built, documents): yield key, node @@ -3341,34 +3369,42 @@ async def create_nodes( AsyncGroup | AsyncArray The created nodes in the order they are created. """ - async for key, node in _store_nodes(store, nodes, _build_nodes(store, nodes)): + async for key, node in _store_nodes(store, nodes, *_prepare_nodes(store, nodes)): yield key, node -def _build_nodes( +def _prepare_nodes( store: Store, nodes: Mapping[str, GroupMetadata | ArrayV2Metadata | ArrayV3Metadata] -) -> dict[str, AsyncGroup | AnyAsyncArray]: - """The array or group each of `nodes` describes, at its path in `store`.""" - return { +) -> tuple[dict[str, AsyncGroup | AnyAsyncArray], dict[str, Buffer]]: + """The array or group each of `nodes` describes, at its path in `store`, and the + metadata documents of `nodes`, by their keys in the store: every node is built and + encoded before anything is stored.""" + built = { path: _build_node(store=store, path=path, metadata=meta) for path, meta in nodes.items() } + documents = { + _join_paths([path, key]): value + for path, metadata in nodes.items() + for key, value in encode_documents(StorePath(store, path), metadata).items() + } + return built, documents async def _store_nodes( store: Store, nodes: Mapping[str, GroupMetadata | ArrayV2Metadata | ArrayV3Metadata], built: Mapping[str, AsyncGroup | AnyAsyncArray], + documents: Mapping[str, Buffer], ) -> AsyncIterator[tuple[str, AsyncGroup | AnyAsyncArray]]: - """Store the metadata of `nodes` and yield the nodes `_build_nodes` built from them - (see `create_nodes`).""" + """Store the `documents` encoded from `nodes` and yield the nodes built from them, as + `_prepare_nodes` returns them (see `create_nodes`).""" # Note: the only way to alter this value is via the config. If that's undesirable for some reason, # then we should consider adding a keyword argument to this function semaphore = asyncio.Semaphore(config.get("async.concurrency")) - create_tasks: list[Coroutine[None, None, str]] = [] - - for key, value in nodes.items(): - # make the key absolute - create_tasks.extend(_persist_metadata(store, key, value, semaphore=semaphore)) + create_tasks = [ + _set_return_key(store=store, key=key, value=value, semaphore=semaphore) + for key, value in documents.items() + ] created_object_keys = [] @@ -3881,23 +3917,6 @@ async def _set_return_key( return key -def _persist_metadata( - store: Store, - path: str, - metadata: ArrayV2Metadata | ArrayV3Metadata | GroupMetadata, - semaphore: asyncio.Semaphore | None = None, -) -> tuple[Coroutine[None, None, str], ...]: - """ - Prepare to save a metadata document to storage, returning a tuple of coroutines that must be awaited. - """ - - to_save = metadata.to_buffer_dict(default_buffer_prototype()) - return tuple( - _set_return_key(store=store, key=_join_paths([path, key]), value=value, semaphore=semaphore) - for key, value in to_save.items() - ) - - async def create_rooted_hierarchy( *, store: Store, diff --git a/src/zarr/core/metadata/io.py b/src/zarr/core/metadata/io.py index 5a9237327d..c1cd927b94 100644 --- a/src/zarr/core/metadata/io.py +++ b/src/zarr/core/metadata/io.py @@ -70,8 +70,28 @@ def _diff( yield DocumentChange(path, stored, new) +def encode_documents( + store_path: StorePath, metadata: ArrayMetadata | GroupMetadata +) -> dict[str, Buffer]: + """The metadata documents `metadata` stores under `store_path`, by key (see + `to_buffer_dict`). + + An operation that deletes or writes store content encodes its documents first, so + metadata that cannot be stored fails with the store untouched; the error then names + the node at `store_path`, as the warnings about stored documents do. + """ + from zarr.core.group import GroupMetadata + + try: + return metadata.to_buffer_dict(default_buffer_prototype()) + except ValueError as e: + node = "Group" if isinstance(metadata, GroupMetadata) else "Array" + e.add_note(f"{node} {str(store_path)!r}: nothing was stored.") + raise + + async def store_documents(store_path: StorePath, documents: Mapping[str, Buffer]) -> None: - """Store metadata documents encoded by `to_buffer_dict` under `store_path`.""" + """Store metadata documents encoded by `encode_documents` under `store_path`.""" await asyncio.gather( *(set_or_delete(store_path / key, value) for key, value in documents.items()) ) @@ -97,7 +117,7 @@ async def upsert_metadata( The documents are encoded before any is stored, so metadata that cannot be stored fails with the store untouched. """ - documents = metadata.to_buffer_dict(default_buffer_prototype()) + documents = encode_documents(store_path, metadata) changes = diff_documents( {key: buffer_to_json_object(buf) for key, buf in stored.items() if key in documents}, {key: buffer_to_json_object(buf) for key, buf in documents.items()}, @@ -114,11 +134,17 @@ async def upsert_metadata( """The store keys of the metadata documents of an array of each Zarr format.""" -def parse_stored_array(documents: Mapping[str, Buffer], zarr_format: ZarrFormat) -> ArrayMetadata: +def parse_stored_array( + documents: Mapping[str, Buffer], zarr_format: ZarrFormat, path: str | None = None +) -> ArrayMetadata: """The metadata of an array from its documents (by store key, see `ARRAY_DOCUMENTS`), read with the repairs but without their warnings (whoever asks has warned, or reads metadata built in code), and marked (see `mark_repaired`) if they had to be - repaired. Raises `ArrayNotFoundError` if there is no array document among them.""" + repaired. Raises `ArrayNotFoundError` if there is no array document among them. + + Only operations that store metadata read documents this way, so documents read as a + rectilinear chunk grid require the rectilinear chunks flag, as storing it does; the + error names the array at `path`.""" from zarr.core.array import ( _array_metadata_dict_v2, _array_metadata_dict_v3, @@ -132,7 +158,8 @@ def parse_stored_array(documents: Mapping[str, Buffer], zarr_format: ZarrFormat) else: raise ArrayNotFoundError(f"No Zarr format {zarr_format} array metadata document.") repaired, readings = repair_array_document(stored, zarr_format) - return mark_repaired(parse_array_metadata(dict(repaired)), stored, readings, None, warn=False) + metadata = parse_array_metadata(dict(repaired), path) + return mark_repaired(metadata, stored, readings, None, warn=False) def _build_parents(store_path: StorePath, zarr_format: ZarrFormat) -> dict[str, GroupMetadata]: @@ -172,7 +199,7 @@ async def save_metadata( ------ ValueError """ - to_save = metadata.to_buffer_dict(default_buffer_prototype()) + to_save = encode_documents(store_path, metadata) await _write_metadata(store_path, to_save, metadata.zarr_format, ensure_parents=ensure_parents) @@ -185,8 +212,8 @@ async def save_new_metadata( ) -> None: """Save the metadata of a new array or group, replacing any existing node if requested. - The metadata is encoded before the store is modified, so metadata that cannot be - encoded raises without deleting an existing node. + The metadata is encoded first (see `encode_documents`), so metadata that cannot be + stored raises, naming the node, without deleting an existing node. Parameters ---------- @@ -201,7 +228,7 @@ async def save_new_metadata( ensure_parents : bool, optional Create any missing parent groups, and check no existing parents are arrays. """ - to_save = metadata.to_buffer_dict(default_buffer_prototype()) + to_save = encode_documents(store_path, metadata) if overwrite and store_path.store.supports_deletes: await store_path.delete_dir() else: diff --git a/src/zarr/core/metadata/repair.py b/src/zarr/core/metadata/repair.py index 06ce97dce7..d8050b9f3d 100644 --- a/src/zarr/core/metadata/repair.py +++ b/src/zarr/core/metadata/repair.py @@ -50,6 +50,13 @@ class Reading(NamedTuple): """Returns `None` if the document needs no repair, else the repaired document and how it was read.""" +RESAVE_HINT: Final = ( + "To store valid metadata, open the array writable and call `array.update_attributes({})`; " + "if a group holds consolidated metadata for the array, then also call " + "`zarr.consolidate_metadata` on that group." +) +"""How to store the repair of a document whose array holds data.""" + RECREATE_HINT: Final = ( "The array holds only its fill value, so recreate it with the chunk shape you want; " "nothing is lost: `zarr.from_array(array.store, name=array.path, data=array, " @@ -95,13 +102,20 @@ def _is_int_list(value: object) -> TypeGuard[list[int]]: return isinstance(value, list) and all(isinstance(v, int) for v in value) +def _abbreviate(value: JSON, limit: int = 60) -> str: + """`value` as JSON, cut to at most `limit` characters.""" + text = json.dumps(value) + return text if len(text) <= limit else f"{text[: limit - 3]}..." + + def _read_chunk_size( size: JSON, span: int | None, unit: int -) -> tuple[int, bool, str | None] | None: - """Read one entry of a stored regular chunk shape as a chunk edge length. +) -> tuple[int | list[JSON], bool, str | None] | None: + """Read one entry of a stored regular chunk shape as a chunk edge length, or as the + chunk edge lengths of its axis. - Returns the edge length, whether it moves chunks (it is not the size a reader of - the stored entry uses), and, where the user must act on how it was read, how it was + Returns the reading, whether it moves chunks (it is not the size a reader of the + stored entry uses), and, where the user must act on how it was read, how it was read; `None` if the entry cannot be read, which leaves it for the metadata constructors to check. A JSON int >= 1 is kept, JSON `true` is read as 1, and 0 or JSON `false` is read as `unit`, the smallest chunk edge length the axis can have (1, @@ -110,7 +124,10 @@ def _read_chunk_size( does not depend on how far the axis has grown since. On an axis of positive length no chunk can have been stored under a chunk size of 0, so the array holds only its fill value. `span` is `None` where no stored 0 is known, as in the inner chunk shape of a - sharding codec: 0 is then left as stored. + sharding codec: 0 is then left as stored. A flat JSON list is kept as the chunk edge + lengths of its axis, which only a rectilinear chunk grid can declare (see + `_invalid_chunk_sizes_v3`); its edges are read as those of a stored rectilinear chunk + grid (see `_invalid_edge_lengths_v3`). """ match size: case True: @@ -130,23 +147,25 @@ def _read_chunk_size( "chunk size of 0" ), ) + case list() if not any(isinstance(edge, list) for edge in size): + return size, False, None return None def _read_chunk_shape( stored: JSON, spans: Sequence[int | None], units: Iterable[int] = () -) -> tuple[list[int], Reading] | None: +) -> tuple[list[int | list[JSON]], Reading | None] | None: """Read a stored regular chunk shape, entry by entry (see `_read_chunk_size`), for axes of lengths `spans` whose chunks are multiples of `units` (1 where not given). - Returns the chunk shape and how it was read, if any entry was read as another value + Returns the chunk shape and how it was read if any entry was read as another value (the warning says how the chunk shape was read and, as the array then holds only its - fill value, recommends recreating it, see `RECREATE_HINT`); `None` if it cannot be - read or needs no repair. + fill value, recommends recreating it, see `RECREATE_HINT`), else `None`; `None` if + it cannot be read. """ if not (isinstance(stored, list) and len(stored) == len(spans)): return None - edges: list[int] = [] + edges: list[int | list[JSON]] = [] changed = moves_chunks = False readings: list[str] = [] axes = zip(stored, spans, chain(units, repeat(1)), strict=False) @@ -161,11 +180,11 @@ def _read_chunk_shape( if how is not None: readings.append(f"{json.dumps(size)} in dimension {axis} as {how}") if not changed: - return None + return edges, None warning = ( - f"The stored chunk shape {json.dumps(stored)} is invalid: chunk sizes must be " - f"integers of at least 1. It is read as {edges}, reading {'; '.join(readings)}. " - f"{RECREATE_HINT}" + f"The stored chunk shape {_abbreviate(stored)} is invalid: chunk sizes must be " + f"integers of at least 1. It is read as {_abbreviate(edges)}, reading " + f"{'; '.join(readings)}. {RECREATE_HINT}" if readings else None ) @@ -177,7 +196,7 @@ def _invalid_chunk_sizes_v2(doc: ArrayDocument) -> tuple[ArrayDocument, Reading] if not _is_int_list(shape): return None match _read_chunk_shape(doc.get("chunks"), shape): - case chunks, reading: + case chunks, Reading() as reading: return {**doc, "chunks": chunks}, reading return None @@ -192,10 +211,13 @@ def _read_codec(codec: JSON) -> JSON | None: case {"name": "sharding_indexed", "configuration": Mapping() as configuration}: repaired: dict[str, JSON] = {} stored = configuration.get("chunk_shape") - if isinstance(stored, list) and ( - read := _read_chunk_shape(stored, [None] * len(stored)) + match ( + _read_chunk_shape(stored, [None] * len(stored)) + if isinstance(stored, list) + else None ): - repaired["chunk_shape"] = read[0] + case chunk_shape, Reading(): + repaired["chunk_shape"] = chunk_shape if isinstance(codecs := configuration.get("codecs"), list) and ( inner := _read_codecs(codecs) ): @@ -249,11 +271,35 @@ def _invalid_chunk_sizes_v3(doc: ArrayDocument) -> tuple[ArrayDocument, Reading] configuration = grid.get("configuration") if not isinstance(configuration, Mapping): return None - match _read_chunk_shape(configuration.get("chunk_shape"), shape, units): - case chunk_shape, reading: - repaired = {**configuration, "chunk_shape": chunk_shape} - return {**doc, "chunk_grid": {**grid, "configuration": repaired}}, reading - return None + read = _read_chunk_shape(configuration.get("chunk_shape"), shape, units) + if read is None: + return None + chunk_shape, reading = read + edge_axes = [axis for axis, size in enumerate(chunk_shape) if isinstance(size, list)] + if not edge_axes: + if reading is None: + return None + repaired = {**configuration, "chunk_shape": chunk_shape} + return {**doc, "chunk_grid": {**grid, "configuration": repaired}}, reading + if len(edge_axes) == len(chunk_shape): + # Only a mix of chunk sizes and edge lists was ever stored in a regular grid. + return None + # The user must act: re-saving this grid needs the rectilinear chunks flag. + as_rectilinear = ( + f"The stored chunk grid is named 'regular', but its chunk shape lists chunk edge " + f"lengths in dimensions {edge_axes}, which only a rectilinear chunk grid can declare. " + "It is read as that rectilinear chunk grid. Re-saving the metadata stores that " + "rectilinear chunk grid, so each step that follows requires " + f"`zarr.config.set({{'array.rectilinear_chunks': True}})`. {RESAVE_HINT}" + ) + rectilinear: JSON = { + "name": "rectilinear", + "configuration": {"kind": "inline", "chunk_shapes": chunk_shape}, + } + warning = " ".join(filter(None, (reading and reading.warning, as_rectilinear))) + # Only zarr 3.2.x reads the stored grid, so the array stores the rectilinear grid + # before it writes chunks, for every other reader to find them. + return {**doc, "chunk_grid": rectilinear}, Reading(moves_chunks=True, warning=warning) def _read_edge_length(edge: JSON) -> JSON: diff --git a/src/zarr/core/metadata/v3.py b/src/zarr/core/metadata/v3.py index e58082aeb9..2d5d169e6b 100644 --- a/src/zarr/core/metadata/v3.py +++ b/src/zarr/core/metadata/v3.py @@ -296,10 +296,6 @@ def from_dict(cls, data: RegularChunkGridMetadataJSON) -> Self: # type: ignore[ return cls(chunk_shape=parse_chunk_shape(configuration["chunk_shape"])) -class RectilinearChunksDisabledError(ValueError): - """Rectilinear chunk grids are used while the `array.rectilinear_chunks` flag is off.""" - - @dataclass(frozen=True, kw_only=True) class RectilinearChunkGridMetadata(Metadata): """Metadata-only description of a rectilinear chunk grid. @@ -319,12 +315,6 @@ class RectilinearChunkGridMetadata(Metadata): chunk_shapes: tuple[int | tuple[int, ...], ...] def __post_init__(self) -> None: - if not config.get("array.rectilinear_chunks"): - raise RectilinearChunksDisabledError( - "Rectilinear chunk grids are experimental and disabled by default. " - "Enable them with: zarr.config.set({'array.rectilinear_chunks': True}) " - "or set the environment variable ZARR_ARRAY__RECTILINEAR_CHUNKS=True" - ) object.__setattr__(self, "chunk_shapes", _validate_chunk_shapes(self.chunk_shapes)) @property @@ -392,6 +382,37 @@ def from_dict(cls, data: RectilinearChunkGridMetadataJSON) -> Self: # type: ign ChunkGridMetadata = RegularChunkGridMetadata | RectilinearChunkGridMetadata +class RectilinearChunksDisabledError(ValueError): + """Rectilinear chunk grids are used while the `array.rectilinear_chunks` flag is off.""" + + +def _check_rectilinear_chunks_enabled() -> None: + """Raise unless rectilinear chunks are enabled. + + The flag gates storing and reading array metadata documents that declare a + rectilinear chunk grid; the chunk grid metadata classes themselves are not gated. + """ + if not config.get("array.rectilinear_chunks"): + raise RectilinearChunksDisabledError( + "Rectilinear chunk grids are experimental and disabled by default. " + "Enable them with: zarr.config.set({'array.rectilinear_chunks': True}) " + "or set the environment variable ZARR_ARRAY__RECTILINEAR_CHUNKS=True" + ) + + +def check_storable(metadata: ArrayV3Metadata) -> None: + """Raise if `metadata` may not be stored: a rectilinear chunk grid requires the + rectilinear chunks flag. `zarr.core.metadata.io.encode_documents` names the node in + the error. + + Every serialization of array metadata for a store calls this, before the store is + touched: `ArrayV3Metadata.to_buffer_dict` and, for the arrays in a group's + consolidated metadata, `GroupMetadata.to_buffer_dict`. + """ + if isinstance(metadata.chunk_grid, RectilinearChunkGridMetadata): + _check_rectilinear_chunks_enabled() + + def create_chunk_grid_metadata( chunks: ChunkGrid, ) -> ChunkGridMetadata: @@ -639,6 +660,7 @@ def encode_chunk_key(self, chunk_coords: tuple[int, ...]) -> str: return self.chunk_key_encoding.encode_chunk_key(chunk_coords) def to_buffer_dict(self, prototype: BufferPrototype) -> dict[str, Buffer]: + check_storable(self) indent = config.get("json_indent") return {ZARR_JSON: json_to_buffer(self.to_dict(), prototype=prototype, indent=indent)} @@ -646,7 +668,17 @@ def to_buffer_dict(self, prototype: BufferPrototype) -> dict[str, Buffer]: def from_dict(cls, data: dict[str, JSON], *, path: str | None = None) -> Self: """Read a stored `zarr.json` array document. An invalid document that `zarr.core.metadata.repair` can read is read as repaired; a reading the user - must act on warns, naming the array at `path`.""" + must act on warns, and a document the rectilinear chunks flag refuses raises, + naming the array at `path`.""" + # The flag gates what the document declares, so it is checked before repairs. + chunk_grid = data.get("chunk_grid") + if isinstance(chunk_grid, Mapping) and chunk_grid.get("name") == "rectilinear": + try: + _check_rectilinear_chunks_enabled() + except ValueError as e: + if path is not None: + e.add_note(f"Array {path!r}: nothing was read.") + raise repaired, readings = repair_array_document(data, 3) # a new dict, because we are modifying it _data = dict(repaired) diff --git a/tests/test_group.py b/tests/test_group.py index 2fbd4d2650..6976ef536c 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -1,5 +1,6 @@ from __future__ import annotations +import asyncio import contextlib import inspect import json @@ -1733,11 +1734,13 @@ async def test_group_getitem_consolidated(self, store: Store) -> None: rg2 = await rg1.get_group("g2") assert rg2.metadata.consolidated_metadata == ConsolidatedMetadata(metadata={}) - async def test_group_delitem_consolidated(self, store: Store) -> None: + async def test_group_delitem_consolidated(self, store: Store, zarr_format: ZarrFormat) -> None: + """Deleting a member removes it from the consolidated metadata in memory and in + every document that stores it, so the group reopens without it.""" if isinstance(store, ZipStore): raise pytest.skip("Not implemented") - root = await AsyncGroup.from_store(store=store) + root = await AsyncGroup.from_store(store=store, zarr_format=zarr_format) # Set up the test structure with # / # g0/ # group /g0 @@ -1759,23 +1762,80 @@ async def test_group_delitem_consolidated(self, store: Store) -> None: x2 = await x1.create_group("x2") await x2.create_array("data", shape=(1,), dtype="uint8") - with pytest.warns( # noqa: PT031 - ZarrUserWarning, - match="Consolidated metadata is currently not part in the Zarr format 3 specification.", - ): - if isinstance(store, ZipStore): - with pytest.warns(UserWarning, match="Duplicate name"): - await zarr.api.asynchronous.consolidate_metadata(store) - else: - await zarr.api.asynchronous.consolidate_metadata(store) + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", "Consolidated metadata is currently not part", ZarrUserWarning + ) + await zarr.api.asynchronous.consolidate_metadata(store) - group = await zarr.api.asynchronous.open_consolidated(store=store) - assert len(group.metadata.consolidated_metadata.metadata) == 2 - assert "g0" in group.metadata.consolidated_metadata.metadata + group = await zarr.api.asynchronous.open_consolidated(store=store, zarr_format=zarr_format) + assert group.metadata.consolidated_metadata is not None + assert sorted(group.metadata.consolidated_metadata.metadata) == ["g0", "x0"] await group.delitem("g0") - assert len(group.metadata.consolidated_metadata.metadata) == 1 - assert "g0" not in group.metadata.consolidated_metadata.metadata + assert sorted(group.metadata.consolidated_metadata.metadata) == ["x0"] + + reopened = await zarr.api.asynchronous.open_consolidated( + store=store, zarr_format=zarr_format + ) + assert reopened.metadata.consolidated_metadata is not None + assert sorted(reopened.metadata.consolidated_metadata.metadata) == ["x0"] + + def test_group_delitem_consolidated_aliased(self, store: Store) -> None: + """A subgroup read from its parent's consolidated metadata shares it, so a member + deleted through the subgroup is gone through the parent too.""" + if isinstance(store, ZipStore): + raise pytest.skip("Not implemented") + + root = zarr.create_group(store) + root.create_group("sub").create_array("b", shape=(4,), chunks=(2,), dtype="i4") + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", "Consolidated metadata is currently not part", ZarrUserWarning + ) + zarr.consolidate_metadata(store) + + group = zarr.open_group(store, mode="r+", use_consolidated=True) + sub = group["sub"] + assert isinstance(sub, Group) + del sub["b"] + assert "b" not in sub + assert "b" not in group["sub"] + with pytest.raises(KeyError): + group["sub/b"] + + async def test_group_delitem_consolidated_concurrent(self, zarr_format: ZarrFormat) -> None: + """Concurrent deletions through one handle each store the deletions made before + them, so the stored consolidated metadata lists what the handle lists. Here the + deletions finish in the reverse of the order they started in.""" + + delays = [0.03, 0.02, 0.01] + + class SlowDeletes(LatencyStore): + async def delete_dir(self, prefix: str) -> None: + await asyncio.sleep(delays.pop(0)) + await super().delete_dir(prefix) + + store = SlowDeletes(MemoryStore()) + root = await AsyncGroup.from_store(store=store, zarr_format=zarr_format) + for name in "abcd": + await root.create_array(name, shape=(2,), dtype="i4") + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", "Consolidated metadata is currently not part", ZarrUserWarning + ) + await zarr.api.asynchronous.consolidate_metadata(store) + group = await zarr.api.asynchronous.open_consolidated(store=store, zarr_format=zarr_format) + + await asyncio.gather(*(group.delitem(name) for name in "abc")) + + reopened = await zarr.api.asynchronous.open_consolidated( + store=store, zarr_format=zarr_format + ) + assert group.metadata.consolidated_metadata is not None + assert reopened.metadata.consolidated_metadata is not None + assert list(group.metadata.consolidated_metadata.metadata) == ["d"] + assert list(reopened.metadata.consolidated_metadata.metadata) == ["d"] def test_open_consolidated_raises(self, store: Store) -> None: if isinstance(store, ZipStore): diff --git a/tests/test_metadata/test_io.py b/tests/test_metadata/test_io.py index 999e5a15a6..d9e35f2c16 100644 --- a/tests/test_metadata/test_io.py +++ b/tests/test_metadata/test_io.py @@ -26,6 +26,7 @@ from zarr.abc.store import Store from zarr.core.buffer import Buffer from zarr.core.common import JSON + from zarr.core.metadata import ArrayV2Metadata, ArrayV3Metadata V3_DOC: dict[str, JSON] = { "zarr_format": 3, @@ -191,17 +192,21 @@ def test_upsert_metadata_identical_stores_nothing(zarr_format: Literal[2, 3]) -> assert store.sets == 0 -def test_upsert_metadata_unstorable_leaves_store_untouched(monkeypatch: pytest.MonkeyPatch) -> None: - """Metadata that cannot be encoded fails before the store is written.""" - store_path, metadata = _legacy(3) +def test_upsert_metadata_unstorable_leaves_store_untouched() -> None: + """Metadata that may not be stored (a rectilinear chunk grid without the rectilinear + chunks flag) fails before the store is read or written, naming the array.""" + store_path, _ = _legacy(3) before = _documents(store_path.store) - - def refuse(*args: object) -> None: - raise ValueError("cannot be stored") - - monkeypatch.setattr(ArrayV3Metadata, "to_buffer_dict", refuse) - with pytest.raises(ValueError, match="cannot be stored"): + with zarr.config.set({"array.rectilinear_chunks": True}): + metadata = zarr.create_array( + MemoryStore(), shape=(3,), chunks=[[1, 2]], dtype="int16" + ).metadata + with ( + zarr.config.set({"array.rectilinear_chunks": False}), + pytest.raises(ValueError, match="experimental and disabled") as info, + ): _upsert(store_path, metadata) + assert info.value.__notes__ == [f"Array {str(store_path)!r}: nothing was stored."] assert _documents(store_path.store) == before diff --git a/tests/test_metadata/test_repair.py b/tests/test_metadata/test_repair.py index da35922da7..be57d0bd82 100644 --- a/tests/test_metadata/test_repair.py +++ b/tests/test_metadata/test_repair.py @@ -21,6 +21,7 @@ from zarr.core.metadata import ArrayV2Metadata, ArrayV3Metadata from zarr.core.metadata.repair import ( RECREATE_HINT, + RESAVE_HINT, repair_array_document, ) from zarr.core.metadata.v3 import RectilinearChunkGridMetadata, RegularChunkGridMetadata @@ -833,7 +834,7 @@ def test_stale_handle_write_after_chunk_grid_change_raises( _rewrite_doc(path, zarr_format, change) documents = {p.name: p.read_bytes() for p in path.iterdir()} - with pytest.raises(ValueError, match="has changed since this array was opened: reopen"): + with pytest.raises(ValueError, match="has changed since this array was opened; reopen"): stale[0:3] = [7, 8, 9] assert stale.metadata._stored_document is not None @@ -1180,7 +1181,7 @@ def test_consolidated_repaired_member_write_after_chunk_grid_change_raises( _rewrite_doc(path / "a", zarr_format, _store_chunk_size_3) documents = {p: p.read_bytes() for p in path.rglob("*") if p.is_file()} - with pytest.raises(ValueError, match="has changed since this array was opened: reopen"): + with pytest.raises(ValueError, match="has changed since this array was opened; reopen"): array[0:3] = [7, 8, 9] assert {p: p.read_bytes() for p in path.rglob("*") if p.is_file()} == documents @@ -1309,3 +1310,340 @@ def test_legacy_chunk_size_consolidated(tmp_path: Path, zarr_format: Literal[2, array = reopened[name] assert isinstance(array, zarr.Array) assert array.chunks == (1,) + + +# A document copied verbatim from a store that zarr 3.2.1 wrote for +# `create_array(shape=(6, 20), chunks=(2, (5, 10, 5)), dtype="float32")`. +MIXED_REGULAR_GRID_DOC = """{ + "shape": [6, 20], + "data_type": "float32", + "chunk_grid": {"name": "regular", "configuration": {"chunk_shape": [2, [5, 10, 5]]}}, + "chunk_key_encoding": {"name": "default", "configuration": {"separator": "/"}}, + "fill_value": 0.0, + "codecs": [ + {"name": "bytes", "configuration": {"endian": "little"}}, + {"name": "zstd", "configuration": {"level": 0, "checksum": false}} + ], + "attributes": {}, + "zarr_format": 3, + "node_type": "array", + "storage_transformers": [] +}""" + + +def _mixed_doc(shape: list[int], chunk_shape: list[Any]) -> dict[str, JSON]: + doc: dict[str, JSON] = json.loads(MIXED_REGULAR_GRID_DOC) + doc["shape"] = shape + doc["chunk_grid"] = {"name": "regular", "configuration": {"chunk_shape": chunk_shape}} + return doc + + +@pytest.mark.parametrize( + ("doc", "expected", "warning"), + [ + (json.loads(MIXED_REGULAR_GRID_DOC), (2, (5, 10, 5)), r"^The stored chunk grid .* \[1\]"), + ( + _mixed_doc([6, 20, 4], [2, [5, 10, 5], [1, 3]]), + (2, (5, 10, 5), (1, 3)), + r"^The stored chunk grid .* in dimensions \[1, 2\]", + ), + (_mixed_doc([6, 20], [2, [20]]), (2, (20,)), r"^The stored chunk grid .* \[1\]"), + (_mixed_doc([6, 12], [2, [5, 10, 5]]), (2, (5, 10, 5)), r"^The stored chunk grid"), + (_mixed_doc([0, 20], [2, [5, 10, 5]]), (2, (5, 10, 5)), r"^The stored chunk grid"), + (_mixed_doc([6, 0], [2, [5, 10, 5]]), (2, (5, 10, 5)), r"^The stored chunk grid"), + # JSON true is read as 1 silently; only the grid reading warns. + (_mixed_doc([6, 20], [True, [5, 10, 5]]), (1, (5, 10, 5)), r"^The stored chunk grid"), + (_mixed_doc([6, 20], [2, [5.0, 10.0, 5.0]]), (2, (5, 10, 5)), r"^The stored chunk grid"), + ( + _mixed_doc([4, 10_000], [0, [10] * 1000]), + (1, (10,) * 1000), + r"^The stored chunk shape \[0, \[10, 10, .*\.\.\. is invalid: .* read as \[1, \[10, .*\.\.\.,", + ), + ], + ids=[ + "written", + "3d", + "one-edge", + "shrunk", + "empty-int-axis", + "empty-edge-axis", + "true", + "float-edges", + "long", + ], +) +def test_read_edge_lists_in_regular_grid( + doc: dict[str, JSON], expected: tuple[int | tuple[int, ...], ...], warning: str +) -> None: + """A `regular` chunk grid whose chunk shape mixes chunk sizes with lists of chunk + edge lengths is read as the rectilinear chunk grid it describes, without the + rectilinear chunks flag. `from_dict` warns once, naming the array and the axes, + quoting a bounded part of the chunk shape, and saying that re-saving requires the + flag.""" + with ( + zarr.config.set({"array.rectilinear_chunks": False}), + warnings.catch_warnings(record=True) as record, + ): + warnings.simplefilter("always") + metadata = ArrayV3Metadata.from_dict(doc, path="group/array") + assert metadata.chunk_grid == RectilinearChunkGridMetadata(chunk_shapes=expected) + [message] = [str(w.message) for w in record] + assert re.search(warning, message.removeprefix("Array 'group/array': ")) + assert message.startswith("Array 'group/array': ") + assert ( + "Re-saving the metadata stores that rectilinear chunk grid, so each step that " + "follows requires `zarr.config.set({'array.rectilinear_chunks': True})`. " + ) in message + assert message.endswith(RESAVE_HINT) + # The 1000-edge list is abbreviated: the message is two sentences and two hints, not + # a dump of the edges. + assert len(message) < 1600 + + +def _rejected_without_warning(doc: dict[str, JSON]) -> pytest.ExceptionInfo[Exception]: + with warnings.catch_warnings(): + warnings.simplefilter("error", ZarrUserWarning) + with pytest.raises((TypeError, ValueError)) as info: + ArrayV3Metadata.from_dict(doc) + return info + + +def test_regular_grid_of_only_edge_lists_rejected() -> None: + """A regular chunk shape made only of edge lists was never stored (a rectilinear + chunk grid was), so it is not read as rectilinear.""" + info = _rejected_without_warning(_mixed_doc([6, 20], [[1, 5], [5, 10, 5]])) + assert info.match(re.escape("Dimension 0: chunk edge length must be an int, got [1, 5]")) + + +def test_run_length_encoded_edges_in_regular_grid_rejected() -> None: + """Run-length encoded edges were never stored in a regular chunk shape.""" + info = _rejected_without_warning(_mixed_doc([6, 20], [2, [[5, 2], 10]])) + assert info.match(re.escape("Dimension 1: chunk edge length must be an int, got [[5, 2], 10]")) + + +@pytest.mark.parametrize("edge", [5.5, "5"], ids=["fractional", "string"]) +def test_non_int_edge_in_regular_grid_rejected(edge: object) -> None: + """An edge that is not an integer is reported as such, not blamed on its list.""" + info = _rejected_without_warning(_mixed_doc([6, 20], [2, [edge, 15]])) + assert info.match(re.escape(f"Dimension 1: chunk edge length must be an int, got {edge!r}")) + + +def test_edge_below_one_in_regular_grid_rejected() -> None: + info = _rejected_without_warning(_mixed_doc([6, 20], [2, [0, 20]])) + assert info.match("Dimension 1: chunk edge length must be >= 1, got 0") + + +def test_short_edges_in_regular_grid_rejected() -> None: + info = _rejected_without_warning(_mixed_doc([6, 20], [2, [5, 10]])) + assert info.match("sum to 15 but array shape extent is 20") + + +def _store_mixed_array(path: Path) -> np.ndarray[Any, np.dtype[np.float32]]: + """Write the chunks of the verbatim document and the document itself at `path`.""" + data = np.arange(120, dtype="float32").reshape(6, 20) + with zarr.config.set({"array.rectilinear_chunks": True}): + arr = zarr.create_array(path, shape=data.shape, chunks=(2, (5, 10, 5)), dtype="float32") + arr[...] = data + (path / "zarr.json").write_text(MIXED_REGULAR_GRID_DOC) + return data + + +def _update_attributes(arr: zarr.Array[Any], data: np.ndarray[Any, Any]) -> np.ndarray[Any, Any]: + arr.update_attributes({}) + return data + + +def _write(arr: zarr.Array[Any], data: np.ndarray[Any, Any]) -> np.ndarray[Any, Any]: + arr[:2] = -data[:2] + return np.concatenate([-data[:2], data[2:]]) + + +@pytest.mark.parametrize("store_metadata", [_update_attributes, _write], ids=["re-save", "write"]) +def test_edge_lists_in_regular_grid_round_trip( + tmp_path: Path, + store_metadata: Callable[[zarr.Array[Any], np.ndarray[Any, Any]], np.ndarray[Any, Any]], +) -> None: + """A store holding the verbatim document opens without the rectilinear chunks flag + and reads its data. With the flag, re-saving the metadata, or writing chunks (which + stores the metadata first), stores the rectilinear chunk grid, which then opens + cleanly.""" + path = tmp_path / "mixed.zarr" + data = _store_mixed_array(path) + + with ( + zarr.config.set({"array.rectilinear_chunks": False}), + pytest.warns(ZarrUserWarning, match="read as that rectilinear chunk grid"), + ): + arr = zarr.open_array(path, mode="a") + np.testing.assert_array_equal(arr[...], data) + + with zarr.config.set({"array.rectilinear_chunks": True}): + expected = store_metadata(arr, data) + assert json.loads((path / "zarr.json").read_text())["chunk_grid"] == { + "name": "rectilinear", + "configuration": {"kind": "inline", "chunk_shapes": [2, [5, 10, 5]]}, + } + with warnings.catch_warnings(): + warnings.simplefilter("error", ZarrUserWarning) + reopened = zarr.open_array(path) + np.testing.assert_array_equal(reopened[...], expected) + + +def _store_mixed_group(path: Path) -> None: + """A group at `path` holding the verbatim document at `mixed` and a regular array + `n`, with consolidated metadata that quotes the verbatim document.""" + group = zarr.open_group(path, mode="w") + _store_mixed_array(path / "mixed") + group.create_array("n", data=np.arange(4), chunks=(2,)) + with zarr.config.set({"array.rectilinear_chunks": True}): + zarr.consolidate_metadata(path) + group_doc = json.loads((path / "zarr.json").read_text()) + group_doc["consolidated_metadata"]["metadata"]["mixed"] = json.loads(MIXED_REGULAR_GRID_DOC) + (path / "zarr.json").write_text(json.dumps(group_doc)) + + +def _resize(path: Path) -> None: + zarr.open_array(path / "mixed", mode="a").resize((6, 5)) + + +def _write_chunks(path: Path) -> None: + zarr.open_array(path / "mixed", mode="a")[...] = 1 + + +def _delete_member(path: Path) -> None: + del zarr.open_group(path, mode="a")["n"] + + +def _overwrite_hierarchy(path: Path) -> None: + mixed = zarr.open_array(path / "mixed") + list(zarr.create_hierarchy(store=LocalStore(path), nodes={"n": mixed.metadata}, overwrite=True)) + + +def _overwrite_with_create(path: Path) -> None: + zarr.create(shape=(4,), chunks=[[2, 2]], dtype="int64", store=path / "n", overwrite=True) # type: ignore[arg-type] + + +def _overwrite_with_create_array(path: Path) -> None: + zarr.create_array(path / "n", shape=(4,), chunks=[[2, 2]], dtype="int64", overwrite=True) + + +def _set_group_attribute(path: Path) -> None: + zarr.open_group(path, mode="a").attrs["x"] = 1 + + +@pytest.mark.filterwarnings( + "ignore:.*read as that rectilinear chunk grid:zarr.errors.ZarrUserWarning" +) +@pytest.mark.filterwarnings("ignore:Consolidated metadata is currently not part:UserWarning") +@pytest.mark.parametrize( + "action", + [ + _resize, + _write_chunks, + _overwrite_hierarchy, + _overwrite_with_create, + _overwrite_with_create_array, + ], + ids=["resize", "write", "overwrite-hierarchy", "overwrite-create", "overwrite-create-array"], +) +def test_store_untouched_without_flag(tmp_path: Path, action: Callable[[Path], None]) -> None: + """An operation that would store the rectilinear chunk grid read from the verbatim + document fails without the flag before it deletes or writes anything.""" + path = tmp_path / "group.zarr" + _store_mixed_group(path) + stored = {p: p.read_bytes() for p in path.rglob("*") if p.is_file()} + with ( + zarr.config.set({"array.rectilinear_chunks": False}), + pytest.raises(ValueError, match="experimental and disabled by default"), + ): + action(path) + assert {p: p.read_bytes() for p in path.rglob("*") if p.is_file()} == stored + + +RECTILINEAR_GRID: dict[str, Any] = { + "name": "rectilinear", + "configuration": {"kind": "inline", "chunk_shapes": [2, [5, 10, 5]]}, +} + + +@pytest.mark.filterwarnings("ignore:Consolidated metadata is currently not part:UserWarning") +@pytest.mark.parametrize("member", ["mixed", "sub/mixed"]) +def test_consolidate_edge_lists_in_regular_grid(tmp_path: Path, member: str) -> None: + """Consolidating a group holding the verbatim document copies it as stored, with or + without the flag, and leaves the member's document as it is, so the group opens and + reads the member without the flag. Once the member's metadata is re-saved (which + needs the flag), consolidating copies its rectilinear chunk grid.""" + path = tmp_path / "group.zarr" + zarr.open_group(path, mode="w").create_group("sub") + data = _store_mixed_array(path / member) + for flag in (False, True): + with ( + zarr.config.set({"array.rectilinear_chunks": flag}), + pytest.warns(ZarrUserWarning, match="read as that rectilinear chunk grid"), + ): + zarr.consolidate_metadata(path) + assert (path / member / "zarr.json").read_text() == MIXED_REGULAR_GRID_DOC + assert _consolidated_member(path, 3, member) == json.loads(MIXED_REGULAR_GRID_DOC) + with ( + zarr.config.set({"array.rectilinear_chunks": False}), + pytest.warns(ZarrUserWarning, match="read as that rectilinear chunk grid"), + ): + mixed = zarr.open_group(path, mode="r")[member] + assert isinstance(mixed, zarr.Array) + np.testing.assert_array_equal(mixed[...], data) + + with zarr.config.set({"array.rectilinear_chunks": True}): + with pytest.warns(ZarrUserWarning, match="read as that rectilinear chunk grid"): + zarr.open_array(path / member, mode="a").update_attributes({}) + zarr.consolidate_metadata(path) + assert _consolidated_member(path, 3, member)["chunk_grid"] == RECTILINEAR_GRID + with warnings.catch_warnings(): + warnings.simplefilter("error", ZarrUserWarning) + reopened = zarr.open_group(path, mode="r")[member] + assert isinstance(reopened, zarr.Array) + np.testing.assert_array_equal(reopened[...], data) + + +@pytest.mark.filterwarnings( + "ignore:.*read as that rectilinear chunk grid:zarr.errors.ZarrUserWarning" +) +@pytest.mark.filterwarnings("ignore:Consolidated metadata is currently not part:UserWarning") +@pytest.mark.parametrize("action", [_delete_member, _set_group_attribute], ids=["delete", "attrs"]) +@pytest.mark.parametrize("flag", [False, True]) +def test_group_write_stores_mixed_member_as_stored( + tmp_path: Path, action: Callable[[Path], None], flag: bool +) -> None: + """A group write, with or without the flag, stores the consolidated copy of the + verbatim document as stored and leaves the member's own document as it is: the + group opens and reads the member without the flag.""" + path = tmp_path / "group.zarr" + _store_mixed_group(path) + with zarr.config.set({"array.rectilinear_chunks": flag}): + action(path) + assert _consolidated_member(path, 3, "mixed") == json.loads(MIXED_REGULAR_GRID_DOC) + assert (path / "mixed" / "zarr.json").read_text() == MIXED_REGULAR_GRID_DOC + with zarr.config.set({"array.rectilinear_chunks": False}): + mixed = zarr.open_group(path, mode="r", use_consolidated=True)["mixed"] + assert isinstance(mixed, zarr.Array) + assert mixed.read_chunk_sizes == ((2, 2, 2), (5, 10, 5)) + + +@pytest.mark.filterwarnings("ignore:Consolidated metadata is currently not part:UserWarning") +def test_delete_member_without_flag_keeps_group(tmp_path: Path) -> None: + """A deletion that would store a rectilinear chunk grid in the consolidated metadata + fails without the flag before it deletes anything, and leaves the group listing the + member.""" + path = tmp_path / "group.zarr" + with zarr.config.set({"array.rectilinear_chunks": True}): + group = zarr.open_group(path, mode="w") + group.create_array("r", shape=(6, 20), chunks=(2, (5, 10, 5)), dtype="float32") + group.create_array("n", shape=(2,), chunks=(1,), dtype="int8") + zarr.consolidate_metadata(path) + group = zarr.open_group(path, mode="a", use_consolidated=True) + stored = {p: p.read_bytes() for p in path.rglob("*") if p.is_file()} + with zarr.config.set({"array.rectilinear_chunks": False}): + with pytest.raises(ValueError, match="experimental and disabled"): + del group["n"] + assert group.metadata.consolidated_metadata is not None + assert sorted(group.metadata.consolidated_metadata.metadata) == ["n", "r"] + assert {p: p.read_bytes() for p in path.rglob("*") if p.is_file()} == stored diff --git a/tests/test_metadata/test_v3.py b/tests/test_metadata/test_v3.py index be2564a570..94240c36b9 100644 --- a/tests/test_metadata/test_v3.py +++ b/tests/test_metadata/test_v3.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import re from typing import TYPE_CHECKING import pytest @@ -19,6 +20,7 @@ ARRAY_METADATA_KEYS, ArrayMetadataJSON_V3, ArrayV3Metadata, + RegularChunkGridMetadata, create_chunk_grid_metadata, parse_codecs, parse_dimension_names, @@ -154,6 +156,15 @@ def test_create_chunk_grid_metadata_unknown_dimension_type() -> None: create_chunk_grid_metadata(grid) +def test_regular_chunk_grid_rejects_edge_lists() -> None: + """A regular chunk grid only accepts integer chunk edge lengths.""" + with pytest.raises( + TypeError, + match=re.escape("Dimension 1: chunk edge length must be an int, got (5, 10, 5)"), + ): + RegularChunkGridMetadata(chunk_shape=(2, (5, 10, 5))) # type: ignore[arg-type] + + # --------------------------------------------------------------------------- # Types # --------------------------------------------------------------------------- diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 32b18c5273..8931875125 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -6,7 +6,7 @@ import shutil import tempfile import zipfile -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Literal import numpy as np import pytest @@ -407,3 +407,23 @@ def test_zipstore_close_lifecycle(tmp_path: Path) -> None: lambda: ZipStoreLifecycleMachine(tmp_path), settings=settings(max_examples=50, deadline=None), ) + + +@pytest.mark.parametrize("zarr_format", [2, 3]) +def test_resize_needing_deletes_leaves_array_unchanged( + tmp_path: Path, zarr_format: Literal[2, 3] +) -> None: + """A zip store cannot delete, so a resize that must delete chunks fails before it + stores the new metadata.""" + path = tmp_path / "a.zip" + store = ZipStore(path, mode="w") + arr = create_array(store, shape=(20,), chunks=(5,), dtype="i4", zarr_format=zarr_format) + arr[:] = np.arange(20) + with pytest.raises(NotImplementedError): + arr.resize((5,)) + assert arr.shape == (20,) + store.close() + names = zipfile.ZipFile(path).namelist() + assert len(names) == len(set(names)) + reopened = zarr.open_array(ZipStore(path, mode="r"), mode="r") + np.testing.assert_array_equal(reopened[:], np.arange(20)) diff --git a/tests/test_unified_chunk_grid.py b/tests/test_unified_chunk_grid.py index c20bca9dab..2d968a80c5 100644 --- a/tests/test_unified_chunk_grid.py +++ b/tests/test_unified_chunk_grid.py @@ -15,6 +15,8 @@ import pytest import zarr +from tests.test_metadata.conftest import minimal_metadata_dict_v3 +from zarr.core.buffer import default_buffer_prototype from zarr.core.chunk_grids import ( ChunkGrid, ChunkSpec, @@ -23,7 +25,9 @@ _is_rectilinear_chunks, ) from zarr.core.common import compress_rle, expand_rle +from zarr.core.dtype import UInt8 from zarr.core.metadata.v3 import ( + ArrayV3Metadata, RectilinearChunkGridMetadata, RectilinearChunkGridMetadataJSON, RegularChunkGridMetadata, @@ -102,30 +106,57 @@ def test_dimension_index_to_chunk_last_valid( # --------------------------------------------------------------------------- +_RECTILINEAR_DOC = minimal_metadata_dict_v3( + shape=(30, 50), + chunk_grid={ + "name": "rectilinear", + "configuration": {"kind": "inline", "chunk_shapes": [[10, 20], [25, 25]]}, + }, +) + + @pytest.mark.parametrize( "action", [ - lambda: RectilinearChunkGridMetadata(chunk_shapes=((10, 20), (25, 25))), - lambda: RectilinearChunkGridMetadata.from_dict( - { - "name": "rectilinear", - "configuration": {"kind": "inline", "chunk_shapes": [[10, 20, 30], [50, 50]]}, - } - ), + lambda: ArrayV3Metadata.from_dict(dict(_RECTILINEAR_DOC)), # type: ignore[arg-type] + lambda: ArrayV3Metadata( + shape=(30, 50), + data_type=UInt8(), + chunk_grid=RectilinearChunkGridMetadata(chunk_shapes=((10, 20), (25, 25))), + chunk_key_encoding={"name": "default"}, + fill_value=0, + codecs=[{"name": "bytes"}], + attributes=None, + dimension_names=None, + ).to_buffer_dict(default_buffer_prototype()), lambda: zarr.create_array(MemoryStore(), shape=(30,), chunks=[[10, 20]], dtype="int32"), ], - ids=["constructor", "from_dict", "create_array"], + ids=["read", "store", "create_array"], ) def test_rectilinear_feature_flag_blocked(action: Any) -> None: - """Rectilinear chunk operations raise ValueError when the feature flag is disabled""" + """Reading or storing an array metadata document that declares a rectilinear chunk + grid raises ValueError when the feature flag is disabled.""" with zarr.config.set({"array.rectilinear_chunks": False}): with pytest.raises(ValueError, match="experimental and disabled by default"): action() -def test_rectilinear_feature_flag_enabled() -> None: - """Rectilinear chunk grid construction succeeds when the feature flag is enabled""" +def test_rectilinear_feature_flag_names_stored_array() -> None: + """Opening a stored array whose document the flag refuses names the array.""" + store = MemoryStore() with zarr.config.set({"array.rectilinear_chunks": True}): + zarr.create_array(store, name="r", shape=(30,), chunks=[[10, 20]], dtype="int32") + with ( + zarr.config.set({"array.rectilinear_chunks": False}), + pytest.raises(ValueError, match="experimental and disabled by default") as info, + ): + zarr.open_array(store, path="r") + assert info.value.__notes__ == [f"Array {str(store) + '/r'!r}: nothing was read."] + + +def test_rectilinear_metadata_classes_not_gated() -> None: + """The flag gates stored documents, not the chunk grid metadata classes.""" + with zarr.config.set({"array.rectilinear_chunks": False}): grid = RectilinearChunkGridMetadata(chunk_shapes=((10, 20), (25, 25))) assert grid.ndim == 2 @@ -533,6 +564,7 @@ def test_rle_roundtrip() -> None: ([[-10, 2]], "Chunk edge length must be >= 1"), ([[5, 0]], "RLE repeat count must be >= 1"), ([[5, -1]], "RLE repeat count must be >= 1"), + ([[5, 2, 1]], r"RLE entries must be an integer or \[size, count\], got \[5, 2, 1\]"), ], ids=[ "zero-edge", @@ -541,6 +573,7 @@ def test_rle_roundtrip() -> None: "negative-rle-size", "zero-rle-count", "negative-rle-count", + "rle-entry-of-three", ], ) def test_rle_expand_rejects_invalid(rle_input: list[Any], match: str) -> None: