Update py-hamt to 3.4.0 - #11
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds an optional Changeszarr_group support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant dClimateClient
participant IpfsRetrieval as _load_dataset_from_ipfs_cid
participant Dataset as xarray.Dataset
Caller->>dClimateClient: load_dataset(cid, zarr_group)
dClimateClient->>IpfsRetrieval: _load_dataset_from_ipfs_cid(cid, zarr_group)
IpfsRetrieval->>Dataset: open_zarr(group) and set _ipfs_zarr_group attr
Dataset-->>dClimateClient: dataset with _ipfs_zarr_group attr
dClimateClient-->>Caller: metadata with zarr_group
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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.
🧹 Nitpick comments (3)
dclimate_client_py/dclimate_client.py (1)
251-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate zarr_group extraction logic.
The
ds.attrs.get("_ipfs_zarr_group")/isinstancecheck is repeated verbatim for both the direct-CID and STAC-resolved paths. Consider extracting a small helper to keep the two call sites in sync as this logic evolves.♻️ Suggested helper extraction
+ `@staticmethod` + def _apply_zarr_group_metadata(ds: xr.Dataset, metadata: DatasetMetadata) -> None: + loaded_zarr_group = ds.attrs.get("_ipfs_zarr_group") + if isinstance(loaded_zarr_group, str): + metadata["zarr_group"] = loaded_zarr_groupThen replace both occurrences with
self._apply_zarr_group_metadata(ds, metadata).Also applies to: 335-337
🤖 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` around lines 251 - 253, The `_ipfs_zarr_group` extraction is duplicated in both the direct-CID and STAC-resolved paths, so updates could drift between the two call sites. Extract the repeated `ds.attrs.get("_ipfs_zarr_group")` and `isinstance(..., str)` logic into a small helper on `DClimateClient` (for example, `_apply_zarr_group_metadata`), then call that helper from both locations so `metadata["zarr_group"]` is set consistently.tests/test_ipfs_retrieval.py (2)
116-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage gap: zarr_group forwarding during a real HAMT fallback isn't tested.
This test covers the "sharded store opens, but the zarr-group open itself fails" no-fallback path. Per the retrieval logic (
_load_dataset_from_ipfs_cid), there's a separate scenario where_open_sharded_zarr_storeitself fails and the code falls back toHAMT.build+ZarrHAMTStore, also passingzarr_groupthrough to_open_zarr_from_store. None of the added tests exercise that fallback path with an explicitzarr_group, so a regression there (e.g.,zarr_groupsilently dropped on the HAMT branch) wouldn't be caught.🤖 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 116 - 140, The current test only covers the non-fallback failure path after ShardedZarrStore.open succeeds, but it does not verify that zarr_group is preserved when _load_dataset_from_ipfs_cid falls back to HAMT.build and ZarrHAMTStore. Add a test that makes ShardedZarrStore.open fail, forces the HAMT fallback, and then asserts _open_zarr_from_store (or xr.open_zarr through it) receives the explicit zarr_group argument on that branch. Use the existing _load_dataset_from_ipfs_cid, _open_sharded_zarr_store, HAMT.build, and _open_zarr_from_store symbols to target the fallback path.
89-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication across Dummy store stubs.
DummyStore/DummyGroupedStore-style classes are redefined per-test with identicalpassbodies. Consider hoisting a shared minimal stub (or a small factory) at module scope to reduce repetition across tests 2-4.🤖 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 89 - 140, There is repeated DummyStore-style test scaffolding with identical empty class bodies across the IPFS retrieval tests, so consolidate it into a shared minimal stub or small factory at module scope and reuse it from the affected tests. Update the tests around _load_dataset_from_ipfs_cid, ShardedZarrStore.open, and HAMT.build so each case uses the common helper instead of redefining the same placeholder class inline.
🤖 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.
Nitpick comments:
In `@dclimate_client_py/dclimate_client.py`:
- Around line 251-253: The `_ipfs_zarr_group` extraction is duplicated in both
the direct-CID and STAC-resolved paths, so updates could drift between the two
call sites. Extract the repeated `ds.attrs.get("_ipfs_zarr_group")` and
`isinstance(..., str)` logic into a small helper on `DClimateClient` (for
example, `_apply_zarr_group_metadata`), then call that helper from both
locations so `metadata["zarr_group"]` is set consistently.
In `@tests/test_ipfs_retrieval.py`:
- Around line 116-140: The current test only covers the non-fallback failure
path after ShardedZarrStore.open succeeds, but it does not verify that
zarr_group is preserved when _load_dataset_from_ipfs_cid falls back to
HAMT.build and ZarrHAMTStore. Add a test that makes ShardedZarrStore.open fail,
forces the HAMT fallback, and then asserts _open_zarr_from_store (or
xr.open_zarr through it) receives the explicit zarr_group argument on that
branch. Use the existing _load_dataset_from_ipfs_cid, _open_sharded_zarr_store,
HAMT.build, and _open_zarr_from_store symbols to target the fallback path.
- Around line 89-140: There is repeated DummyStore-style test scaffolding with
identical empty class bodies across the IPFS retrieval tests, so consolidate it
into a shared minimal stub or small factory at module scope and reuse it from
the affected tests. Update the tests around _load_dataset_from_ipfs_cid,
ShardedZarrStore.open, and HAMT.build so each case uses the common helper
instead of redefining the same placeholder class inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4261196f-275e-401a-b854-e2d956cd7874
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
DATASET_CATALOG_USAGE.mdREADME.mddclimate_client_py/datasets.pydclimate_client_py/dclimate_client.pydclimate_client_py/ipfs_retrieval.pypyproject.tomltests/test_ipfs_retrieval.py
|
Handled the CodeRabbit review feedback in
Validation: |
2 participants
Summary
Updates the client to use
py-hamt3.4.0 and adapts IPFS Zarr loading for the new sharded store behavior introduced in dClimate/py-hamt#86.What changed
py_hamtto>=3.4.0and refresheduv.lockto resolvepy-hamt3.4.0.zarr_groupsupport todClimateClient.load_dataset()and the legacy CID loader."0"when a py-hamt v2 sharded store reports multiple top-level groups and that safe default is available.zarr_groupoption and metadata field.Why
py-hamt 3.4.0 adds path-aware sharded Zarr v2 support and can require explicit Zarr groups for pyramid/multi-group layouts. The client now handles the common group
"0"layout automatically while still allowing callers to specify another group explicitly.Validation
uv run --extra testing pytest tests/test_ipfs_retrieval.py -quv run --extra dev ruff check dclimate_client_py/ipfs_retrieval.py dclimate_client_py/dclimate_client.py dclimate_client_py/datasets.py tests/test_ipfs_retrieval.pySummary by CodeRabbit
load_dataset()now supports an optionalzarr_groupargument for loading grouped/pyramid Zarr datasets.zarr_group."0"when appropriate.zarr_group.py_hamtdependency constraint.