Repository navigation
Conversation
… runs Opening an array with a rectilinear chunk grid cost time and memory proportional to the repeat counts in its stored metadata: `from_dict` expanded every run into one int per chunk, validation and `VaryingDimension` walked and re-accumulated them, and `to_dict` re-compressed them. A ~300 byte document declaring `[[1, 10**12]]` got the process OOM-killed. Add `RunLengthEdges`, an immutable `Sequence[int]` stored as merged `(size, count)` runs with bisect-over-runs lookups, and use it for the explicit edges of `RectilinearChunkGridMetadata.chunk_shapes` and `VaryingDimension.edges`. Metadata parsing, validation, serialization, `update_shape`, the dimension lookups, resize and the sharding codec's divisibility check now cost O(number of runs). Assisted-by: ClaudeCode:claude-fable-5.1
Assisted-by: ClaudeCode:claude-fable-5.1
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4479 +/- ##
==========================================
+ Coverage 94.69% 94.75% +0.06%
==========================================
Files 94 94
Lines 13598 13649 +51
==========================================
+ Hits 12876 12933 +57
+ Misses 722 716 -6
🚀 New features to boost your workflow:
|
A `RunLengthEdges` is not a `tuple`, so `==` against one is now false instead of an edge-by-edge comparison. This also removes the mismatch between equality and hashing. Tests compare `tuple(edges)` or whole metadata objects instead. Assisted-by: ClaudeCode:claude-fable-5.1
Per-chunk access belongs to `VaryingDimension`, which binds edges to an extent; the metadata model only needs runs, their total and the RLE form. A `Sequence[int]` interface on `RunLengthEdges` invited `for e in edges` or `list(edges)` on a grid with 2**40 chunks. Drop `len`, indexing, slicing, iteration, membership and `count`. Add `size_of(index)` for one edge and `expand()` as the single, explicit way to visit every edge. `RectilinearChunkGridMetadata.chunk_shapes` is now annotated as what it holds, `tuple[int | RunLengthEdges, ...]`, while its constructor still accepts tuples and lists of edges. Assisted-by: ClaudeCode:claude-fable-5.1
The per-chunk prefix sums were no longer used by any lookup and cost time and memory in the number of chunks on every access. `chunk_offset` gives the start of a chunk from the runs. Assisted-by: ClaudeCode:claude-fable-5.1
Nothing in the library called `with_extent`, `ngridcells` or `_unique_edge_lengths` on `FixedDimension` / `VaryingDimension`, did an `isinstance` check against `DimensionGrid`, or used `is_regular_1d` / `is_regular_nd`. Remove them along with the tests that only exercised them. `VaryingDimension.chunk_offset` no longer maps a negative chunk index to 0, a guard left over from indexing a tuple of prefix sums. Both dimension types raise the same `IndexError` message from `index_to_chunk`, and docstrings that described the old tuple-backed behavior are corrected. Assisted-by: ClaudeCode:claude-fable-5.1
d-v-b
force-pushed
the
perf/rectilinear-rle-runs
branch
from
October 5, 2026 16:34
71d5bb1 to
e529ad7
Compare
d-v-b
marked this pull request as ready for review
October 5, 2026 18:19
Contributor
Author
|
cc @maxrjones for visibility |
maxrjones
reviewed
Oct 5, 2026
The entry named private internals and read as a bug fix. Describe only the user-visible effect, and classify it as `misc` since nothing was incorrect before. Assisted-by: ClaudeCode:claude-fable-5.1
d-v-b
commented
Oct 5, 2026
| @@ -0,0 +1 @@ | |||
| Opening and indexing an array with a rectilinear chunk grid is now much faster and uses much less memory when the grid has long runs of equal-sized chunks. Previously the time and memory needed grew with the number of chunks, so an array with many millions of chunks could take seconds to open, or run out of memory. | |||
Member
There was a problem hiding this comment.
wow, reads perfect! thanks for fixing!
This branch has not been deployed
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
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.
This PR fixes a scaling bug in our model of rectilinear chunk edges by avoiding premature materialization of each chunk declared by a run-length-encoded sequence. Fixing this amounts to making the
VaryingDimensionclass dumber, which means we can actually remove some methods from that class, which is nice.🤖 AI text below 🤖
Opening and indexing an array with a rectilinear chunk grid is now much faster and uses much less memory when the grid has long runs of equal-sized chunks. Previously the time and memory needed grew with the number of chunks, so an array with many millions of chunks could take seconds to open, or run out of memory.
Problem
With
array.rectilinear_chunksenabled, a ~300 bytezarr.jsondeclaring"chunk_shapes": [[[1, N]]]opened in 0.16 s for N=106, 1.6 s for N=107 and 4.8 s for N=3*107, at roughly 50 bytes per chunk; N=1012 got the process OOM-killed.RectilinearChunkGridMetadata.from_dictexpanded every run into oneintper chunk, validation andVaryingDimensionwalked and re-accumulated them, andto_dictre-compressed them.Change
RunLengthEdges(zarr.core.common): an immutable value stored as merged(size, count)runs, with per-run cumulative chunk counts and offsets, bisect-over-runs scalar lookups (size_of,offset_of,index_at) and a vectorized numpy lookup. It is deliberately not a sequence;expand()is the one method that yields every edge.parse_rlereads stored JSON into it without expanding;expand_rleandcompress_rleremain, with the same error messages.VaryingDimensionholds its edges asRunLengthEdges; construction,index_to_chunk,chunk_offset,chunk_size,data_size,indices_to_chunks, andresizecost O(runs).RectilinearChunkGridMetadataparses, validates, serializes and resizes per run. Serialized output is unchanged.validatechecks one edge size per run.zarr.core.metadata.repairis untouched: it works on the JSON document and never expanded edges.After the change the document above opens, slices and fancy-indexes in about 5 ms with ~0.05 MB peak memory for N = 106, 3*107, 240 and 1012.
Behavior changes
chunk_shapes[i]andVaryingDimension.edgesareRunLengthEdges, not tuples:len, indexing, iteration andinraiseTypeError, and==against a tuple is false. ARunLengthEdgesequals only anotherRunLengthEdgeswith the same edges.tuple(edges.expand())gives the per-chunk tuple. Existing tests that compared against nested tuples now compare the expansion or whole metadata objects.RectilinearChunkGridMetadata.chunk_shapesis annotatedtuple[int | RunLengthEdges, ...]; the constructor still accepts tuples and lists of edges.VaryingDimension.cumulativeis removed. No lookup used it any more, andchunk_offset(i)gives the start of chunkifrom the runs.ChunkGrid.__repr__and the "All edge lengths must be > 0" error show a dimension of more than 100 chunks in RLE form; shorter dimensions are shown as before.VaryingDimension.chunk_offsetpast the last grid cell raisesIndexErrorwith a different message, and a negative chunk index raises instead of returning 0.with_extent,ngridcellsand_unique_edge_lengthson both dimension types and theDimensionGridprotocol,@runtime_checkableon that protocol, andis_regular_1d/is_regular_nd. Tests that only exercised them are removed or rewritten againstresizeandedges.FixedDimension.index_to_chunkandVaryingDimension.index_to_chunkraise the sameIndexErrormessage.Not addressed
ChunkGrid.chunk_sizes(Array.read_chunk_sizes/write_chunk_sizes) returns one entry per chunk by contract and stays O(chunks).Array.resizeto a smaller shape enumerates every dropped chunk, for regular grids as well.packages/zarr-indexinghas its ownVaryingDimensionwith per-chunk tuples and is unchanged.Tests
RunLengthEdges: one parametrized test against the expanded tuple as oracle, one test per error case, and one test that it is not a sequence.VaryingDimensionwith runs of 2**40 chunks, checked against closed-form values.test_open_rectilinear_huge_repeat_count(unsharded and sharded) opens, reads, writes, grows and re-serializes an array whose grid repeats one edge 2**40 times. It has no timing assertion: it can only pass if nothing expands the edges.expand_rleerror tests also run againstparse_rle.The agent ran
prek run --all-files(passing, including mypy) and the full test suite locally withtests/test_store/test_fsspec.pydeselected (12423 passed).Author attestation
TODO
docs/user-guide/*.mdchanges/🤖 Generated with Claude Code