feat(sharding): manage inner chunks as a semi-regular grid (sharding_indexed v1.1) - #4198
Draft
d-v-b wants to merge 10 commits into
Draft
feat(sharding): manage inner chunks as a semi-regular grid (sharding_indexed v1.1)#4198d-v-b wants to merge 10 commits into
d-v-b wants to merge 10 commits into
Conversation
* fix: byte-order handling for structured dtypes in the bytes codec The bytes codec neither byte-swapped structured-dtype fields to its configured endian on encode (numpy reports byteorder '|' for void dtypes, so the top-level byteorder comparison never detected a mismatch) nor honored its endian when decoding, silently corrupting any structured data whose field byte order differed from the stored one (e.g. virtual references to external big-endian data). Encode now detects byte-order mismatches by comparing full dtypes via newbyteorder, and decode reinterprets raw bytes in the stored byte order before converting to the data type's declared byte order, so the stored layout (codec state) and the in-memory layout (array data type) are independent. Closes zarr-developers#4141 Assisted-by: ClaudeCode:claude-fable-5 * test: fold structured byte-order cases into existing bytes codec tests Extend test_endian's parametrization with structured dtypes and test_bytes_codec_sync_roundtrip with endian/dtype parametrization plus stored-layout and decoded-dtype assertions, instead of adding parallel test functions for the same properties. Assisted-by: ClaudeCode:claude-fable-5 * refactor: rename stored_dtype to view_dtype in BytesCodec decode The variable is the dtype used to view the raw chunk bytes (byte order from the codec's endian configuration), not a property of the stored data or of the returned buffer, which always carries the array's declared dtype. Assisted-by: ClaudeCode:claude-fable-5 * docs: note that the decode-side byte-order conversion copies the chunk Assisted-by: ClaudeCode:claude-fable-5
…t" data type (#202) * chore(deps): bump the actions group across 1 directory with 8 updates (#176) Bumps the actions group with 8 updates in the / directory: | Package | From | To | | --- | --- | --- | | [prefix-dev/setup-pixi](https://github.com/prefix-dev/setup-pixi) | `0.9.5` | `0.9.6` | | [codecov/codecov-action](https://github.com/codecov/codecov-action) | `6.0.0` | `6.0.1` | | [github/issue-metrics](https://github.com/github/issue-metrics) | `4.2.2` | `4.2.7` | | [j178/prek-action](https://github.com/j178/prek-action) | `2.0.3` | `2.0.4` | | [actions/upload-artifact](https://github.com/actions/upload-artifact) | `7.0.0` | `7.0.1` | | [actions/download-artifact](https://github.com/actions/download-artifact) | `7.0.0` | `8.0.1` | | [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) | `1.13.0` | `1.14.0` | | [zizmorcore/zizmor-action](https://github.com/zizmorcore/zizmor-action) | `0.5.3` | `0.5.6` | Updates `prefix-dev/setup-pixi` from 0.9.5 to 0.9.6 - [Release notes](https://github.com/prefix-dev/setup-pixi/releases) - [Commits](prefix-dev/setup-pixi@1b2de7f...5185adf) Updates `codecov/codecov-action` from 6.0.0 to 6.0.1 - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@57e3a13...e79a696) Updates `github/issue-metrics` from 4.2.2 to 4.2.7 - [Release notes](https://github.com/github/issue-metrics/releases) - [Commits](github-community-projects/issue-metrics@c9e9838...1e38d5e) Updates `j178/prek-action` from 2.0.3 to 2.0.4 - [Release notes](https://github.com/j178/prek-action/releases) - [Commits](j178/prek-action@6ad8027...bdca6f1) Updates `actions/upload-artifact` from 7.0.0 to 7.0.1 - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v7...043fb46) Updates `actions/download-artifact` from 7.0.0 to 8.0.1 - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v7...3e5f45b) Updates `pypa/gh-action-pypi-publish` from 1.13.0 to 1.14.0 - [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases) - [Commits](pypa/gh-action-pypi-publish@v1.13.0...cef2210) Updates `zizmorcore/zizmor-action` from 0.5.3 to 0.5.6 - [Release notes](https://github.com/zizmorcore/zizmor-action/releases) - [Commits](zizmorcore/zizmor-action@b1d7e1f...5f14fd0) --- updated-dependencies: - dependency-name: prefix-dev/setup-pixi dependency-version: 0.9.6 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: codecov/codecov-action dependency-version: 6.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: github/issue-metrics dependency-version: 4.2.7 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: j178/prek-action dependency-version: 2.0.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: actions/upload-artifact dependency-version: 7.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: actions/download-artifact dependency-version: 8.0.1 dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: pypa/gh-action-pypi-publish dependency-version: 1.14.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions - dependency-name: zizmorcore/zizmor-action dependency-version: 0.5.6 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix: don't warn on the spec'd "struct" data type in v3 The `struct` data type now has a stable Zarr V3 specification (zarr-extensions/data-types/struct), so serializing it no longer warrants an UnstableSpecificationWarning. The legacy `structured` alias and the unspecified bytes data types (null_terminated_bytes, raw_bytes, variable_length_bytes) continue to warn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rRWaxXfZ3ZmbiZYWZoDBP * docs: add changelog fragment for struct warning fix Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rRWaxXfZ3ZmbiZYWZoDBP --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement zarr-specs zarr-developers#370: the sharding codec's chunk_shape no longer needs to evenly divide the shard shape. Inner chunks are laid out on a semi-regular grid managed by ChunkGrid: chunks per shard is the ceiling division of the shard shape by chunk_shape, and chunks straddling the shard boundary are encoded and stored at their clipped shape. - validate() drops the divisibility requirement (dimension-count and grid-type checks remain) - all encode/decode paths (sync and async, full and partial) use per-chunk ArraySpecs derived from the inner grid - the uncompressed whole-shard bulk-decode fast path gates on an evenly divided grid; the scalar-broadcast write memo only reuses encoded bytes across nominal-shape chunks - Array.read_chunk_sizes and cdata_shape account for the inner grid restarting at every shard boundary - composes with rectilinear outer chunk grids and nested sharding Arrays written with a non-divisible chunk_shape require a v1.1-aware implementation to read; v1.0 implementations must reject them. Assisted-by: ClaudeCode:claude-fable-5
Fixes from a multi-agent adversarial review of the semi-regular grid change: - nchunks_initialized used floor division for chunks-per-shard, which undercounts on clipped grids and reports 0 when chunk_shape exceeds the shard shape; use ceiling division to match the inner grid - the scalar-broadcast write memo in _encode_partial_sync gated on ArraySpec object identity, which never holds because _get_chunk_spec is uncached (zarr-developers#3054); gate on shape equality so the memo fires again (one encode instead of one per complete chunk) - the rectilinear clipping test used shard edges [[6, 4]], which normalize to a regular grid; use [[4, 6]] with the rectilinear config flag so RectilinearChunkGridMetadata is actually exercised Adds regression tests for the first two. Assisted-by: ClaudeCode:claude-fable-5
Assisted-by: ClaudeCode:claude-fable-5
Contributor
Author
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4198 +/- ##
==========================================
- Coverage 93.90% 93.86% -0.05%
==========================================
Files 91 91
Lines 12672 12692 +20
==========================================
+ Hits 11900 11913 +13
- Misses 772 779 +7
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements a proposed extension to the sharding codec spec which allows semi-regular inner chunk grids. Meaning, with this sharding codec implementation, the outer chunk shape can be
(10,), and the inner chunk shape can be declared as(8,), resulting in actual inner chunks sized(8, 2), i.e. 10 partitioned into chunks sized 8, with the remainder. This makes the sharding codec much more useful for arrays with rectilinear chunk grids, or arrays with shapes made of prime numbers.Implementation-wise, the biggest change is a layer of dynamism for the per-chunk models used by the sharding codec. chunk spec objects have to be generated dynamically per-chunk, because the inner chunk grid depends on the outer chunk shape.
For reviewers
This is a proof of concept intended to demonstrate the impact of implementing the proposed sharding codec change. Widening our model of the sharding codec has ecosystem-wide implications, so we will not merge this without either ecosystem-wide coordination or an explicit "I accept that I am going off-road" mechanism for zarr-python users to opt into. So do not consider this review-ready until I advertise that one or both of those conditions are met.
Author attestation
TODO
docs/user-guide/*.mdchanges/these changes were written with claude. See the claude-authored PR here: d-v-b#251