Skip to content

Compiled read path by default: inline via hidefix, read_workers pool, cached/uncached benchmark columns (issue #170)#173

Merged
espg merged 9 commits into
mainfrom
claude/170-rust-read-path
Jul 6, 2026
Merged

Compiled read path by default: inline via hidefix, read_workers pool, cached/uncached benchmark columns (issue #170)#173
espg merged 9 commits into
mainfrom
claude/170-rust-read-path

Conversation

@espg

@espg espg commented Jul 6, 2026

Copy link
Copy Markdown
Member

Closes #170.

What

Makes the compiled (hidefix) read path the one zagg actually uses, per the plan on the issue (plan, espg's answers, non-ATL03 design noteOption B chosen by espg (session, 2026-07-06): one unconditional default flip after the full-read seam, no conditional interim).

Phases

  • Phase 1 — inline decodes through the compiled reader (planned route). Per-dataset single-dataset Index (from_chunks ~ms), reads via read_planioRequest(caching=False)read_from_buffers (GIL released); per-dataset degrade to h5coro (warning, never aborts a shard); transient I/O (None buffers) surfaces as OSError, never misclassified as decode failure. Review findings folded + independently verified closed.
  • Phase 2 — compiled full-read route. _read_group_full (and _read_segment_broadcasts) take the same optional read_fn seam as _planned_read_group; read_fn=None keeps the batched h5coro reads byte-identical. InlineIndex now serves read-plan-less (flat) data sources through it — the shape most non-ATL03 targets have — and its spatial_index config requirement is dropped (chunk_boundaries mutual exclusion stays). Bonus: the flat compiled route is immune to h5coro's PR 88S stress shard: pins, 900s timeout, chunk-boundary extraction, a-priori read plan, streaming tdigest (issue #148) #152 start-edge off-by-one (the new parity test's window lands exactly on a chunk boundary; the hierarchical reference cannot serve it).
  • Phase 3 — unconditional default flip: index: Noneinline for every data source (flat and planned alike); hierarchical stays selectable (the uncached benchmark baseline). Sole exception: a-priori read_plan.chunk_boundaries sources keep hierarchical (mutually exclusive with inline's addressing). Flat-route chunk-map build failures degrade per dataset (pre-Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170 flat reads never required a map); the planned route keeps its re-raise (the map is what makes the PR 88S stress shard: pins, 900s timeout, chunk-boundary extraction, a-priori read plan, streaming tdigest (issue #148) #152 boundary workaround possible). Worker-logic tests that stub _read_group now pin the hierarchical seam explicitly.
  • Phase 4 — read_workers thread pool (default 8, data_source.read_workers): the per-dataset reads in _execute_plan_group (coord pair + variable/filter fan-out) and _read_group_full's seamed sites run through a bounded pool. Engages ONLY when a compiled read_fn is in play — the hierarchical baseline keeps its exact serial batched reads (pinned benchmark series). Thread safety verified: h5coro FileDriver locks reads, S3Driver rides boto3's thread-safe client, ioRequest caching has per-cache-line locks; results keyed by path so completion order can't leak into output (byte-identity test at width 1 vs 8).
  • Phase 5 — benchmark columns: all 11 configs pinned to {backend: hierarchical} (series continuity across the default flip), three *_cached companion configs+targets (tdigest HEALPix o9/o10/o11, sidecar + on_miss: build over zagg-index/ATL03/007 — the same prefix the sponsor example populates, so the caches are shared). Matrix-pinning tests extended for the new read axis.
  • Phase 6 — docs: README read-backend section (inline default / sidecar / hierarchical baseline + read_workers), inline.py module docstring updated for the compiled decode and dual routes.

How it was tested

  • Phase 1: TestInlineReadGroup parity suite (row/byte-identical to hierarchical incl. arrow carrier + boundary case vs numpy ground truth); engagement pinned via spy and no-fallback-warnings caplog; poison-dataset isolation (test_bad_dataset_degrades_alone); I/O-error surfacing (test_none_buffer_surfaces_as_io_error).
  • Phase 2: flat-source parity vs numpy ground truth (test_inline_serves_flat_sources), flat-route engagement (test_flat_source_engages_compiled_decoder); read_fn=None default byte-identity covered by the untouched hierarchical/planned suites.
  • Suites: test_index 84, test_read_plan (in the 84 run), test_processing 194 — all green; ruff clean.

Fleet A/B: PR #169 benchmark comment is the pre-change baseline. The deployed fleet runs released zagg, so branch-code fleet numbers require the next release+deploy; a local SERC measurement (inline read_workers 1 vs 8: wall + peak RSS) lands here instead before ready-for-review, and the deployed A/B rides the release (espg-run, per convention).

Questions for review

  • Phase 1/2 make reads build chunk maps where they previously went straight to readDatasets — under write_back this widens manifest coverage to full-read datasets (deliberate: fewer sidecar misses later). Flagging in case wider manifests are unwanted.

@espg espg added the implement label Jul 6, 2026
Comment thread src/zagg/index/inline.py Outdated
from h5coro_hidefix.manifest import datasets_from_manifest

state["vidx"] = Index.from_chunks(
"inline", datasets_from_manifest(granule_manifest(maps))

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)

One undecodable dataset silently disables the compiled route for the whole group — and, under write_back, for the rest of the granule.

_vidx() always rebuilds from all of maps, so a single dataset whose spec Index.from_chunks rejects poisons every rebuild: the first failure pins that path to direct, but its ChunkMap stays in maps, so every subsequent dataset's first _compiled() call re-fails at from_chunks and gets pinned too — with a warning that names the innocent path but carries the poison dataset's error. Under write_back, maps is self._pending, which persists across the granule's groups, so one bad dataset in gt1l degrades gt2l/gt3l as well. The docstring's "degrade to the h5coro decoder per dataset" doesn't hold for reconstruction failures.

Concrete per-dataset triggers for a from_chunks rejection (h5coro-hidefix parse_spec/build_dataset_d): a nonzero per-chunk filter_mask (HDF5 sets it when an optional filter — deflate on an incompressible chunk — is skipped; hidefix's chunk model can't represent it, so parse_spec raises), or a chunk table that doesn't tile the shape (partially allocated dataset).

Verified empirically on this branch: injecting one ChunkMap with filter_mask=1 into _pending before read_group on the fixture makes every dataset warn and fall back:

compiled decode unavailable for /gt1l/heights/lat_ph (dataset /gt1l/heights/poison: chunk 0 has nonzero filter_mask 1; ...); reading through h5coro
compiled decode unavailable for /gt1l/heights/lon_ph (dataset /gt1l/heights/poison: ...); reading through h5coro
compiled decode unavailable for /gt1l/heights/h_ph (dataset /gt1l/heights/poison: ...); reading through h5coro
compiled decode unavailable for /gt1l/heights/signal_conf_ph (dataset /gt1l/heights/poison: ...); reading through h5coro

Rows stay correct (the h5coro fallback is row-identical), so this is a silent performance/observability defect: the compiled path this PR benchmarks quietly turns itself off group-wide on the first bad dataset.

Suggested fix — exclude pinned paths from the rebuild and key the cache on both sets:

def _vidx():
    key = (len(maps), len(direct))
    if state["key"] != key:
        live = {p: m for p, m in maps.items() if p not in direct}
        state["vidx"] = Index.from_chunks(
            "inline", datasets_from_manifest(granule_manifest(live))
        )
        state["key"] = key
    return state["vidx"]

After the bad dataset is pinned, the next rebuild excludes it and the remaining datasets keep compiled decode. maps and direct only ever grow (nothing is deleted or replaced in either), so the tuple key can't go stale.

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 12c299f: replaced the shared rebuilt-from-all-maps Index with one single-dataset Index per path — a rejected spec now pins only itself; test_bad_dataset_degrades_alone pins exactly one warning naming the offending dataset while the rest of the group stays compiled and row-identical.

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)

Resolved by 12c299f. Re-ran the original probe against the fold: a filter_mask=1 ChunkMap injected into _pending now produces zero "compiled decode unavailable" warnings — every real dataset stays on the compiled route (the poison map is simply never consulted, since each path gets its own singleton Index). Also verified the new test_bad_dataset_degrades_alone pins exactly one warning naming the offending dataset, and that the per-dataset build changes no read_plan/read_from_buffers semantics: chunk_idx is computed per dataset from its own grid, and both calls are single-dataset lookups with no cross-dataset state — the byte-parity tests confirm. The singleton rebuild is also strictly cheaper than the old shared rebuild-on-growth (O(n) singleton builds vs O(n²) cumulative spec work), and the 1000+-dataset full-coverage case belongs to the sidecar backend, which is unchanged (one Index per granule from the parquet).

Comment thread src/zagg/index/inline.py Outdated
def _compiled(path, start, end):
vidx = _vidx()
addrs, sizes, _ = vidx.read_plan(path, start, end)
buffers = [h5obj.ioRequest(int(a), int(s), caching=False) for a, s in zip(addrs, sizes)]

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)

A transient S3 failure is misclassified as "compiled decode unavailable" and pins the dataset to the slow path for the rest of the group.

ioRequest(..., caching=False) is a bare driver.read(), and h5coro's S3Driver.read (s3driver.py, h5coro 1.0.4) swallows every exception — throttling, timeout, connection reset — and returns None:

except Exception as e:
    logger.error(f"Error reading {self.key} from {self.bucket_name}: {e}")
    return None

That None lands in buffers, and read_from_buffers fails inside hidefix's extract_buffers with buffer N is not bytes-like: cannot make memory view... — nothing in the message says an S3 GET failed. The broad except Exception in read_fn then adds the path to direct and logs a warning blaming the decoder rather than the network.

Consequences: (a) the warning is not actionable — an operator hunting a slowdown sees decode-capability errors, not S3 throttling; (b) the h5coro fallback refetches and usually succeeds, so a throttling episode silently switches shards off the compiled path, polluting exactly the cached/uncached benchmark columns this PR adds; (c) pre-#170, a planned-read I/O failure surfaced as a read error, not a silent per-dataset downgrade.

Suggested fix — treat a failed fetch as an I/O error, not a decode-capability failure, and let it propagate past the fallback catch:

buffers = [h5obj.ioRequest(int(a), int(s), caching=False) for a, s in zip(addrs, sizes)]
if any(b is None for b in buffers):
    raise OSError(f"{path}: ranged GET failed for a planned chunk")

paired with narrowing read_fn's handler (e.g. except (ValueError, KeyError) — the two shapes the compiled seam actually produces for capability failures: hidefix spec/buffer rejections and dataset-not-in-index) or an explicit except OSError: raise ahead of it.

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 12c299f: _compiled now raises OSError on a None ioRequest buffer, and read_fn re-raises OSError explicitly before the broad except — transient I/O propagates instead of silently pinning the dataset; test_none_buffer_surfaces_as_io_error exercises the seam directly (a read_group-wide ioRequest patch breaks h5coro's own selection reads first, which is itself a nice confirmation of your finding's mechanism).

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)

Resolved by 12c299f. Verified beyond the new test: with a flaky ioRequest (returns None once for caching=False), read_fn raises OSError and does not pin the path — a retry through the same closure with healthy I/O goes compiled again and returns the rows, so a transient blip costs one read error, not the compiled route. The except OSError: raise sits before direct.add, and worker-side the error lands in the per-group handler (read_errors += 1, warning) rather than a silent downgrade. Also checked the write_back interaction: finish_granule still runs after a mid-granule read error, persisting the maps built so far — correct, since each ChunkMap is complete whole-dataset metadata from a successful B-tree walk (independent of the failed data GET) and the manifest contract is explicitly lazy-coverage; the sidecar consumer handles absent datasets via on_miss.

Comment thread tests/test_index.py
_open_fixture(), "gt1l", _fixture_data_source(), 1, _LeafSetGrid(_UNALIGNED_LEAVES)
)
assert df is not None and len(df) > 0
assert calls, "compiled decode was never engaged"

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 test still passes when the compiled decode is completely broken, so it doesn't pin what its comment claims.

The spy fires inside _vidx(), i.e. it proves a compiled attempt was made — not that the compiled route served the read. If Index.from_chunks or read_from_buffers then fails, read_fn degrades per dataset and rows stay byte-identical, so both assertions here still hold. Verified on this branch: with h5coro_hidefix.Index.read_from_buffers monkeypatched to raise unconditionally, this test body passes as-is — which is exactly the "silent per-dataset fallback" the comment says byte-parity can't distinguish. A regression that breaks compiled decode outright would leave the whole suite green.

Suggested fix — also assert that no fallback fired:

with caplog.at_level(logging.WARNING, logger="zagg.index.inline"):
    df = InlineIndex().read_group(
        _open_fixture(), "gt1l", _fixture_data_source(), 1, _LeafSetGrid(_UNALIGNED_LEAVES)
    )
assert df is not None and len(df) > 0
assert calls, "compiled decode was never engaged"
assert "compiled decode unavailable" not in caplog.text
assert "no chunk map for" not in caplog.text

(or spy on Index.read_from_buffers and assert it returned successfully, which pins engagement directly).

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 12c299f: the engagement test now wraps the read in caplog and additionally asserts no 'compiled decode unavailable' / 'no chunk map' warnings fired — your read_from_buffers-raises repro now fails the test as intended.

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)

Resolved by 12c299f. Re-ran the original adversarial probe against the fold: with Index.read_from_buffers monkeypatched to raise unconditionally, the strengthened test's assert "compiled decode unavailable" not in caplog.text now trips — a silently-broken compiled decode can no longer leave the suite green.

@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude (review)

Phase 2 adversarial pass (854efe4): no defects found. Everything below was verified against the actual code in a detached worktree, with pytest tests/test_index.py tests/test_processing.py green (251 passed).

What was probed:

  1. Seam completeness in _read_group_full — all five read sites route through read_fn when set (coords, coarse flags, link arrays, windowed variables/filters, and both sites inside _read_segment_broadcasts). The remaining unseamed h5obj.readDatasets calls are the planned route's selection/coarse reads (read.py lines 357, 514, and the unseamed _read_segment_broadcasts call at 533) — the phase-1 "base-rate addressing only" scope, unchanged and byte-identical either way.
  2. Coarse-flag and link-array parity through the seam — built a flat-with-levels source (no read_plan, cross-level filter on reference_photon_lat, segment-level variables broadcast): inline output is assert_frame_equal to hierarchical with zero fallback warnings — including the first-ever compiled decode of the contiguous-layout link arrays (ph_index_beg/segment_ph_cnt pseudo-chunk). 2-D flag shape parity holds (hidefix never squeezes; h5coro returns shaped arrays).
  3. hyperslice: [] vs read_fn(path) — equivalent full-read semantics; the fallback's plain-path readDatasets([path]) form is the same full read as the dict-with-empty-hyperslice form.
  4. Mixed full+window reads of one path — a coordinate also declared as a variable (full read then windowed read through one closure) is row-identical to hierarchical, no fallback.
  5. Flat + write_back_prebuild_group_maps with si_lvl None is a clean no-op branch; the persisted manifest covers coords + variables + base filters (prebuild) plus the seam-read coarse datasets (flag, both link arrays) — a coverage superset of the planned route, consistent with the documented lazy-coverage contract.
  6. The parity test's PR 88S stress shard: pins, 900s timeout, chunk-boundary extraction, a-priori read plan, streaming tdigest (issue #148) #152 claim is real, not an artifact — on _UNALIGNED_LEAVES the flat-route window starts at photon 512 = 2×256, an interior chunk boundary; the hierarchical flat read doesn't just drop rows, it hard-fails (H5Coro encountered error reading gt1l/heights/h_ph: invalid location to read chunk: 1024, 2048 → null dataset → TypeError in _read_group_full), while inline matches numpy ground truth byte-for-byte on the same window. Gating that test against ground truth instead of the hierarchical reference is the right call.

Two non-blocking observations (no action required to land this phase):

  • The ten test_processing harness comments say "the issue Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial #170 default otherwise resolves to inline", but at this commit index_from_config still defaults to HierarchicalIndex (the flip is explicitly deferred per the index/__init__.py docstring) — the pins are currently no-ops placed ahead of the flip. Fine as forward-defense; just be aware the comments are premature until the flip commit lands, and misleading if it doesn't.
  • Hardening nit for a future pass: now that the flat route sends both call shapes through one read_fn closure, a path whose build_chunk_map raises on a full read (pinned to direct with no cached map) and is later read with a hyperslice would hit len(cm) with cm=None in _h5coro_read (TypeError). Reaching it needs a contrived file+config (e.g. a compact-layout dataset declared as both coordinate and variable), so not a defect today — a cm is not None and len(cm) guard would close it for free.

"aggregator": "tdigest",
"grid_type": "healpix",
"grid_size": "o11",
"codec": "inner",

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 *_cached targets collide with the real inner column in the codec figure: plot_series._codec_layout slots by (grid_size, codec), and these targets carry codec: "inner".

run_benchmark.py records codec=target.get("codec") into the series row but never reads the new read key, so a cached target's row is (grid_size="o9", codec="inner") — the exact slot key of tdigest_healpix_o9_inner. _codec_layout (plot_series.py) fills by_slot[(grid_size, codec)] = target in row order, last write wins: on the first merge after this lands, each order's "inner" panel in the 2×3 codec figure will silently show either the real inner column or the cached column, depending on row order in the series parquet.

Verified empirically against this branch:

hist = pd.DataFrame([
    {"target": "tdigest_healpix_o9_inner",  "grid_size": "o9", "codec": "inner"},
    {"target": "tdigest_healpix_o9_cached", "grid_size": "o9", "codec": "inner"},
])
grid, _, _ = plot_series._codec_layout(hist)
assert grid[0][1] == "tdigest_healpix_o9_cached"   # displaced the real inner panel

(No parquet schema drift — read: "cached" is simply ignored by run_benchmark/bench_metrics — but that's the problem: the read axis exists only in the target name, invisible to the layout.)

Note the trap doors on the obvious dodges: omitting codec from the cached targets routes their rows into the frozen historical series instead (codec.isna split in plot_series._codec_mask), and test_forward_matrix_is_tdigest_healpix_arrow_codec_ab currently pins codec == "inner" for them. Suggested fix: record the read axis into the series (ctx read=target.get("read") + a read column in bench_metrics.SERIES_COLUMNS) and make _codec_layout/_codec_history read-aware — either exclude read == "cached" rows from the 2×3 grid or extend CODEC_COLS with a third cached column. (.github/scripts is CI tooling, so if touching it is out of scope for this PR, the cached targets should move to provisional_targets until the renderer can represent them — as merge-matrix members they corrupt the figure on the first merge.)

One adjacent thing worth confirming before the first run (can't be verified from the repo): on_miss: build needs s3:PutObject on s3://sliderule-public-cors/zagg-index/ATL03/007 from the benchmark Lambda role. If the write is denied, finish_granule swallows the failure (worker logs a warning and continues), the store never populates, and the "cached" column silently measures inline-uncached forever while labeled cached.

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 659382c: run_benchmark now records read=target.get('read') into the series context, and _codec_layout keys cached targets into their own third column (CODEC_COLS grew to sharded/inner/cached; slot label is read-aware). Layout tests updated for the 3x3 grid. Note this touches .github/scripts/ — in-scope here because the issue's phase 5 (two benchmark columns, espg-directed) is unrepresentable in the series without it; flagging per the CLAUDE.md infra rule for review. Your IAM caveat is real and stands for espg: the benchmark Lambda role needs PutObject on sliderule-public-cors/zagg-index/* for on_miss:build self-population (the execution-role grant landed today for the sponsor stack — worth verifying it covers the benchmark function too, else the cached column silently measures inline-uncached until manifests exist).

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)

Not resolved — the read axis never reaches the series parquet, so the collision is intact. 659382c threads read into run_benchmark's ctx and makes _codec_layout read-aware, but the pipe between them was not extended:

  • bench_metrics.build_record builds the record from an explicit key list that has no "read" — the ctx value is dropped there;
  • even if it weren't, update_series.records_to_frame reindexes to bench_metrics.RECORD_COLUMNS (no "read"), which by design drops any column not in the schema.

Verified end-to-end on 659382c with exactly run_benchmark's ctx for a cached target:

record carries read: False
frame has read column: False
o9 row [sharded, inner, cached]: [None, 'tdigest_healpix_o9_cached', None]

The cached target still lands in (and here displaces) the inner slot; the new third column stays blank forever. Fix is the two missing pieces: append "read" to RECORD_COLUMNS (stable-schema rule: last position) and add "read": context.get("read") to build_record. Frozen safety holds in both worlds: a pre-existing parquet has no read column and Series.get("read", "") returns the default; once the column exists, legacy rows read back NaN and str(nan) != "cached", so they keep keying on codec — I verified the layout tests' frozen paths pass either way.

Coverage note (why the suite stayed green through this): both _codec_layout tests assert the third column is None on histories with no cached rows — nothing drives a cached row through records_to_frame_codec_layout. A one-row addition to _full_codec_matrix (a cached record built through build_record) would have caught this and will pin the fix.

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

Right — the first fold wired both ends and dropped the middle. Fixed in the follow-up commit: read appended to RECORD_COLUMNS (stable-schema: newest last; the order test now pins read last, codec second-to-last) and build_record threads context.get("read"). Coverage closed the way you suggested: _cached_row builds through the real bench_metrics.build_record, and the layout test now asserts the full ctx→record→frame→layout pipe lands each cached target in the third column with the true inner target keeping its slot. Your exact repro (o9 row displacement) is what the new assertion fails on if the pipe breaks again.

"""
w = data_source.get("read_workers", 8)
if isinstance(w, bool) or not isinstance(w, int) or w < 1:
raise ValueError(f"data_source.read_workers must be an integer >= 1 (got {w!r})")

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)

read_workers is never validated at config load — a bad value passes validate_config and then turns every group read into a swallowed per-group error: silent empty shards instead of a fast failure.

validate_config (zagg/config.py) has no read_workers check (it validates handoff, aoi_mask, filters, links, bounds... at load), so read_workers: 0 / "8" / 2.5 in a YAML sails through submission. The ValueError then fires here, inside _execute_plan_group/_read_group_full, which sits inside the worker's per-group except Exception (worker.py read_errors += 1; logger.warning(...); continue) — every group of every granule fails identically, the shard completes "successfully" with zero rows and only a read_errors count in metadata. On a fleet run that's a config typo becoming silent data loss across every shard, discovered from an empty store rather than a rejected submission. Verified empirically on this branch:

cfg = PipelineConfig(data_source={..., "read_workers": 0}, ...)
validate_config(cfg)                      # passes
_read_workers(cfg.data_source)            # ValueError -- but only at read time, per group

Two adjacent asymmetries worth noting: (a) with an explicit index: {backend: hierarchical} pin, read_fn is None so _read_workers is never called and the bad value is silently ignored; (b) test_read_workers_validation pins the helper's behavior but nothing pins the load-time path, which is where the operator needs the failure.

Suggested fix: validate in validate_config's data_source section (the same one-liner this helper already implements — e.g. call _read_workers(config.data_source) or duplicate the check), mirroring how aggregation.handoff is validated at load "so a typo in the YAML fails at load, not deep in a worker" (config.py's own words for that 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

Folded in 659382c: validate_config now rejects a bad data_source.read_workers at submission (mirrors the handoff guard; same message as _read_workers). The silently-ignored-under-hierarchical-pin case remains by design — the key is documented as compiled-path-only — but it now at least fails loudly when malformed anywhere.

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)

Resolved by 659382c. Verified empirically: validate_config now rejects read_workers 0 / -1 / True / "eight" / 2.5 at submission (each with the read_workers message) and accepts 4 and absent; the guard mirrors _read_workers' semantics including the bool-is-int trap. A config typo now fails at load instead of surfacing as per-group swallowed read errors and silently-empty shards.

# column. The package default flipped to the compiled `inline` path; an
# unpinned config would silently move this series onto the fast path.
index:
backend: hierarchical

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 pin also freezes the two 88S provisional stress probes on pure h5coro, stranding their documented purpose.

provisional_targets.tdigest_healpix_o9_88s / o10_88s (targets.json) reference this same config (and o10's), and their _comment says they are "the issue #148 stress probes: on-demand runs against the pinned worst 88S ring shard, expected to OOM/timeout until the streaming/cached-read work lands". The cached-read work is landing in this very PR — but with the shared base config now pinned hierarchical, the 88s probes can never see it: they'll OOM/timeout on the h5coro baseline forever, and the one arm built to demonstrate the #148 fix can't demonstrate it.

Keeping the matrix baseline honest is right (that's what this pin is for); the gap is only that the 88s probes inherited the pin through the shared config. Suggested fix, either: (a) add tdigest_healpix_o9_88s_cached / o10_88s_cached provisional companions pointing at the new *_cached.yaml configs with the existing healpix_*_88s shardmaps — on-demand --target runs that can actually answer "does the compiled/cached read survive the worst shard?"; or (b) if that's deliberately deferred, update the provisional _comment so it stops promising an outcome the pinned config now precludes.

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 659382c: added tdigest_healpix_{o9,o10}_88s_cached provisional companions (same pinned 88S shardmaps, *_cached configs, read:'cached') and updated the provisional _comment — the base _88s probes stay pinned hierarchical as the honest issue #148 arm-1 baseline, and the pair now measures exactly the cached-vs-uncached delta at the pole.

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)

Resolved by 659382c (option (a) as confirmed on the thread). Verified: tdigest_healpix_o9_88s_cached / o10_88s_cached point at the *_cached.yaml configs with the existing healpix_*_88s shardmaps, stay provisional (on-demand --target only, never the merge matrix), and the _comment now states the pairing correctly — base _88s pinned hierarchical as the honest uncached reference, _cached companions measuring the compiled/sidecar read on the same worst-case shard.

@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

do (a)

@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Re "do (a)": already landed in 659382c (crossed with your comment) — tdigest_healpix_o9_88s_cached / tdigest_healpix_o10_88s_cached provisional companions on the *_cached.yaml configs + the existing healpix_*_88s shardmaps, with the provisional _comment updated to describe the cached-vs-uncached pairing at the pole. On-demand --target runs as before; nothing joins the every-merge matrix.

@espg espg marked this pull request as ready for review July 6, 2026 22:22
@espg espg added the waiting label Jul 6, 2026
@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Marking ready for review per espg (merge call on the session thread). State: all six phases complete; eight adversarial-review findings across four passes folded and verified closed (the last — the read-column record-schema gap — in adb2764). CI running on adb2764 (ruff green; the commit touches only bench_metrics + its tests). The local read_workers 1-vs-8 A/B (wall + peak RSS, dense SERC shard over https) is still running and will be posted here / on #170 when it lands — espg's call to merge ahead of it, with the definitive fleet A/B riding the 0.15.0 release (espg-run):

  • deploy order suggestion: release → layer/function deploy → SERC fleet A/B (validates the new default + read_workers on-fleet) → the 88s *_cached provisional probes (the issue add 88 south stress test shard + benchmark #148 arm this PR unblocks).
  • release-blockers on the ops side (from the PR Force-cold Lambda dispatch option (issue #171) #172 thread): the benchmark role needs lambda:UpdateFunctionConfiguration (or run_benchmark pins force_cold=False), and Get/Put on sliderule-public-cors/zagg-index/* for the cached column's self-population.

@espg espg merged commit 5f29a04 into main Jul 6, 2026
11 of 13 checks passed
@espg espg deleted the claude/170-rust-read-path branch July 6, 2026 22:40
@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Correction to the release-blockers note two comments up: the force_cold/benchmark-role item is void. PR #172's final form (phases A–D on the thread) flips force_cold back to default False (explicit certification-run tool only) and adds worker-side self-recycle: ZAGG_RECYCLE_MAX_INVOCATIONS=1 on the function environment forces a fresh sandbox for every invocation with no caller IAM at all (the worker mirrors its result to S3 first, then os._exit(0); MaximumRetryAttempts: 0 keeps billing clean), and ZAGG_RECYCLE_RSS_MB bounds the ratchet surgically within runs. espg's no-grant decision is recorded on the #172 thread.

The one remaining ops item for the 0.15.0 cached benchmark column: Get/Put on sliderule-public-cors/zagg-index/* for the benchmark stack's execution role (the sponsor stack got the /* fix today; the benchmark function's role is separate).

@espg

espg commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Deferred read_workers A/B (posted post-merge per espg)

Local run, densest SERC shard (5347404432944398346: 50 granules, 1.54M obs), inline backend, https driver over a residential link, single process, identical config except data_source.read_workers:

read_workers wall peak RSS (ru_maxrss) total_obs cells_error
1 2823.5 s 361.8 MB 1,538,493 0
8 2080.5 s 351.8 MB 1,538,493 0

Takeaways:

Raw JSONs retained locally; store outputs were local scratch zarrs (nothing published).

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.

Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial

1 participant