Harden VA-Spec annotation and CSV exports against shape-dependent failure - #838
Merged
bencap merged 7 commits intoAug 12, 2026
Merged
Conversation
bencap
force-pushed
the
chore/bencap/797/export-hardening
branch
from
August 11, 2026 16:27
e81a941 to
a6880c0
Compare
Coverage Report for CI Build 31627746819Warning No base build found for commit Coverage: 89.335%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Starlette dispatches @app.exception_handler(Exception) from ServerErrorMiddleware, which is installed outside the user middleware stack. Its 500 never passes through CORSMiddleware and so carries no Access-Control-Allow-Origin; the browser rejects it before axios sees the body, and the caller gets an opaque network error instead of an attributable failure. Catch the exception in a pure ASGI middleware installed as the innermost layer, so CORS decorates the response and the correlation id is available to put in the body. Pure ASGI rather than BaseHTTPMiddleware so the NDJSON streaming endpoints are left alone; exceptions raised after the response starts are re-raised, since the status is already committed. The response body uses 'detail' to match every other error path in the app. The handler in server_main stays as a backstop for anything raised outside the middleware.
…stream _stream_generated_annotations caught only MappingDataDoesntExistException, so anything else raised after the first yield ended the body mid-stream. The 200 and its headers went out with the first chunk, leaving the consumer a short file it cannot distinguish from a complete one. Three known raising paths reach it: absent or malformed score data, an unrecognized VRS Allele state type, and an empty post-mapped id list. Classify each variant instead. A failure becomes a record carrying an error object, and serialization is inside the try -- an emitted object that builds and then fails to dump is the same shape-dependent failure, and is what 5c155f4 fixed. A missing mapping stays an expected null so consumers can keep telling the two apart. Outcome counts go to the logs rather than a trailing summary record: every line stays a variant record, the body still holds exactly X-Total-Count lines, and the format stays identical to the public dump's va/{urn}.va.ndjson. The contract change is additive.
…t of variant shapes Annotation tests all ran against one variant shape, so a structural regression that only appears for a particular stored payload passed the whole suite. Two have reached production: a reference-identical variant whose VRS state is a ReferenceLengthExpression (8412508), and a required Extension.value combined with a null baseline score stripped by model_dump(exclude_none=True) (5c155f4). Parametrize the four annotation entry points over 13 mapped-variant shapes and assert one contract per pair: an object that survives serialization and re-validation, or None. Never raises. Reverting either fix above now fails this suite. round_trip_annotation compares emitted JSON to re-emitted JSON rather than comparing objects. VA-Spec declares Statement.hasEvidenceLines as list[EvidenceLine], so a VariantPathogenicityEvidenceLine re-validates as its base class -- verified to lose no data and produce identical JSON, so object equality would report a non-defect on every pathogenicity statement. Shapes for gnomAD records, ClinVar controls, hgvs_g/hgvs_p, and a missing score key are deliberately absent; the first three are never read by the annotation layer, and dataframe validation rejects a score file with no score column. Reasoning is recorded in the module.
The annotation test suite runs against constructed variant shapes. Real data holds shapes nobody thought to construct, and both VA-Spec serialization defects that reached production were of that kind. This walks the corpus and attempts every annotation surface, so those shapes get a chance to fail somewhere other than a user's download. One CSV row per attempted (score set, surface) pair, successes included: a report showing only failures cannot distinguish "nothing broke" from "nothing ran". Outcomes are ok, exception, schema_violation, or skipped, and the exit code is non-zero only for the two that mean something is broken -- most published score sets have no current mapped variants, so failing on skipped would make the exit code useless. The schema check reuses the round-trip helper the conformance tests use. No per-score-set sampling. An earlier draft sampled, but a sampled sweep can only report a score set as unbroken-where-sampled, and measurement put a full run at 43 minutes for 3.4M variants across three surfaces -- the right order for a pre-release check. --max-score-sets bounds how many score sets are looked at; every one it reports on is swept completely. Which exceptions mean "nothing to annotate" rather than "failed to annotate" now lives in EXPECTED_ABSENCE_EXCEPTIONS, since the streaming endpoints and this script both have to draw that line and a sweep that treated an expected absence as a failure would bury real defects in noise.
…le export Running the variant shape list through the CSV row composer found that two post-mapped payload shapes raise out of a cell resolver: a VRS 1.x object, which hgvs_from_vrs_allele refuses deliberately, and an allele carrying no expressions key. The resolvers reach that parse whenever the stored hgvs_g or hgvs_p column is null, which is 3.9M and 3.0M of 4.2M current mapped variants, so the path is live even though neither payload shape occurs in the corpus today. Raised from inside one cell, either exception aborts the entire file -- every other variant in the score set's CSV, and the whole archive build for the public dump. _safe_hgvs_from_post_mapped absorbs both and returns None, which keeps the refusal without spending the caller's download on one variant's shape. Scoped to the CSV resolvers: the worker's mapping and ClinGen jobs call the same helper, and there an unparseable payload should surface rather than blank a cell. Also extends the shape list to the CSV surface. The mock mapped variant is annotation-shaped and left hgvs_g, hgvs_p, hgvs_c, hgvs_assay_level, and vep_functional_consequence unset -- and an unset attribute on a MagicMock is truthy, so the resolvers took the stored-column branch every time and the VRS fallback never ran. hgvs_g and hgvs_p now default to None deliberately, which is what makes the payload matter.
…on surfaces CSV export is a reviewer-facing surface and had never been run across the corpus. It shares its per-row composer with the variant-level CSV, so sweeping it covers the resolvers where every shape-dependent defect so far has lived -- including the one 53a7b6e fixed. One unit of work per score set rather than per variant, since the composer builds the file in a single call, with namespaces from discovery so each score set is exercised over exactly what it can emit. Two checks: composition must not raise, and the output must re-parse to a rectangle of the expected size, which catches a value carrying a delimiter or newline that silently splits a record. On a raise, start and limit bisect to the offending row so the report names a variant rather than a file. The CSV surface is sized by total variants, not by current mapped variants, and is attempted even when a score set has no current mappings: the composer emits a row per variant and outer-joins the mapping, so an unmapped variant still gets a row of NA columns. 2309 of 2850 published score sets have more variants than current mappings, so conflating the two counts would report a false row-count violation across most of the corpus. First full run found 26 published score sets whose CSV emits roughly twice the expected rows: 44,218 variants carry two mapped variants both flagged current, one a legacy record whose flag was never cleared when its replacement was written. The annotation surfaces duplicate those variants too, but count mapped variants and so cannot see it. Data repair tracked separately; the export is deliberately not made to dedupe, which would hide a live problem behind correct-looking output.
bencap
force-pushed
the
chore/bencap/797/export-hardening
branch
from
August 12, 2026 18:27
585868e to
50c3137
Compare
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.
Export paths fail on stored payload shapes no fixture constructs, and they fail in ways
callers cannot see: an opaque network error, a truncated file, or a download that dies
whole. This makes each failure attributable, bounded, and testable.
Changes