Skip to content

Granule-level read concurrency (K in flight, ordered fold)#183

Merged
espg merged 9 commits into
mainfrom
claude/180-granule-concurrency
Jul 7, 2026
Merged

Granule-level read concurrency (K in flight, ordered fold)#183
espg merged 9 commits into
mainfrom
claude/180-granule-concurrency

Conversation

@espg

@espg espg commented Jul 7, 2026

Copy link
Copy Markdown
Member

Closes #180
Closes #179 — folded in per the direction on the issue thread (comment); the planned-read path is now read_fn-routed end to end (sole remnant: range coalescing, see Questions (4)).

What

The worker's read wall is a ~300-link chain of sequential network waits: process_shard loops granules × groups one at a time, and everything shipped under #170 only parallelizes inside one link. This PR overlaps the links (K granules in flight, issue #180) and shrinks every remaining serial link inside a granule read (issue #179), behind a default-off knob so the shipped behavior is byte-identical until the fleet A/B picks a K.

Phases

  • Phase 1 — the correctness trap (ff3481e, hardened in the review fold d8182ac): InlineIndex._pending was an instance-level dict keyed by dataset path; dataset paths repeat across granules (/gt1l/heights/h_ph…), so two in-flight granules would interleave chunk maps and persist corrupted write-back manifests. Now keyed by the full h5obj.resource URL{path -> ChunkMap} (the fold upgraded the original granule-id-stem key after the adversarial review reproduced a stem collision: .../p1/granule.h5 vs .../p2/granule.h5 collapsed to one entry, serving B's reads through A's chunk maps). The manifest store key stays the id stem per the issue Virtual index entry point: pluggable chunk-index backends for the read path #160 convention. finish_granule drains exactly its granule's entry; the VirtualIndex.finish_granule protocol docs now state the concurrency contract.
  • Phase 2 — the pool (238395b): the loop body of process_shard (H5Coro open → group loop → finish_granule → close) is extracted into a per-granule _read_granule closure returning the group carriers in group order; data_source.granule_workers (default 1 = today's serial loop) keeps up to K granules in flight on a bounded ThreadPoolExecutor (threads, not asyncio — espg-confirmed on the issue). Results fold in original granule order (the consumer blocks on the oldest future; parked out-of-order completions are bounded by K; the fold moved the top-up ahead of the yield so a full K stays in flight through every fold), so output is byte-identical to serial. One H5Coro per granule, created and closed inside its worker thread. Per-granule error semantics preserved. Works under both pooled all_reads and StreamingAggregator modes. Validated at submission (validate_config) AND at the worker; README entry next to read_workers.
  • Phase 3 — telemetry (f6da016): cpu_seconds added to the worker envelope next to max_memory_mb/container_hwm_mb in deployment/aws/lambda_handler.py (authorized: issue Granule-level read concurrency: the worker's serial granule x group loop is the dominant read-wall term #180 names this file). os.times() is process-cumulative, so it is snapshotted at handler entry and diffed at the telemetry stamp — per-invocation user+sys across all threads, making utilization = cpu_seconds / duration_s measurable per invocation. (Merged cleanly with the Small fixes: recycle generation counting + stand_up.sh artifact defaults #182 recycle-counting changes to the same file — both semantics verified intact, merge commit f433f09.)
  • Phase 4 — selection reads (ad0f000, completed by 11e4361; issue Selection reads (segment-rate geolocation + link arrays) are serial, uncompiled h5coro on every backend #179): the whole planned read is now one addressing seam. ad0f000 routed the segment-rate geolocation + link-array selection reads (reference_photon_lat/lon, ph_index_beg, segment_ph_cnt) through the backend-supplied compiled read_fn, pooled with the existing read_workers machinery via _read_paths_pooled (the _read_group_full pattern). 11e4361 folded the three remaining planned-path seams (espg-requested): (a) _read_segment_broadcasts gets read_fn at the planned call site; (b) the cross-level (Phase B) coarse flag + link reads route through read_fn, pooled; (c) the plan.full_read selectivity fallback forwards read_fn to _read_group_full so it stays compiled. read_fn=None (hierarchical baseline) keeps the batched readDatasets calls byte-identical everywhere.
  • Review fold (d8182ac, d34cede, a4a93d8) — all findings of the adversarial review addressed: (1) full-URL _pending keys + collision test; (2) h5coro-hidefix pinned at 0.3.1 (>=0.3.1 core, ==0.3.1 lambda extra, a4a93d8) — the sidecar on_miss: build delegate lazy-init race is fixed upstream (double-checked lock in zagg_backend._miss, verified in the installed wheel), so the combination pools freely under K>1 with no runtime clamp (an interim reject-guard was dropped in favor of the pin); (3) pyproj floor raised to >=3.7.0 (first release documenting Transformer thread-safety) — espg-authorized dependency change (d34cede; the interim thread-local mitigation was backed out in favor of the pin); (4) fan-out sizing note in _granule_workers's docstring + README; (5) pool top-up moved before the yield (full K in flight during folds); (6) dead group tag dropped from _read_granule's return; (7) README line for granule_workers; (8) pooled-failure test gated on URL instead of a racy counter.
  • Fleet A/B at K∈{1,2,4,6} + pick default (espg) — user-facing default-flip docs ride along then.

How it was tested

All hermetic; full non-slow suite green locally against h5coro-hidefix 0.3.1 after the merge with main (pytest -q: 1511 passed, 26 skipped — the suite exercises the sidecar entry-point discovery path), plus ruff check / ruff format --check on every touched file.

  • Phase 1: interleaved granule reads over two copies of tests/data/index/atl03_mini.h5 produce byte-identical parquet manifests to the serial order; finish_granule drains only its granule; test_same_basename_granules_do_not_collide (same-basename granules under different prefixes → two isolated pending entries, both manifests persisted — under the stem key, B's manifest was never written).
  • Phase 2 (tests/test_processing.py::TestGranuleReadPool): pooled vs serial output equality with the slowest granule submitted first; fold order pinned via a _concat_and_group spy; one H5Coro per granule created+closed in the same pool thread; failed-granule and raised-group-read semantics match serial; streaming-aggregator parity; the serial default never constructs a pool; granule_workers guards at submission (tests/test_config.py, incl. sidecar combos pooling freely under the 0.3.1 pin) and worker; end-to-end pool+write_back manifests byte-identical to serial (tests/test_index.py::TestInlineInterleavedGranules).
  • Phase 3 (tests/test_lambda_handler.py): cpu_seconds present as a float in the result body; a faked os.times() sequence pins it as the entry→stamp delta. All 82 handler tests (including Small fixes: recycle generation counting + stand_up.sh artifact defaults #182's recycle-billing additions) pass post-merge.
  • Phase 4 (tests/test_index.py::TestSelectionReads): with the inline backend a planned group read never touches readDatasets — selection, data, segment-broadcast, cross-level-filter, and full_read-fallback reads are all compiled — and rows match hierarchical in every case (the full_read test baselines against the planned route at the default threshold: full-read ≡ planned per Generic hierarchical filtering & targeted reads for multi-level point datasets #43 Phase C parity, and the hierarchical full-read route can't serve this fixture shard at all — its mask starts on a chunk boundary, the pre-existing PR 88S stress shard: pins, 900s timeout, chunk-boundary extraction, a-priori read plan, streaming tdigest (issue #148) #152 h5coro start-edge bug the compiled route is immune to). The hierarchical baseline still issues its batched selection read; a compiled-rejected selection dataset degrades alone.
  • Review fold: tests/test_rectilinear.py::TestTransformerThreadSafety (pyproj floor asserted at runtime; shared-transformer concurrent use; assign unchanged).

Questions for review

  1. Pre-existing lint failure, not fixed here: ruff check src tests fails on N818 for UnknownCapability (src/zagg/registry.py:64), present on main and untouched by this PR (the PR lint bot runs --select=E,F,W,I so it never fires in CI). Flagging per §4 rather than renaming a public exception in an unrelated PR.
  2. h5coro-hidefix 0.3.1 docs: the upstream fix landed and is pinned here; its zagg_backend module docstring still describes the serial worker contract ("the worker reads granules serially") — worth updating in the next h5coro-hidefix release to match the VirtualIndex.finish_granule concurrency contract added on the zagg side.
  3. Fan-out budget (review finding 4): worst-case in-flight GETs is granule_workers × read_workers × chunk-fetch width (K=6 → 384) vs the S3 driver's ~100-connection pool; queueing + h5coro's 5 s timeout can surface as spurious read_errors. Documented in _granule_workers's docstring and the README; no hard cap added (the A/B should see the raw behavior — watch read_errors when reading the K-sweep results). Add a validate_config warning if you want one.
  4. Selection reads (segment-rate geolocation + link arrays) are serial, uncompiled h5coro on every backend #179 remnant — range coalescing: cross-dataset range coalescing is the single Selection reads (segment-rate geolocation + link arrays) are serial, uncompiled h5coro on every backend #179 item not done here — a real optimization feature that needs its own issue + benchmark evidence (espg-agreed carve-out).
  5. Sidecar behavior note: selection (and now cross-level/segment) reads consult the manifest when sidecar serves the group, so a sparse manifest lacking those datasets sends the group to on_miss (default fallback = hierarchical) where before they bypassed the manifest. Inline write-back has always prebuilt the spatial-index level's coordinate + link arrays into every manifest, so store-populated manifests are covered (full-coverage manifests carry all 1,055 datasets).
  6. Shard-aware K: the issue sketches a dispatcher-side per-shard granule_workers override (the aoi_payload precedent). Not implemented — the knob is config-level for the A/B; the worker-side re-validation already accepts an event-injected value, so the dispatcher half can land separately once a default K exists.
  7. Fold-side error micro-difference: in the old serial loop a failing buffered.add_read counted as a per-group read_errors increment; now fold-side failures (realistically only a streaming flush inside granule_done) take the granule-level "Error processing file" warn-and-skip path, same as the old outer except. add_read is a plain list append (review-verified), so the changed case is unreachable in practice.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Wp83fdkH2qhtEvoLgyTWU1

@espg espg added the implement label Jul 7, 2026

@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)

Verdict: solid. The shipped default (K=1) is verified byte-identical, the ordered fold is correct (no deadlock/starvation, futures consumed exactly once, H5Coro per granule opened+closed in its worker thread, StreamingAggregator touched only from the fold thread), the phase-1 re-key genuinely closes the spec's _pending trap for distinct-basename granules, and the test coverage is unusually good — the slowest-granule-first tests and the fold-order spy attack the right scenarios. I reproduced the claims empirically: all 20 new tests pass, the full four touched test files pass (563 passed, ~5 min), ruff check/format clean under the bot's flags, worker.py is 669 lines as stated, and Q1's N818 is confirmed pre-existing (src/zagg/registry.py:64, untouched). PR-body Q2's "unreachable" claim checks out (StreamingAggregator.add_read is a plain append). Remaining findings gate the opt-in K>1 configurations, not the default path.

Findings by severity:

Correctness (settle before any K>1 default ships)

  1. Granule-id stem collision under write_back + K>1 is silent data corruption, empirically confirmed — two distinct granules with the same URL basename collapse to one _pending key; granule B's reads are then served through granule A's chunk maps and the first finish_granule drains both into one merged manifest. Serial is immune. Inline comment on src/zagg/index/inline.py:64 with the reproduction.
  2. sidecar + on_miss: build first-use race in installed h5coro-hidefix 0.3.0 — unguarded lazy init of the InlineIndex delegate in _miss can strand a concurrent granule's pending maps on an orphaned instance (silently sparse manifest); the h5coro-hidefix docs also still state the serial contract. The PR-body claim "No h5coro-hidefix release required" is true for _cache but not for this combination. Inline comment on src/zagg/processing/worker.py:52.

Should-fix
3. pyproj Transformer shared across pool threads (rectilinear grid.assign, src/zagg/grids/rectilinear.py:648): thread-safe only from pyproj 3.7.0 and pyproject.toml leaves pyproj unpinned — raise a >=3.7 floor on the thread (§4) or document the constraint. Same inline comment as (2).
4. Multiplicative fan-out budget: worst-case in-flight GETs is now granule_workers × read_workers × chunk-fetch width (K=6 → 384) vs the S3 driver's 100-connection pool the h5coro-hidefix sizing note budgeted at 64; queueing + the 5 s timeout can masquerade as read_errors in the K-sweep. Inline comment on src/zagg/processing/read.py:370.

Nits
5. Top-up after the yield leaves K−1 (not K) granules in flight during every fold (src/zagg/processing/worker.py:396).
6. The (group, carrier) tag in reads is never consumed (src/zagg/processing/worker.py:403).
7. granule_workers has no README entry where read_workers has one — fine to defer to the A/B, but record that (src/zagg/config.py:305).
8. Racy shared counter in test_pooled_granule_failure_skips_and_continues can (rarely) fail the assertion (tests/test_processing.py:5585).

On the PR-body questions: Q2 verified acceptable, Q3/Q6 scope calls seem right, Q4 fine as-is, Q5's sidecar behavior note is accurate but see finding (2) for the build variant. Findings (1) and (2) are questions for the author/owner to resolve before the phase-5 A/B flips a default — nothing here blocks the current default-off landing.


Generated by Claude Code

Comment thread src/zagg/index/inline.py
every driver URL rewrite, so ids derived from ``h5obj.resource`` (the
rewritten URL) and from the worker's original ``granule_url`` agree.
"""
return PurePosixPath(urlsplit(str(resource)).path).stem

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)

Granule-id collision under write_back + granule_workers > 1 is silent data corruption, not just a manifest problem — empirically confirmed. Issue #180 explicitly asked whether the stem key is collision-safe; the docstring answers the URL-form-variance half (stems survive rewrites) but not the other half: two distinct granules whose URLs share a basename (.../p1/granule.h5 vs .../p2/granule.h5) collapse to one _pending key. I ran exactly that interleaving over two fixture copies (InlineIndex(write_back=True), alternating groups as granule_workers=2 would):

  • after interleaved reads, _pending holds a single key 'granule';
  • because _chunk_aligned_read_fn uses _pending_for(h5obj) as its chunk-map cache with if path not in pending, granule B's reads are served through granule A's chunk maps — file A's byte offsets applied to file B's ioRequest, i.e. silently wrong data when the files differ;
  • the first finish_granule drains both granules' maps into one merged granule.parquet; the second writes nothing.

Serial is immune (the entry drains between granules), so this is a new K>1 hazard — narrower than but in the same class as the _pending trap the issue named. Real ICESat-2 basenames carry product+datetime+RGT+version, so exposure is low, but the failure mode is silent. Suggest keying the in-memory _pending by the full h5obj.resource string (the store key can stay the stem per the issue #160 convention), or at minimum raising when _pending_for would reuse a key created under a different resource. Should be settled before any K>1 default ships.


Generated by Claude Code

logger = logging.getLogger(__name__)


def _granule_workers(data_source: dict) -> int:

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 K>1 hazards live outside this diff and deserve a line here (or in the PR body) before the A/B flips any default:

  1. sidecar + on_miss: build: the installed h5coro-hidefix 0.3.0 SidecarIndex._miss lazy-initializes its delegate with an unguarded check-then-act (if self._inline is None: self._inline = InlineIndex(...), zagg_backend.py). Two granule threads hitting concurrent misses can each construct a delegate; the loser's already-accumulated _pending maps strand on the orphaned instance, so that granule's manifest is silently written sparse (and the stranded maps never drain). Its module docstring also still states the serial contract ("the worker reads granules serially"). So the PR-body claim "No h5coro-hidefix release required" holds for _cache but not for on_miss: build under granule_workers > 1 — that combination needs either an h5coro-hidefix fix (eager delegate construction) or a documented restriction here.

  2. Rectilinear grids share one pyproj Transformer across pool threads: read_group (hence grid.assignRectilinearGrid._transformer_to_grid, src/zagg/grids/rectilinear.py:648) now runs on K worker threads. Concurrent .transform() on a shared Transformer is only thread-safe from pyproj 3.7.0, and pyproject.toml pins pyproj unversioned — older environments + rectilinear + K>1 risk PROJ-context races (wrong coordinates, not errors). Either raise a pyproj>=3.7 floor on the issue thread (dependency change, §4 discussion) or note the constraint in this docstring. The lazy self._transformer init race itself is benign (idempotent construction).

The deployed HEALPix + inline path is clear of both; these gate the opt-in configurations only.


Generated by Claude Code

Comment thread src/zagg/processing/worker.py Outdated
logger.warning(f" Error processing file {s3_url}: {e}")
else:
yield s3_url, reads, group_errors
for u in islice(urls, 1): # top up: keep ≤ granule_workers in flight

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Nit (throughput, not correctness): the top-up runs only after the yield resumes, so for the entire time the main thread folds granule N — including granule_done flushes under streaming, which call _concat_and_group — only K−1 granules are in flight. Moving the top-up to just after future.result() (before the yield) keeps the promised K in flight through every fold. Bound (≤ K) and fold order are unaffected either way.

Otherwise the loop checks out: no deadlock or starvation under a failing granule (the top-up runs on the except path too), the pool's with block joins stragglers if the generator is abandoned, and each future is consumed exactly once.


Generated by Claude Code

Comment thread src/zagg/processing/worker.py Outdated
for s3_url, reads, group_errors in _iter_granule_reads():
read_errors += group_errors
try:
for _g, chunk in reads:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Nit (terseness, §4): the group tag in reads' (group, carrier) tuples is never consumed — _g is discarded here and nowhere else reads it. Either drop the tag (reads becomes a plain carrier list, and _read_granule's docstring loses a clause) or use it, e.g. in the fold-side warning below for context. Dead payload in the hot loop's return type invites the next reader to wonder what depends on it.


Generated by Claude Code

if read_fn is None:
coarse_data = h5obj.readDatasets(selection_paths)
else:
coarse_data = _read_paths_pooled(

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)

Sizing note for the K-sweep: this pool now composes multiplicatively with the granule pool, and under sidecar with the per-chunk fetch pool too — worst-case in-flight GETs is granule_workers × read_workers × fetch width (K=6 → 6×8×8 = 384) against h5coro S3Driver's max_pool_connections=100, which the h5coro-hidefix _fetch_chunks sizing note budgets for 8×8 = 64 ("realized, not queued"). Excess requests queue in urllib3; with h5coro's 5 s timeout + degrade-to-None, queueing pressure can surface as spurious OSError read failures (i.e. read_errors, not slowdowns) on dense shards. The same note's HTTPDriver caveat (one shared requests.Session) also scales with K.

Worth one line in _granule_workers's docstring (dial read_workers down as K rises) and an explicit eye on read_errors in the A/B, so a K=6 regression isn't misread as a data problem.


Generated by Claude Code

Comment thread src/zagg/config.py
or granule_workers < 1
):
raise ValueError(
f"data_source.granule_workers must be an integer >= 1 (got {granule_workers!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)

Nit (doc parity): read_workers earned a README line when it landed (README.md, "Read fan-out" note around line 122); granule_workers exists only in docstrings. Deferring user-facing docs until the A/B picks a default is defensible — but then say so in the PR body's phases checklist so it isn't just forgotten when the default flips. The guard itself mirrors the read_workers pattern faithfully, including the bool trap, and is tested at both submission and worker — no issues there.


Generated by Claude Code

Comment thread tests/test_processing.py Outdated
calls = {"n": 0}

def factory(*a, **k):
calls["n"] += 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)

Nit (flaky-in-principle, in a concurrency PR of all places): calls["n"] += 1 is a read-modify-write executed concurrently by two pool threads (the factory runs inside _read_granule on zagg-granule threads). A mid-increment GIL switch can lose an update, in which case no call ever sees n == 2, no granule fails, and files_processed == 3 fails the assertion. Vanishingly rare under CPython's switch interval, but trivially hardened: gate on the URL (as test_pooled_group_read_error_counted_not_fatal below does with s3://bad) instead of a shared counter, or use next(itertools.count())/a lock.


Generated by Claude Code

@espg

espg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

/benchmark

@espg

espg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Per espg's direction, the sidecar lazy-init race (review finding 2) gets fixed upstream in h5coro-hidefix. This session's GitHub access is scoped to englacial/zagg only, so I can't open the issue there — the complete issue text is below, ready to paste (or hand directly to the fixing agent).


Title: SidecarIndex._miss: unguarded lazy init of the inline delegate races under concurrent granule reads (on_miss: build)

Body:

Upstream half of a finding on zagg PR #183 (granule-level read concurrency, K granules in flight per worker), review thread: #183 (review)

The race (0.3.0, zagg_backend.py:260-264)

if self.on_miss == "build":
    if self._inline is None:
        from zagg.index.inline import InlineIndex

        self._inline = InlineIndex(write_back=True, store=self.store)
    return self._inline.read_group(...)

_miss does an unguarded check-then-act on self._inline. Today zagg's worker reads granules serially, so this can't race. zagg PR #183 introduces a bounded thread pool that keeps K granules in flight, with read_group/finish_granule called from pool threads — under on_miss: build, two concurrently-missing granules can both observe self._inline is None and construct two InlineIndex(write_back=True) delegates. One assignment wins; the loser granule keeps reading through the orphaned instance for the remainder of its read_group call, accumulating its pending chunk maps there. finish_granule (zagg_backend.py:324-325) forwards only to self._inline — the winner — so the loser's pending maps are never drained and its manifest is silently sparse (missing datasets, no error raised).

Fix

Double-checked locking is sufficient and keeps the lazy import (which avoids a hard/circular zagg import at module load):

  • __init__: self._inline_lock = threading.Lock()
  • _miss:
    if self.on_miss == "build":
        if self._inline is None:
            with self._inline_lock:
                if self._inline is None:
                    from zagg.index.inline import InlineIndex
                    self._inline = InlineIndex(write_back=True, store=self.store)
        return self._inline.read_group(...)

Eager construction at __init__/from_config time would also work but changes the import contract; the lock is the minimal fix.

Two adjacent items to fold into the same change

  1. Protocol docs: the module docstring / finish_granule contract states the worker "reads granules serially and calls finish_granule after each". zagg PR Granule-level read concurrency (K in flight, ordered fold) #183 changes that contract to: granule reads may interleave across threads; per-granule state must be keyed by granule id (full resource URL), and finish_granule(h5obj, granule_url) is called exactly once per granule from the thread that read it. Restate the contract accordingly.
  2. Shared-delegate note: once a single InlineIndex delegate is shared across in-flight granules, its own safety depends on zagg keying _pending per granule — that lands in Granule-level read concurrency (K in flight, ordered fold) #183 (re-keyed to the full resource URL after review). Worth a version note: on_miss: build + concurrent granules requires zagg ≥ the release carrying that PR; older zagg's path-keyed _pending would interleave chunk maps across granules.

self._cache (granule-id-keyed, single-key get/pop per granule) looks safe under the GIL as-is; no other shared mutable state spotted in SidecarIndex.

Test suggestion

A hermetic two-thread test: two granules, neither covered by the store, on_miss: build, a barrier inside a patched InlineIndex.__init__ (or a slow first construction) forcing both threads into the None branch — assert exactly one delegate is ever constructed and both granules' manifests come out complete.


Generated by Claude Code

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Lambda benchmark — d8182ac

⚠️ Worker = stable main, not this PR's code — fork PRs aren't redeployed, so cost/runtime here don't reflect changes to lambda-deployed code.

target obs runtime (s) cost/shard cost/100 km² % timeout mem (MB) % cap
tdigest_healpix_o11_sharded 200,676 88.9 $0.00237 $0.02338 10% 384 19%
tdigest_healpix_o11_inner 200,676 97.2 $0.00259 $0.02558 11% 388 19%
tdigest_healpix_o11_cached 200,676 94.5 $0.00252 $0.02488 11% 345 17%
tdigest_healpix_o10_sharded 939,217 104.4 $0.00279 $0.00687 12% 417 20%
tdigest_healpix_o10_inner 939,217 142.2 $0.00379 $0.00935 16% 417 20%
tdigest_healpix_o10_cached 939,217 147.2 $0.00393 $0.00969 16% 351 17%
tdigest_healpix_o9_sharded 3,112,138 161.0 $0.00429 $0.00265 18% 455 22%
tdigest_healpix_o9_inner 3,112,138 312.3 $0.00833 $0.00514 35% 467 23%
tdigest_healpix_o9_cached 3,112,138 318.7 $0.00850 $0.00524 35% 417 20%

arm64 · 2.0 GB · $1.33334e-05/GB-s · one shard/target · pre-merge runs are not retained.

@espg

espg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Addendum to the parked h5coro-hidefix issue text above (#183 (comment)), per espg: fold this in as a second work item in the same issue/release.


Item 2: process-global chunk-fetch budget (shared executor or semaphore)

Problem. Each granule's read_fn currently gets its own chunk-fetch pool (width from _fetch_workers(data_source)), and zagg's dataset-level read_workers pool multiplies it. Under zagg PR #183 (K granules in flight per worker), total in-flight S3 range-GETs become K × read_workers × fetch-width — at K=6 with current defaults that's ~384 concurrent GETs against the S3 driver's ~100-connection pool (budgeted 64). The overflow queues inside urllib3 where the 5 s timeout can fire, so pool exhaustion masquerades as read_errors in the worker envelope (flagged in the PR 183 review, finding 4).

Fix. Make all chunk GETs across all in-flight granules draw from one process-global bounded pool sized to the connection budget, instead of K independent per-granule pools:

  • either accept a shared ThreadPoolExecutor (or a threading.BoundedSemaphore) at SidecarIndex/read-fn construction time, passed down from zagg through the existing read_fn seam;
  • or hold a module-level semaphore around the GET call sites, width configurable (default ≈ the S3 client's max_pool_connections).

Queueing then happens in our code — orderly, no timeout artifacts — and budget idled by a draining granule automatically flows to the granules still reading, which is exactly the right behavior for tail-granule skew. zagg-side, PR #183 ships a static guard (warn/clamp on the K × workers product); this upstream item is the dynamic fix that makes the static guard unnecessary.

Sizing note. Keep the budget a single number with one obvious default (the boto3/urllib3 max_pool_connections, typically 100 → budget ~64-80); don't try to auto-tune per shard — zagg's fleet A/B at K∈{1,2,4,6} will produce the evidence for a better default if one exists.


Generated by Claude Code

@espg espg marked this pull request as ready for review July 7, 2026 06:27
@espg espg merged commit 8183577 into main Jul 7, 2026
8 checks passed
@espg

espg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

State: complete and green. All four phases plus the in-session espg directives are landed on head a4a93d8: review findings folded (full-URL _pending keys with a same-basename collision test; pool top-up keeps full K in flight), pyproj>=3.7.0 floor (espg-authorized), h5coro-hidefix pinned >=0.3.1/==0.3.1 (upstream _inline_lock race fix verified installed; interim runtime guard removed), the three remaining #179 seams routed through read_fn (range coalescing is the sole carve-out, per the body), and post-#182 main merged with both sides' lambda_handler.py semantics intact (82/82 tests). Full suite: 1511 passed, 26 skipped. CI green across ruff, builds, and tests on 3.12/3.13; ruff bot posted nothing.

Same process note as PR #182: CLAUDE.md §2 would have me flip draft → ready here, but the run config says drafts only — leaving in draft, the flip is yours. Applying waiting; remaining items are espg-side (fleet A/B at K∈{1,2,4,6} — follow-ons captured in #184 — plus the upstream zagg_backend docstring contract update riding the next h5coro-hidefix release).


Generated by Claude Code

@espg espg added the waiting label Jul 7, 2026
@espg espg deleted the claude/180-granule-concurrency branch July 7, 2026 06:50
@espg

espg commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

K-sweep: granule_workers on the 0.18 prod fleet (o9 full NEON AOI, 4 shards, 19,037,802 obs, 0 errors in all 12 arms)

Layer :20 verified by content (h5coro 1.0.5, h5coro-hidefix 0.3.1); recycle=1 so workers are cold; K∈{2,4,6} rows ran under a concurrent 8-arm sweep (mild mutual interference, same direction), K=1 rows rerun isolated.

sidecar + sharded: true (the production fast path)

K worker median read median mem Σ lambda cost
1 (isolated) 141.4 s ~114 s 560 MB 533 s $0.0142
2 93.2 s 590 MB
4 77.9 s ~53 s 616 MB
6 80.6 s 605 MB
  • K=1 on 0.18 is already −25% vs the 0.17 row (e) baseline (189.8 s) — the 0.3.1 coalescing + Selection reads (segment-rate geolocation + link arrays) are serial, uncompiled h5coro on every backend #179 manifest-covered selection.
  • K=4: −59% vs row (e), −83% vs week start (445 s); saturation at K≈4 (fetch budget working as designed); memory essentially flat (+56 MB — nothing like K × granule footprint, thanks to the streaming fold).
  • Recommendation: granule_workers: 4 in the sharded/sidecar production configs. At K=4 the read (~53 s) is closing on agg+finalize — the worker approaches compute-bound, so the bigger-worker experiment (4 GB / 2× vCPU at K=4–6) is now worth one run (Lambda GB-s pricing makes it free if wall halves — espg's utilization criterion is finally testable).

hierarchical (h5coro 1.0.5) + sharded: true — the fallback question

K worker median mem
1 164.5 s 547 MB
4 118.0 s 897 MB

Plain h5coro is only ~16% behind sidecar at K=1 and ~51% behind at K=4 (sidecar's compiled+coalesced reads keep a real lead that grows with K); memory grows faster (cache lines × K).

⚠️ inline regression on 0.18 (default path, inner-codec arms)

inline worker median read median
0.17 (K-less) 323.4 s ~146 s
0.18 K=1 (isolated) 455.0 s ~276 s
0.18 K=4 296.9 s ~111 s

Inline's reads nearly doubled at K=1 — consistent with #179 routing the segment-rate selection reads through the compiled seam, which for inline means per-granule B-tree chunk-map builds for datasets h5coro 1.0.5 reads faster via cache lines (sidecar is immune: manifests cover selection, no walks — hence its simultaneous −25%). K≥2 hides the cost in the totals (latency-bound work the granule pool overlaps) but the ~2× read gap vs sidecar persists at every K, and K=1 is the shipped default. Codec caveat: these inline arms are inner-codec; the read-phase regression (276 vs 146) is codec-independent.

Follow-up issue with the fix menu filed separately. Refs: the #170 final table for all pre-0.18 baselines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants