perf: add sparse decode option - #13
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesShard read mode
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client as dClimateClient.load_dataset
participant Loader as _load_dataset_from_ipfs_cid
participant Store as ShardedZarrStore.open
Client->>Loader: shard_read_mode
Loader->>Store: shard_read_mode
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (3)
dclimate_client_py/ipfs_retrieval.py (1)
250-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
shard_read_modein the internal loader docstring.The signature adds this argument, but the
Argssection stops atzarr_group. Document its allowed values and default so the internal contract matches the implementation.🤖 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 `@dclimate_client_py/ipfs_retrieval.py` around lines 250 - 264, The internal loader’s docstring omits the shard_read_mode argument. Update the Args section for the loader function to document shard_read_mode, including its allowed values and default of "full", matching the signature and implementation.dclimate_client_py/dclimate_client.py (1)
322-322: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the STAC forwarding branch.
The supplied test exercises
cid=VALID_CID, proving only the direct-CID forwarding at Line 243. Add or verify a STAC-path test that asserts"sparse"reaches_load_dataset_from_ipfs_cidat this separate call site.🤖 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 `@dclimate_client_py/dclimate_client.py` at line 322, The STAC forwarding path must preserve the requested shard read mode. Add or update a test for the STAC branch in the relevant dataset-loading flow, using a STAC identifier rather than a direct CID, and assert that _load_dataset_from_ipfs_cid receives shard_read_mode="sparse" at the call site containing shard_read_mode=shard_read_mode.tests/test_ipfs_retrieval.py (1)
54-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the forwarded mode in a sharded-store test.
These stubs accept
shard_read_modebut ignore it, so the suite would still pass if the forwarding atdclimate_client_py/ipfs_retrieval.py:324were removed. Capture the argument in one sparse-mode case and assert the default path receives"full".Also applies to: 72-73, 96-97, 120-121, 142-148, 175-176
🤖 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_ipfs_retrieval.py` around lines 54 - 55, The sharded-store test stubs ignore the forwarded shard mode, so forwarding regressions are undetected. Update the relevant sharded_open stubs to capture shard_read_mode, then assert the sparse-mode case receives the expected sparse value and the default path receives "full"; apply this across all referenced test cases.
🤖 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 `@dclimate_client_py/ipfs_retrieval.py`:
- Line 19: The py-hamt dependency version does not support ShardReadMode or the
shard_read_mode argument used by ShardedZarrStore.open. Update ipfs_retrieval.py
to use the API exposed by the pinned py-hamt version, removing or replacing the
unsupported import and argument, or bump the dependency to a compatible version
and update its pin consistently.
---
Nitpick comments:
In `@dclimate_client_py/dclimate_client.py`:
- Line 322: The STAC forwarding path must preserve the requested shard read
mode. Add or update a test for the STAC branch in the relevant dataset-loading
flow, using a STAC identifier rather than a direct CID, and assert that
_load_dataset_from_ipfs_cid receives shard_read_mode="sparse" at the call site
containing shard_read_mode=shard_read_mode.
In `@dclimate_client_py/ipfs_retrieval.py`:
- Around line 250-264: The internal loader’s docstring omits the shard_read_mode
argument. Update the Args section for the loader function to document
shard_read_mode, including its allowed values and default of "full", matching
the signature and implementation.
In `@tests/test_ipfs_retrieval.py`:
- Around line 54-55: The sharded-store test stubs ignore the forwarded shard
mode, so forwarding regressions are undetected. Update the relevant sharded_open
stubs to capture shard_read_mode, then assert the sparse-mode case receives the
expected sparse value and the default path receives "full"; apply this across
all referenced test cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d25ed968-ebd0-427c-b15e-c63aa9493fcf
📒 Files selected for processing (3)
dclimate_client_py/dclimate_client.pydclimate_client_py/ipfs_retrieval.pytests/test_ipfs_retrieval.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pyproject.toml`:
- Line 9: Regenerate uv.lock after updating the project version and py_hamt
requirement so it records version 0.5.11 and resolves py_hamt to at least 3.4.1;
commit the resulting lockfile with the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
2 participants
Exposes the new py-hamt sparse shard decoding option when loading datasets from IPFS.
The "full" option remains the default to preserve existing behavior.
Summary by CodeRabbit
New Features
shard_read_modeoption for dataset loading to control how sharded Zarr shards are decoded.fullandsparse, defaulting tosparse.Bug Fixes
Chores
0.5.11.