-
Notifications
You must be signed in to change notification settings - Fork 1
validate_config: refuse a temporal companion without a resolvable clock at submission (issue #472) #473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
validate_config: refuse a temporal companion without a resolvable clock at submission (issue #472) #473
Changes from all commits
329e59b
7079e32
bd073ca
9bfcdfb
ecfd57f
a71fd06
9ac9aa1
07be991
8644f5d
8721951
f91c562
7ad0561
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1960,6 +1960,7 @@ def _validate_temporal_producer(name: str, meta: dict, config=None) -> None: | |
| ``validate_config`` path passes it. | ||
| """ | ||
| from zagg.time_axis import ( | ||
| TOC_NO_CLOCK_ERROR, | ||
| TOC_PER_CELL_FUNCTIONS, | ||
| TOC_PRODUCING_FUNCTIONS, | ||
| TOC_SHAPE_PER_CELL, | ||
|
|
@@ -1983,13 +1984,26 @@ def _validate_temporal_producer(name: str, meta: dict, config=None) -> None: | |
| f"sibling, and a digest kernel's channel is not a dense per-cell array " | ||
| f"(spec §8.2/§8.3)" | ||
| ) | ||
| # 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 | ||
| # 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. | ||
| if config is not None and toc_source(config) is None: | ||
| raise ValueError( | ||
| f"Variable '{name}': 'temporal' requires the store's per-observation clock — " | ||
| f"declare output.time_source {{field, epoch, scale, units}} (or an " | ||
| 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}") | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 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 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 The actual root cause is that the demo's submission seam never calls run_located = Run.from_config(located_config, shardmap=serc_map, store=..., overwrite=True)
handle_located = run_located.dispatch()and 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 Net: this PR changes message text only. A grafted Suggested fix: validate the object branch at the submission seam, e.g. in 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Fixed in 7079e32 — the diagnosis holds on both counts, and the PR body's timeline paragraph (now rewritten) was wrong.
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 Regression pin: — i.e. on main the unclocked graft sails past validation into shard-map resolution, exactly as you described. A positive control ( Left standing for espg: |
||
|
|
||
|
|
||
| def _validate_output_kind(name: str, meta: dict, config=None) -> None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| get_store_path, | ||
| get_sweep, | ||
| get_windowing, | ||
| validate_config, | ||
| ) | ||
| from zagg.dispatch import ( | ||
| BENIGN_ERRORS, | ||
|
|
@@ -339,6 +340,20 @@ def agg( | |
| Summary with keys: ``total_cells``, ``cells_with_data``, | ||
| ``cells_error``, ``total_obs``, ``wall_time_s``, ``store_path``. | ||
| """ | ||
| # The validation choke point for this entry (issue #485): direct agg | ||
| # callers on any backend or pipeline kind, plus zagg.notebook.run, whose | ||
| # dispatch bottoms out here — so a PipelineConfig mutated after | ||
| # load_config's own call (the issue #472 graft) is refused before agg | ||
| # reads a catalog, touches a store, or invokes. Two sibling seams call | ||
| # validate_config themselves rather than routing through this one, and | ||
| # both must stay: client.Run.from_config dispatches via Run/StatusPoller, | ||
| # never through agg (client.py, same error text), and notebook.run repeats | ||
| # the call above its max_cost_preview branch, which reads the shardmap | ||
| # before reaching agg. 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) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude (review) (1) The choke-point comment overclaims on both of the paths it names.
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 The new test does not catch either, because it passes no catalog ( Two ways out, either fine:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Folded both halves in (a) # 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)
(b) The runner comment now names the paths as they are — Regression test: |
||
|
|
||
| # Pipeline kind picks the strategy (issue #12, Phase 5). The strategy seam | ||
| # is dispatch-level: the spatial path is the existing code, moved verbatim | ||
| # into SpatialStrategy so its behavior/output stays byte-identical; the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,6 +91,22 @@ | |
| #: grid is meant to imply. Window ROUTING tolerates that; a word claiming | ||
| #: nanosecond exactness cannot. | ||
| TOC_SOURCE_SCALES = ("gps", "tai") | ||
| #: The refusal for a ``temporal:`` companion whose clock does not resolve — | ||
| #: :func:`toc_source` returns ``None``. Single-sourced (issue #472) so the | ||
| #: submission seam (``config._validate_temporal_producer``) and the worker | ||
| #: 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. Both remedies are named | ||
| #: in the text: on a windowed store the fallback is the only correct one, since | ||
| #: ``_validate_time_source`` refuses an explicit block that disagrees with | ||
| #: ``output.windowing`` — a hint in a code comment is invisible to the person | ||
| #: reading the traceback (fold review). | ||
| TOC_NO_CLOCK_ERROR = ( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 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
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)"
)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 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: Nothing pinned the old exact text: |
||
| "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)" | ||
| ) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude (review) Severity: low — Every other module-level Suggested fix: add Unrelated aside spotted while checking, not for this PR: the existing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Fixed in 9bfcdfb — The |
||
| #: The §8 word-grammar citation — a grammar REVISION token in the ecosystem's | ||
| #: {name}/{major} style (``zagg-ragged/1``, ``morton-hive/2``), never a | ||
| #: documentation URL or a stamp of the writer's installed mortie: store bytes | ||
|
|
@@ -121,6 +137,7 @@ | |
| "TOC_EPOCH", | ||
| "TOC_FIELD_SHAPES", | ||
| "TOC_GRAMMAR", | ||
| "TOC_NO_CLOCK_ERROR", | ||
| "TOC_PER_CELL_FUNCTIONS", | ||
| "TOC_PRODUCING_FUNCTIONS", | ||
| "TOC_SHAPE_COORDINATE", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 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_produceris only reachable from the aggregation-variable loop atconfig.py:671, which sits after both ofvalidate_config's early returns:_toc_word_columnhas no such gate. This is benign today —calculate_cell_statisticsis only reached from the spatial point path (zagg.temporal.process_eventis a separate engine andprocessing/raster.pynever 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_columntoday."(2) "the SAME resolver" is true of
toc_source, but the two clock declarations validate their column against different sets._validate_time_sourcedeliberately accepts a broadcast/segment-level column (config.py:1063-1075, the GEDI shot-rate case);_validate_windowingdeliberately refuses one (config.py:1409-1414, "segment-rate window membership is not supported yet"). So a segment-level clock column is a valid explicittime_sourceand an invalid windowing fallback, even thoughtoc_sourcewould 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 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_produceris reached only through_validate_output_kind→_validate_temporal_shape, called from the aggregation-variable loop atconfig.py:671, which sits after both early returns; and_validate_windowingrefuses atime_fieldin_segment_variable_names(ds)("segment-rate window membership is not supported yet") where_validate_time_sourceaccepts exactly that column.The "SAME resolver" shout is downcased to plain prose in the same edit, since the qualifiers are what carries the meaning now.