Optimize append-only leading-dimension resize performance - #92
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesLeading-dimension resize optimization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ShardedZarrStore
participant ArrayIndex
participant RootManifest
Caller->>ShardedZarrStore: resize_store(new_shape)
ShardedZarrStore->>ArrayIndex: resize leading dimension in place
ShardedZarrStore->>RootManifest: sync arrays and mark root dirty
ShardedZarrStore-->>Caller: complete without shard snapshot
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_z16_fast_leading_resize.py (1)
17-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd property-based coverage for resize geometry.
Use Hypothesis to generate C-order shapes, chunk shapes, shard sizes, and append lengths; retain the focused regression cases. This is required for tests in this repository.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_z16_fast_leading_resize.py` around lines 17 - 29, Extend test_fast_leading_resize_requires_strict_append_only_growth with Hypothesis-based generation of valid C-order array shapes, chunk shapes, shard sizes, and append lengths, asserting the resize geometry contract for generated inputs. Retain the existing focused regression assertions unchanged, and use the repository’s established Hypothesis strategies and test conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_z16_fast_leading_resize.py`:
- Around line 56-60: Apply Ruff’s required formatting in
tests/test_z16_fast_leading_resize.py at lines 56-60, 72-74, 129-134, 142-144,
and 190-195: reformat the AsyncMock expressions and chunk-write calls without
changing their behavior.
- Around line 230-233: Update the count_snapshot helper’s return annotation to
the precise shard-snapshot mapping type returned by _snapshot_shards_for_resize,
replacing the unparameterized dict while preserving its existing behavior.
---
Nitpick comments:
In `@tests/test_z16_fast_leading_resize.py`:
- Around line 17-29: Extend
test_fast_leading_resize_requires_strict_append_only_growth with
Hypothesis-based generation of valid C-order array shapes, chunk shapes, shard
sizes, and append lengths, asserting the resize geometry contract for generated
inputs. Retain the existing focused regression assertions unchanged, and use the
repository’s established Hypothesis strategies and test conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e692c8ff-d0a9-4638-b27f-923b193c906d
📒 Files selected for processing (2)
py_hamt/sharded_zarr_store.pytests/test_z16_fast_leading_resize.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_z16_fast_leading_resize.py (1)
206-233: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert fallback data preservation, not only snapshot invocation.
This test proves that the general path is entered and the shape changes, but it would still pass if shard remapping dropped or corrupted existing chunks. Seed known data before resizing and reopen the store afterward to verify retained chunks and expected shrink behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_z16_fast_leading_resize.py` around lines 206 - 233, Strengthen test_non_leading_change_and_shrink_use_general_resize by writing known chunk data before resize, then reopening the store after resize and asserting retained chunks still contain the expected values while data outside the new_shape is removed. Keep the existing snapshot_calls and array_shape assertions to verify both fallback-path selection and resize behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_z16_fast_leading_resize.py`:
- Line 124: Copy the manifest returned by
writable.array_indices["humidity"].to_manifest() before storing it in
humidity_manifest, ensuring its shard_cids data is independent of the store’s
mutable state for the later resize comparison.
---
Nitpick comments:
In `@tests/test_z16_fast_leading_resize.py`:
- Around line 206-233: Strengthen
test_non_leading_change_and_shrink_use_general_resize by writing known chunk
data before resize, then reopening the store after resize and asserting retained
chunks still contain the expected values while data outside the new_shape is
removed. Keep the existing snapshot_calls and array_shape assertions to verify
both fallback-path selection and resize behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f710f7b0-dda9-4bc1-b6aa-fb3dc44ad853
📒 Files selected for processing (1)
tests/test_z16_fast_leading_resize.py
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| read_only=False, | ||
| root_cid=root_cid, | ||
| ) | ||
| humidity_manifest = writable.array_indices["humidity"].to_manifest() |
There was a problem hiding this comment.
LOW
to_manifest() returns the live shard_cids list, so this baseline aliases the humidity index. An in-place mutation during resize would update both sides and let the array-locality assertion pass despite corruption. Copy the manifest, including shard_cids, before resizing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 3168 3180 +12
=========================================
+ Hits 3168 3180 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
This pull request introduces an optimized "fast path" for resizing Zarr arrays along their leading dimension when only appending new data, avoiding unnecessary shard snapshots and data movement. It adds a static method to detect when this optimization is safe, updates the resize logic to use it, and includes comprehensive tests to verify correct behavior and backward compatibility.
Optimized fast-path for leading-dimension resize:
_can_fast_resize_leading_dimensionstatic method toShardedZarrStoreto detect when a resize is a strict append along the leading dimension and is safe to optimize._resize_array_indexto use the fast path when possible, skipping shard snapshots and preserving chunk and shard layout for appends.Testing and validation:
test_z16_fast_leading_resize.pywith tests verifying:Summary by CodeRabbit
Performance Improvements
C) arrays when only the leading dimension is append-only grown, avoiding unnecessary shard/metadata reprocessing.Bug Fixes
Tests