Repository navigation
merge: main (#663) into v3 - #681
Merged
Merged
Conversation
models is a module, so models[var["type"]] raised TypeError for every input and the "vrs" format was unusable. Resolve the class via VrsType, returning None for unknown types as intended.
SeqRepo silently truncates out-of-range fetches, so translators minted permanent VRS identifiers for locations that do not exist on their sequence, or reported them as a misleading reference mismatch against ''. Zero-width insertions past the end got no diagnostic at all, and the CNV and vrs paths never fetch sequence. Add _DataProxy.validate_location_bounds, which raises DataProxyValidationError (unconditionally; there is no require_validation escape hatch) when any defined start/end value lies outside [0, len]. start and end are checked independently so circular start > end remains valid, pos == len is a valid insertion point, and undefined Range endpoints are skipped. Call it from _create_allele (hgvs, spdi, beacon, gnomad), from _from_gnomad before validate_ref_seq, from CnvTranslator._from_hgvs, and from _from_vrs. The original input accession is threaded through as sequence_id and namespace-coerced with the new coerce_accession_namespace (shared with derive_refget_accession) so the length lookup is a metadata cache hit, adding no requests.
Second layer for routes the translator checks do not cover: normalize() called directly on a hand-built Allele, the annotator, and denormalize/renormalize round trips. Uses the length SequenceProxy already fetched, so it adds no I/O, and runs before the definite-range early return so those locations are checked too. Fails before bioutils can compute on truncated sequence.
Parameterized cases for validate_location_bounds (stub proxy), the normalize guard (local test SeqRepo), and every translator input path: hgvs g./n./c./p. (including p.Ter<len+1>del), spdi (including a zero-width insertion past the end), gnomad (bounds message rather than reference mismatch, with and without require_validation), beacon, vrs, and CNV hgvs. In-bounds edge cases (terminal residue, insertion at end, circular start > end, indefinite ranges) are accepted. Each translator test uses a fresh REST dataproxy so its cassette is self-contained regardless of test order.
- Import Range at runtime instead of under TYPE_CHECKING; there is no import cycle. Use isinstance(pos, Range) rather than duck-typing .root. - Fix validate_location_bounds docstring: every defined Range member is checked, not just the largest. - Drop the warning log before raising; the error is always raised. - Make the accession coercion helper private. - Remove the passthrough _Translator._validate_location_bounds wrapper and call the dataproxy method directly. - Trim tests that duplicated coverage across the helper, normalize, and translator levels, along with their cassettes.
Call bioutils coerce_namespace directly, which already leaves namespaced identifiers unchanged, and restore derive_refget_accession to its original form. Fold _defined_values into _check_location_bounds, its only caller.
Look up the sequence length by the refget accession already in the allele values instead of threading the input accession through every translator. This is the same metadata lookup normalize makes next, so it adds no request on the default path. Out-of-bounds errors from _create_allele now name the ga4gh:SQ id. test_from_beacon (do_normalize=False) gains the ga4gh metadata request in its cassette.
- Leave _from_vrs input as-is: like the rest of that path, it is not validated or normalized. Test the VrsType lookup fix directly, including the unknown and missing type cases. - Validate CNV bounds against the refget accession, matching the allele paths. - Call validate_location_bounds from normalize instead of importing the private helper, and fold the helper into the method. - Drop redundant bounds test rows and their cassettes; add type hints.
Out-of-bounds errors now name the sequence as given (e.g. GRCh38:1) followed by the refget accession it resolved to, taken from the aliases in the same metadata lookup. Pass the input accession from every translator path again so errors are consistent across formats, and so the length lookup reuses the cached input-id metadata. This restores test_from_beacon.yaml to its original form; bounds cassettes are re-recorded.
validate_ref_seq fetches the reference it compares against, and an out-of-bounds fetch may be silently truncated and misreported as a reference mismatch. Checking bounds in the method itself protects every caller instead of relying on each to check first, so _from_gnomad no longer needs its own call. The bounds error is raised regardless of require_validation, as it already was for gnomAD input.
vrs-python treats every sequence as linear. VRS allows start > end only on a circular sequence, for a location spanning its origin, which vrs-python does not implement. Such inputs were handled inconsistently: pysam raised ValueError, seqrepo-rest-service answered HTTP 422, and the CNV translator built a CopyNumberChange on the inverted location. validate_start_le_end now rejects them with DataProxyValidationError. validate_location_bounds applies it (for a Range, using the lowest possible start and the highest possible end), and _SeqRepoDataProxyBase.get_sequence applies it before reaching the backend. Zero-width intervals (start == end) remain valid.
get_position_and_state fetches the reference for identity and duplication edits before _create_allele validates the location. An out-of-bounds fetch may be silently truncated, and a fetch whose start is after its end fails with the backend's own error, so both are now checked first and reported as DataProxyValidationError.
Some recent work done on clinvar validation surfaced a small set of discrepancies which informed this change. htslib does not enforce coordinates are in bounds for the sequence. This seems like intentional and defined behavior. And pysam mostly delegates to htslib for fetches (but disallows negative coords and `start>end`), and seqrepo delegates to pysam, and vrs-python currently delegates to seqrepo. Entry points that now check bounds: - AlleleTranslator.translate_from with fmt="hgvs", "spdi", "beacon" or "gnomad", or with no fmt, which tries each format in turn. The check runs before the Allele is built. For HGVS identity (`=`) and dup, it also runs before the reference is fetched. On gnomAD it runs before the reference comparison, so the error isn't reported as a reference mismatch. - CnvTranslator.translate_from (fmt="hgvs"). This path never fetches sequence, so it previously had no protection at all. - normalize() called directly on an Allele with a LiteralSequenceExpression state and a SequenceReference. The check runs before any fetch. - vrs-annotate / the VCF annotator. It goes through the gnomAD path, so out-of-bounds records now get VRS_Error instead of allele IDs. - DataProxy.validate_ref_seq now checks bounds before its fetch, and raises the bounds error regardless of `require_validation`. - DataProxy.validate_location_bounds(sequence_id, start, end) is itself a new public method that clients can call. Locations whose start is after their end are also rejected now. VRS allows `start > end` only on a circular sequence, for a location spanning its origin, and vrs-python treats every sequence as linear. ClinVar has a few such expressions, e.g. `NC_000012.11:g.110593351_110576466dup`, which previously gave different results depending on the backend. - validate_location_bounds rejects them; a `Range` is compared by its lowest possible start and highest possible end. This covers all the entry points above. - SeqRepo-backed `get_sequence` rejects them before reaching the backend. Previously the local proxy raised pysam's `ValueError` and the REST proxy raised an `HTTPError` (422); both now raise `DataProxyValidationError`. - normalize() now raises for an Allele whose `Range` start is after its end, where it previously returned the Allele unchanged. At a high level these are the layers and their bounds handling (prior to this branch which changes vrs-python): ##### (begin ai) ## htslib (`faidx_fetch_seq`) (converted to interbase for ease of comparison) ``` s = 'ABCD' (1) fetch(s, 0, 4) -> 'ABCD' (2) fetch(s, -1, 3) -> 'ABC' # negative start clamped to 0 (3) fetch(s, 3, 1) -> 'A' # start clamped down to end (4) fetch(s, 1, 100) -> 'BCD' # end clamped to length (5) fetch(s, 100, 101) -> '' (6) fetch(s, 100, 100) -> n/a # inclusive-end API: can't express a zero-width interval ``` The only signal that anything was clamped is the length htslib reports back for what it actually fetched. The `samtools faidx` command prints a "Truncated sequence" warning to stderr but still exits 0. ## pysam (`FastaFile.fetch`), compared with htslib ``` (1) fetch(s, 0, 4) -> 'ABCD' (2) fetch(s, -1, 3) -> ValueError: start out of range (-1) # new: rejects negatives (3) fetch(s, 3, 1) -> ValueError: invalid coordinates: start (3) > stop (1) # new: rejects reversed (4) fetch(s, 1, 100) -> 'BCD' # passed to htslib; the clamped length it reports is discarded (5) fetch(s, 100, 101) -> '' # same result, but pysam returns it itself; htslib is never called (6) fetch(s, 100, 100) -> '' # zero-width also returned early by pysam ``` ## seqrepo (`SeqRepo.fetch`), compared with pysam No differences: start/end are passed straight through. ``` (1) fetch(s, 0, 4) -> 'ABCD' (2) fetch(s, -1, 3) -> ValueError: start out of range (-1) (3) fetch(s, 3, 1) -> ValueError: invalid coordinates: start (3) > stop (1) (4) fetch(s, 1, 100) -> 'BCD' (5) fetch(s, 100, 101) -> '' (6) fetch(s, 100, 100) -> '' ``` ## vrs-python `main`, local proxy (`SeqRepoDataProxy.get_sequence`), compared with seqrepo No differences: `get_sequence` passes start/end straight through, and nothing else on `main` checks location bounds. The one related check, `validate_ref_seq`, runs only on the gnomAD (VCF-style) input path. It compares the fetched reference with the input REF. - A non-empty REF fails as a misleading "Reference mismatch … correct ref is ''" rather than a bounds error. - With `require_validation=False` (the `vrs-annotate` default), that failure is only a logged warning, and an Allele is still returned. HGVS, SPDI and beacon inputs never compare against the reference, and neither do `normalize()` or `translate_to`. An out-of-bounds location from those paths is accepted silently and given a permanent identifier. That is why the ClinVar issue went unnoticed on SeqRepo. `NP_001346993.1:p.Ter194del` (a protein of 193 aa) parses to a deletion at `[193, 194)`, normalization's fetch returns `''`, and a `ga4gh:VA` Allele is produced. It only surfaced because RefgetStore raised an error on the same fetch. The comparison between the two backends then showed different object types: `ga4gh:VA` on SeqRepo, and on RefgetStore the harness's CNV fallback, `ga4gh:CX`. ``` (1) fetch(s, 0, 4) -> 'ABCD' (2) fetch(s, -1, 3) -> ValueError: start out of range (-1) (3) fetch(s, 3, 1) -> ValueError: invalid coordinates: start (3) > stop (1) (4) fetch(s, 1, 100) -> 'BCD' (5) fetch(s, 100, 101) -> '' (6) fetch(s, 100, 100) -> ''
Brings #663 (sequence coordinate bounds) into v3. Conflicts resolved: - HgvsTools.get_position_and_state keeps v3's _hgvs_pos_to_vrs coordinates and restores #663's bounds checks before the reference is fetched for identity and dup. - CnvTranslator._from_hgvs keeps v3's build_cnv_location and checks the bounds of the location it builds. - The beacon translator stays removed, along with #663's beacon bounds test case and its cassette.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the sequence-coordinate validation changes.
No open issues 🎉
What changed in this PR
Merges #663 into v3, adding sequence-coordinate validation to VRS-Python so invalid locations are rejected before sequence fetching or variant construction.
Changes:
- Adds shared bounds and interval-order checks with consistent
DataProxyValidationErrorreporting. - Applies validation to allele and CNV translation, HGVS reference fetching, and allele normalization.
- Adds regression tests and recorded responses for invalid coordinates, reversed intervals, and valid boundary insertions.
| File | Description |
|---|---|
tests/test_vrs_normalize.py |
Tests bounds validation and open-range normalization. |
tests/test_dataproxy.py |
Tests coordinate limits, interval order, and validation errors. |
tests/extras/test_location_bounds.py |
Adds translator regression tests for sequence bounds. |
tests/extras/cassettes/test_start_after_end[hgvs-g-identity].yaml |
Records metadata for reversed HGVS identity intervals. |
tests/extras/cassettes/test_start_after_end[hgvs-g-dup].yaml |
Records metadata for reversed HGVS duplication intervals. |
tests/extras/cassettes/test_start_after_end[cnv-hgvs-dup].yaml |
Records metadata for reversed CNV duplication intervals. |
tests/extras/cassettes/test_out_of_bounds[spdi-insertion-past-end].yaml |
Supports testing SPDI insertions beyond sequence length. |
tests/extras/cassettes/test_out_of_bounds[hgvs-p-ter-at-length-plus-one].yaml |
Supports testing protein coordinates beyond sequence length. |
tests/extras/cassettes/test_out_of_bounds[hgvs-n-insertion-past-end].yaml |
Supports testing out-of-bounds HGVS insertions. |
tests/extras/cassettes/test_out_of_bounds[hgvs-n-dup-past-end].yaml |
Supports testing out-of-bounds HGVS duplications. |
tests/extras/cassettes/test_out_of_bounds[gnomad-past-end].yaml |
Supports testing gnomAD coordinates beyond sequence length. |
tests/extras/cassettes/test_out_of_bounds[gnomad-past-end-no-require-validation].yaml |
Supports bounds checks with optional reference validation disabled. |
tests/extras/cassettes/test_out_of_bounds[gnomad-negative-start].yaml |
Supports testing negative gnomAD start coordinates. |
tests/extras/cassettes/test_out_of_bounds[cnv-hgvs-copy-number-change-past-end].yaml |
Supports testing out-of-bounds CNV locations. |
tests/extras/cassettes/test_in_bounds[spdi-insertion-at-end].yaml |
Records responses for a valid terminal SPDI insertion. |
src/ga4gh/vrs/utils/hgvs_tools.py |
Validates identity and duplication locations before fetching reference sequence. |
src/ga4gh/vrs/normalize.py |
Checks allele bounds before fetching or range-related early returns. |
src/ga4gh/vrs/extras/translator.py |
Validates allele and CNV locations during translation. |
src/ga4gh/vrs/dataproxy.py |
Introduces shared bounds validation and consistent reversed-interval errors. |
Review effort: Balanced
korikuzma
approved these changes
Oct 28, 2026
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 #663 into
v3