Switch default annotator to fast (7.0.0); pin frameshift C-terminus parity - #398
Merged
Conversation
…arity Make fast the default effect annotator again, replacing protein_diff. Variant.effects()/VariantCollection.effects() with no explicit annotator= now route through fast. For SNVs/indels/MNVs the two annotators are fully reconciled (parity + divergence suites), so nearly all output is unchanged; any residual divergence now resolves to fast's classification. protein_diff stays available via annotator="protein_diff"/use_annotator, and remains the substrate for the MutantTranscript/splice-outcome/germline machinery. Rationale: fast is the more battle-tested path -- during the protein_diff bring-up it was effectively the correctness oracle protein_diff was reconciled against (#318-#321), and it has the cleaner bug history (#397). Major bump because default behavior changes. Also adds regression pins in test_annotator_divergence_scenarios.py for the frameshift coincidental-shared-suffix class (#396/#397): CFTR p.L127fs (+) and BRCA1 p.R71fs (-) must retain the full novel C-terminus under both annotators. Docs, registry/fast docstrings, and the default-annotator test updated to match. CHANGELOG + version bumped to 7.0.0. Claude-Session: https://claude.ai/code/session_01VNtZRyKZ7u9jiMGPQEbx4c
tests/test_protein_diff_parity.py was a module-level skip with an empty CORPUS and a pass-body test -- named like the parity gate but exercising nothing, and skipped with a stale '#309 has not landed' reason. Replace it with a real harness. Key difference from test_annotator_parity_adversarial.py: that suite compares only effect class + short_description. This one also compares the full mutant_protein_sequence and frameshift/stop-loss shifted_sequence -- the fields class+description miss, and exactly where #396 hid (class and short_description matched while protein_diff dropped a C-terminal residue). Sweeps CFTR (+) and BRCA1 (-) coding windows with SNVs/insertions/deletions (590-entry corpus) plus the curated #396/#397 coincidental-suffix cases, and guards against the empty-corpus vacuous-pass failure mode. Marker description in pyproject.toml updated; the harness now runs by default. Claude-Session: https://claude.ai/code/session_01VNtZRyKZ7u9jiMGPQEbx4c
Contributor
Author
|
Follow-up commit The real harness now compares the full Suite is 1234 passed / 4 skipped (the 4 remaining skips are data/release-dependent coordinate guards, unrelated to this change and identical on |
Re-run the whole test suite with protein_diff as the default annotator on one Python version, in addition to the existing default (fast) run. Any test whose bare .effects() diverges under protein_diff now fails CI -- keeping the two annotators compatible across ALL tests, not just tests/test_protein_diff_parity.py. This is load-bearing now that fast is the default while protein_diff remains the splice/germline substrate. Verified locally: full suite green under both defaults (1234 passed / 4 skipped each). Runs without --cov so it doesn't clobber the coverage file the Coveralls step uploads. Claude-Session: https://claude.ai/code/session_01VNtZRyKZ7u9jiMGPQEbx4c
The gate previously ran a bare 'pytest tests -q --annotator=protein_diff', which failed at collection: some suite modules (e.g. test_collection_filtering) call .effects() at import against the default (latest) genome, and that only collects cleanly under the same xdist runner the default './test.sh' step uses (the serial invocation imports the module in a different order and trips a pre-existing Ensembl-115 dependency). --annotator is a runtime fixture, so collection under ./test.sh --annotator=protein_diff is identical to the default run; only the in-force annotator differs. COVERAGE_FILE isolates this run's coverage from the default run's, which Coveralls uploads. Claude-Session: https://claude.ai/code/session_01VNtZRyKZ7u9jiMGPQEbx4c
test_collection_filtering.py calls variants.effects() at module import using the db_snp_variants fixtures from tests/data.py. Those Variants defaulted to reference_name "GRCh38", which pyensembl resolves to its LATEST known release (currently 115) rather than an installed one. The CI mirror (openvax/ensembl-data) tops out at GRCh38.95, so collection intermittently died with "GTF database needs to be created" whenever the latest-release GTF wasn't present -- a pre-existing flake on main's default run, not something the annotator switch introduced. Pin the three snps to cached_release(81) -- the installed release the downstream transcript-id assertions (ENST00000371321, ...371763, ...464755, ...613244) were written against. Same class of fix as #399.
A bare reference name like Variant(..., "GRCh38") or from_csv(genome="GRCh38") resolves, via pyensembl, to the latest release pyensembl KNOWS about (currently 115), not one that's necessarily downloaded. CI installs GRCh38 only up to release 95 (the openvax/ensembl-data mirror tops out there), so dozens of tests across the suite would try to auto-fetch release 115 from Ensembl's FTP at runtime. When that FTP is slow/unreachable the whole job fails with "GTF database needs to be created, run: pyensembl install --release 115" -- an intermittent, whole-suite flake (seen here on 3.11; the previous run passed only because the auto-download happened to succeed). conftest now transparently caps GRCh38 string resolution at the newest release whose GTF index actually exists on disk, walking down from the resolved release and flooring at 76 (the first GRCh38 release) so it never crosses into GRCh37. Verified: the full suite passes identically under release 95 and 115 (1235 passed, both fast and protein_diff), so this only removes the network dependency -- no expected value changes. Library behavior is untouched; this lives entirely in the test harness. Complements the import-time fix in #400 (data.py snps), covering the remaining runtime bare-"GRCh38" call sites.
Two call sites resolved to pyensembl's latest known release via paths
that bypass the conftest GRCh38 cap (which only intercepts varcode's
string resolution):
- test_variant.py imported pyensembl's module-level `ensembl_grch38`
(its latest release, 115) directly. Pin to cached_release(81), matching
the convention used across the rest of the suite. test_serialization
asserts only roundtrip consistency, not a release number, so no
expected value changes.
- test_effect_classes.py built mouse_genome via
genome_for_reference_name("grcm38"), which resolves to GRCm38 release
102; CI installs GRCm38.95. Pin to cached_release(95, species="mouse").
The asserted transcript (ENSMUST00000021049 / Psmc5) is identical
across releases 95 and 102, so the effect assertion is unchanged.
Completes the sweep of runtime latest-release dependencies alongside the
conftest cap and #400.
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.
Summary
Makes
fastthe default effect annotator again (replacingprotein_diff), and pins the frameshift coincidental-shared-suffix class in the divergence suite. Major version bump to 7.0.0 because default behavior changes.Variant.effects()/VariantCollection.effects()with no explicitannotator=now route throughfast.protein_diffstays available viaannotator="protein_diff"orvarcode.use_annotator("protein_diff"), and remains the substrate theMutantTranscript/ splice-outcome / germline machinery builds on.Why
fastis the more battle-tested path. During theprotein_diffbring-up it was effectively the correctness oracleprotein_diffwas reconciled against (#318–#321, all cases whereprotein_diffwas wrong andfastwas right), and it has the cleaner bug history — most recently #396/#397, where the defaultprotein_diffpath dropped 1–2 residues off a frameshift's novel C-terminus whilefastwas correct.For SNVs / indels / MNVs the two annotators are now fully reconciled (
test_protein_diff_parity.py,test_annotator_parity_adversarial.py,test_annotator_divergence_scenarios.py), so nearly all output is unchanged; any residual divergence now resolves tofast's classification.Changes
registry.py:_DEFAULT_NAME = "fast"+ updated rationale docstrings.test_annotator_divergence_scenarios.pyfor the frameshift coincidental-suffix class (PR: Fix protein_diff frameshift C-terminus truncation on a coincidental shared suffix #396/protein_diff drops novel C-terminal residues on frameshift/stop-loss with a coincidental shared suffix #397): CFTRp.L127fs(+ strand, tail…GYAFSLL) and BRCA1p.R71fs(− strand, tail…VNLLKSY) must retain the full novel C-terminus under both annotators, plus a direct byte-parity check.effect_annotation.md),fast.pydocstring, andtest_default_annotator_matches_active_configurationupdated to match. The codebase was already half-switched —conftest.py,effect_annotation.md, andvariant.pyalready assumedfast.CHANGELOG.md+version.py→ 7.0.0.Verification
test_filter_effects_by_transcript_expressiondeselected — fails identically onmain).--annotator=protein_diffCI parity mode: green.ruff check: clean.Note for reviewers
Making
fastthe default whileprotein_diffremains the roadmap substrate means the default path and the splice/germline path use different classifiers. They're reconciled today, so this is safe — but it makes thefast↔protein_diffparity suite load-bearing. Recommend making that suite (including the--annotator=protein_difffull-suite run) a required CI gate.https://claude.ai/code/session_01VNtZRyKZ7u9jiMGPQEbx4c