Repository navigation
Conversation
Documentation build overview
24 files changed ·
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4352 +/- ##
==========================================
+ Coverage 94.84% 94.85% +0.01%
==========================================
Files 96 96
Lines 13997 14056 +59
==========================================
+ Hits 13275 13333 +58
- Misses 722 723 +1
🚀 New features to boost your workflow:
|
|
this PR has a performance impact for codec creation with rectilinear chunk grids, where we loop over every chunk shape defined in a grid, checking each one for compatibility with the entire codec chain. I'm working on a mitigation. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
requesting review because this adds to codec API |
| """ | ||
| return chunk_spec | ||
|
|
||
| def resolve_chunk_grid( |
There was a problem hiding this comment.
@ilan-gold this is new API on the codec abc. we need the whole chunk grid to be in-scope for resolution because combining a rectilinear chunk grid with e.g. a transpose or reshape codec creates a large number of chunk shapes, and each chunk size needs to be consistent with the subchunk grid of e.g. a sharding codec. The meaning of "consistent" will in theory vary with the downstream codecs, but for sharding we just need to ensure that each incoming chunk shape tiles the sharding chunk grid.
this would be a lot easier if the sharding codec chunk grid was semi-regular.
kokhlo
left a comment
kokhlo
left a comment
There was a problem hiding this comment.
Verified review — I duplicated the issue #4354 scenario on main (7291469) and on this branch (5a52e85) before reading the diff closely; findings below are all reproduced locally, not read off the code.
Root cause confirmed independently
On main, ArrayV3Metadata.__init__ validates every codec against the array-level chunk_grid (v3.py, old loop: c.validate(shape=spec.shape, dtype=spec.dtype, chunk_grid=chunk_grid_parsed)), and ShardingCodec.validate checks every rectilinear edge of that original grid against chunk_shape. After a shape/rank-changing array→array codec the geometry no longer matches the original grid, so main produces axis-confused false rejections and, worse, would accept chains that read back wrong data. Reproduced on main:
zarr 3.4.1.dev12+g7291469 [main]
rectilinear chunks ((30,),(34,)) + transpose(1,0) + sharding(chunk_shape=(17,15))
REJECTED: ValueError: Chunk edge length 30 in dimension 0 is not divisible by the shard's inner chunk size 17.
(17 divides the transposed dim-0 edge 34, 15 divides the transposed dim-1 edge 30 — the chain is valid; the error is checking the untransposed grid.) Same construction on this branch is accepted and round-trips metadata:
zarr 3.4.1.dev26+g5a52e85 [pr]
metadata ACCEPTED
JSON roundtrip of metadata: OK
Also verified on the branch: representative-chunk evolution cost stays flat (3-d rectilinear, 100 distinct edges/axis, Delta+Bytes+Zstd → 0.0001s metadata construction), the chunk-local accept path ends in the new loud _get_chunks_per_shard error rather than silent mislayout, and tests/test_codecs/test_codec_chain_validation{,_properties}.py pass (40 passed). My a-priori gap candidates — inner sharding grids and create_codec_pipeline's old all-ones placeholder — are both addressed (evolve_and_validate_codecs inside evolve_from_array_spec; representative-chunk evolution in array.py).
Trust-boundary probe: declaration spoofing fails on all three paths
I tried to produce a codec that lies via resolve_chunk_grid (identity declaration while reshaping at runtime), through subclassing rather than a from-scratch codec, since that's the path the docstring's distrust rules exist for:
- Subclassing
TransposeCodecand overriding onlyresolve_metadata→ correctly treated as undeclared (_defining_classMRO walk:metadata_owner is not grid_owner→None→ chunk-local). - Subclassing
TransposeCodecwith an inconsistentresolve_chunk_gridoverride → caught by the repr-disagreement check. - Subclassing a no-
resolve_metadatacodec (GzipCodec) that reshapes at runtime → inherited identity declaration is trusted, but this is the right call: for every shipped codec that inherits the default, the declaration is structurally true, and the runtime divisibility guard catches the damage on the sharding path.
No hole found. One consideration, non-blocking:
PackBits.resolve_metadatareturns a 1-d packed shape, so it now drives the chain chunk-local on rectilinear grids even though the edge structure is preserved in a flattened form; fine, but if a future grid type could express "flattened edges" it would restore exact validation behindPackBits.
One gap worth closing before merge
This PR never references #4354 — body, changelog entry (changes/4352.bugfix.md), and docs have no mention. It was opened ~1 hour before the issue and links only the fork PR, so GitHub won't cross-link them and the issue has no visible resolution path. Since this branch is the resolution of that issue, adding "#4354" to the changelog entry (or the body) would close the loop.
The design matches zarrs' grid-mapping approach, the chunk-local trade-off is documented on all three surfaces (docstrings, extending guide, changelog) with both failure directions stated, and CI is green. Happy to re-review if the reference is added or anything above needs rework.
ilan-gold
left a comment
ilan-gold
left a comment
There was a problem hiding this comment.
zarr has done breaking changes before, and this seems kind of important. It feels like there should be a safer way to do this than allowing users to create technically invalid arrays only to fail at read/write time.
Does that make sense?
| self, *, shape: tuple[int, ...], chunk_grid: ChunkGridMetadata | ||
| ) -> tuple[tuple[int, ...], ChunkGridMetadata]: | ||
| """Permute the array shape and the per-axis chunk edges by `order`.""" | ||
| from zarr.core.metadata.v3 import RectilinearChunkGridMetadata, RegularChunkGridMetadata |
There was a problem hiding this comment.
Why does this need to be a local import?
| makes the remaining chain "chunk-local": later codecs are validated | ||
| against one representative chunk only, so a chain that is invalid for | ||
| some other chunk shape is accepted at creation and fails when such a | ||
| chunk is first encoded or decoded (see | ||
| `zarr.core.metadata.v3.evolve_and_validate_codecs`). |
There was a problem hiding this comment.
Why do we do not fail completely in this case? To protect people who have already written Codecs that don't implement chunk grid resolution from unnecessary errors?
Validate each codec against the chunk geometry produced by the codecs before it, including the inner codecs of a sharding codec and the actual fill value. A sharding codec after a transpose must divide the transposed chunks; it used to be checked against the untransposed grid, which could accept chains that read back wrong data and reject valid ones. Geometry is threaded as a whole chunk grid through the new `BaseCodec.resolve_chunk_grid`, following zarrs, so the cost does not depend on the number of distinct chunk shapes. On a rectilinear grid, a codec that overrides `resolve_metadata` without declaring a grid makes the rest of its chain chunk-local: later codecs see one representative chunk, and `ShardingCodec` checks divisibility at encode and decode time so a shape validation never saw raises instead of corrupting data. Rectilinear codec pipelines are evolved against the same representative chunk shape, replacing an all-ones placeholder. Assisted-by: ClaudeCode:claude-opus-5-5
`ShardingCodec.validate` also validated its inner codecs, resolving them against a zero fill value it made up. `scale_offset` encodes the fill value when it resolves, so a non-zero offset on an unsigned dtype was rejected even though the array is valid. `evolve_from_array_spec` now validates the same chain with the real spec, so the check in `validate` is removed. Assisted-by: ClaudeCode:claude-opus-5-5
A chunk holding only the fill value is never stored, and a representative chunk can lie past the array's extent. The changelog now also says that a stored array with an invalid chain raises on open. Assisted-by: ClaudeCode:claude-opus-5-5
5a52e85 to
60cdf79
Compare
…hing On a chunk-local array, a write spanning good and bad shards raised on the bad one while the good one was still stored shortly after the error. Writes to these arrays now validate the distinct chunk shapes they touch before any I/O. Arrays whose chains were validated exactly skip the check. Assisted-by: ClaudeCode:claude-opus-5-5
🤖 AI text below 🤖
Validate each codec in a chain against the chunk geometry produced by the codecs before it, including the inner codecs of a sharding codec and the actual fill value. For example, a sharding codec placed after a transpose must now divide the transposed chunks; previously it was checked against the untransposed chunk grid, which could accept chains that read back wrong data and reject valid ones.
Fixes #4354.
Codec validation needs the geometry and dtype produced by preceding codecs.
evolve_and_validate_codecsthreads chunk specs through evolution, validation, and metadata resolution. Inner sharding chains use the actual inner spec, including its fill value.Geometry is threaded as a whole chunk grid
Following zarrs (
encoded_chunk_grid/ chunk-local grids), codecs describe how they map the chunk grid through a newBaseCodec.resolve_chunk_grid(shape=..., chunk_grid=...)method, and validation carries the grid through the chain rather than enumerating chunk shapes. Cost is proportional to the number of codecs and dimensions, independent of how many distinct chunk shapes a rectilinear grid has.resolve_metadatadeclare the identity by default.CastValue,ScaleOffsetand the numcodecsDelta,FixedScaleOffsetandAsTypedeclare the identity explicitly (they change dtype or fill value, never shape).TransposeCodecpermutes the shape and the per-axis edge lists.resolve_metadatawithout the hook, or if it disagrees withresolve_metadataon the representative chunk.Trade-off: chunk-local validation
A codec that overrides
resolve_metadataand returnsNonefromresolve_chunk_grid(the default) is still validated exactly on a regular grid, since every chunk has one shape. On a rectilinear grid it makes the rest of the chain chunk-local: later codecs are validated against one representative chunk (largest edge per axis). A chain that is invalid only for some other chunk shape is accepted at creation and fails when such a chunk is first encoded or decoded.To make that failure an error rather than corruption,
ShardingCodec._get_chunks_per_shardnow checks divisibility at run time (previously it floor-divided; with the check removed, a non-dividing shard writes and reads back wrong rows). Codecs whose correctness depends on chunk shape must do the same.The same applies to
resize: growing a rectilinear array behind an undeclared filter can add a non-dividing edge. The resize succeeds, and the error appears when a write touches that shard.Writes to these arrays validate the distinct chunk shapes they touch before any I/O, so a write that includes a bad shard stores nothing. Without that check, a write spanning good and bad shards raised, and the good shard was still stored shortly after the error. Arrays validated exactly when their metadata was created skip the check. A codec that checks chunk shape only while encoding is not covered by this pre-write check.
The trade-off is documented in the docstrings of
evolve_and_validate_codecs,resolve_chunk_gridand the sharding check, in a new "Chunk geometry and validation" section of the extending guide, in the rectilinear sharding docs, and in the changelog entry.Other behavior changes
scale_offsetis accepted again. fix(sharding): validate nested sharding codecs and reject an inner chunk size of 0 #4480 added an inner-chain check toShardingCodec.validatethat resolved against a zero fill value, which has rejected valid arrays since 3.4.1, such asscale_offsetwith a non-zero offset onuint8. This PR removes that check becauseevolve_from_array_specvalidates the same chain with the real fill value. CallingShardingCodec.validatedirectly therefore no longer checks nested codecs; every metadata path still does.main.Tests
Accepted and rejected transpose/sharding chains (also behind
Delta), reshape followed by transpose, nested sharding, rectilinear divisibility, actual fill values, data and metadata round trips. Chunk-local behavior is pinned from both sides: an undeclared filter on a rectilinear grid still rejects a shard shape the representative chunk fails, accepts one that only a smaller chunk fails and then raises on the first write to that shard, and does not enumerate chunks to find a shape-changing codec's error. A table test covers when a declared grid is trusted or ignored. The bounded-call-count regression now includesDeltaand an undeclared filter chains at ranks 4 and 12.Local validation on the rebased branch: full suite 13318 passed, 1 failed, 1319 skipped, 9 xfailed; pre-commit hooks incl. mypy pass. The failure is
tests/test_store/test_zip.py::TestZipStore::test_unused_copy_of_open_writer_does_not_write, an intermittentResourceWarningfor an unclosed file seen in two of four full runs; it passes whentest_zip.pyruns alone, and this PR does not touch zip code.Rebase notes
The agent rebuilt the branch on
mainas three commits; the previous head was5a52e85f944c8309570d0f81ebc883726adb1cfd. Open review questions from earlier comments still stand, including whether an undeclared codec on a rectilinear grid should fail outright instead of becoming chunk-local. Supersedes d-v-b#303.🤖 Generated with Claude Code