Skip to content

fix(verify): close GNN allele-blindness, missing package data, and a silent mock-fallback report gap - #296

Merged
Gavin-Borges merged 2 commits into
mainfrom
fix/gnn-allele-blind-pseudo-sequence-loading
Aug 27, 2026
Merged

fix(verify): close GNN allele-blindness, missing package data, and a silent mock-fallback report gap#296
Gavin-Borges merged 2 commits into
mainfrom
fix/gnn-allele-blind-pseudo-sequence-loading

Conversation

@Gavin-Borges

Copy link
Copy Markdown
Owner

StructuralPeptideMHCDataset.__init__ (src/verify/structural_gnn.py) loaded its 34-residue MHC pocket-sequence table via a CWD-relative Path("src/verify/mhc_pseudo_sequences.json"). Any process CWD but the repo root - including an installed sestrav package invoked from anywhere else - made that path silently fail to resolve, pseudo_seqs default to {}, and every HLA allele fall through to an identical "A"*34 all-alanine placeholder. generate_canonical_groove_coords never uses its own allele_name argument either, so this table was the only source of allele signal anywhere in the graph: two peptide-allele pairs sharing a peptide would produce byte-identical MHC pocket node features and edge attributes regardless of true allele identity - silently, with only a per-allele logger.warning.

Scope - please read before the diff

src/verify/structural_gnn.py is the SESTRAV-VERIFY evaluation harness (sestrav_evaluator.py's report, gating promote_gnn.py's promotion scorecard). It is the only production importer of StructuralPeptideMHCDataset in the repository (grep-confirmed). The v2.3 production GNN path (src/gnn/, GraphPredictorV2, GINEConv) is a separate module tree with zero allele references and is not affected. GNN promotion remains deferred (GPU-gated) and no certified result in the results ledger passes through this code path.

Three fixes, landed together - none alone closes the gap

1. Path resolution + fail loud on absent-or-empty. The table now resolves via a module-level Path(__file__).resolve().parent / "mhc_pseudo_sequences.json", independent of CWD. An absent file raises FileNotFoundError; a present-but-content-empty table (only the tracked file's own _source documentation key, or truly empty) raises ValueError - the existence check alone would not have caught a gutted-but-present file falling through the identical placeholder path.

2. Package data. The wheel built from this repository shipped zero non-.py data files (verified: git archive HEAD -> 75 entries, none outside .dist-info/), so even a CWD-correct invocation of an installed package still hit the missing-file path. Added [tool.setuptools.package-data] ("src.verify" = ["*.json"]).

3. Silent mock-fallback reporting. sestrav_evaluator.py's evaluate_single_virus already caught a construction failure and fell back to mock predictions - correctly - but did so silently. run_evaluation_pipeline's top-level metadata["use_mock_fallback"] was computed once before the per-virus loop and never updated, so a report could read use_mock_fallback: false while some virus's scores were actually mock. Added a used_mock_fallback flag threaded through both except arms (main-cohort and breakout-mutant) into each virus's own result; the pipeline now recomputes its top-level flag as an OR across every virus after the loop.

Verification

The fix proven end-to-end against a real installed package, not just unit tests: built a real wheel, installed it into a fresh venv (including the [gnn] extra), and ran from a directory with no relationship to the repo:

resolved path: .../site-packages/src/verify/mhc_pseudo_sequences.json
exists: True
peptide nodes identical: True   (same peptide - correct)
MHC nodes identical (should be False): False   (different alleles - correct)

Every guard and every fallback-flag site was individually mutation-tested - reverted in isolation, confirmed its dedicated test fails, then restored and confirmed the suite is green again:

  • Path-resolution + raise-on-absent: reverting to the old CWD-relative/silent-{} logic fails 3 tests.
  • Empty-table guard: disabling it fails test_dataset_raises_on_metadata_only_pseudo_seqs_table (and shows both alleles silently collapsing to the placeholder in the failure log).
  • Main-cohort used_mock_fallback site: reverting fails test_evaluate_single_virus_reports_unplanned_fallback.
  • Breakout-mutant used_mock_fallback site (a separate call site from the main-cohort one - deleting only this one left every existing test green before this PR added a test that isolates it): reverting fails test_evaluate_single_virus_reports_breakout_only_fallback.
  • Pipeline-level recompute block: reverting fails test_run_evaluation_pipeline_reports_per_virus_fallback_in_metadata.

Test suite: test_structural_gnn.py + test_sestrav_evaluator_gnn.py + test_sestrav_evaluator.py + test_hla_pseudo_sequence_tables.py: 38 passed, 2 skipped (PyG-absent-only branches). ruff check / ruff format --check / mypy clean on every touched file. Local integrity harness unchanged: 151 PASS / 0 WARN / 2 FAIL / 7 SKIP (the two FAILs are standing, unrelated owner-owned rulings).

Review note

This PR went through two rounds of independent adversarial review before push. The second round found and this PR fixes: a checkable-false claim in a code comment ("zero non-.py members" - actually 6, all .dist-info/ metadata, now worded correctly), a self-contradicting overclaim (a comment said an absent table makes position/edge-index tensors allele-dependent too, when those never depend on allele identity either way - only node features and edge attributes do), two mutation-testing gaps (the breakout-arm and pipeline-recompute sites above, which the first test pass did not actually pin), the empty-table gap now closed by fix #1's ValueError, and a ruff format nit. It also confirmed the scope boundary above - that src/gnn/'s production path is unaffected - which is now stated explicitly in both this description and the CHANGELOG entry rather than left to be inferred.

…silent mock-fallback report gap

StructuralPeptideMHCDataset loaded its MHC pseudo-sequence table via a
CWD-relative path. Any CWD but the repo root - including an installed
sestrav package invoked from anywhere else - made every allele fall
through to an identical all-alanine placeholder with no error, only a
per-allele warning. generate_canonical_groove_coords never uses its
allele_name argument either, so this table was the only source of
allele signal anywhere in the graph.

Scope: src/verify/structural_gnn.py is the SESTRAV-VERIFY evaluation
harness and the only production importer of this class. The v2.3
production GNN path (src/gnn/, GraphPredictorV2) has zero allele
references and is unaffected; GNN promotion remains deferred and no
certified result passes through this code path.

Three fixes, landed together because none alone closes the gap:
- Resolve the table via Path(__file__).resolve().parent instead of a
  CWD-relative path; raise on an absent OR content-empty table rather
  than silently defaulting to {}.
- Declare [tool.setuptools.package-data] so a built wheel actually
  ships the JSON files - verified separately: the wheel carried zero
  non-.py members before this change.
- sestrav_evaluator.py's except arms already caught a construction
  failure and fell back to mock scores, but did so silently, leaving
  run_evaluation_pipeline's top-level use_mock_fallback flag False
  even when a virus's scores were actually mock. Threaded a
  used_mock_fallback flag through both except arms into each virus's
  result, and recompute the top-level flag as an OR across every
  virus after the per-virus loop.

Twelve new/rewritten tests. Every guard and every fallback-flag site
was individually mutation-tested by reverting it in isolation and
confirming its dedicated test fails, then restoring it.

Signed-off-by: Gavin Borges <gavinmborges1104@gmail.com>
…pseudo-sequence-loading

# Conflicts:
#	CHANGELOG.md
@Gavin-Borges

Copy link
Copy Markdown
Owner Author

Post-review corrections (pushed as dfeead2)

An independent audit of this branch found three defects in the PR's own supporting material. All are fixed; no behavioural code changed, and the wheel was rebuilt and re-inspected after the edits (79 entries, both JSONs still present).

1. A CHANGELOG category error - the most consequential of the three. This branch's original edit changed the [Unreleased] block's ### Security heading to ### Fixed. That reads as a one-line heading rename, but it silently recategorised two pre-existing supply-chain entries - the A5 Dependabot path-guard and the persist-credentials: false change across 19 checkout steps - as "Fixed" rather than "Security", in a public CHANGELOG for a project actively held to OpenSSF criteria. Resolved in the merge with main: ### Security is restored with its entries intact, and the GNN entry now sits under its own new ### Fixed section. Both coexist.

2. A false premise in a tracked config comment. The new [tool.setuptools.package-data] block asserted "setuptools does not include package data by default". That is wrong. Resolved against this repo's own pyproject.toml (setuptools 83.0.0):

apply_configuration(Distribution(), "pyproject.toml")  ->  include_package_data = True
bare Distribution()                                    ->  include_package_data = None

Under pyproject.toml the flag is on by default. What it is on for is files the sdist manifest covers, and this project has neither a MANIFEST.in nor setuptools-scm ([build-system].requires is setuptools + wheel only) - so the manifest covers nothing and the flag has nothing to act on. Declaring the files explicitly is what actually ships them.

The conclusion was right and the stated reason was wrong - instance #5's exact shape from .claude/rules/third-party-claims.md, recurring in a tracked config file. The comment now states the resolved mechanism.

3. A test count overstated in the flattering direction. The entry claimed "Twelve new/rewritten tests". Re-counted by diffing ^def test_ between origin/main and this branch:

File main branch added removed
test_structural_gnn.py 11 14 4 1
test_sestrav_evaluator_gnn.py 5 8 3 0

Seven added, three rewritten, one removed - ten, not twelve. Corrected, and the removed test is now named explicitly (test_dataset_no_pseudo_seqs_file, which pinned the silent-empty-table behaviour this PR deliberately converts into a raise) rather than quietly dropped. A reviewer finds this with one grep -c.

Also confirmed by the audit, unchanged

Every empirical claim in the original PR body re-verified independently: the wheel from main has exactly 75 entries with 6 non-.py members all inside .dist-info/ and no JSON; the branch wheel has 77 with both; and "the two tracked non-.py files under the packaged source trees" is exactly right (git ls-tree over src/, sestrav/, functions/ returns precisely those two).

Merged with main after #294 and #295 landed; conflict was in CHANGELOG.md only and is resolved as described above. Branch scope re-verified as 6 files, no merge leakage. 38 tests pass, ruff clean, integrity harness at 151 PASS / 0 WARN / 2 FAIL / 7 SKIP.

@Gavin-Borges
Gavin-Borges merged commit 9457258 into main Aug 27, 2026
20 checks passed
@Gavin-Borges
Gavin-Borges deleted the fix/gnn-allele-blind-pseudo-sequence-loading branch August 27, 2026 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant