-
Notifications
You must be signed in to change notification settings - Fork 1
small fixes 2026-08-05: refuse a sub-second windowing epoch (issue #390) #398
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
Changes from 9 commits
c733d7c
1bca2f1
fd2fdd0
c33b0f1
dd5ed4d
b95ac50
ae408ea
ce7d033
2d30bef
12039a3
bcc95d2
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 |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
| import re | ||
| from collections.abc import Callable | ||
| from dataclasses import asdict, dataclass, field | ||
| from datetime import datetime, timedelta | ||
| from datetime import datetime, timedelta, timezone | ||
| from importlib import resources | ||
| from typing import Any, NotRequired, TypedDict | ||
|
|
||
|
|
@@ -1436,9 +1436,30 @@ def _validate_windowing(config: PipelineConfig) -> None: | |
| "delta_time)" | ||
| ) | ||
| try: | ||
| _windows.parse_utc(epoch) | ||
| parsed_epoch = _windows.parse_utc(epoch) | ||
| except ValueError as e: | ||
| raise ValueError(f"output.windowing.epoch: {e}") from e | ||
| # Validate the RENDERED epoch, not the parsed one (issue #390). get_windowing | ||
| # canonicalizes through windows.iso_utc (timespec="seconds"), so a sub-second | ||
| # epoch parses clean here and is then silently truncated at the one place it | ||
| # is consumed — shifting EVERY window conversion by the dropped fraction. The | ||
| # predicate calls the renderer and round-trips it rather than repeating the | ||
| # truncation rule, so the two cannot drift (the PR #367 pattern). | ||
| # The message renders parsed_epoch rather than repr-ing the declared value: | ||
| # yaml.safe_load types an ISO timestamp scalar as a datetime, which parse_utc | ||
| # passes through, so repr would hand the author a stdlib constructor call | ||
| # instead of the value they wrote. The drop is reported in microseconds | ||
| # because that is exactly what iso_utc discards and nothing else: an integer, | ||
| # so no scientific notation for the 1 µs case, and never negative. | ||
| rendered_epoch = _windows.iso_utc(parsed_epoch) | ||
|
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) non-blocking — the merge brought in a second consumer of the rendered windowing epoch that runs before this guard, and it turns a sub-second epoch into a false "two different clocks" diagnostic.
if parse_utc(block["epoch"]) != parse_utc(windowing["epoch"]):
disagree.append(("epoch", block["epoch"], windowing["epoch"]))So a config that declares both blocks with the same sub-second epoch is refused by the wrong check, with a message that is factually untrue. Repro on The two declarations are byte-identical; the only thing that "disagrees" is one side having been rendered. Dropping This narrows the PR body's merge note. It reasons about Fail-closed either way, so not blocking. Two cheap fixes if you want the right message: move
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 Accepted — fixed in The reorder was the wrong lever here on inspection: So the cross-check now reads the epoch off the raw block: declared_epoch = config.output["windowing"]["epoch"]
if parse_utc(block["epoch"]) != parse_utc(declared_epoch):
disagree.append(("epoch", block["epoch"], declared_epoch))Reaching that line means Pinned in On the merge note: you are right that "No relocation was needed" no longer holds as written — no relocation, but the cross-check did need adjusting. Flagged to the parent for a body patch.
|
||
| if _windows.parse_utc(rendered_epoch) != parsed_epoch: | ||
| raise ValueError( | ||
| f"output.windowing.epoch {parsed_epoch.isoformat()!r} carries sub-second " | ||
| f"precision that the canonical whole-second rendering drops: it is " | ||
| f"recorded as {rendered_epoch!r}, which would shift every window " | ||
| f"conversion by {parsed_epoch.microsecond} µs — declare the epoch at " | ||
| f"second precision" | ||
| ) | ||
| scale = block.get("scale") or "utc" | ||
| if scale not in _windows.EPOCH_SCALES: | ||
| raise ValueError( | ||
|
|
@@ -3510,6 +3531,16 @@ def _is_nan_fill(meta: dict) -> bool: | |
| return isinstance(fill, float) and np.isnan(fill) | ||
|
|
||
|
|
||
| #: The raster branch's fixed windowing epoch (issue #247, ratified): raster | ||
| #: window membership is the acquisition's STAC ``datetime``, already an ISO-8601 | ||
| #: UTC instant, so the encoding is the identity one and is never author-declared. | ||
| #: Held as an *instant*, not a spelling — ``get_windowing`` renders it through | ||
| #: ``windows.iso_utc``, the same renderer its point branch canonicalizes the | ||
| #: declared epoch with, so the two branches cannot write different spellings of | ||
| #: the same instant if that rendering ever changes (issue #390). | ||
| _UNIX_EPOCH = datetime(1970, 1, 1, tzinfo=timezone.utc) | ||
|
|
||
|
|
||
| def get_windowing(config: PipelineConfig) -> dict | None: | ||
| """The normalized temporal windowing declaration, or ``None`` (issue #246). | ||
|
|
||
|
|
@@ -3520,17 +3551,25 @@ def get_windowing(config: PipelineConfig) -> dict | None: | |
| {"schedule", "time_field", "epoch", "scale", "units", "windows"} | ||
|
|
||
| ``epoch`` and explicit-window boundaries are canonicalized to ISO-8601 | ||
| UTC strings; ``windows`` is ``None`` except for ``schedule: explicit``, | ||
| where a ``{label, timestamp}`` point entry is desugared to its second-wide | ||
| range (issue #355) so the normalized list is uniformly ``{label, start, | ||
| end}``. | ||
| UTC strings. The ``epoch`` canonicalization is lossless on any config that | ||
| passed ``validate_config`` — ``timespec="seconds"`` would otherwise truncate | ||
| a sub-second epoch and shift every window conversion, so | ||
| :func:`_validate_windowing` refuses one outright (issue #390). A config | ||
| built without that check (``load_config_from_dict``, a hand-rolled worker | ||
| payload) still truncates here; explicit bounds truncate either way, | ||
| documented on :func:`_explicit_window_bounds`. ``windows`` is ``None`` | ||
| except for ``schedule: explicit``, where a ``{label, timestamp}`` point | ||
| entry is desugared to its second-wide range (issue #355) so the normalized | ||
| list is uniformly ``{label, start, end}``. | ||
| On the raster branch (``reader: raster``) ``time_field`` is the fixed STAC | ||
| ``datetime`` and ``epoch``/``scale``/``units`` are hardcoded to the | ||
| Unix-epoch UTC-seconds encoding any ISO instant normalizes to, rather than | ||
| ``datetime`` and ``epoch``/``scale``/``units`` are fixed to the Unix-epoch | ||
| UTC-seconds encoding any ISO instant normalizes to, rather than | ||
| canonicalizing a declared ``epoch`` off the block (``_validate_windowing`` | ||
| rejects those conversion knobs there). The same dict feeds the manifest | ||
| temporal block (:func:`zagg.hive.build_manifest`) and the dispatch fan-out, | ||
| so the two can never disagree. | ||
| rejects those conversion knobs there); the fixed epoch is still rendered | ||
| through the same ``windows.iso_utc`` as the declared one, so the branches | ||
| cannot spell one instant two ways (issue #390). The same dict feeds the | ||
| manifest temporal block (:func:`zagg.hive.build_manifest`) and the dispatch | ||
| fan-out, so the two can never disagree. | ||
| """ | ||
| from zagg import windows as _windows | ||
|
|
||
|
|
@@ -3554,11 +3593,16 @@ def get_windowing(config: PipelineConfig) -> dict | None: | |
| # #247, ratified): the manifest records the resolved field plus the | ||
| # fixed encoding any ISO-8601 UTC instant normalizes to (UTC seconds | ||
| # since the Unix epoch). _validate_windowing rejects the conversion | ||
| # knobs on raster configs, so nothing here can disagree with it. | ||
| # knobs on raster configs, so nothing here can disagree with it. The | ||
| # epoch is *rendered* through iso_utc rather than spelled out, for the | ||
| # reason _validate_windowing's #390 guard calls the renderer instead of | ||
| # restating its truncation rule: a literal here would be a copy of | ||
| # iso_utc's output format, free to drift from the spelling the point | ||
| # branch below gives the very same instant (see _UNIX_EPOCH). | ||
| return { | ||
| "schedule": block["schedule"], | ||
| "time_field": "datetime", | ||
| "epoch": "1970-01-01T00:00:00+00:00", | ||
| "epoch": _windows.iso_utc(_UNIX_EPOCH), | ||
| "scale": "utc", | ||
| "units": "seconds", | ||
| "windows": declared, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,9 @@ | |
| on the mortie spec page (mortie#62). | ||
| """ | ||
|
|
||
| import re | ||
| from datetime import datetime, timezone | ||
|
|
||
| import pytest | ||
|
|
||
| from zagg import hive | ||
|
|
@@ -41,6 +44,21 @@ def _windowed(cfg, schedule="yearly", **over): | |
| return cfg | ||
|
|
||
|
|
||
| def _raster_windowed(schedule="yearly"): | ||
| """A minimal raster + hive + windowing config (issue #247).""" | ||
| c = default_config("atl06") | ||
| c.data_source = { | ||
| "reader": "raster", | ||
| "bands": {"red": {"asset": "red", "dtype": "uint16"}}, | ||
| } | ||
| c.aggregation = {} | ||
| c.output["grid"] = {"type": "healpix", "parent_order": 6, "child_order": 12} | ||
| c.output["store_layout"] = "hive" | ||
| c.output["windowing"] = {"schedule": schedule} | ||
| validate_config(c) | ||
| return c | ||
|
|
||
|
|
||
| # ── config block (phase 2) ─────────────────────────────────────────────────── | ||
|
|
||
|
|
||
|
|
@@ -234,6 +252,96 @@ def test_bad_epoch_rejected(self, cfg): | |
| with pytest.raises(ValueError, match="ISO-8601"): | ||
| validate_config(cfg) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "epoch, declared, rendered, shift", | ||
| [ | ||
| ( | ||
| "2018-01-01T00:00:00.5Z", | ||
| "2018-01-01T00:00:00.500000+00:00", | ||
| "2018-01-01T00:00:00+00:00", | ||
| "500000", | ||
| ), | ||
| # The fraction survives a non-UTC declaration: the guard compares | ||
| # INSTANTS, not spellings, so it is the dropped fraction that | ||
| # refuses this — never the offset (Python accepts sub-minute | ||
| # offsets, and a whole-second one of those round-trips clean). | ||
| ( | ||
| "2018-01-01T05:30:00.001+05:30", | ||
| "2018-01-01T00:00:00.001000+00:00", | ||
| "2018-01-01T00:00:00+00:00", | ||
| "1000", | ||
| ), | ||
| # The smallest representable violation — a microsecond-precision | ||
| # timestamp pasted in from a data file — reads as an integer count, | ||
| # not as 1e-06. | ||
| ( | ||
| "2018-01-01T00:00:00.000001Z", | ||
| "2018-01-01T00:00:00.000001+00:00", | ||
| "2018-01-01T00:00:00+00:00", | ||
| "1", | ||
| ), | ||
| # The production path is datetime-typed, not str: yaml.safe_load | ||
| # resolves an ISO timestamp scalar to an aware datetime and | ||
| # windows.parse_utc passes it through, so the message must render | ||
| # the author's value rather than repr a stdlib constructor call. | ||
| ( | ||
| datetime(2018, 1, 1, 0, 0, 0, 500000, tzinfo=timezone.utc), | ||
| "2018-01-01T00:00:00.500000+00:00", | ||
| "2018-01-01T00:00:00+00:00", | ||
| "500000", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_subsecond_epoch_rejected(self, cfg, epoch, declared, rendered, shift): | ||
| # The epoch parses at full precision but is CONSUMED through | ||
| # windows.iso_utc (timespec="seconds"), so a sub-second epoch used to | ||
| # validate clean and then shift every window conversion by the dropped | ||
| # fraction (issue #390 — the PR #367 validate-vs-render mismatch class). | ||
| # The message quotes the declared instant, the rendered value and the | ||
| # shift, so the refusal is tied to what get_windowing would actually emit. | ||
| _windowed(cfg, epoch=epoch) | ||
| with pytest.raises( | ||
| ValueError, | ||
| match=rf"epoch {re.escape(repr(declared))} carries sub-second precision.*" | ||
| rf"recorded as {re.escape(repr(rendered))}.*shift every window " | ||
| rf"conversion by {shift} µs", | ||
| ): | ||
| validate_config(cfg) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "epoch", | ||
| [ | ||
| "2018-01-01T00:00:00Z", | ||
| "2018-01-01T00:00:00+00:00", | ||
| # Naive input is taken AS UTC (windows.parse_utc), and a whole-second | ||
| # offset-bearing epoch still round-trips — only the fraction is lost. | ||
| "2018-01-01T00:00:00", | ||
| "2018-01-01T05:30:00+05:30", | ||
| ], | ||
| ) | ||
| def test_whole_second_epoch_accepted(self, cfg, epoch): | ||
| # The guard is exactly "the rendering is lossless", not "the declaration | ||
| # is spelled canonically": every whole-second spelling stays legal and | ||
| # canonicalizes to the same instant. | ||
| _windowed(cfg, epoch=epoch) | ||
| validate_config(cfg) | ||
| assert get_windowing(cfg)["epoch"] == "2018-01-01T00:00:00+00:00" | ||
|
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) The two deliberate carve-outs are unpinned. The guard sits after two early returns in (1) cfg.output["windowing"] = {"schedule": "none", "time_field": "delta_time",
"epoch": "2018-01-01T00:00:00.5Z", "scale": "gps"}
validate_config(cfg) # passes
get_windowing(cfg) # NoneThat is right — (2) The raster branch. The PR body leans on this in its scope note — "the raster branch hardcodes (1) is the one worth adding: two lines in this class, and it pins the interaction between the new guard and the Separately, the comment at Generated by Claude Code
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 Both folded in (1) def test_schedule_none_keeps_subsecond_epoch(self, cfg):
# The guard sits AFTER the ``schedule: none`` early return, and must:
# an inert block is equivalent to an absent one (test_absent_is_none),
# get_windowing returns None, and the epoch is never rendered — so
# there is no conversion to shift and nothing to refuse.
cfg.output["store_layout"] = "hive"
cfg.output["windowing"] = {
"schedule": "none",
"time_field": "delta_time",
"epoch": "2018-01-01T00:00:00.5Z",
"scale": "gps",
}
validate_config(cfg)
assert get_windowing(cfg) is NoneThat is the pair you described — it validates and returns Raster carve-out left as you judged: the existing raster-knob rejection test refuses (2) Comment fixed to the stronger statement: # The fraction survives a non-UTC declaration: the guard compares
# INSTANTS, not spellings, so it is the dropped fraction that
# refuses this — never the offset (Python accepts sub-minute
# offsets, and a whole-second one of those round-trips clean).Thanks for checking Generated by Claude Code |
||
|
|
||
| def test_schedule_none_keeps_subsecond_epoch(self, cfg): | ||
| # The guard sits AFTER the ``schedule: none`` early return, and must: | ||
| # an inert block is equivalent to an absent one (test_absent_is_none), | ||
| # get_windowing returns None, and the epoch is never rendered — so | ||
| # there is no conversion to shift and nothing to refuse. | ||
| cfg.output["store_layout"] = "hive" | ||
| cfg.output["windowing"] = { | ||
| "schedule": "none", | ||
| "time_field": "delta_time", | ||
| "epoch": "2018-01-01T00:00:00.5Z", | ||
| "scale": "gps", | ||
| } | ||
| validate_config(cfg) | ||
| assert get_windowing(cfg) is None | ||
|
|
||
| def test_bad_scale_and_units(self, cfg): | ||
| _windowed(cfg, scale="tt") | ||
| with pytest.raises(ValueError, match="scale"): | ||
|
|
@@ -590,6 +698,20 @@ def test_raster_windowing_validates_on_hive(self): | |
| validate_config(c) | ||
| assert get_windowing(c)["time_field"] == "datetime" | ||
|
|
||
| def test_raster_epoch_renders_through_iso_utc(self): | ||
| # Issue #390: the raster epoch is FIXED (issue #247) but not SPELLED — | ||
| # get_windowing renders it with windows.iso_utc like every other | ||
| # instant it emits. Asserting against the renderer's own output rather | ||
| # than a literal is the point: this stays true through a change to | ||
| # iso_utc's precision or offset form, and only a re-introduced literal | ||
| # in config.py can break it. | ||
| from zagg import windows as _windows | ||
|
|
||
| c = _raster_windowed() | ||
| assert get_windowing(c)["epoch"] == _windows.iso_utc( | ||
| _windows.parse_utc(datetime(1970, 1, 1, tzinfo=timezone.utc)) | ||
| ) | ||
|
|
||
|
|
||
| # ── manifest temporal block + spec bump (phase 2) ──────────────────────────── | ||
|
|
||
|
|
@@ -630,6 +752,21 @@ def test_yearly_manifest_declares_v2_temporal(self, cfg): | |
| "append_policy": "new-window", | ||
| } | ||
|
|
||
| def test_raster_and_point_epochs_spell_one_instant_alike(self, cfg): | ||
| # The drift the rendered raster epoch prevents (issue #390): both | ||
| # branches of get_windowing feed the SAME manifest field, so a given | ||
| # instant has to reach it spelled one way. A point config declaring | ||
| # the Unix epoch and a raster config (whose epoch is fixed to it) | ||
| # therefore must produce byte-identical temporal epochs — this is the | ||
| # assertion a re-introduced literal on either branch breaks, since a | ||
| # literal is free to disagree with whatever iso_utc emits. | ||
| _windowed(cfg, epoch="1970-01-01T00:00:00Z", scale="utc") | ||
| validate_config(cfg) | ||
| point = hive.build_manifest(self._grid(cfg), windowing=get_windowing(cfg)) | ||
| rcfg = _raster_windowed() | ||
| raster = hive.build_manifest(self._grid(rcfg), windowing=get_windowing(rcfg)) | ||
| assert raster["temporal"]["epoch"] == point["temporal"]["epoch"] | ||
|
|
||
| def test_explicit_manifest_carries_windows_and_retemplate_policy(self, cfg): | ||
| _windowed(cfg, schedule="explicit") | ||
| m = hive.build_manifest(self._grid(cfg), windowing=get_windowing(cfg)) | ||
|
|
||
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)
nit — the enumeration is one use short on the post-#410 tree.
"every use is a union into the regenerable root
coverage.mocsummary or the overview rollup" missesMocFamily.finishinsrc/zagg/sweep.py:340-342, which usestime_rangeas a skip-if-current predicate, not a union into a summary:That gates whether the root coverage object is rewritten at all. It is not pruning and it is not a defect — both sides of the comparison are floored by the same renderer, so a rerun whose true
t_maxadvanced by < 1 s produces an identical recorded value and there is genuinely nothing to rewrite. But the sentence is written as an exhaustive survey ("every use is …"), and it is the load-bearing half of the "recorded rather than fixed" ruling, so a reader auditing the claim will find a third call site and have to re-derive that it is harmless.Suggest widening it — e.g. "no in-tree consumer prunes on
time_range: every use is a union into the regenerable rootcoverage.mocsummary or the overview rollup, or a skip-if-current comparison between two identically-rendered values (sweep.MocFamily.finish)". The rest of the caveat checks out against the merged tree:windows.iso_time_range->iso_utc->isoformat(timespec="seconds")andraster._us_iso'sint(us) // 1_000_000both floor to the containing second, the#the-commit-stampanchor resolves (docs/hive_layout.md:591), and the caveat reads coherently where main's +643 lines left it.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
Accepted — fixed in
bcc95d23, essentially your suggested wording. The sentence now reads:I added the parenthetical on what the predicate gates because that is the half that makes it self-evidently harmless to a reader auditing the claim: both sides of
union_time_range(existing.get("time_range"), time_range) == existing.get("time_range")(src/zagg/sweep.py:340-342) come through the same renderer, so a rerun whose truet_maxadvanced by < 1 s compares equal and there is genuinely nothing to rewrite — no pruning, no data skipped.Staged alone; docs-only, no test impact.