Rework benchmark matrix: sharded tdigest, inline-vs-sidecar, K=4, 4 GB#196
Conversation
espg
changed the title
| # issue #193: the sharded inline-vs-sidecar matrix. The read backend is set | ||
| # per target (index_backend: inline|sidecar) by run_benchmark; granule-level | ||
| # read concurrency is pinned to K=4 (issue #180 sizing sweet spot). | ||
| granule_workers: 4 |
There was a problem hiding this comment.
🤖 from Claude (review)
Removing index: backend: hierarchical from this shared config (and the o10 twin) silently reroutes the provisional _88s baseline probes. tdigest_healpix_o9_88s / tdigest_healpix_o10_88s reuse these exact configs (atl03_tdigest_healpix_o9.yaml / _o10.yaml) and set no index_backend, so run_target leaves data_source.index absent → index_from_config resolves to the default inline path (these configs have read_plan.spatial_index: segments, no chunk_boundaries, so the hierarchical exception does not apply).
Before this PR those probes ran hierarchical. That contradicts the standing intent recorded in provisional_targets._comment: "the base _88s targets stay pinned to the hierarchical baseline (issue #148 arm 1, the honest uncached reference)" — the _88s vs _88s_cached pair is supposed to measure hierarchical-vs-sidecar, and it now measures inline-vs-sidecar. (These probes also now inherit sharded: true + granule_workers: 4.)
run_benchmark already grew a hierarchical branch in this PR, so the clean fix is to add "index_backend": "hierarchical" to the two _88s provisional targets in targets.json, restoring the honest uncached reference without re-pinning the shared config.
There was a problem hiding this comment.
🤖 from Claude
Good catch — folded in 454ac1b. Pinned index_backend: hierarchical on tdigest_healpix_{o9,o10}_88s (restores the honest uncached baseline the config lost when I dropped its pin) and sidecar on the _88s_cached companions (explicit, matches their config). Verified via dry-run: the base 88s target now records hierarchical, the cached one sidecar. Also refreshed the two stale '2 GB' prose spots (plot_series memory-cap fallback + bench_metrics header) to 4 GB.
|
🤖 from Claude (review) Fresh-context review of the benchmark-matrix rework. One substantive finding is inline (the Minor — stale "2 GB" prose after the 4 GB flip. Advisory — dead render path (question of judgment). Cost-model coupling (verified acceptable). Verified clean:
|
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Reviewed commit 325ec8d only. Verdict: this is a legitimate fix that tracks the intentional 2→4 GB behavior change, not a weakening of tests to mask a regression.
Verification:
- Source of truth confirmed:
src/zagg/dispatch.py:320LAMBDA_MEMORY_GB = 4.0(# issue #193), and the runner cost path multipliestotal_lambda_time * memory_gb. - Arithmetic on all three updated fixtures is correct:
test_lambda_cost_byte_identical: 4 cells × 2 s = 8.0 lambda_time_s (unchanged), × 4 GB = 32.0 GB-s; cost32.0 * 0.0000133334. ✓test_..._with_mixed_durations:gb_seconds == total * 4.0, cost(total * 4.0) * price— preserves the single-multiply-over-summed-time order the test guards. ✓TestTemporalLambdaStrategy:2.0 s × 4 GB == 8.0. ✓
- The updated comments were kept in sync with the new factor.
No other cost-factor test still hardcodes 2 GB. tests/test_dispatch.py already references LAMBDA_MEMORY_GB symbolically (unaffected). The memory_gb: 2.0 / cap_mb / 2048 occurrences in tests/test_benchmark.py are historical benchmark rows and the memory-fraction twin axis (issue #120) — inputs to plotting, not the dispatch cost factor — so they correctly stay at 2 GB and are out of scope for this change. tests/test_lambda_handler.py's memory_limit_in_mb = 2048 is a Lambda context mock, also unrelated.
One non-blocking nit left inline about symbolic vs. literal constant. Otherwise clean.
Generated by Claude Code
| # 4 cells x 2 s x 4 GB = 32 GB-s; cost = 32 * arm64 price. | ||
| assert summary["lambda_time_s"] == 8.0 | ||
| assert summary["gb_seconds"] == 16.0 | ||
| assert summary["gb_seconds"] == 32.0 |
There was a problem hiding this comment.
🤖 from Claude (review)
Non-blocking nit (follow-up, not for this commit): these three cost fixtures hardcode the 4.0 factor as a literal, whereas the sibling tests/test_dispatch.py imports LAMBDA_MEMORY_GB and asserts against it symbolically (3.0 * LAMBDA_MEMORY_GB), which won't drift if the default changes again. The literal style matches the rest of test_runner.py, so it's acceptable here — just worth a future pass to reference the constant symbolically so a memory bump only needs a source edit, not a fixture sweep.
Generated by Claude Code
|
🤖 from Claude Benchmark render preview (issue #193)Rendered locally from Latest-merge table — the live inline-vs-sidecar matrix:
Cost per shard — 2×2 grid, rows o9/o10, columns inline/sidecar, cost + runtime vs merge history:
Cost per 100 km²:
The retired codec/frozen figures are frozen (retained on the benchmarks branch, embedded in an "Archived" section of (Preview images live on the throwaway |
Documentation Preview Ready!Live PreviewURL: https://zagg-pr-196.surge.sh/ Commit: Preview deployed via |
|
🤖 from Claude Real benchmark render (0.21.0 fleet · 4 GB · K=4)Live dispatch of the 4 matrix targets against
Sidecar beats inline on both orders (o9 −26%, o10 −19%) — the selection-read + compiled-decode win. Memory 320–540 MB of 4096. Latest-merge table:
Cost per shard (rows o9/o10, cols inline/sidecar):
Cost per 100 km²:
(Single-merge render, so each series panel shows one point — the trend fills in over merges. Preview images on the throwaway |
Documentation Preview Ready!Live PreviewURL: https://zagg-pr-196.surge.sh/ Commit: Preview deployed via |
2 participants
Reworks the live benchmark matrix to what actually matters after this week's read-path work, espg-directed (session thread); builds on #192 (wall columns).
The new matrix
tdigest · sharded · inline-vs-sidecar · granule_workers=4 · 4 GB workers · o9 + o10. The read backend is the A/B axis; the old ShardingCodec (sharded/inner) + read (cached) axes are retired. 4 live targets:
tdigest_healpix_{o9,o10}_{inline,sidecar}.Phases
targets.json(4 inline/sidecar targets; old codec/inner/cached retired with a frozen-note); o9/o10 configs pinnedgranule_workers: 4+sharded: truewith index set per-target;run_benchmarkappliesdata_source.indexfrom each target'sindex_backendand records it;LAMBDA_MEMORY_GB2.0→4.0 andtemplate.yamlMemorySizedefault 2048→4096 (issue Granule-level read concurrency: the worker's serial granule x group loop is the dominant read-wall term #180 sizing — 4 GB is faster and cheaper);bench_metricsgains theindex_backendcolumn. Tests updated (74 pass).codec_table.png/*_codec.png— the old PNGs are retained on the benchmarks branch, just no longer updated); add the inline/sidecar matrix figures (2×2, o9/o10 × inline/sidecar) at the top; replace the codec-layout tests with matrix-layout tests.tests/data/benchmark/README.md; frozen codec matrix documented below.test_function_build_succeeds); ruff clean. End-to-end plot render verified: matrix table +*_matrix.pngon top, retained codec/frozen PNGs embedded as an archived section, machine-readable companions track the live matrix.Notes / for review
Retained-not-removed:
make_codec_figure/_codec_layout/make_figureetc. stay inplot_series.py— they regenerate the archived tiers on demand and are the shared rendering engine's test surface;main()simply no longer invokes the codec/frozen tiers. Flagging in case you'd rather rip the codec-specific renderers out entirely.Deploy (yours): the 4096 default lands via a
template.yamlMemorySize change — productionprocess-shardpicks it up on the next stack update/deploy (§1, espg). Until then the CI cost model (LAMBDA_MEMORY_GB=4.0) assumes the benchmark function is at 4096; the benchmark should run against a 4096-deployed function for the numbers to be truthful.Old benchmark PNGs: archived (sent to espg) from benchmarks commit
e0eafda; this PR stops updating them but retains them on the branch.metrics.jsonat repo root is a stray run artifact — left untracked (removed from the phase-1 commit).Branch/commit messages say "#193" from an earlier number assumption; #193 is actually the merged consolidation-skip issue. This matrix rework is session-directed with no separate issue — happy to file a tracking issue if wanted. Commit messages can't be corrected without a force-push.