validate_config: refuse a temporal companion without a resolvable clock at submission (issue #472) - #473
Conversation
| f"output.windowing block on a continuous scale, which it falls back to). " | ||
| f"Without it there is no column to encode toc words from (issue #410)" | ||
| ) | ||
| raise ValueError(f"Variable '{name}': {TOC_NO_CLOCK_ERROR}") |
There was a problem hiding this comment.
🤖 from Claude (review)
Severity: critical — the failure mode issue #472 reports is still fully live after this PR, and the PR body's timeline explanation is falsified by the git history.
The PR body says: "when the failure was observed, the laptop's validate_config predated PR #463's merge ... which is what let the graft dispatch." That cannot be what happened. temporal: per-centroid was introduced into src/zagg/configs/atl03_tdigest_located_healpix.yaml by ef68ef74 ("phase 4 of issue #410") — the same commit that added _validate_time_source's "is not a declared data_source variable" check — and the clock check landed even earlier, in a67be395 ("phase 2 of issue #410"):
$ git log --format="%h %ad %s" --date=iso -S "temporal: per-centroid" -- src/zagg/configs/atl03_tdigest_located_healpix.yaml
ef68ef74 2026-08-17 08:10:27 -0700 phase 4 of issue #410
$ git log -S "requires the store's per-observation clock" --oneline -- src/zagg/config.py
329e59b6 phase 1 of issue #472
a67be395 phase 2 of issue #410
$ git log -S "is not a declared data_source variable" --oneline -- src/zagg/config.py
ef68ef74 phase 4 of issue #410
So the packaged config that declares the companion and the validator that refuses an unclocked companion ship in the same commit. There is no build in which the graft carries temporal: (necessary for the worker to refuse) and the validator lacks the check. Both cross-checks already worked on main — verified by running the exact graft against a clean origin/main checkout:
MAIN RAISES: Variable 'h_tdigest': 'temporal' requires the store's per-observation clock — declare output.time_source {field, epoch, scale, units} (or an output.windowing block on a continuous scale, which it falls back to). ...
MAIN item2 RAISES: output.time_source.field 'delta_time' is not a declared data_source variable (available, including broadcast level variables: ['h_ph']) ...
The actual root cause is that the demo's submission seam never calls validate_config at all. demo/02_write.ipynb cell 25 dispatches with
run_located = Run.from_config(located_config, shardmap=serc_map, store=..., overwrite=True)
handle_located = run_located.dispatch()and zagg.client.Run.from_config validates only str and dict inputs — an already-constructed PipelineConfig is passed straight through (src/zagg/client.py:478-482):
if isinstance(config, str):
config = load_config(config)
elif isinstance(config, dict):
config = load_config_from_dict(config)
validate_config(config)Its own docstring says so: "Dicts and paths are validated on load." The graft mutates a PipelineConfig obtained from default_config(...) (which validated before the graft), so nothing re-validates the mutated object. zagg.runner.agg and zagg.notebook.run never call validate_config either — grep -n "validate_config" src/zagg/runner.py src/zagg/notebook.py is empty. The only other call site is client_transport.py:492 (Run.attach, manifest-dict path).
Net: this PR changes message text only. A grafted PipelineConfig dispatched through Run.from_config / agg / notebook.run will still burn one invoke per shard on the worker's refusal, which is exactly what the issue title ("at submission, not on the fleet") asks to stop.
Suggested fix: validate the object branch at the submission seam, e.g. in Run.from_config:
if isinstance(config, str):
config = load_config(config)
elif isinstance(config, dict):
config = load_config_from_dict(config)
validate_config(config) # also covers a mutated/grafted PipelineConfigplus the same in runner.agg (or a shared _validated(config) helper both call), with a test that dispatches the graft through Run.from_config with a stub lambda client and asserts it raises before any invoke. If that is judged a scope change beyond issue #472 it needs a maintainer decision rather than a silent pass — but as it stands the PR closes an issue whose symptom it does not remove, so at minimum the PR body's timeline paragraph should be corrected and a follow-up filed for the unvalidated seam.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 7079e32 — the diagnosis holds on both counts, and the PR body's timeline paragraph (now rewritten) was wrong.
Run.from_config cross-validates every input shape now, not just str/dict: the validate_config(config) call moved out of the dict branch and down past the v1 scope gates, so an already-built — and possibly mutated — PipelineConfig is checked too:
if isinstance(config, str):
config = load_config(config)
elif isinstance(config, dict):
config = load_config_from_dict(config)
# ... v1 scope gates (NotImplementedError) ...
validate_config(config)Two placement notes: it runs after the scope gates so an out-of-scope pipeline still gets its "use agg" pointer instead of a config error for a facade it was never going to run through (this keeps test_temporal_config_refused / test_raster_config_refused / test_windowed_config_refused meaningful — all three pass deliberately invalid configs), and before the shard-map/store resolution so nothing touches AWS or reads a manifest first.
Regression pin: tests/test_client.py::TestSubmissionValidation::test_grafted_config_refused_before_any_invoke — the graft goes through Run.from_config with the stub Lambda client, must raise re.escape(TOC_NO_CLOCK_ERROR), and asserts stub.events == []. It fails on main: swapping in origin/main's src/zagg/client.py (branch time_axis.py/config.py kept so the constant imports) gives
E AssertionError: Regex pattern did not match.
E Expected regex: 'a\ field\ declares\ a\ temporal\ companion\ but\ the\ store\ has\ no\ per\-observation\ clock ...'
E Actual message: "ShardMap was built for a different grid than this run config. ..."
— i.e. on main the unclocked graft sails past validation into shard-map resolution, exactly as you described. A positive control (test_grafted_config_with_its_clock_constructs) pins that a correctly grafted config still builds a Run.
Left standing for espg: zagg.runner.agg / zagg.notebook.run still never call validate_config. That is the wider choke point, but wiring it there changes behavior for every local- and lambda-backend agg caller across all three pipeline kinds — a scope call rather than a fold, so it is raised under "Questions for review" in the PR body instead of landing silently here.
| base.aggregation["variables"] = copy.deepcopy(located.aggregation["variables"]) | ||
| return base, located | ||
|
|
||
| def test_graft_without_clock_refused_at_submission(self): |
There was a problem hiding this comment.
🤖 from Claude (review)
Severity: major — the two tests the PR body calls regression pins pass unchanged on origin/main, so they pin nothing this PR fixes.
I ran the whole new TestTemporalClockAtSubmission class against a clean origin/main src/ tree (the only edit needed was defining TOC_NO_CLOCK_ERROR = "requires the store's per-observation clock" so the import resolves, i.e. main's own wording):
1 failed, 5 passed
FAILED TestTemporalClockAtSubmission::test_both_seams_raise_the_same_text
test_graft_without_clock_refused_at_submission, test_graft_clock_without_column_refused_at_submission, test_graft_with_full_clock_validates, test_windowing_fallback_satisfies_the_graft and test_every_shipped_temporal_template_validates are all green on main. Only the message-parity pin fails there — and that one pins the refactor, not the reported behavior.
That is consistent with the diff: _validate_temporal_producer's if config is not None and toc_source(config) is None: raise block already existed on main (a67be395), and this PR only swaps its string. So the class docstring's claim — "The regression shape is the observed one ... every shard then burned an invoke on the worker's refusal" — is not what these tests exercise: they call validate_config(cfg) directly, which was never the seam that failed (see my note on src/zagg/config.py:1993; the demo dispatches through Run.from_config(PipelineConfig), which never calls validate_config).
Suggested fix: add a test that pins the submission seam, not the validator — something like
def test_graft_refused_before_any_invoke(self):
cfg, _ = self._graft()
client = _StubLambdaClient() # records invoke() calls
with pytest.raises(ValueError, match=re.escape(TOC_NO_CLOCK_ERROR)):
Run.from_config(cfg, shardmap=_map, store="s3://b/o.zarr", lambda_client=client)
assert client.invocations == []which fails on main and on this branch as written, and only passes once the seam validates a PipelineConfig object. Keep the current five as the validator-level coverage they are, but drop the "regression" framing from the class docstring so a future reader is not misled about what they guard.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 7079e32 (the missing pin) and a71fd06 (the framing) — the measurement is right: five of the six pass on main, so they pin the validator, not the reported symptom.
The true regression now lives one seam up, as suggested: tests/test_client.py::TestSubmissionValidation::test_grafted_config_refused_before_any_invoke dispatches the graft through Run.from_config with the stub Lambda client, requires TOC_NO_CLOCK_ERROR, and asserts stub.events == []. Verified failing with origin/main's client.py in place (details on the config.py:1993 thread).
TestTemporalClockAtSubmission keeps all six tests as the validator-level coverage they are, and its docstring no longer claims otherwise:
These are **validator-level** pins: the checks themselves predate this PR
(``a67be395``/``ef68ef74``, issue #410) and pass on ``main`` — what is new
here is the single-sourced message text (``test_both_seams_raise_the_same_text``).
The regression for the reported symptom is one seam up, where the graft
actually dispatched unvalidated:
``tests/test_client.py::TestSubmissionValidation`` (fold review).
| #: seam (``processing.aggregate._toc_word_column``, defense in depth) read | ||
| #: identically: the first fleet failure burned one invoke per shard on an | ||
| #: error fully determinable from the config dict alone. | ||
| TOC_NO_CLOCK_ERROR = ( |
There was a problem hiding this comment.
🤖 from Claude (review)
Severity: medium — single-sourcing picked the less informative of the two messages, so the submission seam regresses in actionability.
The wording this PR deletes from _validate_temporal_producer named the remedy that actually applies to a windowed store:
'temporal' requires the store's per-observation clock — declare output.time_source
{field, epoch, scale, units} (or an output.windowing block on a continuous scale,
which it falls back to). Without it there is no column to encode toc words from
TOC_NO_CLOCK_ERROR drops the parenthetical. That matters because the fallback is not a footnote — it is the only correct fix for a windowed config: _validate_time_source (config.py:1103-1125) refuses a time_source block that disagrees with output.windowing on any of the four keys, so an author who follows the new message literally ("declare output.time_source") on a store with a scale: utc windowing block gets a second named refusal, and on a continuous-scale one gets a redundant duplicate declaration that must be kept in sync forever. The PR body flags this under "Questions for review" and answers it by moving the hint into a code comment — but the comment is invisible to the person reading the traceback, which is the entire audience for this string.
The issue asks the two seams to read identically; it does not ask the validator to become worse. Since the constant is now shared, the fix improves the worker seam too.
Suggested fix: put the clause back in the shared constant:
TOC_NO_CLOCK_ERROR = (
"a field declares a temporal companion but the store has no per-observation "
"clock — declare output.time_source {field, epoch, scale, units}, or an "
"output.windowing block on a continuous scale, which it falls back to "
"(spec §8.3, issue #410)"
)TestTemporalShapeDeclaration.test_temporal_requires_the_stores_clock and the new parity test both match the constant, so they follow the change for free.
There was a problem hiding this comment.
🤖 from Claude
Fixed in bd073ca — the clause is back, in the shared constant, so both seams carry it:
TOC_NO_CLOCK_ERROR = (
"a field declares a temporal companion but the store has no per-observation "
"clock — declare output.time_source {field, epoch, scale, units}, or an "
"output.windowing block on a continuous scale, which it falls back to "
"(spec §8.3, issue #410)"
)The reasoning about the windowed case is confirmed in the code: _validate_time_source refuses a time_source block that disagrees with output.windowing on any of the four keys, so "declare output.time_source" taken literally on a windowed store leads to a second refusal or a duplicate declaration that must be kept in sync. The #: doc comment above the constant now records why the remedy lives in the message rather than in a code comment.
Nothing pinned the old exact text: tests/test_processing.py:2135 matches the substring "no per-observation clock" (unchanged), and every other assertion matches TOC_NO_CLOCK_ERROR itself, so they followed for free. tests/test_config.py + tests/test_processing.py: 644 passed.
| "a field declares a temporal companion but the store has no per-observation " | ||
| "clock — declare output.time_source {field, epoch, scale, units} (spec §8.3, " | ||
| "issue #410)" | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
Severity: low — TOC_NO_CLOCK_ERROR is missing from time_axis.__all__.
Every other module-level TOC_* constant is listed (TOC_EPOCH, TOC_FIELD_SHAPES, TOC_GRAMMAR, TOC_PER_CELL_FUNCTIONS, TOC_PRODUCING_FUNCTIONS, TOC_SHAPE_*, TOC_SHAPES, TOC_SOURCE_SCALES, TOC_SPEC, TOC_UNOBSERVED, TOC_WORD_COLUMN — time_axis.py:139-166), and this new one is imported by two other modules plus the test suite, so it is public by use. Leaving it out makes the export list quietly non-exhaustive.
Suggested fix: add "TOC_NO_CLOCK_ERROR", to __all__, keeping the existing sort (between TOC_GRAMMAR and TOC_PER_CELL_FUNCTIONS).
Unrelated aside spotted while checking, not for this PR: the existing __all__ is not actually sorted — "toc_source" sits between "read_time_axis" and "temporal_attrs". Worth a separate small-fix if anyone cares; do not fold it here.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9bfcdfb — "TOC_NO_CLOCK_ERROR", added to time_axis.__all__, between "TOC_GRAMMAR" and "TOC_PER_CELL_FUNCTIONS" as suggested.
The "toc_source" misordering is left alone here per your note — it is pre-existing and out of this PR's scope.
| # KeyError one seam later. | ||
| cfg, located = self._graft() | ||
| cfg.output["time_source"] = dict(located.output["time_source"]) | ||
| assert cfg.output["time_source"]["field"] not in cfg.data_source["variables"] |
There was a problem hiding this comment.
🤖 from Claude (review)
Severity: low — this guard asserts against a narrower set than the code under test checks, so it does not actually establish the precondition it claims.
_validate_time_source does not check data_source.variables alone; it checks the union with the broadcast level variables (config.py:1066-1069):
declared = set((config.data_source or {}).get("variables") or {}) | _segment_variable_names(
config.data_source or {}
)
if field not in declared:So assert cfg.output["time_source"]["field"] not in cfg.data_source["variables"] can hold while the field is declared (on a level), and the assertion would still pass — it just would not be describing the branch the test is meant to reach. Today the two agree for atl03_tdigest_healpix_hive, so the test passes for the right reason by luck of the template.
Suggested fix: assert the same set the validator uses, so the guard tracks the code:
from zagg.config import _segment_variable_names
declared = set(cfg.data_source["variables"]) | _segment_variable_names(cfg.data_source)
assert cfg.output["time_source"]["field"] not in declared(or drop the guard entirely and rely on the pytest.raises(match=...), which is already specific to the message).
There was a problem hiding this comment.
🤖 from Claude
Fixed in ecfd57f — the guard now asserts against the same set _validate_time_source builds, so it tracks the code rather than agreeing with it by luck of the template:
declared = set(cfg.data_source["variables"]) | _segment_variable_names(cfg.data_source)
assert cfg.output["time_source"]["field"] not in declared(_segment_variable_names is imported with the other zagg.config names at the top of the module.) The guard is kept rather than dropped: it is what documents which branch of _validate_time_source the test is aiming at, and the pytest.raises(match=...) alone would not distinguish a template that quietly started declaring delta_time.
| # The clock cross-check runs through the SAME resolver the worker encodes | ||
| # with (``toc_source``: ``output.time_source``, falling back to a | ||
| # continuous-scale ``output.windowing`` block), and raises the worker's | ||
| # exact message so the two seams read identically (issue #472) — the |
There was a problem hiding this comment.
🤖 from Claude (review)
Severity: low — the "two seams read identically" comment overstates the parity in two ways worth naming here, since this comment is now the place a future reader will look.
(1) The validator seam is conditional on pipeline kind; the worker seam is not. _validate_temporal_producer is only reachable from the aggregation-variable loop at config.py:671, which sits after both of validate_config's early returns:
ptype = get_pipeline_type(config)
if ptype != "spatial":
_validate_temporal_config(config); return
if (config.data_source or {}).get("reader") == "raster":
_validate_raster_config(config); return_toc_word_column has no such gate. This is benign today — calculate_cell_statistics is only reached from the spatial point path (zagg.temporal.process_event is a separate engine and processing/raster.py never calls it), so nothing on those branches can produce toc words — but it is exactly the shape of gap that gets inherited silently when a raster or event toc path lands. Worth one clause in the comment: "reachable only on the spatial, non-raster branch, which is the only branch that reaches _toc_word_column today."
(2) "the SAME resolver" is true of toc_source, but the two clock declarations validate their column against different sets. _validate_time_source deliberately accepts a broadcast/segment-level column (config.py:1063-1075, the GEDI shot-rate case); _validate_windowing deliberately refuses one (config.py:1409-1414, "segment-rate window membership is not supported yet"). So a segment-level clock column is a valid explicit time_source and an invalid windowing fallback, even though toc_source would resolve either. Not introduced by this PR, but the comment as written invites the reader to assume the fallback is interchangeable with the explicit block, and it is not.
Suggested fix: extend the comment with both qualifiers (no code change needed for either point).
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9ac9aa1 — both qualifiers are in the comment now (no code change), and both were re-verified before wording them: _validate_temporal_producer is reached only through _validate_output_kind → _validate_temporal_shape, called from the aggregation-variable loop at config.py:671, which sits after both early returns; and _validate_windowing refuses a time_field in _segment_variable_names(ds) ("segment-rate window membership is not supported yet") where _validate_time_source accepts exactly that column.
# worker's copy stays as defense in depth. Two limits on that parity, both
# named here because this is where a reader will look (fold review):
# * this seam is reachable only from ``validate_config``'s spatial,
# non-raster branch (the aggregation-variable loop, after both early
# returns), while ``_toc_word_column`` is ungated. Benign today —
# ``calculate_cell_statistics`` is only reached from the spatial point
# path, so no other branch can produce toc words — but a raster or
# event toc path would inherit the gap silently.
# * the resolver is shared, but the two clock declarations validate their
# COLUMN against different sets: ``_validate_time_source`` accepts a
# broadcast/segment-level column (GEDI's shot-rate clock) where
# ``_validate_windowing`` refuses one (segment-rate window membership
# is unsupported), so the fallback is not interchangeable with the
# explicit block.The "SAME resolver" shout is downcased to plain prose in the same edit, since the qualifiers are what carries the meaning now.
|
🤖 from Claude The first review question is resolved (espg, in-session 2026-08-18): this PR merges as-is with |
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the two new commits only — the sync-merge 07be991a and phase 2 8644f5da. Earlier phases were reviewed and folded; not re-reviewed here.
Verified clean
- Merge resolution is correct and lossless.
git diff d52e3063..deebf7ff(main's contribution) andgit diff 9ac9aa19..07be991adiffer only in the one resolved hunk's line numbers/context — nothing from either parent was dropped inspill.py,streaming.py,worker.py,semantics.py,stats/tdigest.py, the configs, or the tests (those files are byte-identical todeebf7ff)._checked_toc_sourcecorrectly keeps main's post-#478columnsparameter + hoist structure with this branch'sTOC_NO_CLOCK_ERRORsingle-sourcing. The defense-in-depth comment still reads correctly under the hoist: thesource is Nonearm is call-site independent, and both callers (_toc_word_columnper-cell,_chunk_toc_wordsper-chunk) reach it identically. - No config that used to run is newly refused. Full suite green in the worktree:
4444 passed, 37 skipped(tests/test_lambda_build.pydeselected —build_function.shfails for environment reasons here, unrelated and pre-existing). Everyaggcaller insrc/was checked:__main__.pypasses aload_config-validated config (already validated, so the second call is a no-op),notebook.runforwards whatever it was given, and nothing else insrc/callsagg. - Ordering vs kwarg overrides is safe.
validate_configrequires neithercatalognoroutput.store, so thecatalog=/store=kwargs cannot be "fixing" something the validator refuses. The only knob that is both validated and kwarg-overridable isaggregation.handoff; refusing an invalid config value there is right, and it is unreachable from a YAML-loaded config anyway.driver,region,function_name,overwriteare not validated at all. - Double validation is safe.
validate_configis pure — no mutation (model_dump()identical before/after), no warnings emitted on either call, and no nondeterminism; theload_config → aggdouble call costs a dict walk.
Findings — 3 inline, plus one PR-level:
(4) The demo notebook that motivated #472 is still broken, and now fails at submission. demo/02_write.ipynb cell 24 does the exact graft this PR refuses (atl03_tdigest_located_healpix variables onto atl03_tdigest_healpix_hive, no output.time_source) and cell 25 dispatches it through Run.from_config. Verified against this branch:
LOCATED GRAFT REFUSED: Variable 'h_tdigest': a field declares a temporal companion but the
store has no per-observation clock — declare output.time_source {field, epoch, scale, units},
or an output.windowing block on a continuous scale, which it falls back to (spec §8.3, issue #410)
The notebook was already failing fleet-side before this PR (that is the #472 symptom), so this is not a regression — but the PR closes #472 and leaves its motivating artifact non-runnable, against the binder-runnable requirement in CLAUDE.md §4. The fix is one cell and the PR already contains its shape: tests/test_client.py::TestSubmissionValidation::test_grafted_config_with_its_clock_constructs copies located.output["time_source"] plus the clock column from located.data_source["variables"]. Attribution: the refusal itself came from phase 1 (client.py), which was reviewed and folded — raising it here because it is the PR's net effect and is still unaddressed at phase 2. Declining in favour of a follow-up issue is a reasonable call; leaving it silent is not.
(Sibling graft in the same notebook checked and clean: cell 27's strata_config — atl03_tdigest_strata_healpix with the hive output grafted on — validates fine, because the strata template declares location:, not temporal:.)
| # graft) is refused before any catalog read, store touch, or invoke. | ||
| # validate_config branches on pipeline kind itself, so the temporal and | ||
| # raster paths get their own (smaller) checks rather than a false refusal. | ||
| validate_config(config) |
There was a problem hiding this comment.
🤖 from Claude (review)
(1) The choke-point comment overclaims on both of the paths it names.
Run.from_config never calls agg. It has its own validate_config(config) at src/zagg/client.py:514 and dispatches through Run/StatusPoller — so the facade does not "cross-validate here", it cross-validates there. The PR body says this correctly ("Run.from_config's call stays as facade-level belt-and-suspenders"), but the comment as written tells a reader there is one call site when there are two, which is exactly the misreading that would let someone delete the client-side call as redundant.
zagg.notebook.run does read the catalog before agg validates. run() calls max_cost_preview first (notebook.py:297-304), which does runner._load_catalog(catalog_path) (notebook.py:71) — a local read or an S3 GET of the shardmap. Demonstrated on this branch with the #472 graft:
notebook_run(base, catalog='/tmp/does_not_exist_shardmap.json')
# FileNotFoundError : [Errno 2] No such file or directory: '/tmp/does_not_exist_shardmap.json'The unclocked config is refused, but only after the catalog read — and a catalog problem masks the config refusal entirely. So "is refused before any catalog read, store touch, or invoke" is true for direct agg callers and false for the wrapper the same sentence names.
The new test does not catch either, because it passes no catalog (total stays None and the preview branch is skipped) — see finding (3).
Two ways out, either fine:
- reword: say
aggis the choke point for its own callers, namefrom_config's call as the parallel facade-level seam, and scope the "before any catalog read" claim toaggitself; or - hoist
validate_config(config)to the top ofzagg.notebook.run(before the preview), which makes the sentence literally true for that path and is one line.
There was a problem hiding this comment.
🤖 from Claude
Folded both halves in 87219517.
(a) notebook.run now refuses before the preview. validate_config(config) is hoisted above the max_cost_preview branch, so the catalog read can no longer mask the config refusal:
# Refuse before the preview, not just before the invoke (issue #485).
# ``agg`` stays the choke point for the dispatch itself; this call is the
# wrapper's ordering guarantee -- ``max_cost_preview`` below reads the
# shardmap (a local read or an S3 GET), so without it a bad config would
# surface as a catalog error first, masking the real refusal. Same shape
# as the facade's own pre-resolution call in ``client.Run.from_config``;
# ``validate_config`` is pure, so the second call inside ``agg`` is free.
validate_config(config)agg's call stays the choke point; this mirrors client.py:514's facade call — refuse-before-resolution at the wrapper's own entry.
(b) The runner comment now names the paths as they are — agg is the choke point for its own callers and for notebook.run's underlying dispatch; from_config is a parallel facade seam raising identical text, not a route through agg; and the "before any catalog read" claim is scoped to agg itself, with the notebook's own repeat called out so nobody deletes either sibling as redundant.
Regression test: test_notebook_run_refuses_before_the_cost_preview drives the graft through notebook_run(..., catalog=<nonexistent path>) and asserts TOC_NO_CLOCK_ERROR. Verified it discriminates — with the hoist commented out it fails with exactly the error you demonstrated:
E FileNotFoundError: [Errno 2] No such file or directory: '.../no_such_shardmap.json'
src/zagg/runner.py:2122: FileNotFoundError
| def _graft(self): | ||
| # The issue #472 shape: temporal variables grafted onto a validated | ||
| # base after default_config's own validation already ran. | ||
| base = default_config("atl03_tdigest_healpix_hive", validate=False) |
There was a problem hiding this comment.
🤖 from Claude (review)
(2) The comment says the base was validated; the code disables exactly that.
# The issue #472 shape: temporal variables grafted onto a validated
# base after default_config's own validation already ran.
base = default_config("atl03_tdigest_healpix_hive", validate=False)
located = default_config("atl03_tdigest_located_healpix", validate=False)validate=False means default_config's own validation never ran, so the fixture is not the shape the comment describes — and the "mutated after a clean validation" property, which is the whole premise of #472, is the one thing this helper does not reproduce.
Both templates do validate on this branch (checked), so dropping validate=False costs nothing and makes the comment true. If the flag is deliberate (keeping the fixture independent of template health), then the comment should say so instead — something like "loaded unvalidated so the fixture does not depend on the templates staying conformant; the graft is the same mutation 02_write makes after a validated default_config".
Note tests/test_client.py::TestSubmissionValidation._graft has the same flag with the same docstring mismatch — that one is phase-1 code, so it is out of this review's scope, but a single fix would cover both.
There was a problem hiding this comment.
🤖 from Claude
Fixed in f91c5625 — dropped both validate=False flags so the fixture is the shape its comment describes:
base = default_config("atl03_tdigest_healpix_hive")
located = default_config("atl03_tdigest_located_healpix")Both templates validate cleanly, so the fixture now reproduces #472's actual premise — a config mutated after a clean validation — rather than one that was never validated. Comment left as written, since it is now true.
tests/test_client.py::TestSubmissionValidation._graft still carries the flag; agreed it is phase-1 code and out of this review's scope, so it stays standing for espg rather than being swept into a phase-2 fold.
| ``PipelineConfig`` handed to ``agg`` directly — the local backend and the | ||
| raster/temporal branches the facade's v1 gates refuse — still reached the | ||
| workers unvalidated. The refusal must land before catalog/store | ||
| resolution, so these tests pass neither. |
There was a problem hiding this comment.
🤖 from Claude (review)
(3) Nothing here pins the seam at agg rather than inside SpatialStrategy, and no test covers the kinds the class docstring claims.
The docstring says the gap was "the local backend and the raster/temporal branches the facade's v1 gates refuse", and the phase-2 comment claims coverage "on any backend or pipeline kind" — but all three tests drive a spatial, non-raster config. Move the validate_config(config) call out of agg and to the top of SpatialStrategy.run and all three still pass, while temporal and raster submissions silently lose validation again. That is precisely the regression this class exists to prevent, and it is currently unpinned.
Concretely the tests pin: (a) validation precedes catalog resolution on the spatial path, and (b) notebook.run reaches whatever agg does. They do not pin (c) that the call is kind-independent.
One case per non-spatial kind closes it, both cheap and offline:
- temporal: a
pipeline.type: temporalconfig with a variable missing one of_TEMPORAL_SPEC_KEYS, dispatched asagg(cfg, events=[...])— must raise the temporal-variable refusal rather than run zero events.tests/test_runner.pyalready has_temporal_config()/_synthetic_events()to build from. - raster: a
reader: rasterconfig with a malformeddata_source.bandsentry,agg(cfg, catalog=...)— must raise the band refusal, not a raster-strategy error.tests/test_raster_runner.pyhas the config fixtures.
Separately, test_notebook_run_shares_the_choke_point is robust to tqdm's absence (_make_progress falls back to _LogProgress on ImportError) — that part checks out. It is silently not exercising the preview branch though: it passes no catalog and neither shipped template carries a catalog: key, so the branch in finding (1) is untested. Adding catalog= to one case would cover it.
There was a problem hiding this comment.
🤖 from Claude
Pinned in 7ad0561d — one case per non-spatial kind, both offline and store-free, each asserting the validator's message rather than the strategy's own first refusal:
def test_temporal_config_validated_at_the_same_seam(self):
cfg = _temporal_config()
del cfg.aggregation["variables"]["max_t2m"]["temporal_reducer"]
with pytest.raises(ValueError, match="missing required key.*temporal_reducer"):
agg(cfg)and the raster one builds a reader: raster config with a band missing dtype (the shape test_raster_pipeline.py::TestRasterConfigValidation::test_band_requires_dtype refuses), dispatched as agg(cfg), expecting requires a string 'dtype'.
They are discriminating: moving validate_config(config) out of agg and to the top of SpatialStrategy.run makes both fail —
FAILED tests/test_runner.py::TestAggValidatesConfig::test_temporal_config_validated_at_the_same_seam
FAILED tests/test_runner.py::TestAggValidatesConfig::test_raster_config_validated_at_the_same_seam
2 failed, 4 passed
with the temporal one hitting TemporalStrategy's "requires events=" error and the raster one No catalog specified — i.e. exactly the silent loss of validation you described. Dropping the agg-level call and passing no catalog is the discriminator, so neither test needs a catalog or a store.
On the preview branch: covered by the new test_notebook_run_refuses_before_the_cost_preview in finding (1)'s fold (87219517), which passes catalog= and so does exercise it.
|
🤖 from Claude Folded findings (1)–(3) from the phase-2 review; (4) declined, left by design — recorded here so it is not silent.
(4) The finding stays standing for espg to fold into the notebook update. Targeted suites green after the folds: |
1 participant
Closes #472. Closes #485.
What this does
Refuses a
temporal:companion whose per-observation clock does not resolve at submission — with the same message the worker raises fleet-side — and makes the laptop-side dispatch seam actually re-validate the config object a notebook hands it, which is the gap the first 0.47.0 run fell through.Approach
Root cause (corrected after adversarial self-review). It is not that the validator was missing the cross-checks.
mainalready refuses an unclocked companion (_validate_temporal_producer, config.py) and already requirestime_source.fieldto name a declared base-rate column (_validate_time_source, config.py) — and both landed in the same issue #410 commits that introduced thetemporal:template keys (a67be395,ef68ef74), so there is no build in which the graft carriestemporal:while the validator lacks the check. An earlier revision of this description blamed a pre-#463 laptop; that was wrong, and the review falsified it by running the exact graft against a cleanorigin/maincheckout (it raises there).The actual root cause is that the submission seam never called
validate_configon aPipelineConfigobject.zagg.client.Run.from_configvalidated only itsstranddictinputs and passed an already-constructed config straight through (its docstring said as much: "Dicts and paths are validated on load"). The02_writedemo builds withdefault_config(...)— validated before the graft — then mutatesaggregation["variables"], so nothing re-checked the mutated object and every shard learned about the error from the worker, one invoke at a time.Two changes:
Seam (
src/zagg/client.py,Run.from_config):validate_config(config)moved out of the dict branch and now runs on every input shape.It runs after the v1 scope gates, so an out-of-scope pipeline still gets its "use
zagg.runner.agg" pointer rather than a config error for a facade it was never going to run through; and before the shard-map / store resolution, so a refusal costs no manifest read, no boto3 client, no invoke.Message (
src/zagg/time_axis.py):TOC_NO_CLOCK_ERRORsingle-sources the refusal so the worker seam (processing/aggregate.py::_toc_word_column, kept as defense in depth) and the validator seam raise the same text. The constant names both remedies — an explicitoutput.time_sourceblock or the continuous-scaleoutput.windowingfallback — because on a windowed store the fallback is the only correct fix (_validate_time_sourcerefuses atime_sourceblock that disagrees withoutput.windowing).Phases
329e59b6)aggas the singlevalidate_configchoke point (issue runner.agg as the single validate_config choke point (follow-up to #472/#473) #485):runner.aggcross-validates every submission at entry, before catalog/store resolution and the kind dispatch;validate_configis already pipeline-kind-aware (temporal and raster branch to their own checks), so no false refusals — proven by the full suite as the caller survey (4477 passed, 0 callers relied on skipping validation).Run.from_config's call stays as facade-level belt-and-suspenders behind its v1 scope gates; both seams keep raising identical single-sourced text. Tests:tests/test_runner.py::TestAggValidatesConfig— the validate_config: refuse a temporal companion without a resolvable clock at submission, not on the fleet #472 graft refused throughaggANDnotebook.runwith no catalog/store passed (validation must win the race to refuse), plus the positive control pinning that a conformant config still reaches the missing-catalog refusal. Includes the sync-merge with post-streaming: expose block_bytes; buffer_granules default 50 -> 20 (issue #474) #475/aggregate: hoist per-cell toc encode + batch segmented reduces (issue #476) #478main(07be991a: kept main'scolumnshoist structure + this branch'sTOC_NO_CLOCK_ERRORsingle-sourcing in_checked_toc_source). (8644f5da)PipelineConfigat the dispatch seam, with a submission-seam regression test (7079e324)bd073cac)TOC_NO_CLOCK_ERRORfromtime_axis.__all__(9bfcdfbf)ecfd57fb)a71fd06f)9ac9aa19)Testing
Submission-seam regression —
tests/test_client.py::TestSubmissionValidation:test_grafted_config_refused_before_any_invoke— the observed shape (atl03_tdigest_located_healpixvariables grafted ontoatl03_tdigest_healpix_hive, nooutput.time_source) dispatched throughRun.from_configwith the stub Lambda client raisesTOC_NO_CLOCK_ERRORand leavesstub.events == []. Fails onmain: withorigin/main'ssrc/zagg/client.pyswapped in, the unclocked graft sails past validation and the test fails onRegex pattern did not match ... Actual message: "ShardMap was built for a different grid than this run config". No AWS credentials or network are involved on either side.test_grafted_config_with_its_clock_constructs— positive control: a correctly grafted config still builds aRun(three shards), so the new call is not a false refusal.Validator-level pins —
tests/test_config.py::TestTemporalClockAtSubmission(six tests; the class docstring now records that five of them also pass onmainand that the symptom's pin is the client test above): the unclocked graft, the clock naming an undeclared column, the fully grafted positive control, the windowing-fallback path, the both-seams text-parity pin, and a sweep asserting every shippedtemporal:-carrying template validates.Local:
ruff check --select=E,F,W,I --ignore=E501 src testsclean;ruff format --checkclean on all touched files;pytest tests/test_config.py tests/test_client.py tests/test_client_transport.py tests/test_processing.py tests/test_notebook.pygreen.Questions for review
zagg.runner.agg/zagg.notebook.runnever callvalidate_configaggthe single choke point — see the phase entry above; runner.agg as the single validate_config choke point (follow-up to #472/#473) #485 records the scope rationale and is closed by this PR.Variable '<name>':, matching every other per-variable refusal inconfig.py; the core sentence is byte-identical across both seams (pinned bytest_both_seams_raise_the_same_text). If "read identically" meant no prefix at all, dropping it is a one-line change.ruff check src testsflagsN818onsrc/zagg/registry.pyUnknownCapability(Nis outside the PR lint bot'sE,F,W,Iselect), andruff format --checkwould reformat a snippet intests/data/benchmark/README.md.