fix: handle non-BGZF gzip VCFs from bcftools -Oz in readVcf (#32)#112
fix: handle non-BGZF gzip VCFs from bcftools -Oz in readVcf (#32)#112jmg421 wants to merge 3 commits into
Conversation
Convert \itemize blocks that used \item{label}{desc} syntax to \describe,
and replace empty \item{}{} labels in \describe blocks with the function
signature from the body, resolving all checkRd warnings in isSNV-methods,
PolyPhenDb-class, readVcf-methods, SIFTDb-class, summarizeVariants-methods,
VariantType-class, VCF-class, VcfFile-class, VCFHeader-class, VRanges-class,
and VRangesList-class Rd files.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Checking the source corresponding to this PR |
|
I want eventually to provide a guideline on PRs in our Bioconductor documentation. Here's a basic concern, I want this to be aired openly. My prompt: We have a situation in which 12 or so PRs have been entered, each one addressing an open issue. But the commits in the PRs are cumulative -- the code changes addressing issue 1 are included in the PR addressing issue 2. Is it appropriate to issue a guideline avoiding such a situation? Claude's response: Yes, that's worth a guideline — this is a common workflow mistake and there's an established fix. What's happening: each PR was branched off the previous one (or off a branch that already had prior fixes merged in), rather than off a shared base like main. So PR #2 shows PR #1's commits as part of its diff, PR #3 shows #1 and #2's, and so on. This causes real problems: reviewers on PR #5 have to mentally filter out four other issues' worth of changes to see what's actually new; if PR #2 gets rejected or reworked, every PR stacked on top has to be rebased; merge order becomes rigid (you can't merge #7 before #3 even if #7 is ready and #3 isn't); and if two PRs are merged out of order, you can get duplicate-commit noise or conflicts in the history. A guideline addressing it should specify two things: Each PR branches from the current tip of main (or whatever the trunk is), not from another feature branch, unless the work genuinely depends on unmerged code. When a PR does legitimately depend on another unmerged PR (a real stacked-PR case), the PR description should say so explicitly and the diff view should be scoped to just the new commits — GitHub and GitLab both support comparing against the dependency branch rather than main, and tools like gh pr or Graphite handle stacks properly so reviewers only see incremental changes. So the guideline isn't "never stack PRs" — sometimes sequential dependent work is unavoidable. It's "branch from trunk by default, and if you must stack, mark it as a stack and keep diffs scoped to the delta." Given you've got 12 independent issues, most of these probably didn't need to depend on each other at all — this sounds like everyone just kept branching off the last person's branch out of convenience. |
|
Since the PRs suffer from this cumulative form I am inclined to close them, but there is still some utility in mapping to the associated issues. Since the latest PR does not lead to a passing R CMD check, I will not merge it. I will spend a little more time considering how to close out open issues of significance. |
…ioconductor#32) Regular gzip files (.vcf.gz compressed with gzip/bcftools -Oz) are valid gzip but NOT BGZF, so htslib/Rsamtools rejects them with a confusing error. Fix: add .is_bgzf() (reads 18-byte header, checks the BC extra subfield), .ungzip_vcf() (decompresses to a temp plain VCF), and .prepareFile() which is called by both character readVcf methods before .checkFile(). The temp file is registered with on.exit() in the caller frame so it is cleaned up automatically after readVcf returns. BGZF files, plain .vcf files, and TabixFile inputs are unaffected.
cab8c6c to
bb6ce15
Compare
Problem
readVcf()crashes with a confusing htslib/Rsamtools error when given a.vcf.gzfile compressed withbcftools -Oz(or plaingzip). These files are valid gzip but are not BGZF — they lack theBCextra subfield that htslib requires. The user sees something like:Reported in #32.
Root cause
.checkFile()unconditionally callsTabixFile(x)on any.gzpath. htslib then rejects the non-BGZF file at the C level with no actionable message.Fix
Three new internal helpers, all in
R/methods-readVcf.R:.is_bgzf(path)— reads the 18-byte gzip header and checks for the BGZFBCextra subfield (bytes 13–14). Zero dependencies, pure R..ungzip_vcf(path)— decompresses a regular-gzip VCF to atempfile()plain VCF..prepareFile(x)— called at the top of bothreadVcf(character, ANY)andreadVcf(character, missing)before.checkFile(). If the file is gzip but not BGZF it decompresses to a temp file and tags it withattr(, "tmpfile")so the caller can registeron.exit(unlink(...))in its own frame — ensuring the temp file lives for the fullreadVcfcall and is deleted immediately after.Behaviour
bcftools -Oz/gzipcompressed.vcf.gzbgzipbgzip) compressed.vcf.gz.vcfTabixFileinputTesting
Verified with four inline
Rscriptcases: plain VCF, BGZF, regular gzip, and temp-file cleanup. All pass. Existing unit tests unaffected (pre-existing failures are due to missing optional packagessnpStats,TxDb.*, etc. — not introduced by this PR).Closes #32