Repository navigation
feature: bgen conversion with bgen limix - #507
jeromekelleher merged 22 commits into
Conversation
Integrate indexed, streaming BGEN probability conversion with the shared VCZ writer. Add CLI and Python APIs, optional dependencies, generated fixtures and documentation; explicitly reject unsupported encodings and data models. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve phased haplotype probabilities, variable ploidy, multiallelic genotype probabilities, original missingness, and per-alternate dosage in the shared VCZ pipeline. Remove the initial diploid/biallelic restrictions rather than treating backend-supported models as unsupported. Use bounded lossless staging for raw layout 2 and layout 1, retaining layout-1 independently rounded probabilities exactly through the public bgen-limix Python API. Avoid unsafe layout-1 binding getters and nonportable C-symbol access. Handle records with no haplotypes without decoding nonexistent probabilities. Add independent fixtures, exhaustive layout-1 integer recovery, mixed-model and worker tests, sentinel and VCF comparisons, and updated documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Read every supported encoding directly through bgen-limix, using its native metadata index and public layout property. Delete the downstream genotype rewriting, manual header parsing, special zero-ploidy decoder, and compatibility module. Preserve probability models and existing VCZ outputs. Require bgen-limix 4.11.1, prepared locally in jpfeuffer/bgen-limix commit 7600fbabbd1b6cdef4b266a972c1021960ca5cc1. That version is not published yet, so regeneration of the public registry lock remains release-blocked; keep the existing lock unchanged rather than record machine-local paths or fabricated registry artifacts. Validation uses the actual locally built corrected wheel. Validate 1297 repository tests, 128 BGEN tests with Zarr 3 defaults, strict documentation rendering, package builds, and isolated installed CLI conversions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve published registry artifacts for the native BGEN fixes instead of the unpublished 4.11.1 development version. Update extras, diagnostics and installation documentation. Validate the released binary with the full repository suite, Zarr 3 BGEN tests and isolated installed-wheel CLI conversions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Expose matching CLI and Python controls for retaining probabilities and/or dosage, float64/float32 storage, and Blosc zstd/lz4 levels 0-9. Preserve existing defaults and all ploidy/missingness/phase/variant/sample metadata. Reject invalid options before output creation and skip omitted encoding passes. Compute dosage from native float64 before casting and stamp exact dtype-specific VCZ missing/fill NaNs. Persist configuration in source root metadata and log it. Reuse schema compressors and existing Zarr adapters without changing the shared writer; document the auxiliary-array codec scope and uncertainty/precision tradeoffs. Add focused mixed-model, sentinel, codec, worker, CLI/API parity and pre-rounding dosage regressions. Validate 1325 tests, 156 BGEN tests with Zarr3 defaults, strict docs rendering and isolated installed-wheel CLI conversions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Vendor pinned reference BGEN fixtures and matching GEN, VCF and HAPS oracles with data-specific Boost license notice and SHA256 integrity checks. Exercise offline CI across both Zarr formats, checking full probability and phase vectors, metadata, missingness, ploidy and independent dosage/frequency summaries. Keep the upstream unsorted example unchanged and explicitly test its rejection. Successful example conversions select its first 98 sorted records without re-encoding probability bytes; companion GEN rows use encoding-aware tolerances. Store only the required compressed oracle rows to respect fixture size limits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Disable Git text conversion for hash-pinned Oxford fixtures and retain LF checksum manifests. Reproduced the exact Windows CI failure with core.autocrlf=true; all eight fixture hashes now match after a Windows-style checkout without changing source bytes or checksum assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jeromekelleher
left a comment
There was a problem hiding this comment.
Thanks for this @jpfeuffer - it's awesome!
I've had a scan through the code and my overall impression is that it's great.
However, I worry about performance. The current code is doing a lot of calls to iter_variants, which is unavoidable because bgen is a row-oriented format. I would be interested to see how this does for large files.
I wonder if the right approach here may be to do a two-pass conversion here like VCF. We could convert bgen to ICF easily enough I think, and then the ICF->Zarr part is well worn (perhaps with some updates to account for new fields?)
|
One more thing - I guess we should also test that the vcztools BGEN output is correctly roundtripped by this converter? |
|
Thanks a lot for your input. Indeed I did not put a lot of effort into performance benchmarking. I can try a little bit next week. |
|
The performance of the old sgkit tools was pretty poor. I think it's worth considering converting to ICF, as this should ensure that performance is predictable at the largest scales. |
|
I tried to relieve some restrictions and strict assumptions of the old converter. I guess that is partly the reason why this became more complex. Do you think it is worth it to lift over the full information from the bgen or should we continue with some of the old assumptions and limitations? E.g. only supporting diploid, not storing phased haplotypes etc. |
|
I think starting with a relatively simple baseline which we can then build on in follow-ups would be a great way to go. |
Discover dimensions with bgen-limix 4.13.0 structured metadata, then decode explicit-width float64 batches once and share them across GP, HP, DS, ploidy, missingness and phase. Retain actual sample-derived output widths, exact layout-1 values and dtype-specific VCZ sentinels. Use a specialised block-first writer over the existing partition, finalisation, compression and indexing lifecycle. Write one field/sample-chunk slab at a time and subdivide native batches under an estimated 256 MiB per-worker buffer budget. Keep the native batch stream separate from output encoding for a future optional ICF path; no ICF mode is implemented. Validate probabilities during payload decoding and stage directory/zip output beside its destination until success, leaving no requested partial output on invalid input. Require and lock the actual public bgen-limix 4.13.0 release. Add decode-count, budget subdivision, ownership, padding and failure-cleanup regressions and document resource caveats. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Prefer the conventional input.bgen.metafile index read-only and create a temporary native index only when absent. Reject older indexes, variant-count mismatches and invalid partition counts without overwriting sidecars. Preserve explicit errors for corrupt native headers. Document native metafile versus SQLite BGI formats and the lack of source fingerprints. Cover both layouts, parallel read-only reuse, temporary fallback, stale/mismatched/truncated indexes and unchanged BGI files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extract distribution identity and native capability checks into check_backend, keeping the published bgen-limix>=4.13.0 dependency constraint as the version requirement. Use self.open_reader for the initial BGEN handle as requested, with regression coverage that index creation uses that handle. The preceding block-first conversion already addresses the initial full probability scan: dimensions use metadata-only discovery and numerical validation runs while each payload batch is decoded once across all fields. Preserve explicit rejection of conflicting legacy cbgen installations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Suspended variant generators retained native index handles in exception tracebacks, masking validation failures with Windows sharing violations. Explicitly close generator owners, and always shut down worker pools before removing staging directories, including when a failed future raises during context exit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Wire actual/declared ploidy discovery and explicit probability models through Python and CLI. With a provided bound and explicit model, skip genotype discovery; use compact native batches and validate model/ploidy assumptions during decoding. Depends on locally verified, unreleased bgen-limix read_dimensions/max_actual_ncomb interfaces. Document the publication gate explicitly; do not fabricate a public version or lockfile entry. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use public 4.14.0 artifacts and hashes in the lockfile, update capability errors, and replace the development-only installation gate with the published backend requirement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Parse QCTOOL sample headers and types, select identifiers explicitly, preserve sample order, and retain existing plain-ID compatibility. Validate sidecars before replacing CLI output and document identifiers-only scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Raise extras and test requirements to the stable 4.15 release, refresh public registry artifacts and hashes, and align installation documentation and capability errors. Preserve the fully integrated dimension and Oxford sample support on the fork CI branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep exporter integration outside default unit collection and in a dedicated test-only environment, with missing capabilities failing explicitly. Cover 24 actual CLI matrix cases, full/subset and embedded-NA cases, precision-aware probabilities, independent dosage, metadata and sentinel bits, sample sidecars and native/SQLite index preservation. Document frozen wheel provenance and the blocked remote integration job without inventing a published dependency. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The Oxford example fixture intentionally contains unsorted positions and must be rejected. Use the independent haplotype fixture for successful installed-wheel conversion checks instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Build the approved jpfeuffer/vcztools commit in a dedicated test-only environment with public bgen-limix 4.15.0. Execute all 30 interoperability cases and reject skipped or missing tests, preserving wheel hashes, source revision, environment versions and JUnit evidence as an artifact. Keep ordinary unit dependencies independent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the importer fork checkout override while retaining the immutable test-only exporter source. Document that the exporter PR is not a released runtime dependency; preserve upstream triggers, runners and publication workflows unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@jeromekelleher For now, I went with a lightweight dimension scan followed by direct BGEN to Zarr conversion instead of going through ICF. The scan still has to decompress the BGEN blocks, so “cheap” here means inspecting metadata without decoding the probabilities. ICF would also mean two decompression steps—first BGEN, then the compressed intermediate—plus writing that intermediate to disk. This way, I reread the original BGEN instead and keep the intermediate batches in memory, without needing the whole dataset in RAM. The actual full decoding is only done once now. |
|
I also added a required VCZ - BGEN - VCZ roundtrip job using the exporter changes in sgkit-dev/vcztools#440. For now, it pins an exact commit from that branch so the check is reproducible and doesn’t change underneath this PR. That’s only a test dependency; once the exporter is merged and released, we can switch it to the released version. In general, it should be clear that a roundtrip will introduce this cyclic dependency. Although I assume that the formats will converge enough in the very near future to not depend on too many syncing changes. |
jeromekelleher
left a comment
There was a problem hiding this comment.
Genreally looks great, thanks @jpfeuffer ! A few minor points to resolve and I think we're good to merge.
| ./cli-test/bin/bgen2zarr convert tests/data/bgen/haplotypes.bgen bgen.vcz --max-ploidy 2 --probability-model phased | ||
| ./cli-test/bin/python -m bio2zarr bgen2zarr convert tests/data/bgen/haplotypes.bgen bgen.vcz.zip --dimension-source declared --zarr-format 3 | ||
|
|
||
| bgen-interoperability: |
There was a problem hiding this comment.
Great, thanks for including this. I think we can just leave it out for now until the vcztools general BGEN support has been released. We can open an issue to track it.
|
|
||
|
|
||
| def read_sample_file(path, *, sample_id_column=None): | ||
| """Read identifiers, in row order, from an Oxford/QCTOOL sample file. |
There was a problem hiding this comment.
Could we read this in using pandas I wonder? Line-by-line Python parsing can get slow.
| "BGEN conversion requires bgen-limix>=4.15.0 with the native " | ||
| "batch interface. Upgrade with bio2zarr[bgen]." | ||
| ) | ||
| if not hasattr(cbgen.BgenFile, "read_dimensions"): |
There was a problem hiding this comment.
I don't get why we need to do this - why can't we just pin to bgen-limix>=4.15.0 or whatever version has these capabilities? This is really fiddly code to test and I just don't see the value.
|
|
||
|
|
||
| @contextmanager | ||
| def native_metafile(path): |
There was a problem hiding this comment.
I guess this comes back to a question I had on the vcztools PR - by "native" you mean "as bgen-limix sees it". I think we should rename this to "bgen_limix_metafile" as "native" implies it's part of the BGEN spec or something.
| self._samples = [vcz.Sample(id=s) for s in sample_ids] | ||
| index_path = pathlib.Path(self.metafile_path) | ||
| if index_path.exists(): | ||
| with cbgen.BgenMetafile(self.metafile_path) as index: |
There was a problem hiding this comment.
This metafile compatibility could be abstracted into a function for clarity and testability.
| self.open_reader() as source, | ||
| closing(self.iter_variants(0, self.num_records)) as records, | ||
| ): | ||
| cursor = 0 |
There was a problem hiding this comment.
Can we extract this file-scanning to a function as well please - the init of this class is very long currently.
I'd be curious to see how this works when we've got 10^6 samples - but let's merge you're approach for now. |
Use pandas for Oxford sample bodies with strict row validation, streamline backend setup, and extract metadata and metafile validation helpers. Defer hosted exporter interoperability until a public release while retaining explicit roundtrip tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Avoid Windows CP1252 failures before the Unicode parser regression reaches the UTF-8 importer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
I checked on 218,368 variants × 408,246 samples (human unphased) and it took 100 minutes with 32 workers with around 1GB RAM per worker. But I have no comparison whatsoever if this is good or at least acceptable 😄 but to be honest, 90% of the time goes into zarr writing and compression, so not much we can do but tweaking chunking and compression level there. |
Good enough for me! |
jeromekelleher
left a comment
There was a problem hiding this comment.
Fantastic - thanks @jpfeuffer!
Summary
Closes #417.
Add
bgen2zarr convertandbio2zarr.bgen.convertthrough the existing Source/schema/writer pipeline, usingbgen-limix>=4.15.0,<5without depending on sgkit.Data preservation
Retain unphased GP and phased HP, derive alternate-allele dosage DS without inferring hard calls, and preserve identifiers, allele order, positions, ploidy, missingness and phase. Support layout 1 (biallelic, diploid, unphased) and layout 2, including multiallelic and variable-ploidy data, with distinct VCZ missing/fill values.
Read embedded sample identifiers, explicit ID lists or Oxford
.samplesidecars. Decode each native batch once across output fields, reuse native metafile indexes, and publish staged output only after successful encoding.Storage controls
Support memory, directory and ZIP output with Zarr 2/3; probability/dosage selection; float64/float32; and Blosc zstd/lz4 compression. Add actual/declared dimension discovery or a checked explicit ploidy bound and probability model.
Scope and constraints
Input is one local BGEN file; positions must be nondecreasing within each chromosome. The first allele is treated as REF without reference-genome orientation inference. Oxford sidecars supply identifiers only. ICF conversion is not implemented, and the per-worker buffer budget is not a total-RSS bound.
Exporter interoperability uses the changes in sgkit-dev/vcztools#440. The dedicated integration job currently pins their immutable staging source; this is a test-only dependency, not a released exporter or runtime dependency.