Skip to content

88S latitude sweep: ring AOI family + reassessment benchmark matrix - #526

Draft
espg wants to merge 12 commits into
mainfrom
claude/148-88s-stress-test
Draft

88S latitude sweep: ring AOI family + reassessment benchmark matrix#526
espg wants to merge 12 commits into
mainfrom
claude/148-88s-stress-test

Conversation

@espg

@espg espg commented Aug 25, 2026

Copy link
Copy Markdown
Member

Closes #148

What this does

Turns the 88S reassessment ask — the 2026-07-18 comment: "things have changed … we should update this and reassess if polar processing is now more tractable", with the matrix [85, 85.5, 86, 86.5, 87, 87.5, 88] degrees south × {4 GB-disk, 8 GB-disk} on sidecar + hive + spill + sharded + o9 + no AOI mask — from an ad-hoc sweep into committed, reproducible benchmark data.

Today the sweep exists only as numbers on the issue thread. Every band except 88°S was measured against a scratch AOI and a scratch catalog that were never committed, so nothing in the tree can rebuild them, and the pinned 88°S stress row (healpix_o9_88s) has no siblings to be compared against. This PR lands the half of that which is offline and deterministic:

  1. The ring AOI family. tools/make_lat_ring_aoi.py generates the seven [-L, -L+0.3] latitude bands to one set of rules, and the six new bands are committed beside the shipped antarctic_88s.geojson. The generator's acceptance test is that it reproduces the committed 88°S ring geometry exactly — so the sweep rows are the same shape as the pinned row they extend, and their granule counts stay comparable. The rules it encodes are the non-obvious ones: the 0.3° band height every issue add 88 south stress test shard + benchmark #148 count was measured over, the eight-sector split (a single -180..180 rectangle collapses to an antimeridian sliver under spherical polygon fill), and the 1°-of-longitude vertex sampling. It refuses to overwrite a committed band whose bytes differ, so a run that includes 88 cannot silently replace the shipped ring's prose with the generic sweep note.

  2. The sweep matrix as data. A targets_88s_latitude_sweep.json manifest under tests/data/benchmark/, alongside the existing targets_order_sweep_neon.json / targets_full_aoi_neon.json auxiliary manifests. Per that directory's own README, "this directory is the benchmark matrix … adding, removing, or reshaping benchmarks is a data edit under tests/data/benchmark/, with no change to the workflow or the runner" — so the matrix lands without touching .github/.

  3. Docs. The README's sweep section (bands, generator, pinning recipe), an on-demand section in docs/deployment/benchmark.md, and a reconciliation of the older 88°S stress-target section, which still told readers to expect an OOM at 2 GB "until the streaming work lands".

Approach

The sweep arm is exactly the shipped per-merge sidecar arm (tdigest_healpix_o9_hive_sidecar in targets.json) pointed at a ring band instead of NEON, so no new pipeline config is needed — run_benchmark.run_target already sets index_backend, streaming_mode, sharded and worker per target off the manifest:

"index_backend": "sidecar", "streaming_mode": "spill", "sharded": true,
"worker": {"memory": 8192, "extra_disk": true}

run_benchmark.resolve_variant turns that worker block into the pre-provisioned process-shard-8192-disk variant, and the 4 GB arm into -4096-disk, which is the "4 GB worker with disk" / "8 GB worker with disk" pair the reassessment asks for.

The 88°S row reuses the committed sm_healpix_o9_88s.json map and pin verbatim, so the drift guard and the issue #444 re-pin driver keep covering the mission's worst shard in the one place they already do; the manifest does not fork it (test_the_88s_row_is_the_committed_pin_not_a_fork).

Phases

  • Phase 1 — the ring AOI family (f77885f). tools/make_lat_ring_aoi.py + the six new band geojsons + tests/test_lat_ring_aoi.py.
  • Phase 2 — the sweep matrix manifest (10e0dc2). targets_88s_latitude_sweep.json (14 rows: 7 bands × 2 worker arms) + tests/test_88s_latitude_sweep.py.
  • Phase 3 — docs (9652ba4). The tests/data/benchmark/README.md sweep section and the docs/deployment/benchmark.md on-demand section.

Adversarial self-review ran after each phase; all nine findings are folded, one commit each (f722d5a, 8d19bcd, 36d2fb8, 7a08675, e04addb, fa2c1f5, a01fbbc, 771978d, f85ed73), with a reply on every thread.

Deferred (not landed here, deliberately)

The six unpinned bands and the live fleet run. A benchmark row is dispatchable only once its shard map is built and its densest shard pinned. For the 85–87.5°S bands that needs a CMR catalog those bands do not have:

  • The committed catalogs/cat_88s.parquet snapshot is the CMR result for bbox [-180, -88.0, 180, -87.7] (35,639 granules) — it is not a superset of the equatorward bands. Every granule in it reaches north of 85°S (verified: ymax >= -85.0 for all 35,639 rows), but granules that turn at, say, 86°S and never reach 87.7°S are absent, so rebuilding an 85°S map from it would silently under-count.
  • Fetching them means one ~20 min full-mission CMR query per band (the README's own figure for the 88°S ring) and committing another ~17 MB stac-geoparquet snapshot per band — ~100 MB of new binaries in the tree, which is a call for a human (CLAUDE.md §3 on large binaries), not something to do silently. See "Questions for review".
  • The fleet run itself is out of scope for this session by construction — no Lambda invoke, no standup, no S3 write. Everything in this PR is offline: geometry, manifest data, and tests that touch neither CMR nor AWS.

So the 88°S row of the matrix is fully pinned and dispatchable today, and the six equatorward rows land as a complete, documented definition awaiting their catalog snapshots. Their targets sit in pending_targets rather than targets on purpose: run_target reads shardmap_meta["shard_key"] unconditionally, so a pin-less row in targets would die mid-dispatch with a bare KeyError instead of being visibly not-ready. _pin_recipe in the manifest is the four-step recipe for moving one across, and test_pinned_and_unpinned_bands_stay_consistent fails loudly on a half-done pin.

How it was tested

  • Full suite green: uv run pytest -q4893 passed, 38 skipped (606 s). The two pre-existing test failures flagged for this branch (test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries and test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds) did not reproduce in this environment — reporting that rather than claiming credit for it.
  • New coverage: tests/test_lat_ring_aoi.py (30) + tests/test_88s_latitude_sweep.py (12). Both offline — geometry, JSON, and one run_target(dry_run=True) wiring check that returns before any zagg.runner.agg import.
  • Generator round-trips the shipped fixture: uv run python tools/make_lat_ring_aoi.py --check 85 85.5 86 86.5 87 87.5 88geometry matches on all seven.
  • Lint: ruff check src tests tools and ruff format --check src tests are clean apart from two pre-existing items, both confirmed present on main and untouched: the registry.py:64 N818, and one formatter diff in a python example block in tests/data/benchmark/README.md (at line 176 on main; it moved to 178 only because the Layout table gained two rows). The CI ruff and build checks are green; the test (3.12) / test (3.13) matrix was still running when this was written.

Questions for review

  1. How should the six band catalogs be obtained and stored? They are the one thing standing between this manifest and a runnable sweep.

    • (1) One union snapshot. A single CMR fetch over [-180, -88.0, 180, -84.7] (the union of all seven bands), committed as one catalogs/cat_85_88s.parquet, with each band's map built by prefiltering it offline — exact for complete rings, per the README's own note that lat-ring maps rebuild offline "only because complete-ring lat-overlap is exact". One fetch, one file, but the file is ~40 MB.
    • (2) Seven per-band snapshots, mirroring how cat_88s.parquet is stored today. Consistent with the existing convention; ~100 MB of new binaries and ~2 h of CMR fetching.
    • (3) No snapshot — leave the six rows unpinned and have whoever runs the sweep fetch live from CMR (the catalog_parquet-less path in bench_metrics.rebuild_shardmap already supports this). Zero bytes added; the sweep is then non-deterministic across runs and re-fetches every time.

    My recommendation is (1): it is one artifact, it keeps the rebuild deterministic and offline the way the 88°S row already is, and the prefilter is exact for these ring AOIs. It still adds a ~40 MB binary, which is why it is a question rather than a commit.

  2. Should the sector seams move off the 45° base-cell grid? The committed 88°S ring splits at multiples of 45°, which is exactly the geometry espg/mortie#103 mis-fills; the 2026-07-09 analysis found that offsetting the seams by 22.5° makes mortie's fill match point-sampled ground truth (1,560 vs 1,560 cells at 83°S) where the unoffset split returns 9,154. The generator keeps the 45° seams on purpose — matching the shipped ring is what makes the sweep comparable to the pinned row — but that means the whole family inherits the defect, and correcting it would re-pin healpix_o9_88s / healpix_o10_88s. Re-pinning committed benchmark fixtures is a deliberate act, so it is left standing here.

  3. Does the sweep belong in its own manifest or in targets.json's provisional_targets? This PR uses its own manifest (following targets_order_sweep_neon.json) to keep the drift check and manifest-consistency test's blast radius at zero. The alternative is adding the 14 rows to provisional_targets, which would put them under bench_metrics.MANIFEST and give the pinned 88°S rows drift coverage in the standard place.

  4. A pre-existing inconsistency this work surfaced, not fixed. targets.json's healpix_o9_88s carries no temporal override, so it resolves to the top-level 2018-10-13 .. 2026-03-15 — but the entry also carries catalog_parquet, and catalogs/cat_88s.parquet was fetched over 2018-10-13 .. 2025-06-01. The offline rebuild path ignores the manifest window entirely, so the mismatch is latent rather than wrong today. This PR's manifest records the snapshot's window explicitly (and test_the_sweep_temporal_window_matches_the_snapshot_that_pins_it pins it against the parquet's own metadata), but targets.json is left alone — out of scope here, and worth its own look.

@espg espg left a comment

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)

Adversarial pass over phase 1 (f77885f) — the ring AOI family — against CLAUDE.md and issue #148's acceptance criteria.

What holds up. The reference test is the right one: generating the committed antarctic_88s.geojson geometry and comparing, rather than asserting properties of the generator in isolation, means the sweep bands provably share the shipped ring's shape and the granule counts stay comparable to the pinned row. test_regenerating_a_band_is_byte_stable covering the serialization (not just the parse) is the right call for a fixture that will be regenerated. The float arithmetic is clean — every band edge serializes exactly (-84.7, -85.2, … -87.7), no -86.69999999999999 in the committed bytes, checked directly against the files.

Three findings, inline.

  1. tools/make_lat_ring_aoi.py — the writer overwrites antarctic_88s.geojson with generic prose if 88 is in the latitude list, which the documented reassessment list is. Geometry survives, so no test fires. Diff-scoped, should be fixed.
  2. tools/make_lat_ring_aoi.pysector_ring(step=…) is unreachable flexibility that silently rounds a non-dividing step (§4). Diff-scoped.
  3. tests/test_lat_ring_aoi.py — one test carries three claims and covers the MISSING branch anonymously. Diff-scoped.

One thing deliberately not raised as a finding. The 45° sector seams sit on the HEALPix base-cell boundary that espg/mortie#103 mis-fills, and the generator inherits that on purpose. That is the right call for this PR — matching the shipped ring is what makes the sweep comparable — and it is already standing as question (2) in the PR body, where it belongs: moving the seams would re-pin healpix_o9_88s / healpix_o10_88s, which is a deliberate fixture move and not a reviewer's to make.

Scope check. Nothing under deployment/aws/ or .github/ is touched, no dependency is added, no wire format or spec marker moves, and nothing in the diff can reach AWS or CMR — the whole phase is geometry and JSON.


Generated by Claude Code

failed = True
continue
path.write_text(dumps(doc))
print(

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 writer will clobber the one file it is supposed to be generated against.

main() writes unconditionally. Run the documented invocation with 88 in the list — uv run python tools/make_lat_ring_aoi.py 85 85.5 86 86.5 87 87.5 88, which is the exact latitude list the reassessment comment gives and therefore the list a reader is most likely to paste — and antarctic_88s.geojson is overwritten with the generator's prose:

  • committed: "name": "Antarctic 88S dense (ICESat-2 turning-latitude stress target)", with the note explaining the turning-latitude stress rationale and the ~576 at o9 figure
  • generated: "name": "Antarctic 88S ring (ICESat-2 latitude sweep band, issue #148)", with the generic sweep-band note

The geometry survives (that is what test_generator_reproduces_the_committed_88s_ring guarantees) so no test catches it, and the module docstring's own claim — "the committed 88S ring stays the reference the rest of the family is generated to match" — is what silently stops being true. The prose that goes is exactly the prose issue #148 relies on.

A guard is cheap: refuse to overwrite an existing file whose bytes differ, and say what to do instead. That also makes the writer idempotent, which is worth having on its own.


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

Fixed in f722d5a. A plain run now refuses to replace a committed band whose bytes differ (EXISTS and differs -- re-run with --force to replace it, exit 1) and --force is the deliberate way through; rewriting a band that is already identical stays silent, so the writer is idempotent.

Two tests carry it: test_writing_88_leaves_the_shipped_ring_alone runs the exact documented latitude list including 88 and asserts the shipped file is byte-unchanged and still carries turning-latitude stress target, and test_force_replaces_a_committed_band covers the guard, the --force path, and the idempotent re-write. The module docstring now states the rule instead of only implying it.


Generated by Claude Code

Comment thread tools/make_lat_ring_aoi.py Outdated
lon_end: float,
lat_south: float,
lat_north: float,
step: float = LON_STEP_DEG,

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)

step is a knob nothing can turn. ring_geometry never threads it, no test varies it, and n = round((lon_end - lon_beg) / step) silently rounds a step that does not divide 45° — so the only reachable effect of the parameter existing is a wrong ring if someone ever does pass one. CLAUDE.md §4: "No dead code, no speculative abstraction."

Read LON_STEP_DEG directly and drop the parameter. (band on ring_geometry is the same shape of thing but earns its place — the band height is the quantity the docstring argues about, and naming it at the call site is what makes ring_geometry(88.0) in the reference test readable.)


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

Fixed in 8d19bcd. sector_ring now takes only the four coordinates and reads LON_STEP_DEG directly; the docstring says why the spacing is a module constant rather than an argument (a per-call step could only round to a different vertex count than the rest of the family shares). --check across all seven bands still reports geometry matches, so nothing moved.

band on ring_geometry is left as-is, for the reason the finding itself gives.


Generated by Claude Code

Comment thread tests/test_lat_ring_aoi.py Outdated
"""``--check`` is the guard: it agrees with every committed band, and never writes."""
argv = [str(v) for v in SWEEP_LATS]
assert ring.main(["--check", *argv]) == 0
assert ring.main(["--check", "--out-dir", str(tmp_path), "88"]) == 1

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)

Two unrelated claims share one test, and the name only advertises one of them. Line 111 exercises the MISSING branch (--check against an empty dir returns 1); lines 110 and 112 exercise "agrees with the committed family" and "writes nothing". A failure here does not say which of the three broke, and the MISSING branch — the one that catches a band that was never committed at all — is covered anonymously, inside a test whose name promises something else.

Split the missing-file case out under its own name, beside test_check_mode_reports_a_band_whose_geometry_moved. The two --check failure modes then read as the pair they are.


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

Fixed in 36d2fb8. The missing-band case is now its own test, test_check_mode_reports_a_band_that_was_never_committed, sitting beside test_check_mode_reports_a_band_whose_geometry_moved so the two --check failure modes read as the pair they are. What remains under test_check_mode_passes_on_the_committed_family is the one claim its name makes, plus the "writes nothing" assertion that has to ride along with it (it can only be checked against a run that had something to write) — now carrying its own message.


Generated by Claude Code

@espg espg left a comment

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)

Adversarial pass over phase 2 (10e0dc2) — the sweep matrix manifest — against CLAUDE.md and issue #148's acceptance criteria.

What holds up. Keeping the unpinned rows in pending_targets rather than targets is the right shape and for the right stated reason: run_target reads shardmap_meta["shard_key"] unconditionally, so a pin-less row in targets would die mid-dispatch with a bare KeyError instead of being visibly not-ready. test_pinned_and_unpinned_bands_stay_consistent closes both halves of that (pinned != (pin == "unpinned") rejects an entry that is neither as well as one that is both). test_the_88s_row_is_the_committed_pin_not_a_fork is the one that matters most long-term — the mission's worst shard behind two pins that can drift, with only one covered by the drift guard, is a trap worth nailing shut. And asserting the config properties (hive, parent_order 9, granule_workers 4, get_aoi_mask false) rather than only the target keys is what actually checks the words "hive" and "o9" in the reassessment's base configuration; run_benchmark never sets those per target.

Four findings, inline. One is a real bug:

  1. tests/test_88s_latitude_sweep.py:210 — the dry-dispatch assertion hard-codes healpix_o9_88s's shard key while looping over all pinned rows, so it goes red the moment someone pins a second band, presenting as a dispatch bug in their new row. Diff-scoped, should be fixed.
  2. tests/test_88s_latitude_sweep.py:76 — in-test sys.path.insert for tools/, which the neighbouring module moved to module scope on purpose. Diff-scoped.
  3. tests/test_88s_latitude_sweep.py:197monkeypatch.chdir(REPO) is inert; run_target resolves off base. Diff-scoped.
  4. targets_88s_latitude_sweep.json:2 — the fleet figures carry no version or comment provenance, and they are version-dependent in a way that changes their meaning. Diff-scoped.

Two things deliberately not raised as findings.

  • The twelve pending_targets rows repeat ten identical keys each. That is verbose, but it is exactly how targets.json spells its own targets out, and test_every_row_is_the_reassessment_base_configuration iterates every row and asserts each key — so a half-applied edit fails loudly rather than drifting. Matching the surrounding data file beats de-duplicating it (§4).
  • The manifest records temporal.end as 2025-06-01 (the cat_88s.parquet snapshot's window) where targets.json's top-level default is 2026-03-15 and healpix_o9_88s carries no override. That inconsistency is pre-existing in targets.json, latent because the offline catalog_parquet path ignores the manifest window entirely. test_the_sweep_temporal_window_matches_the_snapshot_that_pins_it pins the correct value here and documents why; changing targets.json is out of this PR's scope (§4 on pre-existing failures) and is worth its own look.

Scope check. Data and tests only. No .github/, no deployment/aws/, no dependency, no wire format or spec marker. run_target is exercised only through dry_run=True, which returns before any zagg.runner.agg import — nothing in the suite can reach Lambda, S3 or CMR.


Generated by Claude Code

Comment thread tests/test_88s_latitude_sweep.py Outdated
dry_run=True,
)
assert record["target"] == name
assert record["shard_key"] == manifest["shardmaps"]["healpix_o9_88s"]["shard_key"]

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)

This assertion is written to break at exactly the moment the deferred work lands.

The loop iterates manifest["targets"] — every pinned row — but the assertion hard-codes one band's key:

for name in manifest["targets"]:
    ...
    assert record["shard_key"] == manifest["shardmaps"]["healpix_o9_88s"]["shard_key"]

Today targets holds only the two 88S rows so it passes. The whole point of _pin_recipe step 4, though, is that pinning a band moves its two rows into targets — and the first person to do that gets a red test asserting their 85°S row dispatched the 88°S shard. The failure will look like a dispatch bug in the row they just added, not like a stale assertion here, which is the expensive kind of wrong.

Look the expectation up through the row: manifest["shardmaps"][manifest["targets"][name]["shardmap"]]["shard_key"]. Then the test grows with the matrix instead of blocking it, and it starts actually checking that each row dispatches its own band — which is what the assertion is trying to say.


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

Fixed in 7a08675. The expectation is now looked up through the row:

band = manifest["shardmaps"][manifest["targets"][name]["shardmap"]]
assert record["shard_key"] == band["shard_key"]

so the test grows with the matrix as bands get pinned, and it now asserts what it was trying to say — each row dispatches its own band, not a fixed key that happens to be the only one there today. A comment at the assertion names _pin_recipe step 4 as the reason.


Generated by Claude Code

Comment thread tests/test_88s_latitude_sweep.py Outdated
``bench_metrics.resolve_aoi_temporal_cmr``'s NEON fallback). That only holds
while every entry actually carries one.
"""
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)

In-test sys.path.insert is the pattern the neighbouring module explicitly moved away from. tests/test_benchmark_shardmap.py puts it at module scope and says why in a comment:

tools/ is not an installed package, so it goes on the path ONCE here — matching the bench_metrics pattern above — rather than on every _driver() call, which stacked one identical entry per test.

This file already inserts .github/scripts at module scope for exactly that reason, then does the tools/ one inside a test body. Hoist it next to the other insert (CLAUDE.md §4: match the surrounding code).


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

Fixed in e04addb. Both inserts now sit together at module scope with one comment covering them, and make_lat_ring_aoi is imported at the top beside bench_metrics and run_benchmark rather than inside the test body.


Generated by Claude Code

Comment thread tests/test_88s_latitude_sweep.py Outdated
backend/streaming/worker injection and record building all run, and a real
dispatch would be the only remaining step.
"""
monkeypatch.chdir(REPO)

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)

monkeypatch.chdir(REPO) does nothing here. run_target resolves every path off the base argument (_resolve(base, …)), which this test passes as the absolute BENCH; the config, the shard map and the grid all come from that. Dropping it also drops the monkeypatch fixture from the signature.

A chdir that is not needed is worse than neutral in a test: it reads as "this test depends on the working directory", which is the first thing someone debugging a path failure will believe.


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

Fixed in fa2c1f5 — the chdir and the monkeypatch fixture are both gone. Verified the claim rather than assuming it: the module passes run from /tmp with an explicit --project, so nothing in that test reads the working directory.


Generated by Claude Code

@@ -0,0 +1,309 @@
{
"description": "88S reassessment latitude sweep (issue #148): one o9 shard per 0.3-deg latitude band at 85 / 85.5 / 86 / 86.5 / 87 / 87.5 / 88 degrees SOUTH, each run on TWO worker arms -- 4 GB with disk and 8 GB with disk. Every arm is the shipped per-merge sidecar configuration (tdigest, store_layout hive, sidecar read, spill streaming, sharded output, granule_workers=4, no AOI mask) pointed at a ring band instead of the NEON box, so the sweep measures LATITUDE and WORKER SIZE against a pipeline that is otherwise byte-identical to the one that ships. The question it answers is where the tractable band ends: the last recorded fleet result had 85S/8GB completing at 865 s and everything poleward timing out, with the split read 77% / aggregate 22% / write 1% -- read-bound, memory not binding (2.2 GB peak of 8192, spill holding it). Run on demand by explicit --target; this is NOT an every-merge matrix, and a timeout is an expected result here, not a red build.",

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)

Measured numbers with no provenance, in a file that will outlive the thread they came from. 865 s, 190M obs, 2.2 GB peak of 8192, read 77% / aggregate 22% / write 1% and "timed out on both arms" are all real measurements, but the manifest attributes them only to "the last recorded fleet run" — and does not say which code point they were measured at. They were taken on 0.36.0, after the store-cache fix, and that matters: the same sweep on 0.35.0 timed out at every band including 85°S, so a reader who takes these as properties of the bands rather than of a version will draw the wrong conclusion about what changed.

CLAUDE.md §6 asks that claims be grounded so the reasoning can be reconstructed later. Name the zagg version and the issue #148 comment the figures come from, in the description and in the healpix_o9_85s note. It costs one clause each and makes the numbers falsifiable.


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

Fixed in a01fbbc. The description now names the code point and the source comment, keeps the phase split in seconds rather than percentages (read 671 s / aggregate 194 s / write 10 s), and states outright that the figures are a property of 0.36.0 and not of the bands — with the 0.35.0 contrast and the store-cache fix (issue #287 / PR #288) as the evidence, plus an instruction to re-record on the next run. The healpix_o9_85s note carries the same attribution and the exact obs count (189,852,320); the other five band notes now say "on 0.36.0" instead of "on the last fleet run".

Also normalised _pin_recipe.how_to_pin to (1)(4) while in the file, per CLAUDE.md §6 on enumerated list items.


Generated by Claude Code

@espg espg left a comment

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)

Adversarial pass over phase 3 (9652ba4) — the docs — against CLAUDE.md and issue #148's acceptance criteria.

What holds up. The README section leads with the thing a reader actually needs to know (what varies and what does not), and states the pending_targets rationale in terms of the mechanism — run_target reads shard_key unconditionally — rather than as an assertion. Pointing the "Add a target over a different AOI" recipe at the generator, right where it currently tells you to hand-drop a polygon, is the edit most likely to keep the next ring band consistent with this family. The blob/main link style matches what docs/aoi_mask.md, docs/quickstart.md and docs/specification.md already use, and the relative in-docs links are untouched.

Two findings, inline. Both are about the same failure mode — a fact stated in two places where only one will get updated:

  1. tests/data/benchmark/README.md:269 — the new section sits flush against the older ## The 88°S stress targets section, whose closing bullet still says to expect an OOM at 2 GB "until the issue #148 streaming/cached-read work lands". It landed; the two sections now contradict each other on the same issue, on adjacent lines. Diff-scoped, should be fixed — created by this phase's placement, even though the stale sentence predates it.
  2. docs/deployment/benchmark.md:210 — the fleet figures are hand-copied from the manifest, which asks for them to be re-recorded on the next run, with nothing naming the record of truth. Diff-scoped.

One thing deliberately not raised as a finding. The Layout table's new row uses a brace expansion (antarctic_{85,85_5,…}s.geojson) for six real files. That is not a literal path, but the table already globs (configs/*.yaml, shardmaps/*.json) and six separate rows for one family would bury the two rows that matter.

Scope check. Documentation only — no code, no data, no .github/, no deployment/aws/. The one pre-existing ruff format diff in this README (a python example block) is confirmed present on main at line 176 and is untouched; it moved to 178 only because the Layout table gained two rows.


Generated by Claude Code

is the *expected* baseline result until the issue #148 streaming/cached-read
work lands.

## The 88°S latitude sweep (issue #148 reassessment)

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 new section leaves the one directly above it stale, and now they contradict each other about the same issue.

## The 88°S stress targets (issue #148) ends, two lines up, with:

Run on demand via explicit --target … OOM/timeout at 2 GB / 900 s is the expected baseline result until the issue #148 streaming/cached-read work lands.

The streaming work landed — spill is what the sweep runs on, and it is now the shipped per-merge configuration. The reader gets "expect an OOM at 2 GB until streaming lands" immediately followed by "on 0.36.0, spill held 2.2 GB of 8,192 and 85°S completed in 865 s". Both sentences are about issue #148, they are adjacent, and only one of them is still true.

This is fair to charge to this phase: the older section was merely out of date on its own, and putting a current section flush against it is what turns that into a contradiction. Reconcile it — the older section's pins and pruned map and build-the-catalog-once content is all still correct and load-bearing, so the fix is to update its last bullet and point it at the sweep, not to delete it.


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

Fixed in 771978d, by updating the bullet rather than deleting the section — the pins, the pruned-map note and the build-the-catalog-once note are all still correct.

The bullet now says why 2 GB / 900 s is still the right expectation for those two specific rows (they predate the streaming work and dispatch the hierarchical and cached read paths on the base worker, not spill on a -disk worker), then states that the streaming half of issue #148 has landed and points the reader at the sweep below for the current polar picture — including that memory is no longer the wall anywhere as of 0.36.0. The two sections now read as history and current state instead of as a contradiction.


Generated by Claude Code

and the numbers are directly comparable to the per-merge series.

What it answers is where the tractable band ends. On zagg 0.36.0 the crossover
was **85°S at 8 GB** (865 s, 190 M obs, 2,221 MB peak of 8,192 — memory is not

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)

Same measurements, two files, nothing linking them. 865 s, 190 M obs, 2,221 MB of 8,192 and the 671 / 194 / 10 split now live here and in the manifest's description, which says of itself "re-record them when the sweep is next run". Whoever does that run will update one of the two. The published page and the data file will then disagree, and there is no way to tell from either which one is current.

Every other series on this page is backed by a rendered artifact regenerated from the recorded data, so numbers here are normally derived. These are hand-copied, which makes them the exception, and the exception should at least say where the record of truth is: name the manifest as the thing to update, so the drift is a one-line fix instead of an archaeology problem.


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

Fixed in f85ed73. The section now says outright that these figures are the exception on that page — not derived from a rendered artifact, because an on-demand sweep has no recorded series — names the manifest's description as the record of truth and the thing to update on the next run, and tells the reader to treat the page as a copy of it.


Generated by Claude Code

@espg

espg commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Status: all three phases landed, all nine self-review findings folded, applying waiting on one decision.

What is here: the seven-band ring AOI family and its generator (f77885f), the sweep matrix as targets_88s_latitude_sweep.json — 7 bands × 2 worker arms, all on sidecar + hive + spill + sharded + o9 + nomask (10e0dc2), and the docs (9652ba4). Local: full suite 4893 passed / 38 skipped; 42 new tests, all offline.

The one decision that blocks the rest — where do the six band catalogs live?

Only the 88°S row is pinned (it reuses the committed sm_healpix_o9_88s.json verbatim). The 85–87.5°S rows cannot be pinned from anything in the tree: catalogs/cat_88s.parquet is the CMR result for [-180, -88.0, 180, -87.7], and while every one of its 35,639 granules does reach north of 85°S, a granule that turns at 86°S and never reaches 87.7°S is simply absent — so an 85°S map rebuilt from it would under-count silently rather than fail. They need a catalog they do not have, and how that catalog is stored is a size call rather than a mechanical one:

  • (1) One union snapshot — a single CMR fetch over [-180, -88.0, 180, -84.7], committed as one catalogs/cat_85_88s.parquet, each band prefiltered out of it offline (exact for complete rings). One fetch, one file, ~40 MB.
  • (2) Seven per-band snapshots — mirrors how cat_88s.parquet is stored today. Most consistent with the existing convention; ~100 MB of new binaries and ~2 h of CMR fetching.
  • (3) No snapshot — leave the six rows unpinned and let whoever runs the sweep fetch live from CMR (bench_metrics.rebuild_shardmap already takes that path when an entry has no catalog_parquet). Zero bytes; the sweep then re-fetches every run and is not deterministic across runs.

Recommendation: (1). It keeps the rebuild deterministic and offline the way the 88°S row already is, the ring prefilter is exact so one fetch genuinely serves all seven bands, and it is one artifact to regenerate rather than seven. The reason it is a question and not a commit is CLAUDE.md §3 on large binaries — ~40 MB is a deliberate addition.

Once that is answered the remaining work is mechanical and already written down as _pin_recipe in the manifest: fetch, build, prune, then move each band's two rows from pending_targets into targets.

Two smaller things also standing, both in the PR body under "Questions for review": whether the 45° sector seams should move off the HEALPix base-cell grid (it would re-pin healpix_o9_88s / healpix_o10_88s, so not a change to make unasked), and a pre-existing temporal inconsistency in targets.json's healpix_o9_88s entry that this work surfaced but did not touch.

Not run, by construction: no Lambda invoke, no standup, no S3 write, no CloudFormation. Everything in this PR is offline — the only dispatch path exercised anywhere in the tests is run_target(dry_run=True), which returns before importing zagg.runner.agg. The fleet numbers quoted in the manifest and the docs are transcribed from the 2026-07-19 reassessment run on 0.36.0, attributed as such, and flagged for re-recording when the sweep is next actually run.


Generated by Claude Code

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.

add 88 south stress test shard + benchmark

2 participants