Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2893d6a
phase 1 of issue #459
claude Aug 17, 2026
89f20f4
phase 2 of issue #444
claude Aug 17, 2026
d8518c2
fold review: call the guard's rebuild and pin rule instead of restati…
claude Aug 17, 2026
32f2b99
fold review: anchor the targets.json entry lookup on the key (issue #…
claude Aug 17, 2026
844348c
fold review: state the spherely backend note as a forward condition (…
claude Aug 17, 2026
0aa3358
fold review: slice the aoi_mask when pruning to the pinned shard (iss…
claude Aug 17, 2026
ba246df
fold review: put tools/ on the path once, not per driver import (issu…
claude Aug 17, 2026
84678a4
fold review: separate the stale mortie_order excuse from the volatile…
claude Aug 17, 2026
4a96fd5
fold review: make the volatile-line filter check its own work (issue …
claude Aug 17, 2026
1c49621
fold review: pin the acceptance test's exemption instead of importing…
claude Aug 17, 2026
967f9ae
fold review: order re-pins by nesting depth and cover main's write pa…
claude Aug 17, 2026
db14b48
fold review: cut the pyramid opt-out to the sibling one-liner (issue …
claude Aug 17, 2026
0b4f9e2
fold review: correct the driver section header to match the hoisted r…
claude Aug 17, 2026
819ddeb
Merge remote-tracking branch 'origin/main' into claude/small-fixes-20…
espg Aug 24, 2026
57364d5
fold review: move the re-pin driver's engine to bench_metrics (issue …
espg Aug 24, 2026
98ab544
reconcile the s2 parity test to the shipped pyramid opt-out (issue #459)
espg Aug 24, 2026
8aed187
fold review: correct the bench_metrics side-effect claim (issue #444)
espg Aug 24, 2026
2d7d914
fold review: reconcile the s2 bench config comment to the shipped pyr…
espg Aug 24, 2026
7307a38
fold review: point the re-pin README block at the shared bench_metric…
espg Aug 24, 2026
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
8 changes: 8 additions & 0 deletions src/zagg/configs/sentinel2_l2a.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ output:
# int64 microseconds cannot state and a toc word can. Output-defining: a
# store born on toc is a different product from a legacy-axis one.
time_encoding: toc
# Overview family OFF, declared rather than left absent (issue #459): an

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)

The comment attributes a dispatch effect that this key does not have — and it is 8 lines where all seven sibling opt-outs in the repo are one.

The fix itself is right, and I verified the root cause end to end on this branch:

pyramid key: False  resolved: None
block after : {'spec': 'zagg-pyramid/1', 'overview': {'orders': []}}
block before: {'spec': 'zagg-pyramid/1', 'overview': {'spacing': 2, 'orders': [9, 7, 5, 3, 1], …, 'fields': {}}}
hash before == hash after == 65a66c75262e90b5a6b34a8a194873b508bac61c54ee66b2ab6730da3b1626c9

The problem is the prose. It says an absent key means the default is ON "so every raster run dispatches a rollup sweep whose overview family generates nothing". The so is false. Dispatch is gated on store_layout == "hive" and get_sweep(config) — nothing in that expression reads output.pyramid. This config resolves to store_layout: hive (grid-aware default, get_store_layout) and get_sweep → True (default-on for hive), before and after this change, which I confirmed by running it. Setting pyramid: false suppresses zero dispatches; it changes only the manifest declaration, and that is the actual — and sufficient — win. A future reader who takes the comment at face value will conclude the sweep stopped and be wrong.

Two related notes:

  • Style (§4 "match the surrounding code"). Every other opt-out in the tree is a one-liner: atl03_tdigest_healpix_hive.yaml:83, atl03_tdigest_strata_healpix.yaml:157, gedi01b_waveform_healpix_hive.yaml:206, s2_neon_o9.yaml:32, and the three benchmark configs. An 8-line block here is out of house style for a one-word knob. I would cut it to the sibling form and let the PR body carry the essay:
    pyramid: false                 # overview family off: raster leaves are column-less (issue #399, issue #459)
  • PR body Q1 undercounts the dispatch sites. It says "runner.py, both dispatch sites". There are four get_sweep(config) gates: line 1173 (local raster, in-process), 1611 (Lambda raster), 3328 (local point), 4201 (Lambda point). The decline of issue Shipped S2 config's absent pyramid key silently defaults the (no-op) raster sweep on #459 item (3) still looks right to me, but espg is being asked to rule on a follow-up that is 4 call sites, not 2 — worth correcting before that ruling.

Generated by Claude Code

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

You are right on the causation and I have not tried to defend it — folded in db14b48. The 8-line block is now the sibling one-liner, using your wording:

  pyramid: false                 # overview family off: raster leaves are column-less (issue #399, issue #459)

The false so is gone with it: the comment no longer claims anything about dispatch, because the change does not affect dispatch. What it does — drop the manifest's five-level declaration to {"overview": {"orders": []}} — is the win, and the PR body carries that measurement.

PR-body Q1 corrected too. It now says four get_sweep(config) gates with your line numbers (1173 local raster, 1611 Lambda raster, 3328 local point, 4201 Lambda point) instead of "both dispatch sites", so the issue #459 item (3) ruling is made against the real call-site count. I left the decline itself standing — your read of it matches mine, and the reasoning was about the four sweep families, not the number of gates.

Re-verified after the edit that the resolution and the identity are untouched: cfg.output["pyramid"] is False, get_pyramid(cfg) is None, build_pyramid_block(...) == {"spec": "zagg-pyramid/1", "overview": {"orders": []}}, and tests/test_raster_runner.py is green at 61 passed.


Generated by Claude Code

# absent key means the every-2-orders default is ON (`get_pyramid` -> {}), so
# every raster run dispatches a rollup sweep whose overview family generates
# nothing -- raster fields have no fold law (issue #399 ruled option (b), a
# raster column writer, and it is unimplemented; raster leaves stay
# column-less by construction, PR #416 phase 5). Mirrors the benchmark leg's
# opt-out in tests/data/benchmark/configs/s2_neon_o9.yaml (issue #201).
pyramid: false
grid:
type: healpix
parent_order: 11
Expand Down
14 changes: 14 additions & 0 deletions tests/data/benchmark/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,20 @@ places the panel.
print(bench_metrics.select_densest_shard(sm)) # -> (shard_key, n_granules)
```

> **Re-pinning a map that already exists** is one command:
> `tools/repin_benchmark_shardmaps.py` (issue #444) rebuilds through the
> drift check's own recipe, selects the pin (nested rule included), prunes

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)

Stale after 57364d5a: "rebuilds through the drift check's own recipe". As of that commit the recipe is no longer the drift check's — rebuild_shardmap/select_pin live in .github/scripts/bench_metrics.py, and the drift check is one of two callers, not the owner. 57364d5a rewrote exactly this phrasing everywhere else it appeared (the driver's module docstring, repin()'s docstring, the entry() note, and the test module's section comment: "Both CALL bench_metrics.rebuild_shardmap and bench_metrics.select_pin") but missed this README block, which this same PR added.

It matters here because the sentence is the one that tells a re-pinner where the shared logic lives, and the whole point of the fold was that a tools/ script must not depend on a test module for core logic.

Fix: "rebuilds through the shared recipe in .github/scripts/bench_metrics.py (the same one the drift check runs through)".

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 7307a38. The re-pin block now reads that the driver "rebuilds through the shared recipe in .github/scripts/bench_metrics.pyrebuild_shardmap and select_pin, which the drift check and this driver both call", replacing "rebuilds through the drift check's own recipe". That matches the wording 57364d5 landed at the four other sites (the tool docstring, its sys.path comment, and repin()), where the recipe stopped being the guard's and became shared.

The pre-existing python fence just above (~line 176) is untouched — ruff format --check still reports it as the one README finding, deliberately left as before.

> the ring maps, and writes the map plus its `targets.json`
> `shard_key`/`n_granules`. It re-pins **deliberately** — the drift check
> stays the accident detector — so run it only when a convention or grammar
> change makes the committed words wrong. `--check` rebuilds and reports the
> differences without writing. The entry's `note` is prose: restate it by
> hand in the same commit.
>
> ```bash
> uv run python tools/repin_benchmark_shardmaps.py --check healpix_o9_88s
> ```

> **The committed maps span two granule-record schemas.** The five
> `sm_healpix_*.json` maps were last rebuilt after issue #246, so their
> granule records carry `time_start`/`time_end` (and their `metadata` a
Expand Down
105 changes: 98 additions & 7 deletions tests/test_benchmark_shardmap.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,13 @@ def resolve_aoi_temporal_cmr(sm_meta: dict) -> tuple[dict, dict, dict]:
)


pytestmark = [
pytest.mark.slow,
pytest.mark.skipif(
os.environ.get("ZAGG_BENCHMARK_DRIFT") != "1",
reason="set ZAGG_BENCHMARK_DRIFT=1 to run the CMR shard-map drift check",
),
]
#: The network gate is the drift check's own, not the module's (it was
#: module-level while the drift check was the only test here): the issue #444
#: re-pin-driver tests below rebuild from the committed catalogs and need no CMR.
needs_cmr = pytest.mark.skipif(
os.environ.get("ZAGG_BENCHMARK_DRIFT") != "1",
reason="set ZAGG_BENCHMARK_DRIFT=1 to run the CMR shard-map drift check",
)


def _containing_shard(parent_grid, shard_key: int) -> int:
Expand Down Expand Up @@ -101,6 +101,8 @@ def _config_for_shardmap(sm_key: str) -> Path:
raise AssertionError(f"no target references shardmap '{sm_key}'")


@pytest.mark.slow
@needs_cmr
@pytest.mark.parametrize("sm_key", list(MANIFEST["shardmaps"]))
def test_pinned_shardmap_no_drift(sm_key):
from zagg.catalog import load_polygon, polygon_to_bbox
Expand Down Expand Up @@ -170,3 +172,92 @@ def test_pinned_shardmap_no_drift(sm_key):
f"{sm_key}: densest granule count drifted {pinned_n} -> {n} "
f"(rebuilt densest shard {key}). Re-pin the shard map + targets.json."
)


# -- the deliberate re-pin driver (issue #444) --------------------------------
#
# ``tools/repin_benchmark_shardmaps.py`` is the counterpart of the drift check
# above: the guard detects an accidental move, the driver makes a deliberate
# one. It imports the guard's recipe helpers, so these tests pin the parts the
# guard does not exercise -- the pruning, the pin write-back, and the claim the
# driver exists to support: that it reproduces the PR #441 artifacts from the
# committed catalogs.

OFFLINE_PINS = [k for k, v in MANIFEST["shardmaps"].items() if v.get("catalog_parquet")]


def _driver():
"""The re-pin driver, imported from ``tools/`` (not an installed module)."""
sys.path.insert(0, str(REPO / "tools"))

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)

Nit (decline freely). _driver() re-runs sys.path.insert(0, str(REPO / "tools")) on every call — five times across this module's tests — so sys.path accumulates five identical entries for the rest of the session. The module already establishes the other pattern two lines from here: sys.path.insert + import bench_metrics once at module scope (lines 43–44). Hoisting the insert to module scope and keeping only the import inside the helper (or guarding with if p not in sys.path) would match it.

Not a correctness issue — the imports resolve to the same module object either way — purely house-style consistency (§4).


Generated by Claude Code

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 in ba246df — house-style consistency is worth the two lines, and the test module now grew to six _driver() callers, so the accumulation you measured got slightly worse rather than better.

The insert is hoisted to module scope beside the bench_metrics one; only the import stays inside the helper, and the docstring now says why it has to:

sys.path.insert(0, str(REPO / "tools"))


def _driver():
    """The re-pin driver, imported from ``tools/`` (not an installed module).

    The import itself stays lazy: the driver imports THIS module for the
    rebuild recipe, so importing it at module scope would be circular.
    """
    import repin_benchmark_shardmaps

    return repin_benchmark_shardmaps

That circularity is the one reason the whole thing is not simply a module-scope import like bench_metrics, and it was not written down before — worth capturing so the next reader does not "tidy" it into an import error.


Generated by Claude Code

import repin_benchmark_shardmaps

return repin_benchmark_shardmaps


def _without_volatile(text: str, volatile) -> str:

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)

_without_volatile is line-oriented, so it degenerates to a vacuous pass if the writer ever stops pretty-printing.

The filter drops any line containing "build_wall_s" or "mortie_order". That is safe only because ShardMap.to_json currently writes json.dumps(payload, indent=2) (src/zagg/catalog/shardmap.py:1767), putting each metadata key on its own line. I confirmed the current strength: 2 of 38,793 lines dropped.

But the coupling is invisible and the failure is silent. If to_json ever moves to compact JSON (or to a separators= form, or to one-line-per-shard), the whole document becomes a single line containing "build_wall_s", both sides filter down to "", and "" == "" passes — the acceptance test for issue #444 would report success while comparing nothing. Nothing in the diff pins the formatting.

Fix (either is fine):

  • compare parsed structures with the keys popped, which is format-independent:
    def _without_volatile(text, volatile):
        d = json.loads(text)
        d["metadata"] = {k: v for k, v in d["metadata"].items() if k not in volatile}
        return d
  • or keep the line filter and assert it did the expected work: assert len(kept) == len(all_lines) - len(volatile).

Low severity today, but it is the kind of test that reports green forever once it breaks.


Generated by Claude Code

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 in 4a96fd5, taking your second option rather than the first.

I kept the comparison line-oriented on purpose: issue #444's word is "byte-identically", and switching to parsed structures would quietly soften that to structural equality — the same class of relaxation your :228 note is about. So the filter now checks its own work instead:

    lines = text.splitlines(keepends=True)
    kept = [line for line in lines if not any(f'"{k}"' in line for k in volatile)]
    assert len(lines) - len(kept) == len(volatile), (
        f"expected one line per excused metadata key, dropped {len(lines) - len(kept)} of "
        f"{len(lines)} -- has ShardMap.to_json stopped pretty-printing?"
    )

The vacuous-pass path you describe is now impossible: compact JSON would drop 1 line of 1 against an expected 2 and fail with a message naming the cause. It also closes the direction you did not raise — a key string surfacing inside a granule record would drop more lines than expected and fail too, rather than silently widening the exclusion.

The docstring records the coupling to ShardMap.to_json's pretty-printing explicitly, so it is no longer invisible to the next reader.


Generated by Claude Code

"""A written map's text minus the metadata lines a rebuild legitimately moves."""
return "".join(
line
for line in text.splitlines(keepends=True)
if not any(f'"{k}"' in line for k in volatile)
)


@pytest.mark.slow
@pytest.mark.parametrize("sm_key", OFFLINE_PINS)
def test_offline_pin_reproduces_committed_map(sm_key, tmp_path):
"""The driver reproduces the PR #441 artifacts from the committed catalogs.

The acceptance test issue #444 asks for, and the reason it can only cover
the ``catalog_parquet`` (88S ring) entries: the NEON trio rebuilds from CMR
by design -- an ATL03 footprint quad blankets the whole NEON box, so a local
full-catalog snapshot over-includes (``tests/data/benchmark/README.md``).

Byte-for-byte over the whole written manifest -- every granule record,
``shard_keys``, ``grid_signature``, and the metadata the build derives --
except the two keys a faithful rebuild still moves, which are asserted
separately below.
"""
driver = _driver()
mapped, key, n = driver.repin(sm_key)
sm_meta = MANIFEST["shardmaps"][sm_key]
assert (key, n) == (sm_meta["shard_key"], sm_meta["n_granules"])

written = tmp_path / "rebuilt.json"
mapped.to_json(str(written))
assert _without_volatile(written.read_text(), driver.VOLATILE_META) == _without_volatile(

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)

The acceptance test's exemption list is owned by the code under test — issue #444's "byte-identically" is relaxed by whatever the driver decides to relax it by.

assert _without_volatile(written.read_text(), driver.VOLATILE_META) == _without_volatile(
    (BENCH / sm_meta["path"]).read_text(), driver.VOLATILE_META
)

VOLATILE_META lives in tools/repin_benchmark_shardmaps.py, where its other job is cosmetic: labelling --check output with (volatile). Someone quieting a noisy --check by appending a key to that tuple silently widens this acceptance test's blind spot, and no test fails. That is exactly the kind of relaxation issue #444's word "byte-identically" exists to prevent.

To be fair to the diff: I checked and the comparison is genuinely strong right now — 38,791 of sm_healpix_o9_88s.json's 38,793 lines are compared, and both offline params pass (I ran pytest tests/test_benchmark_shardmap.py -q -p no:randomly on the branch: 4 passed, 5 skipped, 66.9 s). The problem is the ratchet, not today's coverage.

Fix: pin the exemption in the test rather than importing it —

assert driver.VOLATILE_META == ("build_wall_s", "mortie_order")  # widening it must fail here

— or just hard-code the tuple locally in _without_volatile's call sites. One line, and the acceptance test stops being editable from the tool it is meant to hold to account.


Generated by Claude Code

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

Agreed on the ratchet — folded in 1c49621. The exemption is now pinned in the test module and no longer imported from the tool it audits:

#: The metadata keys the byte comparison below excuses, pinned HERE rather than
#: imported from the driver. [...] widening it has to be a deliberate edit here too.
EXCUSED_META = ("build_wall_s", "mortie_order")

and the acceptance test both uses the local tuple and cross-checks the driver's:

    assert tuple(driver.EXCUSED_META) == EXCUSED_META, (
        "the driver's excused-metadata set moved -- restate it here deliberately"
    )

So appending a key to the driver's set to quiet a noisy --check now fails here instead of silently widening the blind spot. The cross-check is worth keeping alongside the local pin: it catches the narrowing direction too, and points the reader at the reason.

Thanks for measuring today's strength (38,791 of 38,793 lines) — that number is why this is a ratchet fix and not a coverage fix. Your :197 note on the same test is folded in 4a96fd5, and the line filter now asserts it dropped exactly one line per excused key, so the two protections compose: the exemption cannot widen silently, and it cannot degenerate silently either.


Generated by Claude Code

(BENCH / sm_meta["path"]).read_text(), driver.VOLATILE_META
)

from zagg.config import load_config
from zagg.grids import from_config

# The one live divergence, and why it is not a pin move: PR #447 made the
# unpinned HEALPix ``swath`` cover order the SHARD order, where the
# committed maps recorded the chunk order they were built at. The
# assignment is unchanged -- which is what the byte comparison above just
# showed, over the same catalog.
grid = from_config(load_config(str(_config_for_shardmap(sm_key))))
assert mapped.metadata["mortie_order"] == grid.parent_order


def test_repin_updates_only_the_pin_literals_in_targets():
# The write-back is surgical because targets.json is hand-formatted (compact
# inline ``worker`` objects survive a re-pin); the entry's prose ``note`` is
# the re-pinner's to restate, not the driver's to rewrite.
driver = _driver()
text = (BENCH / "targets.json").read_text()
out = driver.update_targets(text, "healpix_o9", 4242, 7)

entry = json.loads(out)["shardmaps"]["healpix_o9"]
assert (entry["shard_key"], entry["n_granules"]) == (4242, 7)
assert entry["note"] == MANIFEST["shardmaps"]["healpix_o9"]["note"]
changed = [(a, b) for a, b in zip(text.splitlines(), out.splitlines(), strict=True) if a != b]
assert len(changed) == 2, changed


def test_repin_refuses_an_unknown_shardmap(capsys):
driver = _driver()
with pytest.raises(SystemExit):
driver.main(["--check", "healpix_o42"])
assert "unknown shard map(s) ['healpix_o42']" in capsys.readouterr().err
13 changes: 13 additions & 0 deletions tests/test_raster_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,19 @@ def test_sentinel2_l2a_config_loads_and_validates(self):
assert cfg.data_source["bands"]["scl"]["dtype"] == "uint8"
assert cfg.output["grid"]["child_order"] == 19

def test_sentinel2_l2a_config_declares_the_overview_family_off(self):
# Issue #459: an ABSENT output.pyramid resolves to the every-2-orders
# default schedule (``get_pyramid`` -> ``{}``), so a raster run
# dispatches an overview family that generates nothing -- raster leaves
# are column-less by construction (issue #399 option (b), unimplemented).
# The shipped template declares the opt-out rather than inheriting the
# default, and ``None`` is the "family OFF" resolution.
from zagg.config import get_pyramid

cfg = default_config("sentinel2_l2a")
assert cfg.output["pyramid"] is False
assert get_pyramid(cfg) is None


class TestRasterHiveLocalBackend:
"""Local raster hive runs (issue #247 phase 3): manifest, leaves, coverage."""
Expand Down
Loading
Loading