diff --git a/docs/release-notes.md b/docs/release-notes.md index 3fd8a5f360..d3e24f5d67 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -37,6 +37,13 @@ the array's shape), and `None` as a per-dimension chunk size. These all now raise informative errors. Also fix chunk handling for 0-length array dimensions, and add explicit rejection of 0-length chunks. ([#3899](https://github.com/zarr-developers/zarr-python/issues/3899)) +- Further hardened chunk normalization: a per-dimension `None` chunk size now + raises an informative `ValueError` directing users to the `-1` sentinel + (previously an uninformative `TypeError`), per-dimension boolean chunk sizes + are rejected instead of `True` silently producing size-1 chunks, and strings + and other non-iterable chunk inputs raise informative `TypeError`s instead of + bare crashes. Generator inputs to chunk normalization are now materialized + and accepted. ([#4177](https://github.com/zarr-developers/zarr-python/issues/4177)) - Handle missing consolidated metadata in leaf Group nodes. ([#3954](https://github.com/zarr-developers/zarr-python/issues/3954)) - Corrected the JSON type definitions for the `numpy.datetime64` and `numpy.timedelta64` data types in Zarr V3 metadata: the `configuration` object diff --git a/src/zarr/core/array.py b/src/zarr/core/array.py index f75ef72415..8aba35454e 100644 --- a/src/zarr/core/array.py +++ b/src/zarr/core/array.py @@ -4461,7 +4461,17 @@ async def init_array( "chunks=(inner_size, ...), shards=[[shard_sizes], ...]" ) - # Normalize the user's chunks into canonical ChunksTuple form + # Normalize the user's chunks into canonical ChunksTuple form. + # Auto-chunking is an API-level concept, so the guidance toward it is + # raised here rather than in the mechanical normalizer. Validate through + # an object-typed view: None/True are outside ChunksLike but reachable + # from untyped callers. + chunks_input: object = chunks + if chunks_input is None or chunks_input is True: + raise ValueError( + f'{chunks!r} is not a valid chunk input. Use chunks="auto" or omit the chunks ' + "argument for automatic chunking, or pass an int / iterable of ints." + ) if chunks == "auto": max_bytes = None if shards is None else SHARDED_INNER_CHUNK_MAX_BYTES diff --git a/src/zarr/core/chunk_grids.py b/src/zarr/core/chunk_grids.py index 2cb9762775..fc28f7526c 100644 --- a/src/zarr/core/chunk_grids.py +++ b/src/zarr/core/chunk_grids.py @@ -6,6 +6,7 @@ import numbers import operator import warnings +from collections.abc import Iterable from dataclasses import dataclass, field from functools import reduce from typing import ( @@ -31,7 +32,7 @@ from zarr.errors import ZarrUserWarning if TYPE_CHECKING: - from collections.abc import Iterable, Iterator, Sequence + from collections.abc import Iterator, Sequence from zarr.core.array import ShardsLike from zarr.core.metadata import ArrayMetadata @@ -717,18 +718,33 @@ def _guess_regular_chunks( return tuple(int(x) for x in chunks) -def normalize_chunks_1d( - chunks: int | Iterable[object], span: int -) -> np.ndarray[tuple[int], np.dtype[np.int64]]: +def normalize_chunks_1d(chunks: object, span: int) -> np.ndarray[tuple[int], np.dtype[np.int64]]: """ Normalize a one-dimensional chunk specification into a 1D int64 array of chunk sizes that cover the span. - `-1` means "one chunk covering the entire span." + Accepts `object` and narrows internally: `None` and `bool` are rejected + with informative errors (`-1` is the sentinel for "one chunk covering the + entire span"), strings and other non-iterables are rejected, and any other + iterable (including a generator) is materialized as an explicit list of + chunk sizes. For an integer chunk size, all chunks are uniform — the last chunk may overhang the span. The actual data extent of each chunk is determined by the chunk grid at runtime, not by this function. """ + if chunks is None: + raise ValueError( + "None is not a valid chunk size for a dimension. " + "Use -1 for a single chunk covering the full extent of an axis." + ) + # bool is a subclass of int, so without this guard True would silently + # pass through the integer branch below as chunk size 1. + if chunks is True or chunks is False: + raise ValueError( + f"{chunks} is not a valid chunk size for a dimension. " + "Chunk sizes must be positive integers, or -1 for a single chunk " + "covering the full extent of an axis." + ) if chunks == -1: return np.array([span], dtype=np.int64) if isinstance(chunks, int): @@ -738,39 +754,45 @@ def normalize_chunks_1d( return np.array([chunks], dtype=np.int64) n = ceildiv(span, chunks) return np.full(n, chunks, dtype=np.int64) - else: - chunk_list = list(chunks) - if not chunk_list: - raise ValueError("Chunk specification must not be empty") - non_int = [ - (idx, c) for idx, c in enumerate(chunk_list) if not isinstance(c, numbers.Integral) - ] - if non_int: - non_int_idxs, non_int_vals = [*zip(*non_int, strict=False)] - raise TypeError( - f"Each chunk size must be an integer; got non-integer element(s) {non_int_vals!r} " - f"at indices {non_int_idxs!r}. Chunk sizes must be declared as a flat sequence of " - f"positive integers (e.g. [3, 3, 1])." - ) - ints: list[int] = [int(c) for c in chunk_list] # type: ignore[call-overload] - if any(c <= 0 for c in ints): - raise ValueError(f"All chunk sizes must be positive, got {ints}") - if sum(ints) != span: - raise ValueError(f"Chunk sizes {ints} do not sum to span {span}") - return np.asarray(ints, dtype=np.int64) + # str/bytes are iterable but never a valid chunk specification + if isinstance(chunks, (str, bytes)) or not isinstance(chunks, Iterable): + raise TypeError( + f"{chunks!r} is not a valid chunk size for a dimension. " + "Expected an int or an iterable of ints." + ) + chunk_list = list(chunks) + if not chunk_list: + raise ValueError("Chunk specification must not be empty") + non_int = [(idx, c) for idx, c in enumerate(chunk_list) if not isinstance(c, numbers.Integral)] + if non_int: + non_int_idxs, non_int_vals = [*zip(*non_int, strict=False)] + raise TypeError( + f"Each chunk size must be an integer; got non-integer element(s) {non_int_vals!r} " + f"at indices {non_int_idxs!r}. Chunk sizes must be declared as a flat sequence of " + f"positive integers (e.g. [3, 3, 1])." + ) + ints: list[int] = [int(c) for c in chunk_list] + if any(c <= 0 for c in ints): + raise ValueError(f"All chunk sizes must be positive, got {ints}") + if sum(ints) != span: + raise ValueError(f"Chunk sizes {ints} do not sum to span {span}") + return np.asarray(ints, dtype=np.int64) def normalize_chunks_nd( - chunks: Any, + chunks: object, shape: tuple[int, ...], ) -> ChunksTuple: """ Normalize a chunk specification into a `ChunksTuple`. This is a mechanical transformation — no heuristics, no guessing. - Handles `False` ("all data in one chunk"), scalar ints, `-1` sentinels (one chunk - per dimension covering the full span), and explicit per-dimension lists - of chunk sizes (regular or rectilinear). + Accepts `object` and narrows internally: `False` ("all data in one chunk"), + scalar ints, `-1` sentinels (one chunk per dimension covering the full + span), and per-dimension iterables of chunk sizes (regular or + rectilinear). Any non-string iterable — including a generator — is + materialized before use; strings and non-iterables are rejected with + informative errors. For auto-chunking, use `guess_chunks` which returns a `ChunksTuple` directly. `chunks=None` and `chunks=True` are rejected @@ -779,7 +801,8 @@ def normalize_chunks_nd( """ if chunks is None or chunks is True: raise ValueError( - f'{chunks!r} is not a valid chunk input. Use chunks=None or chunks="auto" from the top-level API for auto-chunking, or pass an int / tuple of ints.' + f"{chunks!r} is not a valid chunk input. " + "Expected an int, an iterable of ints, or False." ) # handle no chunking @@ -788,16 +811,24 @@ def normalize_chunks_nd( # handle 1D convenience form. bool is excluded above so this only catches actual ints. if isinstance(chunks, numbers.Integral): - chunks = tuple(int(chunks) for _ in shape) + chunks_tuple: tuple[Any, ...] = tuple(int(chunks) for _ in shape) + elif isinstance(chunks, Iterable) and not isinstance(chunks, (str, bytes)): + # materialize before use so generators are supported and len() is safe; + # str/bytes are iterable but never a valid chunk specification + chunks_tuple = tuple(chunks) + else: + raise TypeError( + f"{chunks!r} is not a valid chunk input. Expected an int or an iterable of ints." + ) # handle bad dimensionality - if len(chunks) != len(shape): + if len(chunks_tuple) != len(shape): raise ValueError( - f"chunks has {len(chunks)} dimensions but shape has {len(shape)} dimensions" + f"chunks has {len(chunks_tuple)} dimensions but shape has {len(shape)} dimensions" ) return ChunksTuple( - tuple(normalize_chunks_1d(c, span=s) for c, s in zip(chunks, shape, strict=True)) + tuple(normalize_chunks_1d(c, span=s) for c, s in zip(chunks_tuple, shape, strict=True)) ) diff --git a/tests/test_api.py b/tests/test_api.py index 1b4414ae63..f074a14bbf 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -79,7 +79,7 @@ def test_create(memory_store: Store) -> None: z = create(shape=(400.5, 100), store=store, overwrite=True) # type: ignore[arg-type] # create array with float chunk shape - with pytest.raises(TypeError, match="'float' object is not iterable"): + with pytest.raises(TypeError, match="is not a valid chunk size for a dimension"): z = create(shape=(400, 100), chunks=(16, 16.5), store=store, overwrite=True) # type: ignore[arg-type] diff --git a/tests/test_chunk_grids.py b/tests/test_chunk_grids.py index b730a43901..b03ad99e2e 100644 --- a/tests/test_chunk_grids.py +++ b/tests/test_chunk_grids.py @@ -1,8 +1,10 @@ +from collections.abc import Callable from typing import Any import numpy as np import pytest +import zarr from tests.conftest import Expect, ExpectFail from zarr.core.chunk_grids import ( ChunkLayout, @@ -142,6 +144,35 @@ def test_chunk_layout_nested() -> None: id="negative-uniform", msg="Chunk size must be positive", ), + ExpectFail( + input=(None, 100), + exception=ValueError, + id="none-chunk-size", + msg="None is not a valid chunk size for a dimension", + ), + # bool is a subclass of int; without an explicit guard, True would + # silently pass through the int branch as chunk size 1. + ExpectFail( + input=(True, 100), + exception=ValueError, + id="bool-chunk-size", + msg="True is not a valid chunk size for a dimension", + ), + # Non-iterable, non-int values must get an informative error rather + # than a bare crash from list(). + ExpectFail( + input=(2.5, 100), + exception=TypeError, + id="float-chunk-size", + msg="Expected an int or an iterable of ints", + ), + # Strings are iterable but never a valid chunk size. + ExpectFail( + input=("5", 100), + exception=TypeError, + id="string-chunk-size", + msg="Expected an int or an iterable of ints", + ), ExpectFail(input=([], 100), exception=ValueError, id="empty-list", msg="must not be empty"), ExpectFail( input=([10, -1, 10], 100), @@ -206,7 +237,44 @@ def test_normalize_chunks_1d_errors(case: ExpectFail[tuple[Any, int]]) -> None: id="true", msg="True is not a valid chunk input", ), - ExpectFail(input=("foo", (100,)), exception=ValueError, id="string", msg="dimensions"), + # A per-dimension `None` (dask-style "full extent") is rejected with a + # pointer to the `-1` sentinel. + ExpectFail( + input=((None, 5), (100, 100)), + exception=ValueError, + id="per-dimension-none", + msg="Use -1 for a single chunk covering the full extent of an axis", + ), + # A per-dimension `True` must not silently become chunk size 1. + ExpectFail( + input=((True, 5), (100, 100)), + exception=ValueError, + id="per-dimension-true", + msg="True is not a valid chunk size for a dimension", + ), + # A non-iterable per-dimension value gets an informative error. + ExpectFail( + input=((2.5, 5), (100, 100)), + exception=TypeError, + id="per-dimension-float", + msg="Expected an int or an iterable of ints", + ), + # Strings are iterable but rejected outright. Note that `chunks="auto"` + # is intercepted by the callers before normalization, so a string here + # always means invalid input. + ExpectFail( + input=("foo", (100,)), + exception=TypeError, + id="string", + msg="not a valid chunk input", + ), + # A non-iterable whole argument gets an informative error. + ExpectFail( + input=(2.5, (100,)), + exception=TypeError, + id="non-iterable", + msg="Expected an int or an iterable of ints", + ), ExpectFail( input=((100, 10), (100,)), exception=ValueError, id="too-many-dims", msg="dimensions" ), @@ -253,3 +321,79 @@ def test_normalize_chunks_1d_returns_int64_array( assert result.dtype == np.int64 assert result.ndim == 1 assert result.tolist() == case.output + + +@pytest.mark.parametrize( + ("chunks", "expected"), + [ + (-1, (10, 10)), + ((-1, -1), (10, 10)), + ((-1, 5), (10, 5)), + ("auto", (10, 10)), + (False, (10, 10)), + ([5, 5], (5, 5)), + (np.int64(5), (5, 5)), + ], + ids=[ + "scalar-minus-one", + "tuple-minus-one", + "mixed-minus-one", + "auto", + "false-single-chunk", + "list", + "numpy-int", + ], +) +def test_create_array_valid_chunk_forms(chunks: Any, expected: tuple[int, ...]) -> None: + """Valid chunk specifications produce arrays with the expected chunk shape.""" + arr = zarr.create_array(store={}, shape=(10, 10), dtype="i4", chunks=chunks) + assert arr.chunks == expected + + +@pytest.mark.parametrize( + ("make_chunks", "shape", "expected"), + [ + # whole-argument generator of per-dimension sizes + (lambda: (10 for _ in range(2)), (100, 10), ((10,) * 10, (10,))), + # rectilinear spec with a generator as one dimension's sizes + (lambda: ((c for c in (60, 40)), [5, 5]), (100, 10), ((60, 40), (5, 5))), + ], + ids=["whole-argument-generator", "per-dimension-generator"], +) +def test_normalize_chunks_nd_accepts_generators( + make_chunks: Callable[[], Any], + shape: tuple[int, ...], + expected: tuple[tuple[int, ...], ...], +) -> None: + """Generator inputs are materialized and normalized like any other iterable.""" + _assert_chunks_equal(normalize_chunks_nd(make_chunks(), shape), expected) + + +def test_create_rejects_per_dimension_true_chunk() -> None: + """A per-dimension True in `chunks` via `zarr.create` must not become chunk size 1.""" + with pytest.raises(ValueError, match="True is not a valid chunk size for a dimension"): + zarr.create(store={}, shape=(10, 10), dtype="i4", chunks=(True, 5)) + + +def test_create_rejects_per_dimension_none_chunk() -> None: + """A per-dimension None in `chunks` via `zarr.create` raises an informative error.""" + with pytest.raises( + ValueError, match="Use -1 for a single chunk covering the full extent of an axis" + ): + zarr.create(store={}, shape=(10, 10), dtype="i4", chunks=(None, 5)) # type: ignore[arg-type] + + +def test_create_array_rejects_per_dimension_none_chunk() -> None: + """A per-dimension None in `chunks` via `zarr.create_array` raises an informative error.""" + with pytest.raises( + ValueError, match="Use -1 for a single chunk covering the full extent of an axis" + ): + zarr.create_array(store={}, shape=(10, 10), dtype="i4", chunks=(None, 5)) # type: ignore[arg-type] + + +def test_create_array_rejects_chunks_none() -> None: + """`chunks=None` via `zarr.create_array` points the user at `chunks="auto"`.""" + with pytest.raises( + ValueError, match='Use chunks="auto" or omit the chunks argument for automatic chunking' + ): + zarr.create_array(store={}, shape=(10, 10), dtype="i4", chunks=None) # type: ignore[arg-type]