feat(hla_string): port generateHLAString.sh to native Python - #4
Merged
Conversation
Replaces the vendored bash script (and its second, still-VERSION-1.0.0-unpatched copy bundled as package data) with a real Python module: generate_hla_string(), parse_polysolver(), parse_hlahd(), tested directly instead of via subprocess. This closes the gap the bash-only fix (modules PR #256) left open: even after the keystone fix lands (container uses the real neoantigen-utils package instead of a vendored script), the bundled copy of generateHLAString.sh in this package would have kept the original cut -c 1-11 truncation bug, silently reintroducing it. There is now exactly one implementation of this parsing, and it is CI-tested. Behavior is unchanged from the fixed bash version (verified against the same POLYSOLVER/HLA-HD fixtures): three-digit second fields are preserved instead of truncated, HLA-HD *_final.result.txt is supported, unparseable entries warn and are skipped, and an unparseable file exits 1. The generateHLAString.sh console-script name is preserved so existing containers/modules invocations don't need to change. Treated as a hotfix: 1.6.0 -> 1.6.1.
johnoooh
force-pushed
the
feature/hla-string-python-port
branch
from
August 13, 2026 17:05
27848a3 to
1250d2d
Compare
- `-f ""` now errors instead of silently printing usage and exiting 0 -- only an omitted `-f` (args.file is None) does that. An explicitly-passed empty path is an unusable argument, not "no flag given". - A missing/unreadable file now exits 1 with a structured `ERROR: ...` message instead of an unguarded traceback, matching the existing ValueError-on-no-alleles handling. - parse_hlahd now warns (instead of silently dropping) on a line with no allele columns at all -- that can't be a real HLA-HD row, so treating it like an expected, routine class II locus skip (DRB1, DQB1, ...) would hide a misdetected format or corrupted file with no diagnostic trail. Expected class II skips remain silent. - Added docstrings to parse_args, _is_hlahd_format, main, console. 4 new tests (17 total in this file, all passing).
- hla_string: distinguish an omitted -f from an explicit -f "" so the latter fails loudly instead of printing USAGE and exiting 0 - hla_string: wrap open() in try/except so a missing/unreadable file produces the module's structured ERROR message instead of a traceback - hla_string: warn (once, aggregated) when parse_hlahd drops every line via the locus filter, to surface a misdetected/malformed file - hla_string: add missing docstrings on _is_hlahd_format, parse_args, main, console - generate_input: give compute_purity and _assign_frequencies contextualized ValueErrors instead of bare KeyError/IndexError on a malformed summ.json; deliberately still fail fast rather than reintroducing the silent per-node degradation removed in 26df863 - generate_input: derive root-ness in _assign_frequencies from build_topology's `start` flag instead of re-deriving it from the clone id, removing a duplicated signal that could desync - generate_input: extract shared parse_polysolver_allele in hla_string.py and have generate_input's convert_polysolver_hla reuse it instead of reimplementing POLYSOLVER field parsing - generate_input: drop dead X/x/new_x placeholders in build_topology's dict literal (immediately overwritten by _assign_frequencies) - generate_input: document the intentional double validation of top_n_trees between the CLI type and select_top_trees All existing tests pass (137 passed, 1 skipped).
…-port Resolves the hla_string.py conflict by keeping both defenses in parse_hlahd rather than picking one: the per-line "no allele column at all" warning (this branch) and the aggregated "every line dropped by the locus filter" warning (PR #5) catch different failure classes -- a single garbage line mixed into an otherwise-good file vs. a wholly misdetected/empty file -- and neither subsumes the other. Also brings in PR #5's generate_input.py fixes unchanged: contextualized ValueErrors in compute_purity/_assign_frequencies instead of bare KeyError/IndexError, root-ness threaded through build_topology's existing `start` flag instead of re-derived and risking desync, POLYSOLVER field parsing shared via the new hla_string.parse_polysolver_allele instead of reimplemented, and the dead X/x/new_x placeholder removed. Added one test (test_hlahd_total_misdetection_warns_once_aggregated) for the aggregated-warning behavior merged in from PR #5, which had no test coverage. generate_input.py's fixes remain untested -- flagged for Nikhil separately, not a merge-conflict-resolution concern. 142 passed, 1 skipped (pre-existing, unrelated).
PR #5 fixed 5 findings in generate_input.py (contextualized ValueErrors on malformed populations data, root-ness threaded through build_topology's `start` flag instead of re-derived, shared POLYSOLVER field parsing, a dead placeholder removed, documented double-validation) with zero test coverage. Adds it: - compute_purity: root child missing from populations / with an empty cellular_prevalence list now raises the contextualized ValueError, not a bare KeyError/IndexError. - _assign_frequencies: same, for a non-root clone. Also a direct regression test for the root-ness fix itself -- root gets X=1.0 purely from the `start` flag even when clone "0" has no populations entry at all, proving root-ness is no longer re-derived from the clone id. - convert_polysolver_hla: was a closure nested in main(), untestable as written -- hoisted to module level (pure function, no closure state, no behavior change) so it can be exercised directly. Covers the shared-parser path and confirms unparseable input now raises a clear ValueError instead of the old bare IndexError. 150 passed, 1 pre-existing skip.
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
2 participants
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.
feat(hla_string): port generateHLAString.sh to native Python
Summary
Replaces the vendored bash
generateHLAString.sh— including a second, silentlyunpatched copy bundled as package data — with a real Python module. Same output
contract, now with actual unit tests instead of a subprocess shim.
Why
modulesPR #256 fixes a truncation bug ingenerateHLAString.sh(three-digitPOLYSOLVER second fields, e.g.
hla_b_18_177, were being cut toHLA-B18:17—a real allele the patient may not carry, silently mis-scoring the sample) and
adds HLA-HD support. That fix only touched the copy of the script vendored in
the
modulesrepo.This package bundled a second, independent copy of the same script
(
src/neoantigen_utils/data/generateHLAString.sh, exposed via asubprocess-exec'ing shim) — still atVERSION=1.0.0, never patched. Once theR2 keystone fix lands (containers stop using the vendored modules-repo script
and start using this package instead), that bundled copy would have quietly
reintroduced the exact bug we just fixed, from a different vendored location.
Rather than patch two bash scripts and hope they stay in sync, this moves the
one real implementation here, in the CI-tested package, and retires the bash
script entirely (both copies).
Treated as a hotfix: 1.6.0 -> 1.6.1.
What changed
hla_string.py:generate_hla_string(),parse_polysolver(),parse_hlahd()— ported 1:1 from the fixed bash logic (verified against thesame real POLYSOLVER/HLA-HD fixtures used in the modules PR).
main()/console()preserve the exact CLI contract (-f FILE,-v, stdout output,exit 1 + stderr on unparseable input).
src/neoantigen_utils/data/(bundled script + package-data config): removed.generateHLAString.shconsole-script name unchanged — no update needed onthe calling side once containers point at this package.
tests/test_hla_string.py: 13 tests covering both formats, the three-digitfix, duplicate-allele preservation for homozygous samples, unparseable-entry
warnings, empty-input errors, and the CLI entry point.
Tests
Also verified end-to-end against a built wheel in an isolated venv — installed
generateHLAString.shconsole entry point resolves and produces byte-identicaloutput to the fixed bash script for both POLYSOLVER and HLA-HD fixtures.
Follow-up (not in this PR)
modulesPR #256 and the R2 keystone item still need to point theNEOANTIGENUTILS_GENERATEHLASTRINGmodule at this package (once a containerbuild bakes it in — see containers PR #64) instead of shipping its own copy of
the script.