Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/zagg/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,9 @@ def from_config(
----------
config : PipelineConfig, dict, or str
A loaded :class:`~zagg.config.PipelineConfig`, a plain config
dict, or a path to a YAML config file. Dicts and paths are
validated on load.
dict, or a path to a YAML config file. **Every** shape is
cross-validated here (issue #472), including an already-built
``PipelineConfig`` that was mutated after ``default_config``.
shardmap : dict or str, optional
A loaded ShardMap manifest dict or a path to its JSON. Falls back
to the config's ``catalog:`` key. The map's grid signature must
Expand Down Expand Up @@ -479,7 +480,6 @@ def from_config(
config = load_config(config)
elif isinstance(config, dict):
config = load_config_from_dict(config)
validate_config(config)

# v1 scope gate: the spatial point path only. The other pipelines
# already run through agg()/zagg.notebook.run; refusing here beats a
Expand All @@ -502,6 +502,17 @@ def from_config(
"zagg.notebook.run until the v2 transport (issue #327)"
)

# Cross-validate EVERY input shape, not just the dict/path ones (issue
# #472): a ``PipelineConfig`` from ``default_config`` validates once, at
# build time, so a notebook that then grafts another template's
# ``aggregation.variables`` onto it (the 02_write graft) dispatched a
# config nothing had re-checked — and the error surfaced one invoke per
# shard later, fleet-side. Runs after the scope gates above so an
# out-of-scope pipeline still gets its "use 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 nothing touches AWS first.
validate_config(config)

if isinstance(shardmap, dict):
catalog_data = shardmap
if not all(k in catalog_data for k in ("shard_keys", "granules", "grid_signature")):
Expand Down
26 changes: 20 additions & 6 deletions src/zagg/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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

Copy link
Copy Markdown
Member Author

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_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 todaycalculate_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).

Copy link
Copy Markdown
Member Author

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_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.

# 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}")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 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 PipelineConfig

plus 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

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.



def _validate_output_kind(name: str, meta: dict, config=None) -> None:
Expand Down
10 changes: 4 additions & 6 deletions src/zagg/processing/aggregate.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,13 @@ def _checked_toc_source(columns, config) -> dict:
a per-cell namespace or the pooled column dict can be checked without
gathering anything.
"""
from zagg.time_axis import toc_source
from zagg.time_axis import TOC_NO_CLOCK_ERROR, toc_source

source = toc_source(config)
if source is None:
raise ValueError(
"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)"
)
# Defense in depth: validate_config refuses this at submission with the
# same single-sourced message (issue #472).
raise ValueError(TOC_NO_CLOCK_ERROR)
if source["field"] not in columns:
raise ValueError(
f"output.time_source.field {source['field']!r} is not in the cell data "
Expand Down
10 changes: 10 additions & 0 deletions src/zagg/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
get_store_path,
get_sweep,
get_windowing,
validate_config,
)
from zagg.dispatch import (
BENIGN_ERRORS,
Expand Down Expand Up @@ -339,6 +340,15 @@ def agg(
Summary with keys: ``total_cells``, ``cells_with_data``,
``cells_error``, ``total_obs``, ``wall_time_s``, ``store_path``.
"""
# The single validation choke point (issue #485): every submission path —
# Run.from_config's v1 facade, zagg.notebook.run's wrapper, and direct agg
# callers on any backend or pipeline kind — cross-validates here, so a
# PipelineConfig mutated after load_config's own call (the issue #472
# 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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

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 agg is the choke point for its own callers, name from_config's call as the parallel facade-level seam, and scope the "before any catalog read" claim to agg itself; or
  • hoist validate_config(config) to the top of zagg.notebook.run (before the preview), which makes the sentence literally true for that path and is one line.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 areagg 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


# 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
Expand Down
17 changes: 17 additions & 0 deletions src/zagg/time_axis.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 _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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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: _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}, or an "
"output.windowing block on a continuous scale, which it falls back to "
"(spec §8.3, issue #410)"
)

Copy link
Copy Markdown
Member Author

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 — 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_COLUMNtime_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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

#: 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
Expand Down Expand Up @@ -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",
Expand Down
54 changes: 54 additions & 0 deletions tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
surfacing, and the worker-invoke post-run tail.
"""

import copy
import errno
import importlib
import json
import re
import sys
import threading
import time
Expand Down Expand Up @@ -229,6 +231,58 @@ def test_windowed_config_refused(self, catalog):
Run.from_config(cfg, shardmap=catalog, store=_STORE)


class TestSubmissionValidation:
"""A mutated ``PipelineConfig`` is cross-validated at the submission seam.

``from_config`` used to validate only its dict and path inputs, so the
observed failure (issue #472) had nothing re-check the ``02_write`` graft:
``default_config`` validated the hive base template, the notebook then
grafted ``atl03_tdigest_located_healpix``'s ``temporal:`` variables onto it
without ``output.time_source``, and the config error only surfaced one
Lambda invoke per shard later, in the worker's refusal.
"""

def _graft(self):
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_grafted_config_refused_before_any_invoke(self, catalog):
from zagg.time_axis import TOC_NO_CLOCK_ERROR

cfg, _ = self._graft()
stub = StubLambdaClient()
with pytest.raises(ValueError, match=re.escape(TOC_NO_CLOCK_ERROR)):
Run.from_config(
cfg,
shardmap=catalog,
store=_STORE,
function_name="process-shard-test",
lambda_client=stub,
source_credentials=_CREDS,
)
assert stub.events == [] # refused at submission, nothing dispatched

def test_grafted_config_with_its_clock_constructs(self, catalog):
# Positive control: grafting the clock block and its column too is the
# correct form, and it still builds a Run (no false refusal).
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]
cfg.output["grid"] = dict(_ATL06_SIG)
run = Run.from_config(
cfg,
shardmap=catalog,
store=_STORE,
function_name="process-shard-test",
lambda_client=StubLambdaClient(),
source_credentials=_CREDS,
)
assert len(run) == 3


# -- dispatch fan-out --------------------------------------------------------


Expand Down
117 changes: 115 additions & 2 deletions tests/test_config.py
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
Expand All @@ -9,6 +10,7 @@

from zagg.config import (
PipelineConfig,
_segment_variable_names,
_validate_output_kind,
default_config,
evaluate_expression,
Expand All @@ -32,6 +34,7 @@
validate_config,
)
from zagg.processing import calculate_cell_statistics
from zagg.time_axis import TOC_NO_CLOCK_ERROR

# ---------------------------------------------------------------------------
# Fixtures
Expand Down Expand Up @@ -2404,10 +2407,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):
Expand Down Expand Up @@ -2722,6 +2726,115 @@ def test_chunk_precompute_may_not_shadow_the_derived_name(self):
validate_config(cfg)


class TestTemporalClockAtSubmission:
"""``validate_config`` refuses a ``temporal:`` companion whose clock does
not resolve, in the worker's own words (issue #472).

The config 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.

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).
"""

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):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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: 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).

# 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"])
# Guard the precondition against the SAME set the validator checks —
# declared variables plus broadcast level variables — so it tracks
# ``_validate_time_source`` rather than agreeing with it by luck of
# this template (fold review).
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))
Expand Down
Loading
Loading