Morton decimal shard ids + hive-partitioned output layout (issue 199)#205
Conversation
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of phase 1 (decimal shard-id strings on the flat layout), against issue #199's phase-1 scope, docs/design/sparse_coverage.md D1–D6, and mortie 0.9.0 source (tag 0.9.0).
What holds up. The boundary pair is correctly built on mortie's decode-through-kernel repr and the issue-104 parse-back; the southern-hemisphere sign, mixed orders, and the packed-word ⇄ decimal round trip are lossless and pinned by tests at orders 0/6/9/18 in both hemispheres. The reader's coord-array alignment correctly switched from int order to parsed-word order (northern-before-southern), pinned by the mixed-hemisphere regression test. All four issue surfaces (CSR subgroup names, .status keys, worker/dispatcher logs, --morton-cell) route through the shard_label seam; the sharded (#108) path is genuinely block-keyed at every K, so leaving it unchanged is right; the Lambda event shard_key staying a packed int matches D1 (canonical wire form). Rect's str(int) label is consistent end-to-end with _select_cells' int parse. A branch grep found no remaining packed-word leaks in externally rendered strings beyond the ones the PR itself defers.
Findings (inline comments carry the detail):
- should-fix —
readers/tdigest_tensor.py::_subgroup_words: the flavor heuristic is weaker than documented. Single-digit block keys"5"/"6"parse as valid decimal ids (lone base digit), so an all-{1..6}block-keyed store silently flips flavor; a leading--name in the int fallback is provably wrong yet accepted; and the "coord array wins" mitigation is vacuous in-repo (nothing in zagg writes{field}/morton). Fail loudly on the provably-wrong cases. - should-fix —
runner.pyaccumulators (lines 1210, 1522): label rendering in error-reporting paths can now raise (morton_decimalraises on invalid words), aborting run accumulation exactly for the malformed key it was reporting. mortie's own scalar repr never raises for this reason; use a defensive fallback in log-only sites. - should-fix (small) —
grids/morton.py::morton_decimal: negative input (a legacy signed id — the exact migration confusion) raisesOverflowError, contradicting the documentedValueErrorcontract. - question-for-author —
morton_word's private_decimal_to_word: a public alternative exists (MortonIndexArray.from_hive_path(label, suffix="")) but requires pandas; the private numpy-only name is defensible — file the upstream export ask now and document the choice. (Answers PR question 1.) - question-for-author —
_select_cells: a legacy signed-key catalog + a well-formed decimal id yields an unhelpful "not in catalog"; decide whether a regenerate-hint is wanted or the bare failure is the intended hard-break surface. - nit — tests: pin
morton_decimal(0)raising (the "never silently wrong path component" claim is untested on the emit side); optionally one order-29 area case. - nit — the payload-cap deferral's "no grid in scope" rationale is weaker than stated:
_cell_workhas the label in hand on exactly the runs that can hit the cap.
Deferred surfaces (PR question 4). ShardMap JSON/parquet packed-word shard_keys are squarely D1 canonical storage — genuinely fine to defer indefinitely. The viz GeoJSON shard_key property is interchange, not storage, so D1's "the signed decimal string [is] the external form" reads as convert-eventually; deferring past phase 1 is a scope call I'd record in the issue rather than leave implicit. Status-key shape (PR question 2) matches O6 (stay flat, basename-only change) — looks intended. Also noting for the docs sweep: src/zagg/auth.py's docstring event example ("shard_key": -6134114) and docs/deployment/lambda.md's examples carry legacy signed values that are not valid packed words — stale before this PR, but phase 2's layout docs are the natural place to fix them.
Conventions. mortie>=0.9.0 is the one dependency change and is authorized on the issue thread. The .github/scripts/run_benchmark.py edit is application-level harness code, not workflow config — I agree with the judgment call (no .github/workflows/ or deployment/aws/ build/deploy scripts touched; the handler edit is the worker's write path, in scope). The __main__.py format normalization is semantics-neutral but widens the diff against the "don't fix pre-existing drift" norm — mild preference for the offered minimal-hunk revert, non-blocking. No module crosses the ~1000-line guidance in this PR (runner.py at ~2.4k lines is a pre-existing offender, not this change). No blocking-correctness findings; the two reader/accumulator should-fixes are the ones I'd want addressed before phase 2 stacks the hive tree on top of the same seams.
Generated by Claude Code
| def _subgroup_words(shard_keys: list[str]) -> dict[str, int]: | ||
| """Parse CSR subgroup names to their numeric keys (issue #199). | ||
|
|
||
| Shard-keyed subgroups are named by the decimal morton string (D1), so the | ||
| numeric key is the parsed packed word; block-index-keyed subgroups (the K>1 | ||
| layout) keep plain ints. The flavor is uniform per store, so parse all | ||
| names as decimal morton and fall back to ``int`` if any name isn't one | ||
| (e.g. ``"0"`` or a digit outside ``1..4``). | ||
| """ | ||
| try: | ||
| return {k: morton_word(k) for k in shard_keys} | ||
| except ValueError: | ||
| return {k: int(k) for k in shard_keys} |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — the flavor heuristic is weaker than this docstring claims, in both directions:
-
The docstring's examples of a non-decimal name (
"0", digit outside1..4) miss the single-digit case:_decimal_to_wordaccepts a lone base digit1..6, so block indices"5"and"6"parse as valid order-0 decimal ids (verified against mortie 0.9.0:morton_word("5") == 5764607523034234880). A K>1 store whose populated block keys are all in{"1".."6"}(or longer names that happen to fit the grammar) silently flips the whole store to packed-word interpretation — no"0"needed to save it. -
In the
intfallback branch, a name with a leading-can never be a block index (block keys are non-negative flattened indices), yet it maps to a nonsense negative "block key". Concretely: a decimal-named store poisoned by one debris subgroup (e.g. a pre-Adopt morton decimal shard ids + hive-partitioned output layout (manifest, commit stamp) #199 leftover after anoverwrite=Truererun into the same store path — the template rewrite doesn't sweep old ad-hoc CSR subgroups) degrades ALL its-31123-style names to negative ints, silently. -
The PR body's mitigation ("the coord array wins when present") is currently vacuous in-repo: nothing in zagg writes the
{field}/mortonsibling — I could find no writer undersrc/zagg/— so the heuristic is the only line of defense until phase 2's manifest.
Cheap tightening that fails loudly instead of silently mis-keying: in the fallback branch, raise (or at least warn) if any name starts with -; optionally also warn when the store mixes parse outcomes. Phase 2's morton_hive.json marker is the real fix, but this reader can return wrong answers with no error today.
Generated by Claude Code
|
|
||
| def _accumulate(report, i, outcome): | ||
| shard_key = outcome["shard_key"] | ||
| label = shard_label(grid, outcome["shard_key"]) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — label rendering in the error-reporting path can now itself raise. shard_label(grid, ...) → morton_decimal raises ValueError on an empty/invalid word (and OverflowError on a negative legacy key — see the companion comment on morton.py). Before this change the log line interpolated the raw int and could never fail.
The failure chain: a malformed key in a catalog makes process_shard raise → _cell_work catches it and returns {"ok": False, ...} → this _accumulate re-renders the same bad key and raises out of the accumulation loop, aborting the whole run's reporting precisely for the shard it was trying to report as failed. Same pattern at the lambda accumulator (line 1522).
mortie itself is deliberate about this: MortonIndexScalar.__str__ never raises ("a repr must never raise" — it renders <invalid 0x...>). The raising morton_decimal is the right choice at the write path (a path component must never be silently wrong), but in these two log-only sites a defensive fallback (try: label = shard_label(...) except Exception: label = str(key), or routing through str(MortonIndexScalar(key))) keeps error reporting unkillable.
Generated by Claude Code
| def morton_decimal(word) -> str: | ||
| """Decimal morton string for one packed shard-key word (issue #199). | ||
|
|
||
| The external/path form of a shard id per the sparse-coverage design record | ||
| (``docs/design/sparse_coverage.md`` D1): the packed ``uint64`` word stays | ||
| the canonical in-memory/wire form, and every externally visible string — | ||
| CSR subgroup names, ``.status`` object keys, log lines — renders through | ||
| mortie's decode-through-kernel decimal repr (e.g. ``-31123``). Raises | ||
| ``ValueError`` on an empty or invalid word (a path component must never be | ||
| silently wrong). | ||
| """ | ||
| from mortie import MortonIndexArray | ||
|
|
||
| return MortonIndexArray.from_words(np.asarray([int(word)], dtype=np.uint64)).decimal_repr()[0] |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix (small) — the docstring promises ValueError on an invalid word, but a negative input raises OverflowError from np.asarray([int(word)], dtype=np.uint64) (verified: morton_decimal(-1) → OverflowError: Python integer -1 out of bounds for uint64). A negative int here is exactly the confusion this migration creates — someone passing a legacy signed decimal key (-4211322) where the packed word belongs — so it's worth normalizing to ValueError with a pointed message (e.g. "negative — is this a legacy signed decimal id? parse it with morton_word(str(id))"). As written, the opaque OverflowError also escapes any caller that catches only ValueError per the documented contract.
Related nit: nothing pins the advertised raise-on-invalid behavior — see the test comment.
Generated by Claude Code
| def morton_word(label: str) -> int: | ||
| """Parse a decimal morton string back to its packed word (issue #199). | ||
|
|
||
| The inverse of :func:`morton_decimal` at the zagg boundary — used where an | ||
| external decimal id re-enters (``--morton-cell``, CSR subgroup names on the | ||
| read path). Raises ``ValueError`` on a malformed id. | ||
| """ | ||
| from mortie.morton_index import _decimal_to_word | ||
|
|
||
| return _decimal_to_word(str(label)) |
There was a problem hiding this comment.
🤖 from Claude (review)
question-for-author (answers PR question 1) — I checked mortie 0.9.0 for a public parse-back. There IS one: MortonIndexArray.from_hive_path(label, suffix="") — with an empty suffix it skips the leaf-suffix check and the directory cross-check (a bare id has no digit chain to anchor on), so it is a pure public scalar parse-back with the same ValueError semantics. Two caveats that make the private name defensible anyway:
MortonIndexArrayis built lazily and requires pandas (_build_classes→_require_pandas), whilemortie.morton_index._decimal_to_wordis numpy-only. Routingreaders/tdigest_tensor.pythroughfrom_hive_pathwould drag the pandas ExtensionArray machinery into the read path per store open._decimal_to_wordis the documented issue-104 surface (its docstring reads like a contract, including the order-29 point/area non-injectivity note), and zagg wraps it exactly once here — small blast radius if it moves.
So: riding the private name seems fine for phase 1, but file the upstream ask (public numpy-only decimal_to_word export) now so the 1.x spec freeze catches it, and note in this docstring why the private import was chosen over from_hive_path (pandas). One substantive caveat worth a line in the docstring: order-29 point ids parse back to the area word (the documented non-injectivity) — irrelevant for shard keys at order ≤ 11, but this helper is exported in __all__ as a general boundary function.
Generated by Claude Code
| def test_morton_word_rejects_malformed(self): | ||
| from zagg.grids.morton import morton_word | ||
|
|
||
| for bad in ("", "0", "7", "150", "abc", "11827859996358475782"): | ||
| with pytest.raises(ValueError): | ||
| morton_word(bad) |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — good coverage of the parse direction (including the raw-packed-word rejection, which is the ambiguity that matters). Two small gaps:
- Nothing pins the emit direction's raising contract: the PR body sells
morton_decimalas "raises on invalid/empty words so a path component is never silently wrong", but no test assertsmorton_decimal(0)raises. One line here would pin it — and would surface the negative-inputOverflowErrorinconsistency flagged onmorton.py. - The round trip covers orders 0/6/9/18 in both hemispheres (good — the southern sign is the case a naive label would drop, and
morton_word(s) == wordgenuinely catches a missing sign). Orders 19–29 — where the decimal form extends past the legacy i64 — are untested; shard orders are ≤ 11 today so this is optional, but a single order-29 area case would pin the full grammar width (<U31).
(test_healpix_shard_label_is_decimal's label == morton_decimal(parent) is tautological with the implementation; the morton_word(label) == parent assert is what carries it — fine as is.)
Generated by Claude Code
| grid_type = (catalog_data.get("grid_signature") or {}).get("type") | ||
| if grid_type == "healpix": | ||
| try: | ||
| target = morton_word(morton_cell) | ||
| except ValueError as e: | ||
| raise ValueError( | ||
| f"--morton-cell {morton_cell!r} is not a decimal morton id " | ||
| f"(shard ids are decimal morton strings since issue #199): {e}" | ||
| ) from e | ||
| else: | ||
| target = int(morton_cell) |
There was a problem hiding this comment.
🤖 from Claude (review)
question-for-author — the grid-type gate and the pointed packed-word rejection are right, and rect stays consistent end-to-end (RectilinearGrid.shard_label = str(int) ⇄ the int(morton_cell) parse here — the protocol contract holds). One residual confusion worth deciding on: a pre-0.9 legacy catalog (signed-i64 shard_keys like -4211322) fed a decimal id will parse cleanly (morton_word("-4211322") succeeds — the legacy form is valid decimal grammar) and then miss every packed-word comparison, surfacing as a bare "shard '...' not in catalog". Hard break says such catalogs are debris, so failing is correct — but the error is indistinguishable from a typo'd id. If cheap, a hint when the catalog's keys look legacy (any negative / all < 2^63 at healpix signature) — "regenerate the shard map" — would save the first person who trips on it. Fine to decline as shim-adjacent; flagging so it's a decision, not an accident.
Generated by Claude Code
| key = f"{int(shard_key)}.json" | ||
| # Status objects are named by the shard label — the decimal morton | ||
| # string for HEALPix (issue #199) — not the raw packed word. | ||
| key = f"{shard_label(grid, shard_key)}.json" |
There was a problem hiding this comment.
🤖 from Claude (review)
nit (re PR question 4, payload-cap deferral) — the write/poll symmetry here is correct (both sides share the one computed key). But note this line undercuts the deferral rationale for the packed-int payload-cap error in _invoke_lambda_cell: "no grid in scope at that depth" is true of the function signature, yet _cell_work is the caller and has both grid and (on async runs, the only runs that can hit the 256 KB Event cap) the already-rendered label right here. Threading a label= kwarg down (defaulting to str(shard_key)) is a two-line change that would make the one user-facing error message about a specific cell agree with every other rendered id in the run. Fine to fold into phase 2 — flagging that it's cheaper than the PR body implies.
Generated by Claude Code
|
🤖 from Claude Two commits pushed: Review findings — disposition
Phase 2 — hive layout (grounded summary)Per the issue and
Not in this commit, by design: write-throughput validation (moved to #202 per your comment), Generated by Claude Code |
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Round-2 adversarial review, fresh context: commits 27c27bd (review fold) and 4b949c0 (phase 2, hive layout), against the D1–D6 contract in docs/design/sparse_coverage.md §2–§3 and mortie 0.9.0's hive_path.
Round-1 folds — all five verified genuine, not superficial:
- Flavor grammar: ≥2-digit floor + signed-name raise, both directions tested (
test_lone_digit_names_are_block_keys,test_signed_name_in_block_keyed_store_raises); floor holds for order-1 ids. Residual multi-digit ambiguity from round 1 remains, acknowledged — nit inline. _safe_label: non-raising, wired at both accumulation sites (local + Lambda), tested for fallback and happy path.ValueErrorcontract: negative input now raisesValueErrorwith the legacy-id remedy named;morton_decimal(0)and-4211322pinned.- Legacy-catalog hint: fires only when keys look legacy (
< 2^60at healpix);python -m zagg.catalogis a real entry point; both hint-fires and stays-bare tested. - Payload-cap label threading:
label=kwarg down to_invoke_lambda_cell, message asserted in the test. Order-29 round trip and themorton_worddocstring rationale also landed.
D-contract: leaf path is exactly mortie's hive_path (pinned by equality test, incl. the order-0 -3/-3.zarr shape via check_node_invariant's len(parts) >= 2); the tree-walk test enforces zero objects above leaves and manifest-only root (D5); stamp is genuinely the final write, only on error-free shards, with absent/unstamped/malformed all None (D4); no-data shards leave no prefix. I verified empirically that the retry's overwrite=True wipes the whole leaf prefix (zarr delete_dir), so torn-attempt debris is fully cleared — but the suite doesn't pin that, and the manifest rerun-check has two real problems (inline): comparing the sweep-mutable pyramid block will brick resumes once the §7 sweep exists, and the "pass overwrite=True to re-template" remedy leaves stale committed old-order leaves that a reader cannot distinguish from legal mixed-order data.
Config surface: the three rejections (rectilinear/sharded/consolidate) are tested; hive + aoi_mask threads correctly; Lambda rejection sits pre-dispatch in the lambda branch only — local runs never touch it, local dry-run returns before the manifest write, and a lambda dry-run fails fast with the pointed message (defensible). One question inline on the hoisted raising shard_label making sync-Lambda dispatch run-fatal.
Conventions: no CI/deploy/infra touched (mkdocs nav only); no new deps in round 2; hive.py is 216 tight lines; docs page matches the implementation (one addition suggested under the overwrite finding). read_commit has no production caller yet — fine, it's the follow-on reader's entry point and the docs point there, but worth remembering it is currently test-only surface.
Findings (6): 3 should-fix — manifest match-check includes pyramid (resume brick); overwrite remedy strands stale committed leaves; torn-write test doesn't pin wholesale-wipe. 1 question — raising label render on the sync-Lambda dispatch path. 2 nits — residual flavor-grammar ambiguity; runner.py module size vs the ~1000-line convention. None block the next phase.
Generated by Claude Code
| """ | ||
| import obstore | ||
|
|
||
| store = open_object_store(store_root, **store_kwargs) | ||
| existing = _read_json(store, MANIFEST_NAME) | ||
| if existing is not None and not overwrite: | ||
| drop = ("generated_at",) | ||
| if {k: v for k, v in existing.items() if k not in drop} != { | ||
| k: v for k, v in manifest.items() if k not in drop | ||
| }: | ||
| raise ValueError( |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — the rerun match-check compares a field the design says will mutate, so the first pyramid sweep bricks every later resume. drop = ("generated_at",) excludes only the timestamp, but the design record is explicit that the pyramid block is "populated/updated by the §7 sweep" (docs/design/sparse_coverage.md §3, D11), and this PR's own docs call it "declared-only in round one". The moment the sweep lands and writes real orders into pyramid, build_manifest's fresh {"orders": [], "aggregation": {}} no longer equals the stored copy, and every subsequent run into that store raises here — with a remedy (overwrite=True) that would clobber the sweep's pyramid declaration to get going again.
Cheap fix now, while the manifest shape is young: exclude pyramid alongside generated_at (or, inverted, compare only the frozen keys — spec, dataset, cell_order, shard_order, split_schedule). The guard's real job is the flat path's _check_signature analogue — orders + identity — and none of the mutable-by-design fields belong in it.
Generated by Claude Code
| f"{MANIFEST_NAME} at {store_root} does not match this run " | ||
| f"(existing {existing!r} vs {manifest!r}); pass overwrite=True to " | ||
| f"re-template the store" | ||
| ) | ||
| return existing | ||
| obstore.put(store, MANIFEST_NAME, json.dumps(manifest, indent=1).encode()) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — the error message's remedy is a silent-corruption footgun: "pass overwrite=True to re-template the store" replaces only the JSON. Unlike the flat path — where emit_template(overwrite=True) routes through zarr's group overwrite and delete_dirs the whole group — a hive "re-template" leaves every committed leaf from the OLD configuration in place: stamped, complete: true, walker-discoverable. And because mixed shard orders are a legal, first-class feature of the layout (D2 — "coarse shards in sparse regions"), a reader walking a store re-templated from shard_order 6 to 5 has no way to tell the stale order-6 leaves from intentional mixed-order data. The stamp certifies them.
Concretely reachable via the message itself: mismatch → user does what the error says → new run writes its shards → old-order leaves it never touches survive as committed data.
Options, cheapest first: (a) reword the message to say overwrite replaces only the manifest and the root must be cleared manually when orders change; (b) have overwrite=True on an orders-mismatch refuse unless the root has no {sign+base} children (one delimiter-LIST); (c) actually sweep the digit tree on overwrite. I'd take (a)+(b); (c) is deletion machinery this phase doesn't need. At minimum docs/hive_layout.md should state that re-templating does not remove existing leaves.
Generated by Claude Code
| def test_torn_write_leaves_debris_then_retry_succeeds(self, monkeypatch, cfg, tmp_path): | ||
| # Torn-write simulation: the worker dies after the dense write, before | ||
| # the stamp. The prefix exists (debris), read_commit says incomplete, | ||
| # and a clean retry overwrites it wholesale and stamps. | ||
| from zagg import runner | ||
| from zagg.store import open_store | ||
|
|
||
| grid = self._grid(cfg) | ||
| shard = _shard_word() | ||
| root = str(tmp_path / "store") | ||
| leaf = hive.shard_leaf_path(root, shard) | ||
|
|
||
| def torn(g, shard_key, urls, **kwargs): | ||
| carrier = self._carrier(grid, shard_key) | ||
| kwargs["write_chunk"](grid.block_index(int(shard_key)), carrier, {}) | ||
| raise RuntimeError("worker died mid-shard") | ||
|
|
||
| monkeypatch.setattr(runner, "process_shard", torn) | ||
| with pytest.raises(RuntimeError, match="died mid-shard"): | ||
| runner._process_and_write_hive( | ||
| shard, [_rec(1)], grid, {}, root, cfg, store_kwargs={}, driver="s3" | ||
| ) | ||
| assert os.path.exists(leaf) # the prefix exists... | ||
| assert hive.read_commit(open_store(leaf)) is None # ...but is debris | ||
|
|
||
| # Retry: same leaf, overwritten wholesale, stamped at the end. | ||
| monkeypatch.setattr(runner, "process_shard", self._streaming_fake(grid)) | ||
| runner._process_and_write_hive( | ||
| shard, [_rec(1)], grid, {}, root, cfg, store_kwargs={}, driver="s3" | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix (test gap) — the torn-write test exercises the easy half of D4. The torn attempt and the retry write the same chunk set, so the test passes even if overwrite=True merely re-put the metadata documents without clearing anything. The load-bearing claim — "overwritten wholesale" — is that stale objects the retry does NOT rewrite are gone: a torn attempt may have written a CSR ragged subgroup or a dense chunk that the retry (different granule outcome, fewer populated chunks) never touches; if those survive, they read back as valid data inside a leaf whose stamp certifies it complete.
I verified the current stack does wipe correctly — pydantic-zarr's root GroupSpec.to_zarr(..., overwrite=True) routes through zarr.group(mode="w") → AsyncGroup.from_store → store_path.delete_dir() (zarr 3.2.1), and both LocalStore and the obstore ObjectStore report supports_deletes=True. But nothing in the suite pins it, and it hangs off two upstream internals: the delete only happens because supports_deletes is true (the fallback branch raises on existing nodes instead — a store without deletes would fail retries outright), and pydantic-zarr only reaches the wipe through its extant-group branch.
Two-line strengthening: in torn(), also write a ragged subgroup (e.g. kwargs["write_chunk"](..., carrier, {"h": ([np.array([1.0])], [0])})), retry with the no-ragged streaming fake, and assert the h/{label} subgroup is absent post-retry. That pins wholesale-overwrite against upstream drift.
Generated by Claude Code
| shard_key, records = payload | ||
| # Rendered once per cell: the status-object name (below) and the | ||
| # payload-cap error message in _invoke_lambda_cell both carry it | ||
| # (issue #199). Raising here is correct — it becomes a path component. | ||
| label = shard_label(grid, shard_key) |
There was a problem hiding this comment.
🤖 from Claude (review)
question-for-author — hoisting the raising shard_label out of the result_prefix branch quietly widens its blast radius to sync Lambda runs, where the label is never a path component. On async runs the raise is right (the label names the status object — round-1 agreement). But on sync runs its only consumer is the payload-cap error message in _invoke_lambda_cell, i.e. purely cosmetic — and an exception out of _cell_work is run-fatal on the Lambda path: dispatch() re-raises anything a future surfaces (src/zagg/dispatch.py future.result() → raise), unlike the local backend where _cell_work catches per cell and one bad key counts as one cells_error. So a single malformed catalog key now aborts an entire sync fan-out mid-flight, where pre-hoist it would have gone to the worker and failed as one cell.
The comment says "Raising here is correct — it becomes a path component", which is only true for result_prefix is not None. Cheapest tightening if you agree: label = shard_label(...) if result_prefix is not None else _safe_label(grid, shard_key) — async keeps fail-loud path semantics, sync keeps error reporting unkillable, matching the round-1 fold's own rationale. Fine to decline as deliberate fail-fast on a corrupt catalog; flagging so the local/lambda asymmetry is a decision, not a side effect of the hoist.
Generated by Claude Code
| def _is_decimal_shard_name(name: str) -> bool: | ||
| """Whether a CSR subgroup name fits the decimal morton *shard-id* grammar. | ||
|
|
||
| Grammar: optional sign, base digit ``1..6``, then one ``1..4`` digit per | ||
| order — with at least ONE order digit. zagg shard orders are >= 1, so a | ||
| real shard id always has >= 2 digits; that floor is what disambiguates the | ||
| lone digits ``"1".."6"``, which the raw grammar would accept as order-0 ids | ||
| but which in practice are small block-index keys (review finding, PR #205). | ||
| """ | ||
| body = name[1:] if name.startswith("-") else name | ||
| return len(body) >= 2 and body[:1] in "123456" and all(d in "1234" for d in body[1:]) |
There was a problem hiding this comment.
🤖 from Claude (review)
nit (residual, acknowledged) — the fold is genuine: the ≥2-digit floor kills the lone-digit "5"/"6" flip (and holds for order-1 ids, whose bodies are always 2 digits), and the signed-name raise turns the debris-poisoning case from silent mis-keying into a loud failure — both directions now pinned in tests/test_readers.py. Recording the ambiguity that remains, per round 1's "or longer names that happen to fit the grammar": a K>1 store whose populated block keys all land in the grammar (e.g. only blocks 11–14 of a K=16 store hold data — no "0", no "5"+ to save it) still flips wholesale to packed-word interpretation with no error. Unreachable for K==1 stores (label-keyed by construction) and self-correcting the moment any non-grammar block is populated, so this is acceptably narrow for a heuristic — but the real disambiguator now exists on the write side (morton_hive.json / the flat store's own attrs). When the hive read path lands, consider stamping the flat K>1 store too (one attrs key naming the subgroup key flavor) so this function can stop guessing. No action needed this PR.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude (review)
nit (conventions) — runner.py is at 2,600 lines after this phase (+219 here), far past the repo's ~1000-line raise-it threshold (CLAUDE.md §4). Pre-existing, not this PR's doing — but the phase-2 additions didn't have to land here: _leaf_block_index and _process_and_write_hive (~110 lines) are hive-layout mechanics with no runner state beyond what's passed in, and hive.py sits at a comfortable 216. Moving them (runner keeps only the store_layout == "hive" dispatch branch) would stop the growth and put the leaf write path next to the stamp/manifest code it exercises. Worth doing either in the Lambda-wiring follow-on (which will touch this seam anyway) or now — either way the module's size deserves a line on the issue thread per the convention.
Generated by Claude Code
|
🤖 from Claude Round-2 fold pushed: Round-2 findings — disposition
Left standing for review (unchanged from the PR body)Lambda+hive wiring scope (PR-body question (1) — follow-on phase here vs fresh issue), the upstream mortie ask (public numpy-only parse-back), manifest identity fields, the viz GeoJSON packed-int property, and the #200 unblock once (1) is decided. Generated by Claude Code |
|
🤖 from Claude Status: both phases implemented and both adversarial-review rounds folded (head c3a8f2d). CI green on every pushed head (ruff, tests 3.12/3.13, docs, lambda builds x86_64+arm64); local suite 1612 passed / 28 skipped (pre-existing credential-gated skips); no ruff-bot findings. Applying Lambda + hive wiring. The hive layout is fully functional on the local backend, but the Lambda backend currently rejects
I lean (b): the store-root conventions this PR freezes are exactly what #200 (coverage MOC) needs, and the worker event-schema change deserves its own review. Related: you asked on #199 to mark #200 unblocked "after all implementation phases here are finished" — say whether that means after this PR (conventions frozen) or after Lambda wiring lands, and I'll act accordingly next run. Also left standing for you (details in the review threads): the upstream mortie ask (public numpy-only Generated by Claude Code |
|
🤖 from Claude Scope settled (recording the in-session discussion so this thread is self-contained):
Removing Generated by Claude Code |
|
🤖 from Claude Phase 3 — Lambda hive wiring — pushed: What the handler now does, per mode
Test coverage
Deliberately excludedPer-shard MOC / Generated by Claude Code |
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Round-3 adversarial review of d193b79 only (phase 3 — Lambda hive wiring), fresh context, against the D2–D6 contract and the scope-settling comment. MOC work explicitly out of scope, not reviewed for absence. No blocking-correctness findings; 2 should-fix, 3 nits, 1 question-for-author (inline).
CI on head d193b79: all green — ruff, test (3.12), test (3.13, concluded 18:32 UTC), docs build, lambda build x86_64 + arm64 (deploy/pr-preview skipped as usual).
Findings
- should-fix (inline,
_handle_setup): a not-yet-redeployed function turns a hive run into a silent flat store — old setup returns the byte-identical 200 body, old workers passstore_layout: hivethrough unvalidated (no output-key whitelist pre-#199) and write flat data; suggest a one-linelayoutecho in the setup response + dispatcher assert, or at least a deploy-ordering sentence in the PR body. Thedatasetkey itself is forward/backward safe. - should-fix (inline,
test_hive.py): the flat-setup byte-identity pin stops at the kwarg level — both dispatch tests monkeypatch_invoke_lambda_setup, so theif dataset is not Noneevent-dict conditional is unexercised. - question-for-author (inline, handler hive branch): PR question (2) assessed — the 500-envelope asymmetry is body-shape only; no partial-success stranding (wholesale wipe pinned on the handler path), same async mirror channel; the only loss is failed-shard duration/memory in the rollups. Recommend accepting as-is.
- nit (inline): stamp-last op ordering is structural but unpinned as a test on either backend.
- nit (inline):
TestRunnerWiringclass docstring still claims the lambda backend rejects hive. - nit (here):
deployment/aws/lambda_handler.pyis now 1,120 lines (1,054 onmain— already past the ~1000-line convention before this PR; +66 here, mostly comments/indentation from the else-wrap). Same pre-existing-size discussion asrunner.pyfrom round 2; flagging for the record, not for this PR.
Verified sound (traced, not just read)
- Shared code path is genuine:
_handle_process's hive branch and the local_cell_workboth callzagg.hive.process_and_write_hiveliterally (handler L920, runner L1232) — stamp-after-stream, error-free-only,overwrite=Truewholesale leaf re-template. A stamped-shard-with-500 is reachable only via a post-stamp handler exception and converges on retry; a 200-without-stamp only on the deliberate zero-chunk no-prefix path. - Frozen
datasetidentity cannot diverge between backends: local passes the whole ShardMapmetadatadict, the dispatcher passes the two-key projection, andbuild_manifestnormalizes both through the samedataset.get(...)— missing metadata yields{"short_name": null, "version": null}on both paths, and the JSON event round-trip is type-preserving, so a mixed local/lambda rerun cannot brick the frozen-key resume check. - Flat path byte-identity holds: the else-branch is the old code verbatim (indentation +
chunk_resultsinit hoist + one line-wrap);store_box/write_error/profile-fold/telemetry stamping unchanged for flat. - Dispatch sweep clean: finalize is gated on
consolidate_metadata, which hive config validation rejects, so no hive finalize invoke; a setup 500 (frozen-key mismatch) raises in_invoke_lambda_setupand aborts before fan-out; status objects stay at the flat sibling.status/prefix; cost/summary rollups use.get()throughout;--morton-cellselection is layout-independent.shard_specnever touchespopulated_shards, so dense-layout configs can't produce different leaves per backend. - Leaf-path equality vs mortie's
hive_pathis pinned on the handler path; handler tests enter via_handle_process/_handle_setup, matching the file's existing convention (24 prior_handle_processcall sites). - Conventions: 6 files touched, no deploy scripts/workflows/
template.yaml, no new deps;hive.pyat 352 lines;docs/hive_layout.mdstatus section accurate for phase 3.
Generated by Claude Code
| if get_store_layout(config) == "hive": | ||
| from zagg.hive import build_manifest, ensure_manifest | ||
|
|
||
| grid = from_config(config, parent_order=event.get("parent_order")) | ||
| ensure_manifest( | ||
| event["store_path"], | ||
| build_manifest(grid, dataset=event.get("dataset")), | ||
| overwrite=event.get("overwrite", False), | ||
| **_output_store_kwargs(event), | ||
| ) | ||
| return {"statusCode": 200, "body": json.dumps({"ok": True, "mode": "setup"})} |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — a stale deployment turns a hive run into a silent flat store. The dataset event key itself is forward/backward safe (an old handler reads only the keys it knows). The hazard is the whole hive mode against a not-yet-redeployed function: the old _handle_setup has no hive branch, so it emits the flat global template at the hive store root and returns literally the same body this branch does ({"ok": true, "mode": "setup"}, 200) — the dispatcher cannot tell the difference. Old workers then pass output.store_layout: hive straight through load_config_from_dict (the pre-#199 validator has no output-key whitelist — checked main's config.py, only bounds keys are whitelisted) and write flat global-block data. Net: the run "succeeds", no manifest, no leaves, no error anywhere, and the user who asked for hive gets a flat store with a template object squatting where the digit tree should be.
Cheap loud guard: have this branch return a distinguishing body (e.g. add "layout": "hive") and have _invoke_lambda_setup assert it when it sent a hive config — one line each side, and a stale deployment fails at setup instead of after a full fan-out. At minimum, one sentence in the PR body noting the function must be redeployed before the first hive run (deploy is espg-side, so the ordering is out of the code's hands).
Generated by Claude Code
| if get_store_layout(config) == "hive": | ||
| # Hive layout (issue #199 phase 3): the worker owns its WHOLE leaf | ||
| # zarr — it derives the leaf path from shard_key + the event's | ||
| # config orders, emits its own leaf template (lazily, on the first | ||
| # chunk), writes its data, and stamps completion as its FINAL PUT | ||
| # (D4), on error-free shards only. process_and_write_hive is the | ||
| # same code path the local dispatcher runs, so leaf semantics | ||
| # cannot drift between backends. A write failure raises out to the | ||
| # handler's exception envelope: the leaf is then unstamped debris, | ||
| # overwritten wholesale on retry — the same recovery model as the | ||
| # local path (no per-chunk error recording needed). | ||
| from zagg.hive import process_and_write_hive | ||
|
|
||
| metadata = process_and_write_hive( | ||
| shard_key, | ||
| event["granule_urls"], | ||
| grid, | ||
| s3_creds, | ||
| store_path, | ||
| config, | ||
| store_kwargs=_output_store_kwargs(event), | ||
| handoff=handoff, | ||
| aoi_payload=aoi_payload, | ||
| profile=profile, | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
question-for-author — answering PR-body question (2), the hive write-failure envelope. I traced the asymmetry end to end and it is body-shape only; nothing strands:
- No partial-success stranding. A multi-field shard that dies after some CSR/dense PUTs leaves an unstamped prefix; the retry's
emit_shard_template(overwrite=True)wipes the leaf wholesale before rewriting —test_hive_worker_death_leaves_debris_then_retry_cleanspins the stale subgroup gone on this exact handler path. A post-stamp handler exception (stamp landed, then e.g. the structured-logjson.dumpsthrows) yields the one stamped-but-500 case, and the retry converges the same way (_leaf()'s comment covers committed-write redo). The reverse — 200 without a stamp — happens only on the deliberate zero-chunk path (no prefix at all), where absence is the answer (D4). - Same status channel both layouts.
_handle_processnever raises out (theexceptat the bottom folds everything into the 500 envelope), so_write_resultmirrors a hive failure toresult_urlexactly as a flat one; async accounting is identical. - What actually differs: a flat write failure returns the full worker metadata (+
error), so the summary rollups keepduration_s/max_memory_mb/total_obsfor the failed shard; a hive write failure returns the bare{"error", "shard_key", "request_id"}envelope —body.get("duration_s", 0)reads 0, memory absent. That's the whole price, and it matches the local backend's_cell_workexcept-path (which also records only the exception).
I'd accept as-is: one shared code path is worth more than failed-shard telemetry. If you want the recorded-error body instead, the fold belongs in process_and_write_hive (catch around process_shard, fold into metadata), not here — a handler-side catch would diverge from local semantics again.
Generated by Claude Code
| ) | ||
| monkeypatch.setattr( | ||
| runner, "_invoke_lambda_setup", lambda *a, **kw: captured.update(setup=kw) | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix (test gap) — the "flat setup events stay byte-identical" pin stops one hop short of the wire. Both dispatch tests monkeypatch runner._invoke_lambda_setup and assert on the captured kwargs (dataset is None / the dict), so the conditional that actually shapes the event — if dataset is not None: event["dataset"] = dataset in _invoke_lambda_setup — is never exercised. A regression that unconditionally adds "dataset": None to the event would pass this suite while breaking the byte-identity claim in the PR body. One direct test of _invoke_lambda_setup with a mocked lambda_client capturing Payload (the pattern _invoke_lambda_cell tests already use) asserting "dataset" not in json.loads(payload) for flat and == {...} for hive would pin the real event dict.
nit — the TestRunnerWiring class docstring (line 479, just above this hunk) still reads "the lambda backend rejects hive with a pointed error" — stale as of this commit; it now asserts the opposite.
Generated by Claude Code
| stamp = hive.read_commit(leaf_store) | ||
| assert stamp["complete"] is True | ||
| assert stamp["cells_with_data"] == 5 | ||
| assert stamp["granule_count"] == 1 |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — stamp-last is structural but unpinned as an op ordering. The leaf-path-equals-mortie-hive_path pin above is exactly right, and the debris tests pin "no stamp on failure" both ways. What no test on either backend pins is the D4 ordering claim itself — that the stamp PUT is the shard's final write. Today that's guaranteed structurally (stamp_commit runs after the synchronous process_shard stream returns in process_and_write_hive), but a future refactor that, say, stamps before a trailing CSR flush would pass every current test while breaking "presence certifies everything before it landed." Since both backends share the one function, a single ordering pin in test_hive.py covers Lambda too: wrap write_ragged_to_zarr/stamp_commit with recording shims and assert the stamp is the last recorded op. Cheap, and it turns the design doc's central invariant into a regression test.
Generated by Claude Code
|
🤖 from Claude Round-3 fold pushed: Round-3 findings — disposition
CI is triggered on Generated by Claude Code |
…ve-layout # Conflicts: # src/zagg/runner.py
|
🤖 from Claude Merged return matches
if max_cells:
pairs = pairs[:max_cells]
# Shuffle after selection/truncation (max_cells keeps its morton-first-N
# subset) so concurrent fan-out doesn't write morton-contiguous -- i.e.
# byte-prefix-sharing -- S3 keys to one partition (issue #197). Seeded from
# the selected shard keys, so a rerun or resume sees the same order.
random.Random(",".join(str(k) for k, _ in pairs)).shuffle(pairs)
return pairsThe shuffle seed string is unchanged by the decimal work ( Generated by Claude Code |
|
🤖 from Claude Mergeable again. PR #204's merge to main conflicted with this branch in Verified on the pushed head (e2fe76d): full suite 1630 passed / 28 skipped — this is the first run with both PRs' test suites composed (issue #197's shuffle/bucket-order tests + this PR's hive/decimal/handler tests); Merge-readiness summary (from the review discussion): all three phases done, three adversarial-review rounds folded, and no open question gates the merge — Q1 ( Generated by Claude Code |
2 participants
Closes #199
Refs #198
Adopts the morton decimal shard-id string as the external form of every shard id, and lands the morton-hive store layout behind a config flag on both backends — the layout migration from the issue, per the sparse-coverage design record (
docs/design/sparse_coverage.md, D1–D6). The packeduint64word stays the canonical in-memory/wire/storage form; every string that leaves the process — CSR subgroup names, async.statusobject keys, log lines,--morton-cell, hive paths — carries the decimal repr (e.g.-4211322instead of11827859996358475782).Phases
morton_hive.jsonmanifest + commit stamp + node invariant + layout docsPhase 1 — decimal shard-id strings
Boundary pair (
src/zagg/grids/morton.py):morton_decimal(word) -> str(mortie 0.9.0decimal_repr(); raisesValueErroron invalid/empty/negative words — negative gets a pointed "this looks like a legacy signed id" message) andmorton_word(label) -> int(mortie's issue-104_decimal_to_wordparse-back; docstring records why the private numpy-only surface was chosen over the public-but-pandasfrom_hive_path, and the order-29 non-injectivity caveat).Grid seam:
grid.shard_label(shard_key)on theOutputGridprotocol —HealpixGridrenders the decimal morton string (D1);RectilinearGridkeeps packed-tile int digits.zagg.grids.base.shard_label(grid, key)falls back tostr(int(key))for minimal grid stand-ins;runner._safe_labeladditionally never raises, for error-reporting sites (async keeps fail-loud labels — they name status objects; sync falls back).Surfaces routed through the label: CSR subgroup paths (K==1; K>1 keeps flattened block-index ints),
.status/<run_id>/<label>.json, worker + dispatcher logging, the async payload-cap error message,--morton-cell(decimal parse for healpix catalogs, with a "regenerate the shard map" hint when the catalog's keys look legacy), the tdigest reader's name parse-back (explicit ≥2-digit grammar; a signed name in a block-keyed store fails loudly), and.github/scripts/run_benchmark.py'smorton_cell(the harness could not dispatch at all post-break; flagged since it lives under.github/).Hard break, no shims (pre-1.0): pre-#199 stores keep packed-int names and won't round-trip. The Lambda event
shard_keyand ShardMapshard_keysstay ints (canonical wire/storage form, D1).Dependency:
mortie>=0.9.0(espg-authorized); neither thelambdaextra nor the layer build pins mortie.Phase 2 — hive store layout (
output.store_layout: hive)src/zagg/hive.py:output.store_layout: flat|hive(defaultflat, byte-identical). Validation rejects hive with rectilinear grids,sharded: true, andconsolidate_metadata.{store_root}/{sign+base}/{d1}/…/{full_id}.zarrvia mortie 0.9.0'shive_path(spec-owned), re-checked against the node invariant before any write. Example:-5112333→root/-5/1/1/2/3/3/3/-5112333.zarr.morton_hive.jsonwritten once at template time — spec version, dataset identity, orders, explicit split schedule, declared-only pyramid block. Reruns verify the frozen keys (spec/dataset/cell_order/shard_order/split_schedule); the sweep-mutablepyramidandgenerated_atare excluded. Re-templating never removes leaves: an overwrite that changes frozen keys refuses when the digit tree has children (one delimiter-LIST) — clear the root instead.hive.process_and_write_hive— leaf template (HealpixGrid.emit_shard_template/shard_spec), leaf-local block placement, CSR naming, stamp ordering — used by BOTH dispatchers (phase 3).docs/hive_layout.md, in the mkdocs nav under Design.Phase 3 — Lambda hive wiring
deployment/aws/lambda_handler.py): under a hive config,_handle_processcalls the samezagg.hive.process_and_write_hivethe local dispatcher runs — the worker derives its leaf path fromshard_key+ the event config's orders, emits its own leaf template lazily, writes, and stamps as its FINAL PUT on error-free shards only. A write failure raises out to the handler's 500 envelope: the leaf is unstamped debris, overwritten wholesale on retry — the same recovery model as local. The flat write machinery is untouched (wrapped in theelsebranch verbatim; existing flat tests pin it).mode: "setup"hive branch: writesmorton_hive.jsononly viahive.ensure_manifest— no global template (D5), same frozen-key resume/overwrite semantics as local. The setup response now echoes the layout it acted on ("layout": "hive"|"flat"); flat setup is otherwise byte-identical.src/zagg/runner.py): the lambda-backend hive rejection is removed. Per-shard events need no new keys —store_layoutrides in the config dict already serialized into every event. One optional event key was added to setup only:"dataset"({"short_name", "version"}from the ShardMap metadata, the same source the local path uses) so the manifest's identity block matches across backends; flat setup events omit it and stay byte-identical (pinned on the wire). Async status objects stay at the flat sibling prefix{store_root}.status/<run_id>/<label>.json— outside the digit tree, per O6.coverage.moc(design being settled on Write store coverage MOC at end of run (dispatcher-side domain declaration) #200 — no stamp fields added for it);mode: "finalize"for hive (validation already rejectsconsolidate_metadata+hive); no deployment script or workflow changes (zagg/hive.pyships in the function bundle as part of the package).How it was tested
tests/test_lambda_handler.py::TestProcessHive/TestSetupHive,tests/test_hive.py::TestRunnerWiring/TestInvokeLambdaSetupEvent): handler hive event → leaf at the exact mortiehive_path, real data written through the real template/write path, label-named CSR, stamp with worker counters; worker-death mid-shard → 500, unstamped debris with a stale CSR subgroup → retry overwrites wholesale (stale object gone) and stamps; no-data shard leaves no prefix; setup-mode hive → manifest only (no template arrays) with thelayoutecho, frozen-key mismatch → 500 "clear the store root", matching rerun resumes; wire-level setup-event pins (mocked boto3 client): hive events carrydataset, flat events omit it and match the pre-phase-3 baseline exactly, hive dispatch against an echo-less/flat-echo (stale) function fails fast with the redeploy message, flat dispatch tolerates echo-less old functions; stamp-last op-ordering pin via recording shims (one test covers both backends — they shareprocess_and_write_hive). Local/lambda equivalence holds by construction._safe_labelfallback + async/sync label asymmetry; manifest build/resume/mismatch/overwrite-guard; node-invariant tree walk; torn-write simulation with the stale-object wholesale-wipe pin; K==1/K==16 leaf-local block mapping; config-flag validation with flat default unchanged.uv run pytest -q→ 1625 passed, 28 skipped (usual AWS-cred/network skips);ruff check/ruff format --checkclean on every touched file; pre-commit mypy shows zero new errors vsmain.N818onzagg.registry.UnknownCapability, codespellgirinrectilinear.py, format drift in six untouched files.Questions for review
datasetkey (the one event-schema addition): sourced from ShardMap metadata dispatcher-side so local and Lambda manifests carry identical identity blocks (datasetis a frozen key — a mixed local/lambda rerun must not mismatch). Alternative considered: sourcing fromconfig.data_sourceworker-side (no event key, but a second identity source that can drift from the catalog). Confirm the event-key choice.write_errorfold. The round-3 review traced it end to end and assessed accept-as-is (no stranding; same async mirror; only failed-shard duration/memory rollups differ) — accepted, with the noted pointer that a future fold belongs inprocess_and_write_hive, not the handler. Final call is yours.morton_wordridesmortie.morton_index._decimal_to_word(numpy-only; rationale in the docstring). The upstream ask (public export before the 1.x freeze) stands for you to file.shard_keyproperty still emits the packed int;coverage.moc+ pyramid sweep are follow-ons (Write store coverage MOC at end of run (dispatcher-side domain declaration) #200 gates the MOC); per the issue comment, Write store coverage MOC at end of run (dispatcher-side domain declaration) #200 can be marked unblocked once this PR's phases are accepted.