Release 2026.2.7.1 - #842
Open
bencap wants to merge 38 commits into
Open
Conversation
…iewer A calibration's READ rule is stricter than its score set's: publishing a score set does not publish its calibrations, and reading one does not entitle a caller to read the private calibrations hanging off it. Reads that fanned out from a score set to its calibrations left the router's assert_permission boundary behind and served every calibration they found. Introduce Principal and the generic Viewer, so "may this caller see this?" can be asked at the point of fan-out rather than assumed to have been asked upstream. Both default to anonymous, so a caller that constructs one with no argument serves the public view rather than everything in the database. Apply it to the paths that fan out today: VA-Spec annotation building, the recently-published listing, and the public data export. Annotations are emitted per viewer rather than public-only, so an entitled caller still receives evidence drawn from calibrations they may read. Because a VA-Spec statement carries no stable identifier, every emitted annotation now discloses its own scope through a mavedb_calibration_scope extension, so a consumer can tell whether the record is the one anyone would receive or one widened by the requester's access.
…core-set listing
GET /experiments/{urn}/score-sets returns the full score set view model,
which carries score_calibrations, but applied no calibration filter. A
published score set can hold an unpublished calibration, so an anonymous
caller listing an experiment's score sets received every private
calibration's baseline score, threshold ranges, oddsPath ratios and ACMG
criteria.
Filter through ScoreCalibrationViewer, the same rule the score set
detail and recently-published endpoints use.
The filter is applied to the serialized view rather than by reassigning
ScoreSet.score_calibrations. That relationship is mapped with
cascade="all, delete-orphan", so narrowing the ORM collection marks the
withheld rows as orphans and the next flush deletes them; read paths
avoid that today only because the session is built with autoflush=False
and no read handler commits.
The export narrowed its data by assigning filtered lists back onto ExperimentSet.experiments and ScoreSet.score_calibrations. Both relationships are mapped with cascade="all, delete-orphan", so those assignments did not merely shape the dump -- they marked every withheld row as an orphan. This script can flush: with_database_session commits when the command is invoked with --commit, at which point the withheld private calibrations are deleted, along with every published experiment carrying no CC0 score sets and, by cascade, its score sets and variants. Only --dry-run, the default, made this survivable. Validate the experiment sets first, then narrow the resulting ExperimentSetPublicDump views. Pydantic models are throwaway response objects, so narrowing them stages nothing. The ORM graph is now only read: visible calibration ids are collected in a read-only pass and the views are filtered by id. Score set ids for the per-score-set files are now taken from the narrowed views rather than the ORM graph, so the csv/, mapped/ and va/ entries cover exactly what main.json describes.
A calibration's READ rule is stricter than its score set's in both directions: publishing a score set does not publish its calibrations, and owning a score set does not entitle its owner to a community calibration -- one contributed by a non-contributor -- attached to it. The owner-facing endpoints returned the score set wholesale, so creating, updating, uploading to or publishing a score set handed its owner calibrations they cannot fetch directly. Route every ScoreSet response in this module through _score_set_response, which narrows both sub-resources whose rules diverge from the score set's own: calibrations, and the superseding score set. The search routes are the documented exception -- they answer with ShortScoreSet, which carries neither. Centralizing this removes the last four ORM-mutation filters here. ScoreSet.score_calibrations cascades delete-orphan, so narrowing it in place marked the withheld rows as orphans; assigning superseding_score_set = None nulls the other score set's replaces_id, but only once the attribute has been read, which the permission check did immediately before. fetch_score_set_by_urn now returns the score set as it is, which is also what its non-response callers -- supersession lookup and publication -- actually want. _score_set_response is module-private deliberately. A shared response constructor would not cover the CSV, VA-Spec NDJSON or public-dump serializations of the same graph, so the durable fix belongs at the session rather than the response layer.
…e ORM fetch_experiment_set filtered its response by assigning into item.experiments in place. ExperimentSet.experiments is mapped with cascade="all, delete-orphan", so that assignment marked every experiment the caller could not read as an orphan, and any subsequent flush would have deleted those experiments along with their score sets and variants. Verified: the assignment plus a commit drops the experiment count. Nothing flushed on this path, so nothing was lost -- SessionLocal is built autoflush=False and the handler never commits. Both are coincidences rather than guarantees. Build a local list instead, and sort that rather than the ORM collection.
`PermissionResponse` defined no `__bool__`, so the object was always truthy. `routers/collections.py` was the only module testing the response directly rather than reading `.permitted`, which left every permission check in it inert. - 18 association filters kept every member, disclosing the URNs of private score sets and experiments to any caller who could read the collection - 6 roster checks never took their narrowing branch, disclosing the full collection user roster, names and ORCID iDs included, to non-admins and to anonymous readers of public collections Raise `TypeError` from `PermissionResponse.__bool__` so a bare `if has_permission(...)` cannot be written again. Collapse the six duplicated roster blocks into `_narrow_user_roles_for_non_admins` and route `list_my_collections` through it as well. That endpoint held the only working version of the rule, gating on the caller's own collection role; sharing one implementation stops the two endpoints drifting apart again. The rule is `Action.ADD_ROLE`: whoever may add a user to a collection may see who is in it. Four existing tests asserted that editors and viewers see themselves in the roster, which was the bug's behaviour rather than the intended rule, and now expect the narrowed roster that `list_my_collections` has always returned. New tests cover both disclosures from the outside via response bodies, the roster rule parameterized over each contribution role, and the fact that the newly active filters must not delete the association rows they decline to show, since they assign to `delete-orphan` collections.
…nnotation helpers
Move all CSV-generation code for score set variant downloads out of score_sets.py into a new dedicated mavedb/lib/score_set_csv.py module, splitting it into pure functions (column planning, header assembly, row formatting) and DB-bound fetching to keep the logic testable in isolation. - Add parse_clinvar_namespace() to clinvar/utils.py, replacing the inline regex parsing that lived in score_sets.py - Add is_csv_output_null() to mave/utils.py to centralize null-value detection used when formatting CSV output - Move variant_to_csv_row, variants_to_csv_rows, get_score_set_variants_as_csv, and drop_na_columns_from_csv_file_rows into score_set_csv.py, and update routers/score_sets.py and scripts/export_public_data.py to import from the new location - Move corresponding tests into tests/lib/test_score_set_csv.py and add new unit tests for parse_clinvar_namespace and is_csv_output_null
Split the monolithic score_set_csv.py into mavedb/lib/csv/, a package organized by responsibility (namespaces, columns, entries, fetch, annotations, specs, deprecated_params) instead of one file mixing column planning, row formatting, and DB fetching for a single record type. score_set.py and variant.py sit on top as the two record-specific entry points, sharing the namespace/column machinery beneath them. This is the foundation for variant-level CSV export and namespace discovery (following commits): the old module had no way to answer "what does this record have data for" or to serve a single variant, so extending it in place would have meant bolting both concepts onto code that already conflated planning and fetching. - Introduce namespace discovery as first-class: CsvNamespace enum, CsvNamespaceGroup, and available_score_set_csv_namespaces / available_variant_csv_namespaces, replacing the hand-maintained CLINVAR_NS_PATTERN validity check in the router - Add lib/annotation/flatten.py to project a variant's VA-Spec interpretation onto scalar CSV columns, sharing acmg.py's new acmg_evidence_outcome_code() with the VA-Spec evidence-line builder so the two representations can't drift - Thread an optional containing_classification_ids set through functional_classification_of_variant and pathogenicity_classification_of_variant, resolving the long-standing TODOs about O(1) membership checks when classifying many variants - Move parse_clinvar_namespace (clinvar/utils.py) and CSV-null-output detection (mave/utils.py) into the csv package, since both were export-side concerns living outside it - Add drop_unused_hgvs_columns, scores_custom, and score_set namespaces; keep drop_na_columns, include_post_mapped_hgvs, and include_custom_columns working as deprecated aliases via resolve_deprecated_csv_params - Add title to ShorterScoreSet so namespace-discovery responses can label a calibration's owning score set without a second lookup BREAKING CHANGE: score_set_csv.py is removed. Callers importing get_score_set_variants_as_csv or variants_to_csv_rows from it must import from mavedb.lib.csv.score_set / mavedb.lib.csv.columns instead.
…points
Add GET /variants/{urn}/csv and GET /variants/{urn}/csv-namespaces,
mirroring the score-set CSV endpoints but scoped to a single variant.
The variant endpoint widens over the variant's equivalent measurements
(as the annotation endpoints already do), so a calibration belonging
to another score set that also measured this allele is included too,
and emits one row per current measurement with the requested variant
first.
Add GET /score-sets/{urn}/csv-namespaces alongside it, and switch
GET /score-sets/{urn}/variants/data to build its namespace list and
validation from discovery instead of a hand-maintained
_VALID_STATIC_NAMESPACES set and the ClinVar regex check.
- Accept drop_unused_hgvs_columns on the score-set CSV endpoints,
keeping drop_na_columns, include_post_mapped_hgvs, and
include_custom_columns working as deprecated query params via
resolve_deprecated_csv_params
- Ask calibration READ permission separately from score-set READ on
every CSV endpoint, since a private calibration's interpretation is
not implied by being able to read the measurement it applies to
- /score-sets/{urn}/scores now includes both score namespaces
("scores" plus "scores_custom"), matching its historical behavior of
returning every score column the investigator uploaded
- Make _stream_generated_annotations resilient to a single variant's
annotation failing mid-stream: log and emit it as unannotated rather
than raising, since the response has already started and an
unhandled exception would silently truncate the file
Replace the hand-maintained list of ClinVar release namespaces in export_public_data.py with annotation_export_namespaces(), which asks available_score_set_csv_namespaces() what the score set actually has and subtracts the score/count/identity groups already covered by their own files in the dump. The previous list named releases one by one, so it silently emitted all-NA columns for a release never ingested and needed a code change for every new one. Discovery-derived namespaces also add score calibration interpretations to the annotations CSV for the first time, since those are now part of what discovery reports. Update scripts/resources/README.md to document the calibration column group and clarify that the ClinVar and calibration groups vary by score set and should be read from the header rather than assumed fixed.
…ll tests The namespace refactor moved risk into the compatibility layer while the test mass stayed where it had always been, on the shared NA coercion. - Add test_deprecated_params.py. This layer is all that keeps a pre-namespace client working: FastAPI ignores unknown query parameters, so an unmapped old spelling would silently return different columns rather than erroring, and Galaxy calls these endpoints. It is pure and total, so it is specified here rather than reached incidentally through the routers — unit coverage goes from 47% to 100%. Pins the precedence rules (the current name wins, the boolean flags append rather than replace), namespace-append idempotency, the published token spellings, and the RFC 8594 response headers. - Collapse TestVariantToCsvRowNullHandling from 15 tests to 5. Eight of them asserted the value -> NA rule through different namespaces, but every namespace reaches it through the single _value_or_na call in variant_to_csv_row, and test_is_output_null already specifies that rule over 18 values — so those tests could not fail unless it failed too. What remains covers the layer above: building the per-row source a resolver is handed, where absent data has several distinct shapes. Score and count columns are one mechanism selected by RowSource, so they are parametrized together instead of hand-duplicated. Deliberately not added: per-namespace null tests, and registry invariants. test_specs.py already asserts every namespace has a spec, every declared column resolves, every resolver tolerates a None source, and that a namespace reading through a relationship declares the fetch it needs.
get_digest_from_post_mapped is renamed to get_id_from_post_mapped and
now returns the VRS object's `id` field verbatim (e.g.
`ga4gh:VA.{digest}`) instead of the bare `digest`. Only `id` is
indexed by ix_mapped_variants_post_mapped_id and matched by
GET /mapped-variants/vrs/{identifier}, so exporting the digest produced identifiers
that could not be resolved back against MaveDB.
- rename the CSV column mavedb.post_mapped_vrs_digest to
mavedb.post_mapped_vrs_id and update README.md's column reference
- never synthesize an id from a digest-only object, and leave nested
VRS 1.x variation.id unread, matching the lookup endpoint's reach
- add coverage for the digest/id divergence, the digest-only case, and
the VRS 1.x nesting case
The namespace carried gnomad_af alone, which is not enough to act on a frequency: linking out to the gnomAD variant page needs the variant id, and judging whether a frequency is well sampled needs AC/AN and FAF95 (ACMG BA1/BS1). - Extend the gnomad namespace spec to seven columns: gnomad_af, gnomad_ac, gnomad_an, gnomad_faf95_max, gnomad_faf95_max_ancestry, gnomad_id and gnomad_version. - Widen the namespace label to "gnomAD population frequency", which no longer describes a single column. The label is served to the CSV column picker. - Document the new columns in the public data dump README, since the dump requests this namespace and its output gains them too. Covers the columns at the row-building layer rather than re-testing the shared NA coercion: variant_to_csv_row routes every namespace through _value_or_na, which test_is_output_null already specifies exhaustively.
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.
VEP consequences were sourced from the top-level most_severe_consequence, which reports the worst call across all overlapping transcripts instead of the transcript the variant was actually mapped to, producing misleading annotations. The job definition is left commented out rather than deleted so it can be restored once VEP annotates Allele rows directly. Refs #772
…ed-transcript-for-vep-consequence fix(workflow): stop scheduling VEP annotation in the pipeline
…embership-leakage fix(collections): check .permitted so permission filters take effect
Add run_score_set_pipelines.py to bulk-drive map/annotate pipelines across a cohort of score sets, replacing the one-score-set-per-invocation run_pipeline.py for large campaigns. It resolves a cohort by collection, publication state, taxonomy, or explicit URNs, orders it to exploit ClinGen's 24h cache, bounds concurrency against a campaign-wide in-flight window, skips already-current work, and reports per-score-set outcomes. - Extend PipelineFactory.create_pipeline with a custom_pipeline param (mutually exclusive with pipeline_name) so a caller can run an ad-hoc job subset under its own tracked name, enabling --phase presets (caid, fast-annotate, vep) that isolate slow VEP annotation from the fast jobs that unblock everything else. - Normalize finished_at to UTC before date comparison in is_current, since DB-returned timestamps can be in the server/session timezone and shift the day near midnight. - Filter out the start_pipeline JobRun's empty job_params in pipeline/ score-set queries, which could otherwise nondeterministically report score_set_id as None and let plan_enqueue miss an already in-flight pipeline for a score set. - Add tests/scripts/ package with local fixtures and factories, plus new PipelineFactory unit tests for the custom_pipeline contract.
…e-operator-script feat(scripts): add operator script for bulk score-set pipeline reruns
…ered CA IDs The old PAID we were using here got registered and the test regressed. Use an all 0s PAID that will hopefully never be registered.
bencap
marked this pull request as ready for review
August 12, 2026 21:53
Coverage Report for CI Build 31820775128Warning No base build found for commit Coverage: 89.072%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
The CSV package respelled hgvs_nt/hgvs_splice/hgvs_pro in two places and restated the null-token set a third time. Both now come from validation.constants.general and mave.utils.NULL_VALUES. This also settles a disagreement inside columns.py: cells were rendered with `_is_output_null` while `drop_unused_hgvs_columns` judged emptiness with validation's `is_null`, which additionally counts "-" as null. A column of "-" therefore rendered as "-" but was eligible to be dropped. Both now use the export's own predicate, which drops the package's dependency on validation.utilities entirely. CORE_NAMESPACE's resolvers keep the three names spelled out rather than taking `hgvs_columns`: that dict's declaration order is the published header order, and `hgvs_columns` is sorted.
scores.csv was composed from ["scores"], which meant every score column while
`include_custom_columns` defaulted to True, and means only the required `score` column since the
namespace refactor. /score-sets/{urn}/scores was updated to name both score namespaces; the dump
was missed, so it would have shipped an archive contradicting its own README. Never released — the
namespace refactor is on this branch.
Also here, because they touch the same code:
- `annotation_export_namespaces` takes a required viewer, and `annotations_csv` threads one viewer
through both namespace selection and cell resolution, so a calibration cannot be offered as a
column group and then withheld as data.
- The archive's research-use-only rule is now stated rather than implied: annotations.csv carries
RUO calibrations flagged by `research_use_only`, va.ndjson does not, and the README documents the
asymmetry instead of claiming the dump excludes them.
- The README's claim that mapped/ retains superseded records was wrong — the query has always
filtered `current`. Corrected, with the contrast against the API endpoint that does return
history.
- The command is decomposed into per-artifact functions so each branch the README documents is
reachable without running the whole export. `counts_csv` also stops indexing
dataset_columns["count_columns"] directly, which would KeyError and abort the entire dump on a
score set lacking the key.
66 tests over the archive contract: which files appear for which score-set shape, the `accession`
join key across files, RUO included-and-flagged versus private excluded, every ClinVar release
present, NDJSON line count and newline termination, and the published + CC0 selection gate.
Fixtures build the published/CC0 shape explicitly rather than reusing the generic score-set
factories, whose license is deliberately not CC0. Two carry non-obvious constraints:
TEST_MINIMAL_MAPPED_VARIANT sets post_mapped={}, a shape production never stores and the annotation
layer cannot parse; and a private calibration may not be marked primary.
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.
Features
Bug Fixes
Maintenance