Granule-level read concurrency (K in flight, ordered fold)#183
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 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)
- 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_pendingkey; granule B's reads are then served through granule A's chunk maps and the firstfinish_granuledrains both into one merged manifest. Serial is immune. Inline comment onsrc/zagg/index/inline.py:64with the reproduction. sidecar+on_miss: buildfirst-use race in installed h5coro-hidefix 0.3.0 — unguarded lazy init of theInlineIndexdelegate in_misscan 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_cachebut not for this combination. Inline comment onsrc/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
| 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 |
There was a problem hiding this comment.
🤖 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,
_pendingholds a single key'granule'; - because
_chunk_aligned_read_fnuses_pending_for(h5obj)as its chunk-map cache withif 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'sioRequest, i.e. silently wrong data when the files differ; - the first
finish_granuledrains both granules' maps into one mergedgranule.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: |
There was a problem hiding this comment.
🤖 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:
-
sidecar+on_miss: build: the installed h5coro-hidefix 0.3.0SidecarIndex._misslazy-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_pendingmaps 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_cachebut not foron_miss: buildundergranule_workers > 1— that combination needs either an h5coro-hidefix fix (eager delegate construction) or a documented restriction here. -
Rectilinear grids share one pyproj
Transformeracross pool threads:read_group(hencegrid.assign→RectilinearGrid._transformer_to_grid,src/zagg/grids/rectilinear.py:648) now runs on K worker threads. Concurrent.transform()on a sharedTransformeris only thread-safe from pyproj 3.7.0, andpyproject.tomlpinspyprojunversioned — older environments + rectilinear + K>1 risk PROJ-context races (wrong coordinates, not errors). Either raise apyproj>=3.7floor on the issue thread (dependency change, §4 discussion) or note the constraint in this docstring. The lazyself._transformerinit 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
| 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 |
There was a problem hiding this comment.
🤖 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
| for s3_url, reads, group_errors in _iter_granule_reads(): | ||
| read_errors += group_errors | ||
| try: | ||
| for _g, chunk in reads: |
There was a problem hiding this comment.
🤖 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( |
There was a problem hiding this comment.
🤖 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
| or granule_workers < 1 | ||
| ): | ||
| raise ValueError( | ||
| f"data_source.granule_workers must be an integer >= 1 (got {granule_workers!r})" |
There was a problem hiding this comment.
🤖 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
| calls = {"n": 0} | ||
|
|
||
| def factory(*a, **k): | ||
| calls["n"] += 1 |
There was a problem hiding this comment.
🤖 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
|
/benchmark |
|
🤖 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: 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,
|
Lambda benchmark —
|
| 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.
|
🤖 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 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:
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 Generated by Claude Code |
|
🤖 from Claude State: complete and green. All four phases plus the in-session espg directives are landed on head 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 Generated by Claude Code |
|
🤖 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 sidecar +
|
| 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: 4in 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.
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_shardloops 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
ff3481e, hardened in the review foldd8182ac):InlineIndex._pendingwas 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 fullh5obj.resourceURL →{path -> ChunkMap}(the fold upgraded the original granule-id-stem key after the adversarial review reproduced a stem collision:.../p1/granule.h5vs.../p2/granule.h5collapsed 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_granuledrains exactly its granule's entry; theVirtualIndex.finish_granuleprotocol docs now state the concurrency contract.238395b): the loop body ofprocess_shard(H5Coro open → group loop →finish_granule→ close) is extracted into a per-granule_read_granuleclosure 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 boundedThreadPoolExecutor(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. OneH5Coroper granule, created and closed inside its worker thread. Per-granule error semantics preserved. Works under both pooledall_readsandStreamingAggregatormodes. Validated at submission (validate_config) AND at the worker; README entry next toread_workers.f6da016):cpu_secondsadded to the worker envelope next tomax_memory_mb/container_hwm_mbindeployment/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_smeasurable 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 commitf433f09.)ad0f000, completed by11e4361; 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.ad0f000routed the segment-rate geolocation + link-array selection reads (reference_photon_lat/lon,ph_index_beg,segment_ph_cnt) through the backend-supplied compiledread_fn, pooled with the existingread_workersmachinery via_read_paths_pooled(the_read_group_fullpattern).11e4361folded the three remaining planned-path seams (espg-requested): (a)_read_segment_broadcastsgetsread_fnat the planned call site; (b) the cross-level (Phase B) coarse flag + link reads route throughread_fn, pooled; (c) theplan.full_readselectivity fallback forwardsread_fnto_read_group_fullso it stays compiled.read_fn=None(hierarchical baseline) keeps the batchedreadDatasetscalls byte-identical everywhere.d8182ac,d34cede,a4a93d8) — all findings of the adversarial review addressed: (1) full-URL_pendingkeys + collision test; (2) h5coro-hidefix pinned at 0.3.1 (>=0.3.1core,==0.3.1lambda extra,a4a93d8) — the sidecaron_miss: builddelegate lazy-init race is fixed upstream (double-checked lock inzagg_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 documentingTransformerthread-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 forgranule_workers; (8) pooled-failure test gated on URL instead of a racy counter.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), plusruff check/ruff format --checkon every touched file.tests/data/index/atl03_mini.h5produce byte-identical parquet manifests to the serial order;finish_granuledrains 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).tests/test_processing.py::TestGranuleReadPool): pooled vs serial output equality with the slowest granule submitted first; fold order pinned via a_concat_and_groupspy; oneH5Coroper 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_workersguards 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).tests/test_lambda_handler.py):cpu_secondspresent as a float in the result body; a fakedos.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.tests/test_index.py::TestSelectionReads): with the inline backend a planned group read never touchesreadDatasets— selection, data, segment-broadcast, cross-level-filter, andfull_read-fallback reads are all compiled — and rows match hierarchical in every case (thefull_readtest 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.tests/test_rectilinear.py::TestTransformerThreadSafety(pyproj floor asserted at runtime; shared-transformer concurrent use;assignunchanged).Questions for review
ruff check src testsfails onN818forUnknownCapability(src/zagg/registry.py:64), present on main and untouched by this PR (the PR lint bot runs--select=E,F,W,Iso it never fires in CI). Flagging per §4 rather than renaming a public exception in an unrelated PR.zagg_backendmodule docstring still describes the serial worker contract ("the worker reads granules serially") — worth updating in the next h5coro-hidefix release to match theVirtualIndex.finish_granuleconcurrency contract added on the zagg side.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 spuriousread_errors. Documented in_granule_workers's docstring and the README; no hard cap added (the A/B should see the raw behavior — watchread_errorswhen reading the K-sweep results). Add avalidate_configwarning if you want one.sidecarserves the group, so a sparse manifest lacking those datasets sends the group toon_miss(defaultfallback= 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).granule_workersoverride (theaoi_payloadprecedent). 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.buffered.add_readcounted as a per-groupread_errorsincrement; now fold-side failures (realistically only a streamingflushinsidegranule_done) take the granule-level "Error processing file" warn-and-skip path, same as the old outerexcept.add_readis a plain list append (review-verified), so the changed case is unreachable in practice.🤖 Generated with Claude Code
https://claude.ai/code/session_01Wp83fdkH2qhtEvoLgyTWU1