Skip to content

small fixes 2026-08-17 (2): s2_neon_o9 toc flip, CHANGELOG entry for PR #447 - #455

Merged
espg merged 8 commits into
mainfrom
claude/small-fixes-2026-08-17-2
Aug 17, 2026
Merged

small fixes 2026-08-17 (2): s2_neon_o9 toc flip, CHANGELOG entry for PR #447#455
espg merged 8 commits into
mainfrom
claude/small-fixes-2026-08-17-2

Conversation

@espg

@espg espg commented Aug 17, 2026

Copy link
Copy Markdown
Member

Closes #451. Closes #453.

The second small-fixes bundle of 2026-08-17 — claude/small-fixes-2026-08-17 is already taken by PR #450, hence the -2 branch suffix. Two independent one-issue commits, no shared files with PR #450 (that bundle touches auth.py/config.py/worker.py/runner.py; this one touches a benchmark config, its test, and CHANGELOG.md).

Phases

#451s2_neon_o9 flips to time_encoding: toc

PR #446 added output.time_encoding and set the shipped src/zagg/configs/sentinel2_l2a.yaml to toc (spec §8, issue #443), deliberately leaving the benchmark leg's copy on the default so the raster release series' product identity did not move mid-PR. This is that flip, at its own named commit per the issue #451 ruling.

Product-identity break, deliberate and attributable. output.time_encoding is output-defining (semantic_core, src/zagg/semantics.py), so the raster release-benchmark leg (raster_s2_neon_2025) gets a new semantic hash at this commit — the release-benchmark series breaks here on purpose, and this is the commit to attribute it to:

config semantic hash
s2_neon_o9.yaml before 803fdcff57f38c09f641be51656114d924c4f31ef1df699ed195ccf173a293be
s2_neon_o9.yaml after 65a66c75262e90b5a6b34a8a194873b508bac61c54ee66b2ab6730da3b1626c9
shipped sentinel2_l2a.yaml 65a66c75262e90b5a6b34a8a194873b508bac61c54ee66b2ab6730da3b1626c9

After the flip the leg's semantic identity is the shipped product's — which is the point of the "identical except parent_order" claim.

The comment was adjusted, not just kept. Verifying the claim mechanically turned up a second, pre-existing divergence the old comment did not name: the benchmark config carries pyramid: false (the Phase-E overview opt-out, issue #201) where the shipped config leaves the key absent, and get_pyramid maps those to None vs {} — different declarations. Both divergences are packaging, not semantics (neither parent_order nor pyramid is in semantic_core), so the comment now says so explicitly rather than overclaiming:

# ... Identical to
# the shipped src/zagg/configs/sentinel2_l2a.yaml except ``parent_order: 9`` --
# ... -- and the harness-local ``pyramid: false``
# opt-out below. Both are packaging: the semantic hash (D19) of this config
# equals the shipped one, so the leg measures the shipped product.

store_layout: hive also stayed, restated as "the default, not a divergence" — it is what get_store_layout returns for the shipped config anyway.

Test. No benchmark-config parity test existed to extend, so the check went next to the manifest-consistency tests that already load this config, in tests/test_raster_benchmark.py:

def test_s2_neon_o9_tracks_the_shipped_sentinel2_config():
    ...
    assert semantic_hash(bench) == semantic_hash(shipped)
    b, s = normalized(bench), normalized(shipped)
    assert b["grid"].pop("parent_order") == 9
    assert s["grid"].pop("parent_order") == 11
    assert b == s
    assert bench.output["pyramid"] is False
    assert "pyramid" not in shipped.output

It compares the effective configs (defaults resolved, run-local output.store and the descriptive-only grid.indexing_scheme dropped) rather than raw YAML, so the whole divergence budget is parent_order + the pyramid opt-out, which is asserted on both sides (fold 52b2531): pyramid is exempt from the dict comparison, and output.pyramid is not in semantic_core, so a shipped config that grew pyramid: {overviews: [...]} would otherwise pass both the dict comparison and the D19 hash while the leg kept measuring overviews off. The bench config is resolved through the targets manifest rather than a hardcoded path, like the manifest-consistency test beside it (fold 44bc654). A knob added to one config and not the other — which is exactly how time_encoding drifted — now fails here instead of silently.

#453 — CHANGELOG entry for PR #447

Per the issue #453 ruling: the mortie_order metadata change deserves release notes, not just the docstring/PR record. Added to CHANGELOG.md [Unreleased] in the file's existing style (one top-line bullet with the PR link, matching the surrounding - title ([#N](url)) by @espg form; nested sub-bullets carry the detail, since the file's only other multi-point entry — ## [0.2.1] — spends a ### subsection heading on it, which is more structure than one unreleased bullet warrants), covering the three things the issue named:

  • the default change — unpinned unindexed HEALPix swath builds cover at parent_order, so metadata["mortie_order"] records the shard order (e.g. 9, not 13); measurably identical at production order pairs (the clone-scale digest table in PR #447: all five runs share dd43dd5f55a0f002 at 555,867 granules, including across the order change), a documented conservative superset at coarse grids, and the order an explicit pin resolves to is unchanged (fold 4f02e5c — the assignment for a pinned build did move for indexed catalogs, which is what the third bullet discloses);
  • the build-path change in one line — cover from the WKB column and intersect before materializing records. Numbers now match the bullet's subject (fold df6714d): the unpinned default, which is what this entry changes, goes 1,075 s → 39 s (~27x) at clone scale, with the already-pinned case (86.6 s → 39.3 s, ~2.2x) as the secondary clause. The earlier "~2.5x" was neither ratio — 87 → 39 is 2.20x, and Unindexed builds: cover from WKB and intersect before materializing records (issue #445) #447's own ~2.5x is old-path cost vs index_footprints(9) + indexed query, a different pair of quantities;
  • the pinned-indexed live-cover disclosure — an explicit pin always covers live, indexed or not, so a MultiPolygon footprint assigns as a union-of-parts superset; single-part CMR granules (every CMR granule zagg catalogs today) are unaffected.

Note the [Unreleased] section is stale — its newest existing entry is PR #42 — so this lands at the top of a list that has not been maintained since. Left as-is; backfilling it is not this bundle's scope.

How tested

  • pytest -q: 4025 passed, 38 skipped, 1 failed — the failure is tests/test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds, the known environmental (docker/build-toolchain) failure, unrelated to this diff.
  • ruff check tests and ruff format --check src tests: clean. ruff check src reports one pre-existing N818 on src/zagg/registry.py:64 (UnknownCapability), present unchanged on origin/main and outside the PR lint bot's --select=E,F,W,I set — flagged, not "fixed" (§4).
  • Semantic hashes above computed with zagg.semantics.semantic_hash on this branch vs. the pre-flip config.
  • Review fold (df6714d, 4f02e5c, 52b2531, 44bc654): uv run --extra test pytest tests/test_raster_benchmark.py -q11 passed; ruff check + ruff format --check clean on the touched files.

Questions for review

  1. Flip s2_neon_o9 benchmark config to time_encoding: toc (post-#446) #451 comment scope. The pyramid: false divergence was pre-existing and unnamed; naming it in the config comment (and asserting it in the test) is slightly beyond "flip the knob". The alternative — dropping pyramid: false so the configs really do differ only in parent_order — would turn the overview family on for the raster leg, which is a fleet-sizing decision explicitly deferred on issue Pyramid/overview generation sweep (second pass): overview zarrs, MOC regen, optional interop materialization #201, so it was not taken here. Say if the opt-out should instead move out of the config and into the harness override.
  2. CHANGELOG entry for the PR #447 mortie_order default + build-path change #453 placement. The entry went into the stale [Unreleased] list rather than a new ## [0.45.1] heading, since 0.45.1 is not cut yet in this file. If the release that carries PR Unindexed builds: cover from WKB and intersect before materializing records (issue #445) #447 should get its own heading now, say which version string.

@espg espg added the implement label Aug 17, 2026
Comment thread CHANGELOG.md Outdated
coarse grids the new default is a documented conservative superset, never a
subset. Explicit `mortie_order=` pins are unchanged.
- Unindexed builds cover from the catalog's WKB column and intersect before
materializing granule records — ~2.5x faster at clone scale (87 s -> 39 s).

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 ~2.5x multiplier disagrees with its own quoted endpoints, and the 2.5x figure in PR #447 is a different ratio entirely. 87 s → 39 s is 2.2x.

From PR #447's measured table (555,867-granule clone, same machine): before, mortie_order=9 total 86.6 s, after, default total 39.3 s → 86.6 / 39.3 = 2.20x. The most generous pairing available (86.9 s from the issue vs the best after-run 37.0 s) is still only 2.35x; pinned→pinned (86.6 → 37.4) is 2.32x. No pairing yields 2.5x.

Where 2.5 comes from in #447 is its opening paragraph, and it is not a speedup at all:

that was 86.9 s to assign 2,357 granules, ~2.5x the cost of index_footprints(9) + an indexed query put together

i.e. old-records-path cost vs index-build-plus-indexed-query cost — a different pair of quantities than the before/after this bullet describes.

Two ways out: say ~2.2x faster at clone scale (87 s → 39 s), or quote the reading that actually matches this entry's subject — the first bullet is about the unpinned default, whose before/after is 1075 s → 39 s (~27x), the number #447 calls "the cost espg's ruling removes".

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 df6714d — took the second option: the bullet now quotes the reading its own subject describes.

At clone scale (555,867 granules) the unpinned default — the case the bullet above changes — goes 1,075 s -> 39 s (~27x), no longer covering every footprint at the chunk order to answer a shard-order question; a build that already pinned mortie_order=9 goes 86.6 s -> 39.3 s (~2.2x).

Both numbers come straight from PR #447's measured table (before, default 1075.0 s / before, mortie_order=9 86.6 s / after, default 39.3 s), and neither is the ~2.5x of #447's opening paragraph, which you're right is old-path cost vs index_footprints(9) + indexed query — a different pair of quantities. The PR body's matching sentence was corrected too.

Comment thread CHANGELOG.md Outdated
is measurably identical at the production order pairs (the
`bench/neon_order_sweep.py` invariant, verified at 555,867 granules); at
coarse grids the new default is a documented conservative superset, never a
subset. Explicit `mortie_order=` pins are unchanged.

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)

"Explicit mortie_order= pins are unchanged" is contradicted by this entry's own third bullet and by PR #447's delta 6.

What #447 claims unchanged is narrow — "An explicit mortie_order= is still honored literally and still validated against parent_order (#92)" — i.e. the resolved order. The assignment for a pinned build did move, measurably, for one population:

6. A pinned mortie_order puts an indexed catalog on the live cover too. … measured on a two-part MultiPolygon at HealpixGrid(11, 19, chunk_inner=13) with mortie_order=13, 8 shards on main -> 13 here for the multi-part granule.

Pinned in the merged suite at tests/test_shardmap.py:1470 (TestLiveCover::test_a_pinned_indexed_build_covers_live_too).

So a reader gets "your pinned build is unaffected" on line 17 and, three lines down, "an explicit mortie_order= pin always covers live, indexed or not, so a MultiPolygon footprint assigns as a union-of-parts superset". Both cannot be read at face value. Scoping the first to what #447 actually claims — e.g. "The order an explicit mortie_order= pin resolves to is unchanged" — leaves the third bullet to carry the assignment consequence without the entry arguing with itself.

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 4f02e5c — scoped to what #447 actually claims, so the first bullet no longer argues with the third:

subset. The order an explicit mortie_order= pin resolves to is unchanged (it is still honored literally and still validated against parent_order).

That leaves the assignment consequence entirely to the third bullet (delta 6 / TestLiveCover::test_a_pinned_indexed_build_covers_live_too), which is where it belongs.

assert b["grid"].pop("parent_order") == 9
assert s["grid"].pop("parent_order") == 11
assert b == s
assert bench.output["pyramid"] is False

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 divergence budget is enforced one-sidedly: pyramid is dropped from the comparison on both sides but asserted only on the bench side, so the shipped config can grow any pyramid declaration and this test still passes.

normalized() excludes "pyramid" (line 65), and line 74 pins only bench.output["pyramid"] is False. Nothing constrains shipped.output.

Verified by mutation on this branch — a harness reproducing the test body verbatim (data_source/aggregation equality, semantic_hash, normalized(), the parent_order pops, b == s, the line-74 pin), run against load_config-loaded copies of both configs:

PASS   baseline
PASS   shipped gains pyramid: {spacing: 2}
PASS   shipped gains pyramid: false          # divergence disappearing also unnoticed
FAIL   shipped gains emit_cell_ids           # correct
FAIL   bench pyramid becomes a mapping       # correct
FAIL   shipped reverts time_encoding         # correct

The hole is not incidental: the one output key exempted from the dict comparison is the same key the docstring names as half the divergence budget ("Divergence budget: parent_order and the harness's pyramid opt-out"). The stated purpose — "a knob added to one and not the other (as time_encoding was, issue #443) cannot drift silently" — holds for every knob except this one. And pyramid is not inert like the two other drops: src/zagg/configs/sentinel2_l2a.yaml acquiring pyramid: {overviews: [...]} (the issue #382 grammar) turns the overview family on for the shipped product while the leg keeps measuring it off, at an unchanged semantic hash — output.pyramid never enters semantic_core, so the D19 assertion above cannot see it either.

One line next to 74 closes it: assert "pyramid" not in shipped.output.

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 52b2531 — your one-liner, with a comment naming why the hole exists (the pyramid drop from normalized() plus output.pyramid being outside semantic_core, so neither the dict comparison nor D19 can see it):

    assert bench.output["pyramid"] is False
    assert "pyramid" not in shipped.output

The normalized() drop stays on both sides as-is; only the assertion is now two-sided. uv run --extra test pytest tests/test_raster_benchmark.py -q → 11 passed.

Separately in 44bc654, this test now resolves the bench config through the targets manifest (base / manifest["targets"]["raster_s2_neon_2025"]["config"]) instead of the hardcoded BENCH / "configs" / "s2_neon_o9.yaml", matching test_raster_targets_manifest_consistent beside it — so a manifest repoint cannot leave this test parity-checking a stale file.

@espg
espg marked this pull request as ready for review August 17, 2026 03:58
@espg espg added the waiting label Aug 17, 2026
@espg

espg commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Question (2) resolved by events: espg tagged 0.46.0 (which carries PR #447), so c3f9010 files the entry under ## [0.46.0] - 2026-08-17 — matching the tag rather than guessing a version. [Unreleased] keeps only the pre-existing stale entries, whose backfill remains out of scope as noted. Question (1) (pyramid opt-out placement) still stands.

@espg
espg merged commit 242a458 into main Aug 17, 2026
6 checks passed
espg added a commit that referenced this pull request Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CHANGELOG entry for the PR #447 mortie_order default + build-path change Flip s2_neon_o9 benchmark config to time_encoding: toc (post-#446)

1 participant