| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Previously rectilinear chunk grids and regular chunk grids normalized chunks inconsistently. This change ensures that chunk specifications are always normalized by the same routines in all cases. This change also ensures that chunks=(-1, ...) consistently normalizes to a full length chunk along that axis.
Codecov Report❌ Patch coverage is 61.53846% with 35 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #3899 +/- ##
==========================================
- Coverage 92.98% 92.70% -0.28%
==========================================
Files 87 87
Lines 11246 11261 +15
==========================================
- Hits 10457 10440 -17
- Misses 789 821 +32
|
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #3899 +/- ##
==========================================
+ Coverage 93.23% 93.26% +0.03%
==========================================
Files 87 87
Lines 11696 11721 +25
==========================================
+ Hits 10905 10932 +27
+ Misses 791 789 -2
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was a problem hiding this comment.
I really like the direction of the refactor.
I found the description of the PR somewhat misleading. The bug fix (make chunk normalization properly handle -1) is totally unrelated to the additional of rectilinear chunk support; the bug report showed the issue on prior releases. The rectilinear chunk addition made the pre-existing jank related to duplicated normalization logic worse.
there are a few cases in the deprecated Array.create() method that possibly regress in this PR:
def _create_deprecated(**kwargs):
"""Call the deprecated Array.create(), suppressing the deprecation warning."""
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)
return zarr.Array.create(**kwargs)
def test_deprecated_underspecified_chunks_padded():
"""Fewer chunk dims than shape dims — missing dims padded from shape."""
arr = _create_deprecated(store={}, shape=(100, 20, 10), chunks=(30,), dtype="uint8")
assert arr.metadata.chunk_grid.chunk_shape == (30, 20, 10)
def test_deprecated_underspecified_chunks_with_none():
"""Partial chunks with None — padded from shape."""
arr = _create_deprecated(store={}, shape=(100, 20, 10), chunks=(30, None), dtype="uint8")
assert arr.metadata.chunk_grid.chunk_shape == (30, 20, 10)
def test_deprecated_none_per_dimension_sentinel():
"""None inside chunks tuple means 'span the full axis'."""
arr = _create_deprecated(store={}, shape=(100, 10), chunks=(10, None), dtype="uint8")
assert arr.metadata.chunk_grid.chunk_shape == (10, 10)I'm not sure if these were intentional API design choices, versus quirks in the old API. It may be a good time to remove deprecated functions, as a separate PR, first to reduce the surface area for potential regressions when fixing/adding functionality to the new API.
Sorry, something went wrong.
good catch, the change that broke -1 normalization was this one: #2761. We basically forked array creation routines and didn't reach feature / testing parity with the new one 🤦 I don't see value in supporting cases like this, other than backwards compatibility.
Are there any non-deprecated functions that supported this? |
Sorry, something went wrong.
💯 |
Sorry, something went wrong.
I couldn't find any non-deprecated cases of supporting underspecified chunks (fewer than the number of dims) or using None as a sentinel value like -1. |
Sorry, something went wrong.
|
#3903 removes the deprecated methods |
Sorry, something went wrong.
|
the latest changes make the representation of chunks recursive, in order to express nested sharding. This is future-proofing the design here against the possibility that we give our high-level routines a simple way to declare nested sharding. |
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch normalize_chunks_1d to return np.ndarray[tuple[int], np.dtype[np.int64]] instead of tuple[int, ...]. The uniform-chunks branch now constructs in O(1) via np.full, recovering the single-allocation fast path that regressed when the canonical ChunksTuple representation was introduced. Update create_chunk_grid_metadata in v3.py to convert arrays to tuples of ints before passing to is_regular_nd and RectilinearChunkGridMetadata, keeping those downstream functions' signatures unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit 14788aa was meant to only touch chunk_grids.py (Tasks 2+3 of the ChunksTuple → int64-array refactor). It also modified create_chunk_grid_metadata in v3.py — that change belongs to a later task with a different approach (widen annotations rather than materialize tuples) and a better perf profile. Restoring v3.py to its pre-14788aa state. The proper v3.py change will land in a follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…unk_grid_metadata Accept ndarray[int64] in is_regular_1d/is_regular_nd alongside Sequence[int]. Cast only the first element per axis on the regular path so D ints are allocated rather than N*D. Materialize fully on the rectilinear path because _validate_chunk_shapes checks isinstance(dim_spec, int) which rejects np.int64. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review on PR #236, normalize_chunks_1d and normalize_chunks_nd now take object and narrow with explicit isinstance/identity checks instead of growing an ad-hoc union annotation. Behavior changes: - A per-dimension bool chunk size (e.g. chunks=(True, 5)) is now rejected with an informative ValueError. Previously bool being a subclass of int let True through as a silent size-1 chunk — the exact behavior the zarr-developers#3899 release notes say was removed. - Strings/bytes and non-iterable values (e.g. chunks=2.5 or chunks=(2.5, 5)) now raise informative TypeErrors instead of bare crashes (list(2.5), len(generator)) or a misleading dimension-count error for whole-argument strings. - Generator inputs to normalize_chunks_nd are now materialized and accepted, both as the whole argument and as a per-dimension size list in rectilinear specs, consistent with numpy-style APIs. The type: ignore[call-overload] on int(c) is no longer needed after proper narrowing. Assisted-by: ClaudeCode:claude-fable-5
…evelopers#4272) A nested-sequence chunks spec is an explicit rectilinear request, but since zarr-developers#3899 the stored grid kind was chosen from the edge *values* (is_regular_nd) rather than the form of the request. A spec like [[10, 10, 4]] -- uniform plus a short trailing chunk -- was therefore silently collapsed to a regular grid. The two grids behave identically at creation time but diverge under resize: a regular grid extends the uniform pattern while a rectilinear grid appends an edge, so an append-only workload gets a different (chunk-rewriting) layout from the one it asked for, with no warning and nothing in the metadata recording the substitution. create_chunk_grid_metadata gains a keyword-only requested_rectilinear flag; both v3 creation call sites derive it from the raw user input: a nested-sequence chunks (or shards) spec requests rectilinear, flat and "auto" specs keep inferring from the edge values. This restores 3.2.x semantics (resolve_chunks dispatched on input syntax). Fixes zarr-developers#4272
| Back | FazBrowse Home | New Git URL |
The addition of rectilinear chunks left us with some jank in our internal chunk normalization logic. We had a lot of redundant chunk normalization routines, and we also weren't handling user input correctly, e.g. #3898. We need some internal changes to ensure that user input is consistently handled regardless of whether we are generating regular chunks or irregular chunks. That's what this PR does. Also, this PR closes #3898
I will give my summary, then a summary generated by claude.
My summary
ChunksTuple
This PR addresses this by introducing a canonical internal representation of the fully normalized chunk layout for an array, which is a tuple called ChunksTuple. Feel free to suggest better names.
ChunksTuple is just tuple[tuple[int, ...], ...], i.e. a representation compatible with regular or irregular chunks, but I wrap this type in NewType.
I use NewType because tuples of tuples of ints can be very easily confused with tuples of ints (regular chunks), or tuples of tuples of tuples of ints (e.g., rectilinear chunking with RLE). So I think it's helpful to be defensive here and reduce ambiguity.
There are 2 functions that produce ChunksTuple:
ResolvedChunking
ChunksTuple is used in ResolvedChunking (bad name, I would rather use ChunkSpec but that's in use already), which is this:
ResolvedChunking is what you get when you jointly normalize the chunks and shards keyword arguments to create_array.
I introduce some new terminology here for internal purposes. outer_chunks denotes the shape of the chunks qua stored objects, and inner_chunks denotes the shape of the subchunks inside an outer chunk, if that outer chunk uses sharding. If the outer chunk doesn't use sharding, then inner_chunks is None.
These two data types are used to consolidate our chunk normalization routines.
Claude's Summary
Refactors chunk and shard handling during array creation to fix a naming ambiguity where chunk_shape meant "outer grid partition" without sharding but "inner sub-chunk" with sharding, silently changing meaning based on context.
Introduces a three-layer architecture for chunk resolution:
Normalization — normalize_chunks_nd and guess_chunks convert raw user input into ChunksTuple, a NewType-branded tuple[tuple[int, ...], ...] that represents both regular and rectilinear chunks uniformly. This is the only boundary between untyped user input and the internal representation.
Resolution — resolve_outer_and_inner_chunks takes a ChunksTuple (the user's chunks=) and raw shard input (shards=), and returns a ResolvedChunking NamedTuple with two unambiguous fields:
Metadata construction — create_chunk_grid_metadata takes a ChunksTuple and dispatches to RegularChunkGridMetadata or RectilinearChunkGridMetadata based on whether the chunks are uniform.
Key design decisions
ChunksTuple as a NewType: Zero runtime cost, but the type checker prevents accidentally passing raw user input where normalized chunks are expected. Both regular and rectilinear chunks use the same representation — regular is just the case where each inner tuple has uniform values.
inner_chunks: None models capability, not configuration: An unsharded chunk is opaque (read the whole thing or nothing). A shard has internal structure (an index that enables sub-chunk addressing). None means "this chunk has no internal structure" — it's not a flag you toggle, it's the absence of a capability.
normalize_chunks_nd rejects None: Top-level None means "auto" everywhere else in the codebase. Having normalize_chunks_nd silently treat it as "span all" would be a bug waiting to happen. Callers must use guess_chunks for auto-chunking.
Rectilinear shard detection absorbed into resolve_outer_and_inner_chunks: The function handles all shard input forms (None, "auto", dict, flat tuple, nested sequence) internally, eliminating the shards_for_partition / rectilinear_shard_meta dance that callers previously had to manage.
Changes by file
src/zarr/core/chunk_grids.py
src/zarr/core/metadata/v3.py
src/zarr/core/array.py
tests/conftest.py
tests/test_chunk_grids.py
tests/test_array.py