fix: tolerate duplicate VCF header definitions#17
Conversation
📝 WalkthroughWalkthroughAdds lenient VCF header parsing to ChangesLenient VCF Header Parsing and Compression Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
noodles' header parser rejects any redeclared `##INFO`/`##FORMAT`/`##FILTER`/ `##ALT`/`##contig` ID outright (`duplicate INFO ID: BREAKSIMLENGTH`), so `simulate`/`methylate` choked on VCFs that bcftools reads without complaint -- duplicates are routine in output from Manta, DELLY, and hand-merged files. holodeck shouldn't be stricter than bcftools. Read the raw header before handing it to noodles and drop every repeated structured definition, keeping the first (matching bcftools), then re-parse the cleaned header. The record stream is untouched. Compression is detected from the leading magic bytes instead of the file extension, reusing the same opener the methylation reader already used, so the variant reader now also accepts plain-gzip and extension-less inputs. That opener moves from `vcf::methylation` to `vcf` so both readers share one copy.
7b9cf6d to
68919c5
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/vcf/mod.rs (1)
503-523: ⚡ Quick winAdd a dedicated duplicate-
##ALTtolerance test.You cover
INFO/FORMAT/FILTER/contig, butALTis also part of the supported duplicate-structured-header contract and should be pinned by test to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vcf/mod.rs` around lines 503 - 523, Add a new dedicated test function to verify that duplicate ##ALT declarations are tolerated by the parser. Create a test similar to the existing parse_variants_by_contig_tolerates_duplicate_format_filter_contig function, but include duplicate ##ALT header entries in the VCF string (e.g., two ##ALT declarations with the same ID), and assert that the parsing succeeds without errors. This ensures that ##ALT duplicate tolerance is explicitly covered by tests alongside the already-tested ##INFO, ##FORMAT, ##FILTER, and ##contig duplicate handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vcf/mod.rs`:
- Around line 243-247: The deduplication logic in the is_meta condition block is
too broad and will drop any structured metadata line matching the ##...=<ID=...>
pattern, not just the intended structured directives. Modify the logic to first
check if the line contains one of the targeted directive types before attempting
deduplication, so that unrelated structured header definitions are preserved.
Apply this same constraint to all affected deduplication checks (including the
additional occurrences noted in the review).
---
Nitpick comments:
In `@src/vcf/mod.rs`:
- Around line 503-523: Add a new dedicated test function to verify that
duplicate ##ALT declarations are tolerated by the parser. Create a test similar
to the existing
parse_variants_by_contig_tolerates_duplicate_format_filter_contig function, but
include duplicate ##ALT header entries in the VCF string (e.g., two ##ALT
declarations with the same ID), and assert that the parsing succeeds without
errors. This ensures that ##ALT duplicate tolerance is explicitly covered by
tests alongside the already-tested ##INFO, ##FORMAT, ##FILTER, and ##contig
duplicate handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 846ca6fe-6238-44af-bb75-7f3d79c6a8ee
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vcf/methylation.rssrc/vcf/mod.rs
| if is_meta | ||
| && let Some(key) = structured_header_key(&line) | ||
| && !seen.insert(key.clone()) | ||
| { | ||
| log::warn!("dropping duplicate VCF header definition: ##{}=<ID={},...>", key.0, key.1); |
There was a problem hiding this comment.
Constrain deduplication to the intended structured directives.
Current logic deduplicates any ##...=<ID=...> line, which can silently drop unrelated structured metadata. Limit dedup to the targeted directive set so non-target header semantics are preserved.
💡 Proposed fix
- if is_meta
- && let Some(key) = structured_header_key(&line)
- && !seen.insert(key.clone())
- {
+ if is_meta
+ && let Some(key) = structured_header_key(&line)
+ && (matches!(key.0.as_str(), "INFO" | "FORMAT" | "FILTER" | "ALT")
+ || key.0.eq_ignore_ascii_case("contig"))
+ && !seen.insert(key.clone())
+ {
log::warn!("dropping duplicate VCF header definition: ##{}=<ID={},...>", key.0, key.1);
continue;
}Also applies to: 294-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vcf/mod.rs` around lines 243 - 247, The deduplication logic in the
is_meta condition block is too broad and will drop any structured metadata line
matching the ##...=<ID=...> pattern, not just the intended structured
directives. Modify the logic to first check if the line contains one of the
targeted directive types before attempting deduplication, so that unrelated
structured header definitions are preserved. Apply this same constraint to all
affected deduplication checks (including the additional occurrences noted in the
review).
2 participants
Summary
simulate/methylateaborted on any VCF that redeclares a header ID withduplicate INFO ID: BREAKSIMLENGTH(or the FORMAT/FILTER/ALT/contig equivalent). Such duplicates are routine in output from Manta, DELLY, and hand-merged files, and bcftools reads them without complaint — holodeck shouldn't be stricter than bcftools.Root cause: noodles' header parser rejects redeclared
##INFO/##FORMAT/##FILTER/##ALT/##contigIDs outright, andparse_variants_by_contig/validate_vcf_samplefed the file straight to it viabuild_from_path.Fix
open_lenient_vcf_reader: read the raw header before handing it to noodles, drop every repeated structured definition (keeping the first, matching bcftools), then re-parse the cleaned header. The record stream is untouched — only the header is rewritten.simulateandmethylateboth read through it.1f 8b) rather than the file extension, reusing the sameMultiGzDecoder-based opener the methylation reader already used. As a side benefit the variant reader now also accepts plain-gzip and extension-less inputs — previously it was BGZF-and-.gz-only.open_vcf_buf_reader) moves fromvcf::methylationup tovcfso both readers share one copy instead of duplicating the peek-and-decompress logic.Test plan
cargo ci-fmtcleancargo ci-lintclean (clippy pedantic,-D warnings)cargo ci-test— 422 tests pass, including new coverage:parse_variants_by_contigtolerates duplicate##INFO, and duplicate##FORMAT/##FILTER/##contigvalidate_vcf_sampletolerates duplicate definitionsstructured_header_keyunit test: directive/ID extraction, no-ID lines skipped, ID-not-first conservatively skippedvcf/methylate/simulatetests green (the shared opener move is covered by the methylation suite)##INFO=<ID=BREAKSIMLENGTH,...>duplicate that producedduplicate INFO ID: BREAKSIMLENGTHnow runs to completion (simulate -v dup.vcf→ 33 read pairs)Summary by CodeRabbit