spatial-signature shardmap reuse (#89)#90
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the spatial-signature shardmap-reuse change. No correctness bugs found — this is sound and well-tested; recommend merge after the one LOW nit (optional). Scrutiny summary:
signature()byte-identity (HIGH-priority check) — PASS. Verified empirically on a HEALPix grid:{**spatial_signature(), "output_fields": …}yields the exact same keys, order, and JSON serialization asmain's single-literalsignature()(type, indexing_scheme, parent_order, child_order, layout, output_fields). Same holds structurally for rect (type, crs, affine, shape, chunk_shape, output_fields).nests_withand any persisted full-signature comparison are unaffected.nests_withunchanged — PASS. Both grids key it onoutput_field_signature(self.config)directly (healpix.py:351, rectilinear.py:281), never routed throughsignature()/spatial_signature()._check_signatureprojection edge cases — PASS. (a) old full-sig stored map validates a matching grid AND reuses across agg fields (projection dropsoutput_fields); (b) new spatial-only map validates; (c) stored map missing a spatial key →expected.get(k)isNone≠ real value → raises (no silentNone==Nonepass, since a real grid's spatial sig is non-empty); (d) genuine spatial mismatch (parent/child order for HEALPix, crs/affine/shape/chunk for rect) raises. Theexpected is Noneearly-return is preserved; the empty-dict{}case only no-raises against a stub grid whosespatial_signature()is also{}, which is the existing test convention.- ShardMap storage + round-trip — PASS.
buildnow storesgrid.spatial_signature();to_json/from_jsonround-trip it; new maps no longer carryoutput_fields. Confirmed the only other full-signature()comparison in the tree isgrids/__init__.py:102(thenests_witherror path), which never touchesshardmap.grid_signature.grid_from_signatureis indeed not onmain. - Rect parity — PASS.
spatial_signature()excludesoutput_fields; cross-agg reuse works; resolution/CRS mismatch raises.
Local run: pytest tests/test_runner.py tests/test_shardmap.py tests/test_runner_concurrency.py tests/test_grids.py tests/test_rectilinear.py → 169 passed, 1 skipped.
Generated by Claude Code
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review) — one inline LOW nit on the reuse-guard error message (optional). No correctness issues; full review summary posted separately.
Generated by Claude Code
| raise ValueError( | ||
| "ShardMap was built for a different grid than this run config.\n" | ||
| f" shard map : {expected}\n" | ||
| f" shard map : {stored_spatial}\n" |
There was a problem hiding this comment.
🤖 from Claude (review) — LOW (debuggability nit, optional): the error now prints stored_spatial (the projected dict — i.e. the spatial keys the run grid declares, filled from expected with None for any key the stored map lacks). For the diagnostically interesting case — an old/incompatible stored map missing a spatial key — this renders that key as None and drops any extra keys the stored map did carry (e.g. a stored output_fields), so the message can obscure what the stored map actually was. Consider echoing the raw expected alongside the projection, e.g. f" shard map : {stored_spatial} (raw stored: {expected})\n". Purely cosmetic — the raise/no-raise logic is correct. (You already flagged this under "Questions for review"; noting it here for completeness.)
Generated by Claude Code
Closes #89.
Problem
A ShardMap is a spatial artifact (
shard_keys+ granule→shard assignment); it depends only on the spatial grid and the AOI/product/time query, not on what you aggregate. But the reuse guard over-constrained it:runner._check_signature(runner.py:287) did exact dict-equality between the run config'sgrid.signature()and the shardmap's storedgrid_signature.signature()(bothHealpixGridandRectilinearGrid) includesoutput_fields = output_field_signature(config).So two configs sharing the spatial grid but declaring different aggregation fields produced different signatures and the runner raised
ValueError: ShardMap was built for a different grid than this run config— even though the shardmap content is byte-identical for both. Concretely,atl03_tdigest_healpix.yamlandatl03_gain_bias_healpix.yamlshare parent_order 11 / chunk_inner 13 / child_order 19, so one shardmap maps both, yet you had to build (and CMR-fetch) two.Fix
spatial_signature()on both grids — the structural fingerprint withoutoutput_fields(HEALPix:type, indexing_scheme, parent_order, child_order, layout; rect:type, crs, affine, shape, chunk_shape).signature()now composesspatial_signature()+{"output_fields": …}, so the two stay in sync andsignature()'s output is byte-identical to before (verified — existingsignature()/nests_withtests unchanged)._check_signaturecompares the spatial signature, projecting the stored signature onto the spatial keys:stored_spatial = {k: expected.get(k) for k in grid.spatial_signature()}; raise only ifstored_spatial != actual. The projection dropsoutput_fields, so both newly-built spatial-only maps and old full-signature maps validate, and a map is reusable across agg fields. Theexpected is Noneearly-return is kept.ShardMap.buildnow storesgrid.spatial_signature()(field/JSON key staysgrid_signaturefor back-compat;to_json/from_jsonround-trip unchanged). Docstrings updated.nests_withis unchanged — it keys onoutput_field_signature(self.config)directly (the Support non-scalar aggregation outputs (vectors + ragged per-cell payloads) #29 co-aggregation guarantee), independent of the signature split.grid_from_signatureis not onmain(it lives in the unmerged #44), so nothing consumesoutput_fieldsfrom the stored signature — no extra compat needed there.Scope note
Orchestrator/catalog-side only — no Lambda redeploy. The worker never calls
_check_signature; this only affects shardmap build + the run-time reuse guard.Files
src/zagg/grids/healpix.py,src/zagg/grids/rectilinear.py— addspatial_signature();signature()composes it.src/zagg/runner.py—_check_signatureprojects + compares spatial only.src/zagg/catalog/shardmap.py—buildstores spatial signature; docstrings.tests/test_runner.py,tests/test_shardmap.py,tests/test_runner_concurrency.py— new coverage + mock-grid stubs for the new method.How tested
uv run pytest -q→ 771 passed, 1 skipped.ruff check --select=E,F,W,I --ignore=E501 src testsclean. mypy unchanged (no new errors/categories vsmainbaseline).New tests:
atl03_tdigest_healpix.yamlvalidates a run configured withatl03_gain_bias_healpix.yaml(same parent11/chunk_inner13/child19 spatial grid, different agg fields), and the reverse, with no raise.parent_order/child_order(HEALPix); different resolution/shape and different CRS (rect).grid_signaturecarryingoutput_fields(full signature) validates against a matching spatial grid and is reusable across differing agg fields (the projection handles it).nests_withsemantics unchanged — covered by existingsignature()/nests_withtests (still distinguishes output-field sets), plusspatial_signature()invariance-to-agg-fields tests.ShardMap.to_json/from_jsonpreserves the spatial-only signature;_check_signaturedriven directly with built signatures.spatial_signature()excludesoutput_fieldson both grids;signature() == {**spatial_signature(), "output_fields": …}.Questions for review
ValueErrornow prints the projectedstored_spatial(the spatial keys actually compared) rather than the raw stored dict, per the issue'sstored_spatial != actualframing. If you'd prefer it also echo the rawexpected, easy to add.🤖 Generated with Claude Code
https://claude.ai/code/session_01EN7X53ZAyaCca9rjwv9qZw
Generated by Claude Code