Release 0.3.2 (pending full-pipeline validation) - #33
Open
qclayssen wants to merge 93 commits into
Open
Conversation
…11) * add params input for sigrap tools
* update "HIGH_INF_POS" to "HIGH_I_I_POS" in VCF header for latest pcgr version * update constant for pcgr 2.2.5
* change sage vcf header to match 2024 version * move check function * add check in rescue for sage vcf * change for consistencency with pcgr
…arallelisation because OOM hypermutated samples
…'t use more core and so more memory
…e_command. Fixes #26 - Commands now fail immediately instead of continuing silently
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
# Conflicts: # .bumpversion.cfg # docker/Dockerfile.gpgr # pyproject.toml
- parse_genomic_change: guard against double 'chr' prefix if GENOMIC_CHANGE already includes it, matching the defensive check already used in get_annotation_entry_tsv's fallback path - setup_logging: force=True on basicConfig so handler setup is deterministic regardless of prior logging configuration in-process - annotate.py: fix stale comment listing transferred PCGR annotations to match transfer_annotations_somatic's actual info_field_map
TESTING.md and test docstrings still pointed at the deleted tests/test_pcgr_hypermutated.py; update to tests/test_pcgr.py and tests/test_smlv_somatic_report.py. Also drop an unused HEADER import left over from the split.
…rge_vcf_files
Locks the sites-only invariant that keeps bcftools merge safe for the PCGR
hypermutated chunk-merge path: TestMergeVcfFiles builds disjoint sites-only
chunks, runs merge_vcf_files, and asserts no loss/dup, position-sorted, and
indexed output. Guarded with skipUnless(shutil.which('bcftools')) so it runs
in the conda CI env and skips cleanly elsewhere. Documents the rationale with
a NOTE in merge_vcf_files and updates TESTING.md/CHANGELOG.md.
Ports the warn-and-skip fix from oa-only-v2 (fddc7a7), which was never merged into release/0.3.0. PCGR can emit duplicate variant entries in its output VCF when a variant maps to multiple transcripts (sash sample L2600141 hit this). The assert crashed annotate; now duplicates are logged and the first entry is kept, consistent with the TSV-side dedup. Covered by TestGetAnnotationsVcf in tests/test_pcgr_annotation.py — verified to fail with AssertionError against the pre-fix code.
Same bug class as quick-p3k's get_annotations_vcf fix, on the sibling TSV-parsing paths. Both were still unfixed on release/0.3.0: - collect_pcgr_annotation_data (somatic): the tier-aware dedup fix for this existed on another branch but never landed here. Extracted tier normalisation into _normalise_tier(), added _TIER_ORDER, and on a duplicate key keep whichever entry has the more actionable (lower) tier, warning either way. - collect_cpsr_annotation_data (germline): no fix existed anywhere. CPSR TSV rows carry no actionability tier to break ties on, so this uses warn-and-keep-first, matching get_annotations_vcf's approach. Covered by TestCollectPcgrAnnotationData and TestCollectCpsrAnnotationData in tests/test_pcgr_annotation.py. Regression-proofed: 3 of the 5 new tests fail with AssertionError against the pre-fix code (the 2 that don't hit the assert path correctly pass either way).
…ion path report.py's entry() calls pcgr.run_somatic() for hypermutated samples via select_pcgr_variants (tiered selection), but never passed disable_estimates=True — unlike run_somatic_chunk, which always disables estimates for its equally-partial chunked inputs. TMB/MSI computed on a tiered-selection subset are not meaningful and the report gave no indication they were derived from a trimmed variant set. Tracks pcgr_input_trimmed (the same condition already used to decide whether select_pcgr_variants runs at all) and passes it through as disable_estimates. Covered by two assertions in TestEntrySkipsPcgrOnOverflow: the trimmed path gets disable_estimates=True (new test), the untrimmed path keeps disable_estimates=False (extended existing test). Both fail with KeyError against the pre-fix code (disable_estimates kwarg absent).
rescue.py's annotate_existing_sage_calls called util.check_annotation_headers(info_field_map_sage, input_fp) — but input_fp is the DRAGEN VCF being annotated, not the SAGE VCF the check is meant to validate. Introduced in PR #24 (commit 6a93f6b, 2025-11-24), implementing a TODO that explicitly asked to check headers against the SAGE fp. Because check_annotation_headers silently skips any field absent from its target VCF, and the DRAGEN VCF has none of the SAGE_* fields, the check ran but validated nothing. Not on main. Also adds first-ever test coverage for rescue.py and for util.check_annotation_headers, which had none: - tests/test_util.py: TestCheckAnnotationHeaders — matching description passes, mismatched description exits, field absent from target VCF is skipped. - tests/test_smlv_somatic_rescue.py (new): regression test proving the header check now reads sage_vcf_fp. Regression-proofed: against the pre-fix code the test fails (not with a clean assertion failure, but a downstream KeyError from FORMAT/AD access — proving the check silently passed and execution continued straight past it). Updates TESTING.md (rescue.py, get_annotations_vcf, collect_pcgr_annotation_data, collect_cpsr_annotation_data coverage rows were stale from earlier quick tasks) and CHANGELOG.md.
2 tasks
run_somatic_chunk -> merging_pcgr_files unconditionally called util.merge_vcf_files, which shells out to `bcftools merge`. That command requires 2+ inputs, so a sample whose variants fit in a single chunk (no splitting needed) produced a bcftools merge Usage error instead of a valid annotated VCF. Guard on len(pcgr_vcf_files) == 1: copy the single chunk's VCF directly to the expected merged-output path and tabix-index it, matching the indexed .vcf.gz contract that merge_vcf_files normally produces. Two or more chunks still go through the existing bcftools merge -> sort -> index path unchanged.
…nts) Merge resolution strategy: - Version/config (.bumpversion.cfg, pyproject.toml, build.yaml, CHANGELOG): take ours (0.3.2) - constants.py: take ours + add MAX_SOMATIC_VARIANTS rationale comment from #35 review - pcgr.py: take ours (has _normalise_tier, drops --estimate_signatures per sash#57) + incorporate documentation comments from #35 review - util.py: take ours + add logging setup comment - logging_config.py: take ours (force=True fix) - annotate.py, report.py, rescue.py: take ours + add review comments - docs/adr/001-max-somatic-variants-450k.md: new from main (kept) - tests/test_pcgr_hypermutated.py: new from main (kept)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/test_pcgr.py:303
TestMergingPcgrFilesshells out tobcftoolsviautil.execute_command(...), so it will fail in environments wherebcftoolsis not installed. This contradicts the stated goal that the unit test suite should run binary-free, and also makes local runs brittle. Guard this class (or the individual tests) with@unittest.skipUnless(shutil.which('bcftools'), ...)so it cleanly skips whenbcftoolsis unavailable.
class TestMergingPcgrFiles(unittest.TestCase):
TESTING.md:30
- This section says the only binary-dependent exception is
TestMergeVcfFiles, buttests/test_pcgr.py::TestMergingPcgrFilesalso depends onbcftools(it callsbcftools view/index). Update the wording so the documentation matches the actual test suite behavior (and the skip-guarding used for these integration tests).
Tests in `tests/` are pure Python + in-memory `cyvcf2` logic and must run
without any bioinformatics binary installed (no `bcftools`, `pcgr`, `cpsr`,
`vcfanno`, `snpEff`, `gpgr`, VEP). Functions that shell out to those tools are
either mocked/patched in tests or are not unit-tested (see below). The one
exception is `TestMergeVcfFiles`, an integration test that exercises the real
`merge_vcf_files` → `bcftools merge` path; it is guarded with
`@unittest.skipUnless(shutil.which('bcftools'), ...)`, so it runs in the conda
CI env and skips cleanly (never fails) where `bcftools` is absent.
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
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.
Merges
release/0.3.0intomain. Covers bolt 0.3.0 → 0.3.1 → 0.3.2.0.3.2
Fixes (sash #52)
PCGR_MUTATION_HOTSPOT=.treated as truthy in retention check — was preventing tiered filtering from running for any sample with >450k PASS variants (#32)select_pcgr_variantscannot reachMAX_SOMATIC_VARIANTS— logs warning, continues without cancer report, non-PCGR outputs still publish (#32)--estimate_msi/--estimate_tmbin chunked PCGR annotation runs — estimates on partial VCFs are not meaningfuljlumbroso/free-disk-spaceCI action tov1.3.0sage_vcf_fp, notinput_fpTests added:
TestRunSomaticCommandArgs(estimate flags),TestEntrySkipsPcgrOnOverflow,TestSelectPcgrVariantsRaisesOnUnresolvableOverflow,disable_estimatesforwarding inTestRunSomaticChunkArgMapping, duplicate-key regression coverage, bcftools-guarded lossless regression formerge_vcf_files, comprehensive unit tests forbolt/common/pcgr.pyandbolt/util.py0.3.1
Fixes
--estimate_signaturesfrom PCGR somatic invocation — signature analysis is downstream in gpgr/sigrap; keep--estimate_msi/--estimate_tmb(#31, sash #57)merge_vcf_fileswrong output filename —Path.with_suffix()stripped.passcomponenttransfer_annotations_somatic/transfer_annotations_germline— BGZip output could be truncatedsplit_vcfwriting uncompressed.vcfchunks — now.vcf.gzwith tabix indexPCGR_ACTIONABILITY_TIERVCF header description mismatched stored short-form valuespkg_resourcesmissing inbolt:0.3.0-multiqc— addsetuptools <81r-gpgrto 2.3.10.3.0
Prerequisites