feat(streaming): var_fields + variant-windows — Wave B PR-B3/PR-B4 (#304)#325
Merged
Conversation
…dows) Design for the last two items in the streaming variants-output surface (#304): `var_fields` and `with_seqs("variant-windows")`. Corrects three assumptions in the parent Wave B design, based on the landed code: - `format_by_carrier` is unreachable on gvl's streaming path (VCF uses the natively-dense `VcfRecordSource`; genoray only emits carriers for the k-way merge over single-sample VCFs). - FORMAT/dosage has no written parity oracle on VCF/PGEN — `gvl.write` links genoray's index to `variants.arrow` and never persists dosage or custom FORMAT for those sources, so they are structurally SVAR-only. - `assemble_windows_mode` (src/variants/windows.rs) already exists and is Python-free, so PR-B4 is plumbing plus REF bytes, not a new kernel. Consequently `ref` moves into PR-B3 (it is a var_field, and SVAR1 already reads REF then discards it), and PR-B3 splits into B3a (per-variant fields, all three backends) and B3b (per-call FORMAT fields, SVAR1 only with guard-parity elsewhere). Relates to #304, #277. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
11 tasks across three phases, one per PR: - Phase 1 (PR-B3a, Tasks 1-6): N staged INFO columns on DecodedWindow, ride-along columns on VariantsBatch, REF bytes on all three backends, FFI marshaling, the var_fields public surface, and the parity suite. - Phase 2 (PR-B3b, Task 7): SVAR1 per-call FORMAT/dosage fields, gathered by CSR position in the existing walk; VCF/PGEN decline symmetrically. - Phase 3 (PR-B4, Tasks 8-10): generate_variant_windows over the existing assemble_windows_mode kernel, the with_seqs surface, and parity across all four (ref, alt) mode combinations and both token dtypes. Phases 2 and 3 are independent and can run in parallel. Relates to #304, #277. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…304) Code review of f79125b flagged the RecordBackend::generate_variants reference-slice loop (ref_len = alt_len - ilen, pad_char substitution) as untested -- the shipped tests only exercised the gather_alleles plumbing in variants::mod. Add a Rust test covering SNP, insertion, deletion, and deletion-off-contig-end cases against a synthetic contig, asserting the literal REF bytes and offsets. Also rewords a stale comment in stream_engine.rs that mis-cited "Task 3" for SVAR1's still-missing per-window INFO columns; the real follow-on is Wave B PR-B3b (SVAR1 per-call FORMAT/dosage fields).
…ields (PR-B3a, #304) Adds the user-selectable var_fields surface for StreamingDataset's with_seqs("variants") output: available_var_fields/active_var_fields properties, with_settings(var_fields=...) validation, a non-variants-output guard, and the Rust FFI wiring (want_ref on both engines, info_fields on the VCF RecordStreamEngine) that Tasks 1-4 left unreachable from Python. Adaptations from the brief (real code differs from the assumed version): - No _seq_kind_name attribute exists; added a module-level _SEQ_KIND_NAMES inverse map for the non-variants error message. - The written path's `_declared_info_fields` helper referenced in the brief is a genoray VCF instance method, not a module-level gvl helper; added `_declared_info_numeric_dtypes` next to it, scanning cyvcf2 header_iter() for all declared Integer/Float INFO fields (genoray's own helper only filters a fixed candidate tuple). - SVAR1's index.arrow nests declared INFO under a single non-numeric `INFO` Struct column (only a cached `AF` column is ever flat/numeric), so Svar1Backend::generate_variants has no general INFO/index-column gather (info_out is unconditionally empty, deferred follow-up work). Guarded with a clear NotImplementedError in the packing loop instead of a bare KeyError if such a field is ever actually requested. - Name-collision guard: excluded a fixed reserved-name set (the FFI dict's fixed keys: alt/start/ilen/ref/offsets/ref_offsets) from every backend's available_var_fields at the source, so a same-named INFO/index column can never be selected via var_fields in the first place (with_settings' normal "not available" ValueError covers it) -- cheaper and semantically cleaner than validating post-hoc, since those names already mean the builtin fields. Default var_fields=["alt", "ilen", "start"] reproduces today's with_seqs("variants") output unchanged on all three backends. Relates to #304, #277.
…vable fields early (PR-B3a, #304) Code-review fix pass on Task 5's var_fields surface: - Critical: `_RESERVED_VAR_FIELD_NAMES` omitted `alt_offsets`, one of the FFI dict's 7 fixed keys (re-derived from both `next_batch_variants` marshalers). A source INFO/index column named `alt_offsets` was advertised, accepted, and silently clobbered the ALT sequence offsets at read time. Added the missing name; guard now covers both the VCF INFO path and the SVAR1 index-column path. - Important: added a per-backend `servable_var_fields` (narrower than `available_var_fields` for SVAR1, since its Rust engine only gathers `ref` beyond the three defaults) and check it in `with_settings`, so an available-but-not-yet-servable field (e.g. a cached SVAR1 `AF`) raises immediately at configuration time instead of from inside the per-batch packing loop after a full build_engine + producer thread + first window read. - Important: VCF backend construction reuses the already-open `genoray.VCF` for its INFO-header scan instead of building a second one (which eagerly loads the `.gvi` index) -- paid by every VCF-backed StreamingDataset. - Added 3 regression tests (collision, AF-unservable, ref-forwarding smoke). - Minors: hoisted a duplicate index.arrow schema scan, hoisted the allocating `active_var_fields` property out of the per-batch packing loop, made the `ilen` Ragged conditional on it being requested, and updated a stale Rust doc comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ertions (PR-B3a, #304) test_streaming_ref_var_field_matches_written's total counter used atleast_1d(ref[h]).shape[0], which is always 1 for an opaque-string field (REF alleles collapse to one concatenated bytes blob per hap) -- including for empty haps, making the vacuity guard non-functional. Count via .start[h] instead, and additionally compare per-variant REF lengths via the public to_chars()[h].lengths accessor so boundary/count divergences are actually caught, not just total-byte matches. Also: strengthen test_available_var_fields_matches_written with an AF-cached SVAR1 case (the plain streaming_case fixtures never exercise the backend-derived field set), correct its docstring's overstated claim, drop the now single-valued AF parametrize, and fix a faq.md sentence that implied the var_fields output-kind guard fires at with_settings time when it actually fires at iterate time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extends the Wave B PR-B3a var_fields surface with the remaining field
class: per-call FORMAT fields (dosage + any genoray custom Number=G
FORMAT column), SVAR1-only -- VCF/PGEN decline them symmetrically with
the written path, since gvl.write never persists per-call dosage for
those sources.
_Svar1Backend discovers dosage/custom-fmt fields via the written path's
_svar_format_fields helper and extends both available_var_fields and
servable_var_fields (unlike PR-B3a's numeric INDEX columns, these ARE
servable). Only the requested subset crosses into the Rust engine,
split by dtype into new call_field_{f32,i32}[_names] constructor
params, minimizing allocation for the byte-unchanged default path.
Per-call fields are stored parallel to variant_idxs.npy on the SAME
hap-major CSR offsets -- indexed by CSR position, not by variant id
(the opposite of PR-B3a's INFO columns). Svar1Backend::generate_variants
gathers them inside the same keep branch that pushes kept.push(gvi),
using the loop's CSR-position variable, and rides the existing
VariantsBatch.info_out channel (no new FFI).
Gated by test_streaming_svar1_dosage_matches_written (parametrized over
AF filtering, proving the per-call column is compacted by the SAME
AF/region keep mask as start/ilen, with arange-valued synthetic
dosages that make a CSR-position-vs-variant-id indexing bug fail
loudly) and test_dosage_var_field_rejected_on_record_backends
(VCF/PGEN).
Docs updated: docs/source/dataset.md, skills/genvarloader/SKILL.md,
docs/roadmaps/streaming-dataset.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… (PR-B3b, #304) Review fix for Task 7 (PR-B3b, #304): SVAR1 streaming's build_engine coerced every non-float per-call FORMAT field to int32, breaking byte-identical parity with the written path for genoray's real int16 custom field (mutcat) and leaving the streaming int branch entirely untested. - Widen the Rust FFI's CallVals/InfoVals with an I16 bucket alongside F32/I32 so a registered int16 field crosses into Rust and back out with its exact dtype, not a coerced int32. - Gate _Svar1Backend.servable_var_fields on the field's exact dtype being one of float32/int32/int16 (the only dtypes genoray registers today); anything else stays available but not servable, so with_settings rejects it loudly at configuration time instead of build_engine silently casting it. - Move per-call field memmap + CSR-length validation from __init__ (eager, for every discovered field) to build_engine (lazy, only for requested fields), so a stale/truncated field file no longer blocks opening a store for requests that never touch it. - Add a custom-FORMAT-field streaming-vs-written parity test (dtype-asserting, exercising the new int16 branch) and lock in the AF-filter partial-drop property the dosage test previously left unasserted. - Reword the available-but-not-servable message/docstring (now correctly scoped to SVAR1 index columns like AF, not FORMAT fields) and bring faq.md/dataset.md/SKILL.md/the roadmap up to date. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#304) Code-review fix pass on Task 8's generate_variant_windows (PR-B4, #304): - Important: Svar1Backend::generate_variant_windows no longer silently drops per-call FORMAT/dosage fields (info_out: Vec::new()). Factors the CSR-position gather into a shared gather_call_bufs helper used by both generate_variants and generate_variant_windows, so the two output modes can never diverge on this gather. - Minor: collapse the four duplicated 16-arg assemble_windows_mode call sites (both backends' u8/i32 arms) into one shared variants::windows_tok_bufs helper. - Minor: avoid generate_variant_windows' redundant ALT-byte gather by reusing windows::gather_starts_ilens for scalars.start/ilen instead of assemble_variants_window. - Minor: replace WindowModeConfig's stringly-typed ref_mode/alt_mode i64 fields with a WindowMode enum (None/Window/Allele), making the invalid "out of range mode" state unrepresentable. - Minor: update the streaming-dataset roadmap's PR-B4 entry to reflect the Rust core landing (Python surface + parity suite remain). Adds a Rust test (svar1_generate_variant_windows_gathers_call_fields) that fails against the pre-fix behavior and passes after. Full cargo test suite: 212 passed, 0 failed; all pre-existing generate_variants tests pass unmodified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) Wires Task 8's Rust generate_variant_windows through to Python: adds next_batch_variant_windows to both Svar1StreamEngine and RecordStreamEngine (symmetric marshaling, dtype-preserving u8/i32 token buffers), a shared WindowModeConfig::from_python decode point so the two engines' #[new] constructors can't silently diverge, and StreamingDataset.with_seqs( "variant-windows", opt) on the Python side (requires VarWindowOpt, rejects opt for other kinds, rejects the SVAR2 backend). _iter_batches packs the output as a plain dict[str, Ragged] mirroring _FlatWindow.to_ragged (token buffers, two ragged axes) and _Flat.to_ragged (start/ilen, one ragged axis). Updates skills/genvarloader/SKILL.md and docs/roadmaps/streaming-dataset.md for the new with_seqs kind; docs/source/*.md prose is Task 10's job.
…windows (PR-B4, #304) Code-review fix pass on Task 9 (ebf5d0d): - Convert the branch's new Google-style docstring sections (Args:/Returns:/ Raises: on with_seqs and three earlier var_fields Returns: blocks) to NumPy style, matching the rest of _streaming.py and napoleon_google_docstring = False in docs/source/conf.py. - Reword three Rust doc comments (win_mode field docs on Svar1Backend and RecordBackend, plus a stale "no test accessor" comment) that were made false by Task 9 actually wiring the Python surface through. - Extend the readahead prefetch strategy's existing NotImplementedError guard to also cover with_seqs("variants") and with_seqs("variant-windows"), not just "annotated" -- both previously fell through to generate_batch and silently yielded haplotypes instead of raising. - Strengthen the variant-windows config test to assert real Ragged instances, correct ragged-axis shapes, and matching token dtype, not just dict keys. - Drop the misleading dict[str, Ragged] annotation on a reused `out` name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kends (PR-B4, #304) Closes out Wave B: adds the byte-identical parity gate for StreamingDataset.with_seqs("variant-windows", opt) across all four (ref, alt) mode combinations, both token dtypes, and all three backends (svar1/vcf/pgen), plus an empty-group regression test. Compares whole batches via Ragged.to_padded() rather than per-cell indexing, since seqpro's Ragged does not do independent per-axis fancy indexing on the doubly-ragged (variant count, window length) token fields. Carves out the pre-existing PGEN start-dtype divergence (int64 written vs int32 streaming, present identically on the already-merged "variants" path) from the dtype assertion, still asserting value equality. Updates docs/source/dataset.md, docs/source/faq.md, and skills/genvarloader/SKILL.md for the "variant-windows" streaming surface (including the unphased_union/dummy_variant non-support), and marks PR-B4 + Wave B (#304) complete in docs/roadmaps/streaming-dataset.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t-windows (PR-B4, #304) StreamingDataset has no with_output_format method; the flat-output requirement for with_seqs("variant-windows", opt) only applies to the written Dataset oracle path, not the streaming path. Remove the false streaming-side attribution from dataset.md, faq.md, and the genvarloader skill. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#304) Close the final-review findings for Wave B PR-B3a/B3b/B4: - Add test_streaming_vcf_af_dp_composite_matches_offset_arithmetic, the missing live-decoder gate for src/record_stream/transpose.rs's af_offset arithmetic: request a non-AF INFO var_field (DP) while an AF filter is active on the live VCF backend, so genoray stages info_staged=[AF, DP] and the decoder must read DP at offset 1, not 0. Asserts the ride-along column doesn't perturb the AF keep-mask and that DP values at surviving positions match the fixture's known ground truth. - Fix tests importing VarWindowOpt via the private genvarloader._dataset._flat_variants path; use the public genvarloader.VarWindowOpt everywhere instead. - Add a forward-looking comment on _RESERVED_VAR_FIELD_NAMES noting the variant-windows token-buffer keys must also be covered if/when var_fields ride-alongs are enabled in window mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…(PR-B3a/B4, #304) Wave B PR-B3a/B4's newly-added docstring sections (_declared_info_numeric_dtypes, available_var_fields/servable_var_fields/active_var_fields, with_seqs) were mistakenly converted to NumPy style in f739b1d. main converted the repo to Google style in PR #294 and enforces it via ruff's pydocstyle=google convention; this worktree's stale streaming-branch config (no docstring_style.py, no ruff D rules) masked the regression locally. Revert those 7 sections back to Google style; the 4 pre-existing NumPy docstrings in this file predate this branch and are left alone to avoid needless merge conflicts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d-laub
marked this pull request as ready for review
July 23, 2026 21:17
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.
Completes the streaming variants-output surface for Wave B, byte-identical to the written path (
gvl.write()+Dataset[r, s]atjitter=0). Three stacked sub-PRs onspec/streaming-waveb-b3b4.PR-B3a — per-variant
var_fieldsAFINFO column into N staged INFO columns onDecodedWindow, plus per-variant REF allele bytes, across all three backends (SVAR1 / VCF / PGEN).StreamingDataset.with_settings(var_fields=[...]), and theavailable_var_fields/servable_var_fields/active_var_fieldsproperties. The available set is backend-derived (SVAR1: numeric index columns +ref; VCF: declared numeric INFO fields +ref; PGEN:ref).var_fieldsstays["alt", "ilen", "start"]— every existing caller is byte-unchanged.PR-B3b — per-call FORMAT fields (SVAR1 only)
dosageand custom FORMAT fields, gathered by CSR position parallel tovariant_idxs. Native dtypes preserved end-to-end (InfoVals/CallValswidened with anI16variant; unrepresentable dtypes rejected loudly).PR-B4 —
with_seqs("variant-windows")assemble_windows_modekernel; returnsdict[str, Ragged]. All four (ref, alt) mode combinations, both token dtypes (u8/i32).kept_v_idxsso thevariantsandvariant-windowsmodes select variants identically.Testing
Byte-identical parity suites for every new column/mode against the written oracle: REF (3 backends), INFO Int/Float (VCF), dosage float32 + custom int16 (SVAR1), variant-windows (3 backends × 4 modes × 2 token dtypes), plus empty-group, reserved-name-collision,
af_offsetcomposition, and interior-exclusion cases. All parity tests guard against vacuous passes by counting variants via a numeric field. Full tree 1235 passed / 0 failed;cargo test222 passed.Notable
_RESERVED_VAR_FIELD_NAMESomittedalt_offsets, so a VCF INFO field of that name would clobber ALT sequence offsets viaPyDict::set_item(regression test added).startis int64 on the written path vs int32 streamed — a pre-existing, mode-independent divergence (values equal); the variant-windows parity test scopes a dtype carve-out to exactly that column.Follow-ups (tracked separately, not in this PR)
var_fieldsride-along columns invariant-windowsmode (written path supports it; streaming Rust gather exists but the Python windows-packing branch doesn't surfaceinfo_outyet).Relates to #304.
🤖 Generated with Claude Code