-
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 1 commit
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,13 @@ 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. | ||
| 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 |
|---|---|---|
|
|
@@ -91,6 +91,17 @@ | |
| #: 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. | ||
| 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} (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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| """Tests for the YAML pipeline configuration system.""" | ||
|
|
||
| import json | ||
| import re | ||
| from dataclasses import asdict | ||
|
|
||
| import numpy as np | ||
|
|
@@ -32,6 +33,7 @@ | |
| validate_config, | ||
| ) | ||
| from zagg.processing import calculate_cell_statistics | ||
| from zagg.time_axis import TOC_NO_CLOCK_ERROR | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Fixtures | ||
|
|
@@ -2404,10 +2406,11 @@ def test_shape_and_reducer_must_agree(self): | |
|
|
||
| def test_temporal_requires_the_stores_clock(self): | ||
| # Without output.time_source (or a continuous-scale windowing block to | ||
| # fall back to) there is no column to encode words from. | ||
| # fall back to) there is no column to encode words from. The refusal is | ||
| # the worker's own text (issue #472) prefixed with the variable name. | ||
| cfg = self._centroid_cfg() | ||
| del cfg.output["time_source"] | ||
| with pytest.raises(ValueError, match="requires the store's per-observation clock"): | ||
| with pytest.raises(ValueError, match=re.escape(TOC_NO_CLOCK_ERROR)): | ||
| validate_config(cfg) | ||
|
|
||
| def test_windowing_satisfies_the_clock(self): | ||
|
|
@@ -2722,6 +2725,104 @@ def test_chunk_precompute_may_not_shadow_the_derived_name(self): | |
| validate_config(cfg) | ||
|
|
||
|
|
||
| class TestTemporalClockAtSubmission: | ||
| """A ``temporal:`` companion without a resolvable clock is refused at | ||
| submission, not first by the worker on the fleet (issue #472). | ||
|
|
||
| The regression shape is the observed one: the ``02_write`` demo grafted | ||
| ``aggregation["variables"]`` from ``atl03_tdigest_located_healpix`` (whose | ||
| variables declare ``temporal: per-centroid``, PR #463) onto the hive base | ||
| template without also grafting ``output.time_source`` and the | ||
| ``delta_time`` source column — every shard then burned an invoke on the | ||
| worker's refusal, for an error fully determinable from the config dict. | ||
| """ | ||
|
|
||
| def _graft(self): | ||
| import copy | ||
|
|
||
| base = default_config("atl03_tdigest_healpix_hive", validate=False) | ||
| located = default_config("atl03_tdigest_located_healpix", validate=False) | ||
| base.aggregation["variables"] = copy.deepcopy(located.aggregation["variables"]) | ||
| return base, located | ||
|
|
||
| def test_graft_without_clock_refused_at_submission(self): | ||
|
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: major — the two tests the PR body calls regression pins pass unchanged on I ran the whole new
That is consistent with the diff: 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
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 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:
|
||
| # The exact observed shape, pinned to the worker's message text so the | ||
| # two seams read identically. | ||
| cfg, _ = self._graft() | ||
| assert (cfg.output or {}).get("time_source") is None | ||
| with pytest.raises(ValueError, match=re.escape(TOC_NO_CLOCK_ERROR)): | ||
| validate_config(cfg) | ||
|
|
||
| def test_graft_clock_without_column_refused_at_submission(self): | ||
| # The graft's second missing piece: time_source present but its field | ||
| # not a declared data_source variable — refused here, not as a worker | ||
| # 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"] | ||
|
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 — this guard asserts against a narrower set than the code under test checks, so it does not actually establish the precondition it claims.
declared = set((config.data_source or {}).get("variables") or {}) | _segment_variable_names(
config.data_source or {}
)
if field not in declared:So 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
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 ecfd57f — the guard now asserts against the same set declared = set(cfg.data_source["variables"]) | _segment_variable_names(cfg.data_source)
assert cfg.output["time_source"]["field"] not in declared( |
||
| with pytest.raises(ValueError, match="not a declared data_source variable"): | ||
| validate_config(cfg) | ||
|
|
||
| def test_graft_with_full_clock_validates(self): | ||
| # Grafting BOTH missing pieces (the clock block and its column) is the | ||
| # correct form of the demo's config, and it validates. | ||
| cfg, located = self._graft() | ||
| cfg.output["time_source"] = dict(located.output["time_source"]) | ||
| field = cfg.output["time_source"]["field"] | ||
| cfg.data_source["variables"][field] = located.data_source["variables"][field] | ||
| validate_config(cfg) | ||
|
|
||
| def test_windowing_fallback_satisfies_the_graft(self): | ||
| # The continuous-scale windowing fallback (PR #463) resolves the clock | ||
| # through the same resolver the worker uses (toc_source), so the graft | ||
| # with a windowing block and its column — but no time_source — is valid. | ||
| cfg, _ = self._graft() | ||
| cfg.data_source["variables"]["delta_time"] = "{group}/heights/delta_time" | ||
| cfg.output["windowing"] = { | ||
| "schedule": "yearly", | ||
| "time_field": "delta_time", | ||
| "epoch": "2018-01-01T00:00:00", | ||
| "scale": "gps", | ||
| } | ||
| validate_config(cfg) | ||
|
|
||
| def test_both_seams_raise_the_same_text(self): | ||
| # Parity pin: the worker's defense-in-depth refusal is the exact string | ||
| # the validator embeds (single-sourced in zagg.time_axis, issue #472). | ||
| from zagg.processing.aggregate import _toc_word_column | ||
|
|
||
| cfg, _ = self._graft() | ||
| with pytest.raises(ValueError) as worker_exc: | ||
| _toc_word_column({}, cfg) | ||
| with pytest.raises(ValueError) as submit_exc: | ||
| validate_config(cfg) | ||
| assert str(worker_exc.value) == TOC_NO_CLOCK_ERROR | ||
| assert TOC_NO_CLOCK_ERROR in str(submit_exc.value) | ||
|
|
||
| def test_every_shipped_temporal_template_validates(self): | ||
| # Sweep the packaged configs for aggregation variables carrying the | ||
| # ``temporal:`` key; each such template must pass validate_config, and | ||
| # the sweep must actually find the known carriers (a guard against the | ||
| # discovery matching nothing). | ||
| from importlib import resources | ||
|
|
||
| import zagg.configs | ||
|
|
||
| names = sorted( | ||
| p.name[: -len(".yaml")] | ||
| for p in resources.files(zagg.configs).iterdir() | ||
| if p.name.endswith(".yaml") | ||
| ) | ||
| carriers = set() | ||
| for name in names: | ||
| cfg = default_config(name, validate=False) | ||
| agg_vars = (cfg.aggregation or {}).get("variables") or {} | ||
| if any(isinstance(m, dict) and m.get("temporal") for m in agg_vars.values()): | ||
| carriers.add(name) | ||
| validate_config(cfg) | ||
| assert carriers >= {"atl03_tdigest_located_healpix", "gedi01b_waveform_healpix_hive"} | ||
|
|
||
|
|
||
| class TestOverviewDelta: | ||
| def test_valid_overview_delta_validates(self): | ||
| validate_config(_ragged_cfg(inner_shape=[2], overview_delta=512)) | ||
|
|
||
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.