fix: sha2-256 CIDv1 (#43) + restore performance benchmarks (#58) - #90
Conversation
Keeps the py-hamt 3.5.0 version sync (main's lock was stale at 3.4.1) but reverts revision 3 -> 2, which was an artifact of an older local uv rather than an intended change.
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe Kubo add URL now pins CIDv1, with regression tests for codec and payload correctness. The benchmark suite was replaced with deterministic, instrumented pytest coverage for store operations, sharded read modes, caching, and concurrency. ChangesCID correctness
Benchmark coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (1)
tests/test_k16_sha2_256_cid_version.py (1)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Hypothesis coverage for the Kubo add URL.
tests/test_k16_sha2_256_cid_version.pyhas only example-based tests, while Hypothesis is already available in the test suite. Add a property-based test overpin_on_addand validchunkervalues that assertscid-version=1is present in every parsed Kubo add URL.🤖 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_k16_sha2_256_cid_version.py` around lines 29 - 41, Add a Hypothesis property-based test in test_k16_sha2_256_cid_version.py covering all pin_on_add settings and valid chunker values, constructing the corresponding KuboCAS add URL and parsing its query parameters. Assert that every parsed URL contains cid-version=1, while preserving the existing hasher coverage.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_benchmark_stores.py`:
- Around line 283-347: Correct the benchmark’s physical chunk-count calculation
by multiplying the per-dimension block counts in ds.chunks rather than summing
them, and update the dataset parameters or chunks_per_shard so the actual count
is strictly greater than the shard threshold and spans multiple shards. Revise
the nearby comments/docstring to state the accurate count, and keep the
shard-load assertions based on the corrected n_chunks value.
---
Nitpick comments:
In `@tests/test_k16_sha2_256_cid_version.py`:
- Around line 29-41: Add a Hypothesis property-based test in
test_k16_sha2_256_cid_version.py covering all pin_on_add settings and valid
chunker values, constructing the corresponding KuboCAS add URL and parsing its
query parameters. Assert that every parsed URL contains cid-version=1, while
preserving the existing hasher coverage.
🪄 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 Plus
Run ID: c04236da-96d5-4221-85dc-e6b2ea340bdf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
py_hamt/store_httpx.pytests/conftest.pytests/test_benchmark_stores.pytests/test_k16_sha2_256_cid_version.py
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| # verify_content would silently pass every block through unverified. | ||
| # Requesting v1 makes Kubo store the raw bytes under the codec asked | ||
| # for, which is what blake3 (not CIDv0-representable) already got. | ||
| self.rpc_url: str = f"{rpc_base_url}/api/v0/add?hash={self.hasher}&chunker={self.chunker}&pin={pin_string}&cid-version=1" |
There was a problem hiding this comment.
MEDIUM
cid-version=1 only makes leaf chunks raw. Once data exceeds the configured chunk size (1 MiB by default), Kubo returns a UnixFS dag-pb root (layout documentation); save() then declines to apply the requested codec and verify_content still skips verification. Handle/reject multi-chunk inputs or use an endpoint that stores the payload as one block, and add a payload-larger-than-chunker regression test.
|
|
||
| xr.testing.assert_identical(xr.concat([ds, ds], dim="time"), actual) | ||
|
|
||
| assert write.cas_loads > 0, "write did no content-store work" |
There was a problem hiding this comment.
MEDIUM
This is not measuring write request counts: cas_load.total records loads, not /add saves, and checking only > 0 cannot catch an increase from 11 requests to thousands. The sharded write benchmark has the same problem because shard_cache.total counts cache accesses. Instrument save/POST requests and assert a stable count or upper bound.
| # Both must return the same data, or the comparison is meaningless. | ||
| xr.testing.assert_identical(hamt_result, sharded_result) | ||
|
|
||
| n_chunks = sum(len(chunks) for chunks in ds.chunks.values()) |
There was a problem hiding this comment.
MEDIUM
ds.chunks describes 20 × 1 × 1 = 20 primary-array chunks here, while this sum produces 22; with chunks_per_shard=50, the fixture also creates only one primary shard. Because shard_loads uses shard_cache.total (hits plus misses), a per-chunk 20-call regression still passes 20 < 22. Force multiple shards, compute chunk count with math.prod, and assert cache misses/network fetches.
| root = hamt.root_node_id | ||
|
|
||
| async with KuboCAS( | ||
| rpc_base_url=rpc, gateway_base_url=gateway, concurrency=concurrency |
There was a problem hiding this comment.
LOW
The test passes concurrency but never observes whether it changes request parallelism. If KuboCAS ignored the argument and serialized all requests, data identity, unique_cids > 0, and zero retries would still pass for every parameter. Measure simultaneous HTTP requests with a blocking/mock transport and assert the configured limit.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 3166 3168 +2
=========================================
+ Hits 3166 3168 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| xr.testing.assert_identical(xr.concat([ds, ds], dim="time"), actual) | ||
|
|
||
| assert write.work_units > 0, "write did no storage work" |
There was a problem hiding this comment.
MEDIUM
The sharded write benchmark does not count writes. work_units sums CAS loads and shard-cache accesses, neither of which records /api/v0/add; this assertion still passes if writes amplify from a handful of POSTs to thousands. Use CountingTransport here and assert an upper bound on tally.adds, as the HAMT benchmark does.
| results[mode] = report(phase.result) | ||
| xr.testing.assert_identical(ds, scanned) | ||
|
|
||
| assert results["sparse"].cas_loads > results["full"].cas_loads, ( |
There was a problem hiding this comment.
LOW
Only the scan-cost half of the advertised sparse/full trade-off is tested. Both phases load the entire dataset, so a regression that makes sparse point reads decode/cache full shards still passes as long as sparse scans remain more expensive. Add an isolated point-read comparison with instrumentation for decoded/cached shard work, or narrow the test's claim.
| ) | ||
|
|
||
| # The ceiling is honoured: never more requests in flight than configured. | ||
| assert tally.peak_in_flight <= concurrency, ( |
There was a problem hiding this comment.
MEDIUM
The 8 and 32 concurrency cases never exercise their limits. The fixture exposes only about four overlapping requests, so peak_in_flight <= concurrency is non-binding for both values and the later > 1 check would accept a hard-coded limit of two. Drive more than 32 blocked uncached loads and assert the observed ceiling for each parameter.
2 participants
Closes #43. Closes #58. Addresses #59.
Two issues, verified against a local Kubo 0.39.0 daemon.
#43 —
sha2-256returns a CIDv0/dag-pb CIDReproduced directly against the RPC API:
Fix: append
&cid-version=1to the/api/v0/addURL (store_httpx.py:716).Two things worth flagging beyond the issue text:
The fix suggested in the issue would not have worked. #43 proposes reworking the returned CID into one with the correct multicodec. The stored blocks differ, so that is not possible:
The CIDv1 digest is exactly
sha256(payload). Relabelling a CIDv0's codec would yield a CID that does not hash to its own block, so this has to be fixed at add time. Existing code atstore_httpx.py:1086already declined to relabel dag-pb CIDs (if cid.codec.code != self.DAG_PB_MARKER) — a workaround for precisely this.Severity is higher than cosmetic.
_cid_is_verifiable()skips verification for dag-pb CIDs, soverify_content=Trueon asha2-256store was silently passing every block through unverified. The defaultblake3is not CIDv0-representable and was never affected, which is why this went unnoticed.Tests:
tests/test_k16_sha2_256_cid_version.py— 14 cases covering the add URL, CID version/codec, digest-matches-stored-block, Kubo's own block store, verifiability, and a full HAMT round trip undersha2-256. Reverting the one-line fix fails 9 of them; theblake3control cases pass in both states, so they are not passing vacuously.#58 — Performance benchmarks
tests/test_benchmark_stores.pywas entirely commented out, and could not have run as written: it importedFlatZarrStore(no longer exported), pointed at productionipfs-gateway.dclimate.netwith a blankX-API-Key, usedDataset.dims.values()(removed in modern xarray), and printed timings without asserting anything.Rewritten to assert on request counts from the
instrumentationmodule rather than wall-clock time — deterministic, so it holds up in CI. Datasets use a seeded RNG so chunk bytes, CIDs, and counts are stable. Timings are still measured and printed via--benchmark-reportfor humans, but nothing fails on them.Coverage: HAMT and sharded write/append/read, sharded-vs-HAMT round trips, read-cache effectiveness, and a
concurrency=[1, 8, 32]parametrization (which also addresses #59 — the existingtest_k3/test_k9cover semaphore scoping mechanics, not behaviour across concurrency values).A caveat these benchmarks now pin down
ShardedZarrStoredefaults toshard_read_mode="sparse"(sharded_zarr_store.py:484). Sparse mode — added in #87 — fetches one entry per chunk to keep point reads cheap (~9x on 30 years of ERA5 dailies, the AEGIS workload) and deliberately does not populate the shard cache.On a whole-array scan that is the wrong mode by design. Measured at 320 chunks:
sparse(default)fullIn
fullmode 7 shard fetches serve all 320 chunks with zero duplicates — sharding behaves exactly as #88/#56 describe.The trap: this gap is invisible below
chunks_per_shard, where both modes tie exactly. Benchmarking a whole-array read on the default with a small dataset gives numbers that look fine, then a ~2x regression at production scale. I hit this myself while writing these benchmarks and initially misread it as a broken shard cache.So every scan benchmark here sets
shard_read_modeexplicitly, and two tests pin the behaviour:test_sparse_mode_trades_scan_cost_for_point_read_latency— asserts the trade-off in both directions, so the cost is visibly intentionaltest_sharded_store_defaults_to_sparse_read_mode— pins the default, since it decides which trade-off users gettest_sharded_full_mode_read_batches_chunks_into_shard_fetchesasserts the batching property directly (0 < shard_loads < n_chunks, zero duplicates) rather than comparing raw totals, so it fails if per-chunk round trips ever return to the full path.Note these are network round-trip benchmarks. #88's headline figures measure different quantities — bulk writes against
InMemoryCAS, warm-cache get latency, per-request RTT depth — and are not comparable to the totals above.Also included
--benchmark-reportpytest flag (tests/conftest.py) to print the tables; assertions run either wayuv.lock: syncs thepy-hamtentry to 3.5.0, matchingpyproject.toml(the lock onmainwas stale at 3.4.1)Verification
pytest --ipfs— 427 passed, 1 skipped (up from 418 onmain), no xfailscoverage report --fail-under=100— 100% across all 8 modulespre-commit run --all-files— ruff check, ruff format, slotscheck, mypy all passRun the benchmarks with:
Not addressed
The
ShardedZarrStoredocstring does not mention thatshard_read_mode="sparse"is a poor fit for whole-array scans, or that the difference only appears abovechunks_per_shard. That is a docs gap rather than a code bug, so I left it out of this PR — happy to add it here or separately.Summary by CodeRabbit
Bug Fixes
Tests