Make the shared RLE sequence limit configurable - #660
Open
Shubham-Padkonde wants to merge 2 commits into
Open
Shubham-Padkonde wants to merge 2 commits into
Shubham-Padkonde wants to merge 2 commits into
Conversation
korikuzma
requested changes
Sep 30, 2026
korikuzma
left a comment
Contributor
There was a problem hiding this comment.
@Shubham-Padkonde Thanks for making this PR! I'd like to see the tests changed before approval/merge.
|
|
||
|
|
||
| def _get_rle_seq_limit() -> int | None: | ||
| """Read the maximum optional RLE sequence length from the environment.""" |
Contributor
There was a problem hiding this comment.
I think it'd be good to have docs here too
Suggested change
| """Read the maximum optional RLE sequence length from the environment.""" | |
| """Read the maximum optional RLE sequence length from the environment. | |
| Set `GA4GH_VRS_RLE_SEQ_LIMIT` environment variable to change | |
| the maximum length of optional `ReferenceLengthExpression.sequence` values. | |
| The default is `50` bases. Use a non-negative integer, `0` to omit the optional | |
| sequence, or `none` (case-insensitive) for no limit. | |
| :raise ValueError: If provided value is not an integer or is a negative value | |
| """ |
Contributor
There was a problem hiding this comment.
I'm wondering if we could avoid using subprocess with a full script (it's a little hard to read and I imagine it could be hard to maintain). Could we instead look into mocking the environment variables in our existing tests (such as
and https://github.com/ga4gh/vrs-python/blob/main/tests/extras/test_annotate_vcf.py)?
Contributor
There was a problem hiding this comment.
yeah generally the recommended way to injectj/manage env vars in tests is with the pytest monkeypatch module
https://docs.pytest.org/en/stable/how-to/monkeypatch.html#monkeypatching-environment-variables
This branch has not been deployed
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.
The RLE sequence limit is currently repeated as
50in normalization, translation, and VCF output. Add a sharedGA4GH_VRS_RLE_SEQ_LIMITenvironment setting so these paths agree, as requested in #598.The default remains 50 bases. A non-negative integer selects the limit,
0omits optional sequence values, andnonedisables the limit. The setting is read at import time; invalid values produce an error naming the setting. Explicit normalization/translation arguments still override the default. Documentation includes a CLI example.Validation on Python 3.12/Linux:
Prepared with Codex assistance. Fixes #598.
Running the three affected modules alone produces three existing VCR cassette failures; the same failures reproduce against unchanged source. The full suite above passes.