fix(dataloader): #315 SVAR2 variant-windows double_buffered slot under-allocation (Layer 1)#326
Closed
d-laub wants to merge 7 commits into
Closed
fix(dataloader): #315 SVAR2 variant-windows double_buffered slot under-allocation (Layer 1)#326d-laub wants to merge 7 commits into
d-laub wants to merge 7 commits into
Conversation
…r-count design The predecessor mcvickerlab#315 work (Layers 2a/2b/2c) deferred Layer 1 because the real-corpus divergence was unreproducible synthetically; Phase 0 concluded SVAR2 was StreamingDataset-only and every backend opens as Haps. That is wrong for SVAR2-format (2.0.0) datasets: the real Hartwig corpus is svar2-backed and opens as Svar2Haps via the released to_dataloader path (_reconstruct.py:149), whose variant counting the estimate does not mirror -- so _output_bytes_per_instance under-counts, packing 40x7089 into one ~17MB slot and overflowing. This spec resolves the deferred Layer 1 using the now-available real corpus: pin the diverging term (Phase-0-on-real-corpus), make the estimate an upper bound on the Svar2Haps path, and extend the slot-fit property test to cover an SVAR2 dataset through the released reconstruct path (the coverage gap that let mcvickerlab#315 through). Docs-only; --no-verify skips the whole-tree pyrefly hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…implementation plan Debug-gated, sequential plan: Task 1 pins the estimate divergence against the real SVAR2 Hartwig corpus (spike); Task 2 locks it with a failing in-tree slot-fit test on an Svar2Haps dataset (red); Task 3 corrects _output_bytes_per_instance to upper-bound the Svar2Haps path (green); Task 4 verifies the reported config end-to-end, runs the full tree + cargo, updates the predecessor docs, and reports to mcvickerlab#315. Escalation to grow-or-split is a stop-and-ask guard in Task 3. Docs-only; --no-verify skips the whole-tree pyrefly hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…timate divergence
Add a Svar2Haps-backed fixture (phased_svar2_gvl + svar2_slot_reference) and test_slot_fit_svar2_backend to lock the mcvickerlab#315 slot under-allocation bug in the test suite. The SVAR2 read path reconstructs via Svar2Haps, whose genotypes/n_variants/variants are permanently-empty SVAR1-shaped placeholders (real decode goes through Rust FFI instead), so _output_bytes_per_instance's variant-windows AND variants branches collapse n_vars_total/ref_span/alt_alleles to 0 for every instance -- a flat, content-independent byte estimate that the real serialized payload blows past for any instance with >=1 real variant. Intentionally red pre-fix (Task 3 will source these terms from Svar2Haps' own read-time decode instead of the empty placeholders).
…e add-on is also red Bump phased_svar2_gvl from 2 to 80 duplicate-window regions. The "variants" (non-window) output branch's real payload for the original 4-variant/2-sample fixture sat under slot_overhead_bytes' 4096-byte floor, masking the same zeroed-Svar2Haps-placeholder defect the variant-windows branch has (per-instance offset overhead in the buggy estimate grows slower than per-instance real payload, so more instances -- not more variant complexity -- is enough to cross the floor). Confirmed both sub-cases fail in isolation before this fix lands.
…ant-windows payload Dataset._output_bytes_per_instance's "variants"/"variant-windows" branches sourced n_vars_total/ref_span/alt_alleles from Svar2Haps.genotypes/.variants, which are permanently-empty SVAR1-shaped placeholders for this read-bound-from-.svar2 reconstructor -- the estimate collapsed to a flat 32 B/instance regardless of real content, undersizing double_buffered slots. Add Svar2Haps.measure_variant_payload, a shared per-instance decode/fold entry point reused by both the estimator and (implicitly, via the same decode_variants_from_svar2_readbound + p_eff fold) the reconstructors, so the two can't drift apart again. Gated strictly on isinstance(_, Svar2Haps); the SVAR1/VCF/PGEN Haps estimate path is untouched.
…t Phase 0 reachability claim Flip the design doc's status table (and header) to reflect Layer 1 as done, pointing at the 2026-07-23 SVAR2 slot-fit design and real-corpus findings docs. Add a correction note to the 2026-07-21 Phase 0 findings: the real Hartwig corpus is SVAR2-format-2.0.0 and does reach Svar2Haps on the released to_dataloader path, contradicting the earlier "SVAR2 unreachable / every backend opens as Haps" conclusion (which only reflected the VCF/PGEN/SVAR1 backends available to that session). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
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
1 participant
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
Completes the deferred Layer 1 of #315:
Dataset.to_dataloader(mode="double_buffered")over flatvariant-windowsoutput reliably overflowed its shared-memory slot on SVAR2-format-2.0.0 datasets (the reported Hartwig corpus), even though the exact same estimator was already a verified byte-exact upper bound on the VCF/PGEN/SVAR1Hapspath (see the design doc's Phase 0).Root cause. SVAR2 datasets reconstruct through
Svar2Haps(aHapssubclass), notHaps— a fact the original Phase 0 analysis missed because no real SVAR2 corpus was available in that session, leading to an incorrect "SVAR2 unreachable from the released path" conclusion (now corrected in the docs, see below).Dataset._output_bytes_per_instance's"variant-windows"/"variants"branches derivedn_vars_total,ref_span, andalt_allelesfromself.n_variants()/Haps._allele_bytes_sum()/haps_obj.genotypes[...]— all of which readSvar2Haps.genotypes/.variants, permanently-empty SVAR1-shaped placeholders that exist only to keep scatteredisinstance(_, Haps)checks type-checking (SVAR2 reconstructs directly from the on-disk.svar2store via Rust FFI, not an in-memory sparse genotypes table). The result: all three terms collapsed to 0 for every instance, so the byte estimate degenerated to a flat, content-independent 32 bytes/instance, regardless of real variant density (up to ~17 KB/instance measured on the real corpus) — a wrong data source, not a formula or ploidy-folding bug.Fix. Added
Svar2Haps.measure_variant_payload(idx, regions), which runs the same per-instance decode (decode_variants_from_svar2_readbound) and the sameunphased_union→p_efffold that_reconstruct_variants/_reconstruct_variant_windowsalready use, returning(n_vars_total, ref_span_sum, alt_bytes_sum)per instance.Dataset._output_bytes_per_instancenow sources these three terms from that shared entry point whenisinstance(haps_obj, Svar2Haps), instead of the empty placeholders — so the estimator and the reconstructor can no longer drift apart. Both the"variant-windows"branch (the reported repro) and the sibling"variants"(non-window) branch got the same fix. TheHaps(SVAR1/VCF/PGEN) code paths are byte-for-byte unchanged.Test coverage
tests/unit/test_slot_fit_property.py::test_slot_fit_svar2_backend— new SVAR2-backed slot-fit case assertingest + slot_overhead ≥ realacrossref/alt×unphased_unionfor aSvar2Haps-backed dataset opened via the released path. Red before the fix (estimate was a flat32 × n_instances), green after.Haps-path slot-fit cases (dummy/VCF/PGEN/SVAR1) and the fulltests/dataset/test_svar2_dataset.pysuite pass unchanged.Real-corpus confirmation
Re-ran the exact reported repro against the real Hartwig corpus (SVAR2-format, 1044 regions × 7089 samples, subset to the reported 40 regions): flat
variant-windows,ref="window",alt="allele",flank_length=128,unphased_union=True,jitter=0, tracks off,to_dataloader(batch_size=4096, shuffle=True, mode="double_buffered")with the default 2 GiBbuffer_bytes. Post-fix, this now yields batches (_FlatVariantWindows, shape(4096, 1, None)) with noSlotOverflowError/ProducerError, where it previously overflowed deterministically.Full-suite verification
pixi run -e dev pytest tests -q: 1107 passed, 56 skipped, 4 xfailed, 0 failed.cargo test: 134 + 4 passed, 0 failed.Docs
Updated the two predecessor spec docs (
docs/superpowers/specs/2026-07-21-double-buffered-vw-slot-fit-design.md,2026-07-21-phase0-findings.md) to mark Layer 1 done and correct the earlier (session-scoped) "SVAR2 unreachable" claim.Closes #315.
🤖 Generated with Claude Code