refactor: run on zarr's loop and make the pool's buffer unit the stored chunk - #41
Merged
Conversation
…op it borrows Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… zarr's loop Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
emfdavid
commented
Sep 1, 2026
emfdavid
left a comment
Owner
Author
There was a problem hiding this comment.
Awesome simplification
Two docstrings explained the design by contrast -- with a zarr shard, and with the code this PR deletes -- which reads as history to anyone meeting the type for the first time. `_Slot` now leads with what a slot is: the unit of residency, one array's sample-axis chunk held as the stored chunks it is made of, with admission, pinning, the byte budget, eviction and `wait_ready` all working at that granularity so a chunk is never half-resident. The shard contrast and the tile/stored-chunk naming note are gone; naming is #40's job. `_advance` says every transition is decided from the slot's own counters rather than listing where those decisions used to live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JLvuoKdYkGheAfNcmxkur
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #30. Two changes that had to land together, because both rewrite
Scheduler._one:sequencing them separately meant writing the concurrency core twice with a guaranteed
conflict.
1. Orchestration runs on zarr's event loop. There is no second loop, no per-read
bridge, and no per-pass thread churn — gcsfs and obstore are both a plain inline
await.Deleted:
_io,_foreign_loop,_fsspec_io_loop,_run_loop, theinsitu-schedthread,and the per-pass
ThreadPoolExecutor(three of four execution contexts were rebuilt everyepoch).
What made this possible is that
zarr.core.chunk_utils.ChunkTransform.decode_chunk(zarr3.3.0) is synchronous. The async codec pipeline dispatches to the loop's default
executor, so keeping our own decode pool meant claiming that slot — which on a borrowed
loop retunes zarr's concurrency process-wide and then breaks it on close. Calling a sync
decode ourselves means we pass our executor explicitly and never touch the default.
2. The pool's buffer unit is the stored chunk. A slot was one assembled ndarray that
every decoded tile was memcpy'd into; it is now the decoded tiles themselves, adopted by
reference, with placement deferred to
gather. Stored chunks are kept whole — onebuffer unit, one shape.
gatherhas no dispatch at all: a transformed chunk republishes as a one-tile slot(
output_geometryalready sets post-transformchunksto the full inner shape), and apersisted chunk is stored tile-major so a revived chunk is tiled exactly like a fresh one.
There is never an "assembled or tiled?" branch.
DESIGN.md's scatter-assemble was a documented design decision, so perCLAUDE.mdthis went to #30 for discussion before code. That decision is now recorded as superseded
rather than quietly removed.
Built on zarr's abstractions, not a private dict of ndarrays
A deliberate goal, not incidental: the chunked pool speaks zarr's own vocabulary for
"a grid of inner chunks", so the structure stays legible to zarr and to the
decode(out=)conversation upstream.
ArrayGeometry.tile_placementzarr.core.indexing.ChunkProjectionchunk_coords,chunk_selection,out_selection,is_complete_chunk) instead of a private(dst, src)pairzarr.core.chunk_utils.ChunkTransform.decode_chunkV2CodectrapsArrayGeometryinner gridDimensionGridLiketests/test_zarr_indexing_parity.py, unchanged heredecode_and_scatter_chunkis not used, and that is the point: it is the decode+scatterfusion the design note wanted hand-rolled, and a chunked slot removes the scatter entirely,
so there is nothing left to fuse.
Where we deliberately do not map. A
_Slotis not a zarr shard, despite theidentical shape: a shard is a storage unit (one addressable object holding a grid of
chunks), a slot is a residency unit (the stored chunks sharing a sample-axis index,
never stored or addressed as one object). An earlier draft of this PR called it a "decoded
shard"; that was wrong and is corrected. Separately, what this codebase calls a tile is
what its own public API calls a stored chunk — two names for one concept. Both the
duplication and the broader question of aligning our "chunk" (a group, for which zarr has
no word) are split out to
#40, deliberately out of scope:
a rename that wide would hide the functional changes in this diff.
Two zarr facilities deliberately not adopted:
FusedCodecPipeline.read_synctakesByteGetters — it owns the IO. Adopting itsurrenders our scheduler,
max_inflightand back-pressure.decode_chunkis IO-free,which is why it is the one we want.
codec_pipeline.pathis not flipped. Same objection asmallopt:it retunes the substrate under user code. We construct our own
ChunkTransformfrom thearray's declared codecs instead.
Sold as simplicity, not throughput
Wall clock is a null. 4 vCPU (L3 33 MiB) and 16 vCPU / 62 GB, local zarr on NVMe,
file://via obstore, zarr 3.3.0,batch_size=16,block_chunks=8,max_inflight=32,decode_threads=8, 3 epochs, medians of 3 interleaved A/B/A/B reps, fingerprint-verifiedper arm.
mainis the control, measured in the same session:arco_many(256×720×1440, spc 16)ragged(250×721×1440, spc 16)tiled512(512×2048×2048, spc 1)The memcpy this deletes is 7.7–10.1% of process CPU on an inner-tiled store and never
surfaces at wall level, because it was not the bottleneck. No throughput claim is made.
The gather cost is not intrinsic — a controlled sweep varying only the inner tile grid
(same array, same config, warm so decode contention is zero):
Chunked
gatheris faster at coarse grids and loses only at the fine end, which restatesthe chunking guidance already in
docs/tuning.mdrather than indicting the design.#30's acceptance gate
Passes. Consumer stalled 2 s/batch with
prefetch_depth=1so_driveparks in_admit; a second thread hammers an unrelated zarr array through the plain sync API.3 reps each,
mainas the control in the same session:main(private per-pass loop)But the gate as written asked the wrong question. Back-pressure never starved anything
—
_admitparks on a realawait, so it yields the loop. Every actual failure was inclose(), and all three were found only by testing teardown:_shutdowncancelled every task on the loop (asyncio.all_tasksis the whole loop'sset) →
CancelledErrorinside unrelated zarr reads.close()stopped and closed a loop it did not own →Cannot close a running event loop,and zarr's global loop dead for the rest of the process.
cannot schedule new futures after shutdown.
They are races (the same arm produced them 2/3, 3/3 or 0/3 across runs), which is why
they are pinned by
tests/test_loop_ownership.pyrather than left to review. A schedulernow cancels only the tasks it created and tears down nothing shared.
Verified against real GCS
The bridge existed for gcsfs, so
file://alone would not have proved this. Fingerprints(SHA-256 over every batch byte +
sample_indices) are identical tomainon bothbackends and both zarr formats:
gs://insitubatch-bench-insitubatch/adv_sweep_synth128_c64_i128.zarradb1de656eef444badb1de656eef444bgs://weatherbench2/.../1959-2022-6h-128x64_equiangular_conservative.zarr05cc29eee047e0e105cc29eee047e0e1WeatherBench2 ARCO is zarr-v2, whose pipeline is a single
V2Codec(filters, compressor)rather than v3's codec chain — a v3-only sync-decode path would have silently missed our
main weather benchmark. Also verified byte-identical locally on a ragged grid and on
sample_axis=1.For reviewers
Behavior is meant to be unchanged except for the two items under "User-visible changes"
below. Most valuable second look, in order:
Scheduler.close/_shutdown— the whole contract is "own nothing, tear downnothing shared". Worth checking the paths that release from the loop thread
(
tile_write'sfinally, on failure and cancellation) genuinely cannot reach_advance's assembly branch: they leave eitherpending > 0or stateFAILED.ChunkPool._advanceandpool.assembles. Delivery is inline on the tiled path (adict write and a counter) but an executor hop when the pool assembles, because
delivering the last tile also runs the assembly memcpy, the user
chunk_transformandthe mmap write-back. An earlier draft delivered inline unconditionally and ran user code
on
zarr_io— pinned now bytest_user_code_never_runs_on_the_shared_event_loop.slot_charge_bytesis shared by the pool and the auto-sizer, deliberately. Sizingfrom the output shape while charging stored tiles under-provisions the budget and the
pool starves mid-epoch — a hang-shaped failure, not a slow one. It caught me: raising
the charge without raising the sizer broke 5 tests.
gatheruses the source grid, notout_geom.output_geometryalways setschunksto the full inner shape, so
out_geomdescribes a 1-tile grid — right for an assemblingslot, wrong for a tiled one.
Two notes on scope. The starvation numbers are
file://on NVMe, 4 vCPU, a single dataset,and the tiled path; they do not cover a remote store where the loop also carries network
waits. And
decode_pool()sizes on first use, so the warning is the only signal a seconddataset's
decode_threadswas ignored.User-visible changes
.npynow holds(n_tiles, *tile_shape)tile-major instead of one assembled array (file count per chunkunchanged). An existing
cache_dirraises the usual stale-cache error and is rebuilt withreset_stale_cache=True. A version-2 file read as tile-major would be plausible-lookinggarbage, so it is refused rather than reinterpreted.
decode_threadsis process-wide, sized by the first dataset built in the process; alater different value is ignored with a warning. Thread count is a property of the machine,
not of a dataset, and two datasets should not run two pools competing for the same cores.
180×360; 1.997× on a short final outer chunk). The automatic budget accounts for it; a
hand-set
cache_budget_byteson a ragged grid needs to be proportionally larger. Gridsthat divide evenly are unaffected. A
chunk_transformstill receives the logicalclipped chunk and never sees padding.
Independent verification (examples + benchmarks, post-review)
The unit suite is green, but it does not run the examples — CI only lints and typechecks
them — and it does not touch a real object store or a GPU. So the examples and benchmarks
were run directly against
mainas a control, looking for what the tests could miss.Setup. Two boxes: g2-standard-16 (16 vCPU, 62 GB, NVIDIA L4 — the box the tables
above were measured on) and an 8 vCPU / 31 GB box. Local zarr on NVMe,
file://viaobstore, zarr 3.3.0, torch 2.12.0+cu130, identical resolved envs on both arms. Control is
main@6282195, measured in the same session on the same stores. Probe stores rebuiltwith
bench.make_dataset(arco_sq180×360,arco_many45×90,ragged721×1440 @ 180×360with a short final outer chunk,
tiled2048×2048 @ 256×256 spc=1).Correctness: byte-identical to
maineverywhereThe failure mode this refactor can produce is plausible wrong data — a tile placed at the
wrong offset, an edge tile clipped wrong, a revived cache entry read with the wrong layout.
Shapes, dtypes, throughput and "it ran" all pass through that, so every arm is SHA-256 over
every delivered byte plus
sample_indices, sorted into sample order so the digest isshuffle-independent.
17 arms, run on both boxes, 34/34 digests identical to
main: the four geometryquadrants on the plain path; the windowed multi-geometry shape the Earth2Studio adapter
drives (two variables at one anchor +
shift(),sample_range, shuffled 3-way split);chunk_transformon three stores; two-epoch runs (cross-epoch reuse and unpinning);and
cache_dircold/warm/transform pairs. Cold and warm digests match within each arm, sothe tile-major persist round-trips; and
arco_sqandarco_many— same data, differentinner chunking — produce the same digest, so the chunk grid does not leak into delivered
bytes.
Real GCS, both backends. The deleted fsspec bridge is invisible to
file://. OnWeatherBench2 ARCO (
gs://weatherbench2/.../1959-2022-6h-128x64_equiangular_conservative.zarr,zarr-v2, so the
V2Codecpath rather than a v3 codec chain), obstore and gcsfs × shuffledand unshuffled all give
91df010e652bc478on both arms.Examples, including paths CI never executes.
advectionon torch, JAX, TF and CUDA:persistence RMSE 0.884 in every run on both arms and both boxes — that number is
model-free, a property of the delivered val data, and is the only signal that has ever caught
a loader defect here.
microscopy(sample_axis=2, per-variable chunking) on CPU and CUDA:Otsu IoU 0.386 across all nine runs. Model skill varies run to run (0.46–0.63 on a
4-epoch CNN, on both arms) and is training noise, not loader signal.
transforms(includingthe reshaping
Coarsen, which exercises the one-tile collapse) andfit_scalerproduceoutput identical to
main. The bench smoke grid runs clean across all five engines.The two user-visible claims above, checked directly. A version-2
cache_dirwritten bymainis refused by this branch with the documented stale-cache error namingreset_stale_cache=True, not reinterpreted; passing that flag rebuilds it to the correctdigest. The ragged residency multiplier computes to 1.248× on 721×1440 @ 180×360,
matching the figure quoted above.
slot_charge_bytesis confirmed to be the single functionused by both the auto-sizer (
source.py:226) and the pool's own charge (pool.py:899) —the coupling reviewer note 3 flags. And under-sizing is not hang-shaped in practice:
cache_budget_bytesis a floor (max(user, working_set),source.py:273), and genuineexhaustion raises
Scheduler._starvation's diagnostic rather than hanging.The wall-clock table is load-bearing on
compute_ms— please state itThe warm wall-clock nulls above reproduce, but only at a stated consumer load, and the
same stores on the same box give a very different ratio at zero consumer compute. Warm
medians on
arco_many(45×90, 256 tiles/chunk — the worst quadrant), 16 vCPU,batch_size=16 block_chunks=8 max_inflight=32, 4 epochs, median of the warm epochs, oneprocess per rep,
maininterleaved as the control:maintime.sleep(the harness's own proxy,bench/engines.py:63)gatherruns on the producer thread (source.py:454, insideproduce()) behind aprefetch_depthqueue, so its cost overlaps consumer compute and disappears from wall clockas soon as the consumer leaves the loader either cores or the GIL. The last row is core
starvation from a 16-thread BLAS consumer, not a gather defect — the same workload pinned to
one thread is at parity.
So the
1.004×reported above is right for a GPU-shaped consumer and wrong to read as theloader's own throughput: the warm ceiling on a 256-tile grid is ~2.8× lower (and 1.88×
on 8 vCPU). That is the same effect the gather sweep already discloses at 1.84–2.20×, and it
only reaches wall clock when the loader is the bottleneck. It costs one line to say which
quantity the table reports; without it the table invites exactly the wrong reading.
The mainstream grids are a wash rather than a regression:
arco_sq(16 tiles/chunk) andragged(20) came out 1.18–1.19× slower at zero consumer compute on 16 vCPU and 0.85×faster on 8 vCPU, and the spc=1
tiledstore — where each chunk is gathered once — is0.82×, i.e. faster, on the branch.
Author attestation
have verified the claims made in this description.
Left unchecked deliberately — this was drafted by Claude, and that box is the human
author's to tick.
Checklist
tests/test_loop_ownership.py(6) andtests/test_chunked_slots.py(14) are new; the loop-ownership tests all fail onmain,as do 4 of the chunked-slot tests, for the documented reasons
uv run ruff check src tests bench examples,uv run mypy src bench examplesanduv run pytest -qare green locally (318 passed, 21 skipped)decode_pool,reset_decode_pool,slot_charge_bytes,ChunkPool.assembles,ArrayGeometry.tile_shape/inner_index, andDecodedChunk(which now statesoutright that user code never sees stored-chunk padding)
docs/*.md—docs/architecture.md(one loop, thehandoff diagram, teardown ownership),
docs/tuning.md(process-widedecode_threads,ragged-grid residency),
DESIGN.md(scatter-assemble superseded, M-GCS closed)## UnreleasedinCHANGELOG.mdBatchis still numpy, the hot path is stillO(chunks) (
gatheris O(chunks × tiles-per-chunk), never per-sample), parallelism isstill in the event loop, no dask, no reshard, no
xr.DataArrayChunkPool, the scheduler, and cross-thread readiness → free-threaded runpasses: 3.13t with
PYTHON_GIL=0(assertedsys._is_gil_enabled() is False),99 passed / 2 skipped across pool, lifecycle, chunked-slots, loop-ownership, scheduler,
residency, prefetch, pinned and window
above exist to show wall clock is a null. Setup and a
maincontrol are given for everytable regardless.