-
-
Notifications
You must be signed in to change notification settings - Fork 462
fix(metadata): validate codec chains against the threaded chunk spec #4352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 14 commits
4810e88
8e324b3
7c1d01b
db9bd2a
aaab193
d849cf3
04dd144
aaefe61
b87fd36
e8c9cde
28537dc
2c56eed
8a8f3b4
55c8bfa
5a52e85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| 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. | ||
|
|
||
| Geometry is threaded through the chain as a whole chunk grid, so validation cost no longer depends on the number of distinct chunk shapes. Codecs describe how they map the grid with the new `BaseCodec.resolve_chunk_grid` method; the built-in dtype and fill-value codecs (`cast_value`, `scale_offset`, and the numcodecs `delta`, `fixedscaleoffset` and `astype` filters) declare the identity and `transpose` declares a permutation. | ||
|
|
||
| Trade-off: on a rectilinear chunk grid, a codec that overrides `resolve_metadata` without implementing `resolve_chunk_grid` makes the rest of its chain "chunk-local". Codecs after it are validated against one representative chunk (the largest edge along each axis), so a chain that is invalid only for some other chunk shape is accepted when the array is created and fails when such a chunk is first written or read. `ShardingCodec` now checks divisibility at encode and decode time, so this case raises an error instead of corrupting data. Regular chunk grids are always validated exactly. See "Chunk geometry and validation" in the extending guide. | ||
|
|
||
| The codec pipeline of a rectilinear-chunked array is now evolved against that same representative chunk shape, instead of an all-ones placeholder that broke shape-dependent codecs at array creation even when the metadata validated. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,6 +148,55 @@ def resolve_metadata(self, chunk_spec: ArraySpec) -> ArraySpec: | |
| """ | ||
| return chunk_spec | ||
|
|
||
| def resolve_chunk_grid( | ||
| self, *, shape: tuple[int, ...], chunk_grid: ChunkGridMetadata | ||
| ) -> tuple[tuple[int, ...], ChunkGridMetadata] | None: | ||
| """The array shape and chunk grid seen by the codecs after this one. | ||
|
|
||
| This is the whole-array counterpart of `resolve_metadata`: where | ||
| `resolve_metadata` maps the spec of one chunk, this maps the geometry | ||
| of every chunk at once. It is used to validate a codec chain when the | ||
| array metadata is created, so that a later size-sensitive codec (such | ||
| as `ShardingCodec`) is checked against the chunks it will actually | ||
| receive, without enumerating the chunks of the grid. | ||
|
|
||
| Return `None` when the chunks after this codec cannot be described by | ||
| a single chunk grid computed from `chunk_grid` alone. On a regular grid | ||
| that costs nothing, because all chunks have one shape and | ||
| `resolve_metadata` describes them exactly. On a rectilinear grid it | ||
| 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`). | ||
|
Comment on lines
+167
to
+171
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we do not fail completely in this case? To protect people who have already written |
||
|
|
||
| The default declares the identity when `resolve_metadata` is not | ||
| overridden, and returns `None` otherwise. Codecs that override | ||
| `resolve_metadata` without changing the chunk shape (for example to | ||
| change the data type or fill value) should override this method to | ||
| return `(shape, chunk_grid)` unchanged, and codecs that change the chunk | ||
| shape in a way expressible as a grid (for example a permutation of the | ||
| axes) should return the mapped geometry. | ||
|
|
||
| A declaration is ignored if a subclass overrides `resolve_metadata` | ||
| without also overriding this method, or if it disagrees with | ||
| `resolve_metadata` on the representative chunk. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| shape : tuple[int, ...] | ||
| The array shape seen by this codec. | ||
| chunk_grid : ChunkGridMetadata | ||
| The chunk grid seen by this codec. | ||
|
|
||
| Returns | ||
| ------- | ||
| tuple[tuple[int, ...], ChunkGridMetadata] | None | ||
| """ | ||
| if type(self).resolve_metadata is BaseCodec.resolve_metadata: | ||
| return shape, chunk_grid | ||
| return None | ||
|
|
||
| def evolve_from_array_spec(self, array_spec: ArraySpec) -> Self: | ||
| """Fills in codec configuration parameters that can be automatically | ||
| inferred from the array metadata. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,6 +95,21 @@ def resolve_metadata(self, chunk_spec: ArraySpec) -> ArraySpec: | |
| prototype=chunk_spec.prototype, | ||
| ) | ||
|
|
||
| def resolve_chunk_grid( | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need to be a local import? |
||
|
|
||
| permuted_shape = tuple(shape[d] for d in self.order) | ||
| if isinstance(chunk_grid, RegularChunkGridMetadata): | ||
| return permuted_shape, RegularChunkGridMetadata( | ||
| chunk_shape=tuple(chunk_grid.chunk_shape[d] for d in self.order) | ||
| ) | ||
| return permuted_shape, RectilinearChunkGridMetadata( | ||
| chunk_shapes=tuple(chunk_grid.chunk_shapes[d] for d in self.order) | ||
| ) | ||
|
|
||
| def _decode_sync( | ||
| self, | ||
| chunk_array: NDBuffer, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.