rename parent_morton event field to shard_key (#24)#42
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial self-review of cb35564. Traced the data flow through from_config → process_shard and both the runner and handler gates, and ran the changed tests.
No blocking defects. The hard rename is complete and the child_order-optional logic is correct and symmetric.
What I verified:
- No stray
parent_mortonevent-field references. Remaining hits are all intentional:processing.py(the deprecatedprocess_morton_cellparam),tests/conftest.py(local morton var),docs/index.md:84(aprocess_morton_cell(parent_morton=...)call — the kept legacy API, not an event), anddocs/design/architecture.md:125(internal morton-algorithm prose). None are event-field usages. child_ordergate is symmetric. Runner (agg) and handler (_handle_process) both key offconfig.output.get("grid", {}).get("type", "healpix"). A HEALPix run cannot slip through withoutchild_order(runner'sget_child_orderraises; handler independently re-gates). Rectilinear is not wrongly rejected (runner setschild_order=Noneand omits the field; handler gate only fires for healpix). Confirmedatl06→healpix,atl06_polar→rectilinear.event.get("parent_order")introduces no None bug.from_configprefersgrid_cfg["parent_order"]from config and only falls back to the kwarg for HEALPix; rectilinear ignores it, soNoneis harmless.parent_orderstays required (key always sent by the orchestrator).- Result-key rename
morton→shard_keyhas exactly one consumer (the per-cell error-warning log), correctly updated; the concurrency-test stub was updated to match.metadata["shard_key"]matches the renamedProcessingMetadatafield. - Tests genuinely assert the rename — legacy
parent_morton→ 400, missingshard_key→ 400, HEALPixchild_ordergate, and both dispatch shapes.
Standing (non-blocking, already disclosed in the PR body):
- The setup-mode event still sends
child_orderunconditionally (None for rectilinear). Out of scope for this process-mode rename and harmless (the setup handler reads it only inside thehealpixbranch). Left for a follow-up if/when setup mode is generalized to non-HEALPix. - The pre-existing
ruff formatdrift on the touched modules (present onmainbefore this PR) is left untouched per "don't fix unrelated CI failures."
Generated by Claude Code
|
🤖 from Claude Status: complete and green — single-phase small-fix, all phases done. CI: Two non-blocking items are flagged under "Questions for review" in the PR body: (1) pre-existing Generated by Claude Code |
|
🤖 from Claude Resolved the merge conflict — the branch is no longer Cause: PR #33 (the #30 sort/hash grouping + Arrow handoff + experimental kernel reducer) merged to How resolved: merged Verified after merge: full suite 272 passed, 1 skipped (the spherely-fork skip), Still Generated by Claude Code |
Closes #24
What this does
Hard-renames the grid-neutral shard identifier in the Lambda event payload from the HEALPix-flavored
parent_mortontoshard_key, and makes the HEALPix-specificchild_orderoptional for non-HEALPix grids (still required/forwarded for HEALPix). No back-compat aliases — a clean break, per @espg.Checklist
parent_morton→shard_keyend-to-end (event field + theProcessingMetadatacontract + handler/runner logging & error responses)child_orderoptional for non-HEALPix (mirror existingparent_orderhandling); required for HEALPixparent_mortonreferences (docs, auth example)child_order, rectilinear without; gate acceptsshard_key, rejects missing / legacy field)§1 by-name authorization (deployment/aws/)
CLAUDE.md §1 forbids editing
deployment/aws/unless an issue names it. @espg explicitly authorized editingdeployment/aws/lambda_handler.pyby name and asked that the handler and runner be kept in sync: #24 (comment) ("yes, edit the lambda_handler; I want the lambda_handler and runner kept in sync for this edit" and "hard rename").Approach
Kept
lambda_handler.pyandrunner.pyin lockstep on the wire contract:deployment/aws/lambda_handler.py— event fieldparent_morton→shard_key(docstring, structured logging, the required-params gate, dispatch intoprocess_shard, and both error/result payloads).child_orderdropped from the unconditional required list and instead required only when the resolved config's grid type ishealpix:parent_orderstays in the base required list (the orchestrator always sends the key —Nonefor non-HEALPix), and the dispatch now readsevent.get("parent_order")(was a hard index), matching the setup path.src/zagg/runner.py—agg()now computeschild_orderonly for HEALPix (get_child_order(config) if grid_type == "healpix" else None), so a rectilinear run no longer raisesoutput.grid.child_order is required._invoke_lambda_cellemitsshard_key(wasparent_morton) and only includeschild_orderin the event when it is set; its result dict keymorton→shard_key(and the one consumer log line).src/zagg/schema.py+src/zagg/processing.py— theProcessingMetadataTypedDict field and theprocess_shardmetadata key are renamedparent_morton→shard_key(theprocess_sharddocstring already advertisedshard_key, so this resolves a latent mismatch the handler read across atmetadata["parent_morton"]). The internal local/param inprocess_shard/_read_groupis renamed too, since the function is already grid-agnostic (shard_keyis its parameter). The deprecated, explicitly HEALPix-flavoredprocess_morton_cell(parent_morton=...)keeps itsparent_mortonname intentionally.src/zagg/auth.py,docs/deployment/lambda.md— event-payload examples/tables updated; the lambda doc's "rename tracked in Reconcile Catalog API #24, not done here" note is rewritten to describe the landed grid-neutral schema.ATL templates: no change needed
@espg asked whether the ATL config templates need updating. They do not:
parent_mortonnever appears insrc/zagg/configs/atl06.yamloratl06_polar.yaml— it's purely an event field, never a config field.child_orderinatl06.yamlis the HEALPix grid-spec field (output.grid.child_order: 12), which is legitimate and stays.atl06_polar.yamlis a rectilinear grid and already has nochild_order— which is exactly the case the optional-child_orderchange now supports end-to-end.What was not touched (deliberately, out of scope):
docs/design/architecture.md(internal morton algorithm prose),tests/conftest.py(a local morton variable in a fixture),docs/index.md(aprocess_morton_cell(parent_morton=...)call — the kept legacy API), and themortonDataFrame coordinate column (a real grid coord, unrelated to the event field).How tested
tests/test_lambda_handler.py(loads the handler by path): missingshard_key→ 400; legacyparent_mortonno longer accepted → 400; HEALPix event missingchild_order→ 400; HEALPix event (withchild_order) and rectilinear event (nochild_order,parent_order=None) both dispatch and the shard key reachesprocess_shard.TestInvokeLambdaCellEventintests/test_runner.py: the per-cell event built by_invoke_lambda_cellcarriesshard_key(noparent_morton);child_orderpresent for HEALPix, omitted whenNone._invoke_lambda_cellstub intests/test_runner_concurrency.pyto the newshard_keyresult key.uv run pytest -v→ 256 passed, 1 skipped (pre-existing skip).uv run ruff check src tests→ clean. mypy on the changed source files → 0 new errors vs themainbaseline (6 pre-existing, all missing-stub / unrelated).Questions for review
ruff format --checkdrift (pre-existing, not from this PR). OnmainHEAD (7cabbfc),ruff format --checkalready reportsprocessing.py,runner.py,schema.py,lambda_handler.py, andtest_runner.pyas "would reformat" — true with both the env's ruff and the CI-pinnedv0.14.10. I verified this against a clean checkout before my edits, so it's standing drift, not something I introduced. Per the "don't fix unrelated pre-existing CI failures" rule I did not mass-reformat those files; my added lines follow the surrounding style and my newtest_lambda_handler.pyis alreadyruff-format-clean. Flagging in case you'd like a separate format-only sweep.child_orderstill sent in setup-mode events. I scoped the optional/omit behavior to the per-cell process event (_invoke_lambda_cell). The setup event still includeschild_order(valueNonefor non-HEALPix); the setup handler only reads it inside thegrid_type == "healpix"branch, soNoneis harmless there. Say the word if you'd like setup made symmetric (omit-when-None) too.Generated by Claude Code