From a5c4044d3eea617eec98cc08e98eda5d953409a2 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 09:19:46 -0700 Subject: [PATCH 01/19] feat(calibration-controls): add status enum and calibration_controls schema Groundwork for #754: CalibrationControlStatus enum (#748) and the CalibrationControl ORM model + calibration_controls table, with disease / controls_not_phi columns on score_calibrations and the ScoreCalibration.controls relationship (#749). Migration parents on the release head; no RT dependency. Validation (#751), PHI gate (#752), view models (#750), and endpoint wiring (#753) are deliberately not included. --- ...add_calibration_controls_table_and_phi_.py | 65 +++++++++++ src/mavedb/models/__init__.py | 1 + src/mavedb/models/calibration_control.py | 77 +++++++++++++ .../enums/calibration_control_status.py | 14 +++ src/mavedb/models/score_calibration.py | 20 ++++ ...e_calibration_functional_classification.py | 9 ++ tests/lib/test_calibration_control.py | 108 ++++++++++++++++++ 7 files changed, 294 insertions(+) create mode 100644 alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py create mode 100644 src/mavedb/models/calibration_control.py create mode 100644 src/mavedb/models/enums/calibration_control_status.py create mode 100644 tests/lib/test_calibration_control.py diff --git a/alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py b/alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py new file mode 100644 index 000000000..1743d8ff6 --- /dev/null +++ b/alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py @@ -0,0 +1,65 @@ +"""add calibration controls table and phi/disease columns + +Revision ID: 024370c4bca7 +Revises: a7f3c2e9b104 +Create Date: 2026-09-15 16:01:06.013964 + +""" + +import sqlalchemy as sa + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "024370c4bca7" +down_revision = "a7f3c2e9b104" +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_table( + "calibration_controls", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("calibration_id", sa.Integer(), nullable=False), + sa.Column("variant_id", sa.Integer(), nullable=False), + sa.Column( + "clinical_status", + sa.Enum("pathogenic", "benign", name="calibrationcontrolstatus", native_enum=False, length=32), + nullable=False, + ), + sa.Column("created_by_id", sa.Integer(), nullable=False), + sa.Column("modified_by_id", sa.Integer(), nullable=False), + sa.Column("creation_date", sa.Date(), nullable=False), + sa.Column("modification_date", sa.Date(), nullable=False), + sa.ForeignKeyConstraint(["calibration_id"], ["score_calibrations.id"], ondelete="CASCADE"), + sa.ForeignKeyConstraint(["variant_id"], ["variants.id"]), + sa.ForeignKeyConstraint(["created_by_id"], ["users.id"]), + sa.ForeignKeyConstraint(["modified_by_id"], ["users.id"]), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("calibration_id", "variant_id", name="uq_calibration_controls_calibration_id_variant_id"), + ) + op.create_index( + op.f("ix_calibration_controls_calibration_id"), "calibration_controls", ["calibration_id"], unique=False + ) + op.create_index(op.f("ix_calibration_controls_variant_id"), "calibration_controls", ["variant_id"], unique=False) + op.create_index( + op.f("ix_calibration_controls_created_by_id"), "calibration_controls", ["created_by_id"], unique=False + ) + op.create_index( + op.f("ix_calibration_controls_modified_by_id"), "calibration_controls", ["modified_by_id"], unique=False + ) + + op.add_column("score_calibrations", sa.Column("disease", sa.String(), nullable=True)) + op.add_column("score_calibrations", sa.Column("controls_not_phi", sa.Boolean(), nullable=True)) + + +def downgrade(): + op.drop_column("score_calibrations", "controls_not_phi") + op.drop_column("score_calibrations", "disease") + + op.drop_index(op.f("ix_calibration_controls_modified_by_id"), table_name="calibration_controls") + op.drop_index(op.f("ix_calibration_controls_created_by_id"), table_name="calibration_controls") + op.drop_index(op.f("ix_calibration_controls_variant_id"), table_name="calibration_controls") + op.drop_index(op.f("ix_calibration_controls_calibration_id"), table_name="calibration_controls") + op.drop_table("calibration_controls") diff --git a/src/mavedb/models/__init__.py b/src/mavedb/models/__init__.py index 2f0d65b48..ba5334e99 100644 --- a/src/mavedb/models/__init__.py +++ b/src/mavedb/models/__init__.py @@ -1,6 +1,7 @@ __all__ = [ "access_key", "acmg_classification", + "calibration_control", "collection", "clinical_control", "controlled_keyword", diff --git a/src/mavedb/models/calibration_control.py b/src/mavedb/models/calibration_control.py new file mode 100644 index 000000000..363756616 --- /dev/null +++ b/src/mavedb/models/calibration_control.py @@ -0,0 +1,77 @@ +"""SQLAlchemy model for calibration controls. + +A *calibration control* is a variant whose clinical significance is independently +known (from a source outside MaveDB) and which was used as empirical ground truth +when deriving a calibration's score thresholds. Storing these controls lets a +clinician audit the evidence a calibration rests on before trusting its thresholds +for interpretation. +""" + +from __future__ import annotations + +from datetime import date +from typing import TYPE_CHECKING + +from sqlalchemy import Column, Date, Enum, ForeignKey, Integer, UniqueConstraint +from sqlalchemy.orm import Mapped, relationship + +from mavedb.db.base import Base +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus + +if TYPE_CHECKING: + from mavedb.models.score_calibration import ScoreCalibration + from mavedb.models.user import User + from mavedb.models.variant import Variant + + +class CalibrationControl(Base): + """A variant with independently known clinical significance, used as ground truth for a calibration. + + Distinct from ``ScoreCalibrationFunctionalClassification`` and its ``variants`` + relationship, which record *bin membership* — the range a variant's functional score + happens to fall into. A ``CalibrationControl`` instead records *clinical ground truth*: + "this variant's pathogenicity is known from external evidence and was used to anchor + the calibration's thresholds." A control variant will usually also land in a bin, since + its score should agree with its known status, but the two relationships answer different + questions and are kept separate. + + Controls are constrained to variants in the calibration's own score set — a control + without a functional score is meaningless as calibration evidence. That constraint is + enforced at the API layer rather than in the schema (see issue #751). + """ + + __tablename__ = "calibration_controls" + __table_args__ = ( + # A variant may appear at most once per calibration and cannot be double counted. + UniqueConstraint("calibration_id", "variant_id", name="uq_calibration_controls_calibration_id_variant_id"), + ) + + id = Column(Integer, primary_key=True) + + # Deleting a calibration deletes its controls: a control has no meaning apart from the calibration it anchors. + # The DB-level cascade backstops the ORM ``delete-orphan`` cascade declared on ``ScoreCalibration.controls``. + calibration_id = Column( + Integer, ForeignKey("score_calibrations.id", ondelete="CASCADE"), nullable=False, index=True + ) + calibration: Mapped["ScoreCalibration"] = relationship("ScoreCalibration", back_populates="controls") + + variant_id = Column(Integer, ForeignKey("variants.id"), nullable=False, index=True) + variant: Mapped["Variant"] = relationship("Variant") + + clinical_status = Column( + Enum(CalibrationControlStatus, native_enum=False, validate_strings=True, length=32), + nullable=False, + ) + + created_by_id = Column(Integer, ForeignKey("users.id"), index=True, nullable=False) + created_by: Mapped["User"] = relationship("User", foreign_keys="CalibrationControl.created_by_id") + modified_by_id = Column(Integer, ForeignKey("users.id"), index=True, nullable=False) + modified_by: Mapped["User"] = relationship("User", foreign_keys="CalibrationControl.modified_by_id") + creation_date = Column(Date, nullable=False, default=date.today) + modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) + + def __repr__(self) -> str: # pragma: no cover - repr utility + return ( + f"" + ) diff --git a/src/mavedb/models/enums/calibration_control_status.py b/src/mavedb/models/enums/calibration_control_status.py new file mode 100644 index 000000000..95c2fbb6e --- /dev/null +++ b/src/mavedb/models/enums/calibration_control_status.py @@ -0,0 +1,14 @@ +import enum + + +class CalibrationControlStatus(str, enum.Enum): + """Clinical significance of a calibration control variant. + + Deliberately restricted to the two-tier ACMG poles used to anchor a calibration's + thresholds. Intermediate tiers (VUS, likely pathogenic, likely benign) are excluded + by design: a control's value as empirical ground truth comes from a confident, binary + clinical call, not from a graded one. + """ + + pathogenic = "pathogenic" + benign = "benign" diff --git a/src/mavedb/models/score_calibration.py b/src/mavedb/models/score_calibration.py index 38ce1f286..08f4b3a0f 100644 --- a/src/mavedb/models/score_calibration.py +++ b/src/mavedb/models/score_calibration.py @@ -16,6 +16,7 @@ from mavedb.models.score_calibration_publication_identifier import ScoreCalibrationPublicationIdentifierAssociation if TYPE_CHECKING: + from mavedb.models.calibration_control import CalibrationControl from mavedb.models.publication_identifier import PublicationIdentifier from mavedb.models.score_set import ScoreSet from mavedb.models.user import User @@ -41,12 +42,31 @@ class ScoreCalibration(Base): baseline_score = Column(Float, nullable=True) baseline_score_description = Column(String, nullable=True) + # Optional free-text disease/disorder context for the controls, at the calibration level. Deliberately not a + # controlled vocabulary and never recorded per-control: a per-control disease label could let a phenotype be + # inferred for an individual variant, which risks PHI. + disease = Column(String, nullable=True) + + # Submitter's affirmation that the control data contains no PHI. Tristate on purpose: # None = not yet addressed, + # False = explicitly declined, True = affirmed. Publishing a calibration that has controls is gated on this being + # True (see #752). + controls_not_phi = Column(Boolean, nullable=True) + functional_classifications: Mapped[list["ScoreCalibrationFunctionalClassification"]] = relationship( "ScoreCalibrationFunctionalClassification", back_populates="calibration", cascade="all, delete-orphan", ) + # Ground-truth controls anchoring this calibration's thresholds. Separate from the per-bin ``variants`` on each + # functional classification, which record score-range membership rather than known clinical significance + # (see ``CalibrationControl``). + controls: Mapped[list["CalibrationControl"]] = relationship( + "CalibrationControl", + back_populates="calibration", + cascade="all, delete-orphan", + ) + publication_identifier_associations: Mapped[list[ScoreCalibrationPublicationIdentifierAssociation]] = relationship( "ScoreCalibrationPublicationIdentifierAssociation", back_populates="score_calibration", diff --git a/src/mavedb/models/score_calibration_functional_classification.py b/src/mavedb/models/score_calibration_functional_classification.py index b9cdabc49..9adc46a6e 100644 --- a/src/mavedb/models/score_calibration_functional_classification.py +++ b/src/mavedb/models/score_calibration_functional_classification.py @@ -22,6 +22,15 @@ class ScoreCalibrationFunctionalClassification(Base): + """A labeled functional-score range (or class) within a calibration, plus the variants that fall in it. + + The ``variants`` relationship records *bin membership* — which variants' functional scores land + in this range. That is distinct from ``CalibrationControl`` (see ``models/calibration_control.py``), + which records variants with independently known clinical significance used as ground truth for the + calibration. A variant can be both, but the two are stored separately because they answer different + questions: "where did this score land?" versus "what is this variant's known clinical status?". + """ + __tablename__ = "score_calibration_functional_classifications" id = Column(Integer, primary_key=True) diff --git a/tests/lib/test_calibration_control.py b/tests/lib/test_calibration_control.py new file mode 100644 index 000000000..2296ae0c2 --- /dev/null +++ b/tests/lib/test_calibration_control.py @@ -0,0 +1,108 @@ +# ruff: noqa: E402 + +import pytest + +pytest.importorskip("psycopg2") + +from sqlalchemy.exc import IntegrityError + +from mavedb.models.calibration_control import CalibrationControl +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus +from mavedb.models.score_calibration import ScoreCalibration +from mavedb.models.user import User +from tests.helpers.constants import TEST_USER + + +def _make_calibration(session, score_set_id: int, user: User) -> ScoreCalibration: + """Persist a bare calibration on the given score set; controls need a parent calibration.""" + calibration = ScoreCalibration( + title="Calibration with controls", + score_set_id=score_set_id, + created_by=user, + modified_by=user, + ) + session.add(calibration) + session.commit() + session.refresh(calibration) + return calibration + + +def test_calibration_control_persists_and_links(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + calibration = _make_calibration(session, variant.score_set_id, user) + + control = CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.pathogenic, + created_by=user, + modified_by=user, + ) + session.add(control) + session.commit() + session.refresh(control) + + assert control.id is not None + # Enum value round-trips through the varchar column. + assert control.clinical_status is CalibrationControlStatus.pathogenic + assert control.creation_date is not None + # Relationship resolves both directions via back_populates. + assert control.calibration is calibration + assert control.variant is variant + assert control in calibration.controls + + +def test_calibration_control_unique_per_calibration_and_variant(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + calibration = _make_calibration(session, variant.score_set_id, user) + + session.add( + CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.pathogenic, + created_by=user, + modified_by=user, + ) + ) + session.commit() + + # A second control for the same (calibration, variant) pair violates the unique constraint, + # even with a different clinical status — a variant is one piece of evidence per calibration. + session.add( + CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.benign, + created_by=user, + modified_by=user, + ) + ) + with pytest.raises(IntegrityError): + session.commit() + session.rollback() + + +def test_deleting_calibration_cascades_to_controls(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + calibration = _make_calibration(session, variant.score_set_id, user) + + control = CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.benign, + created_by=user, + modified_by=user, + ) + session.add(control) + session.commit() + control_id = control.id + + # A control has no meaning apart from its calibration, so deleting the calibration removes it. + session.delete(calibration) + session.commit() + + assert session.get(CalibrationControl, control_id) is None From 1b6b8770b8fa80358f74b1c035898bcb0d0f06a2 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 09:50:23 -0700 Subject: [PATCH 02/19] feat(calibration-controls): add view models for controls (#750) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CalibrationControlBase/Modify/Create/SavedCalibrationControl in a new view_models/calibration_control.py. SavedCalibrationControl derives variant_urn from the variant relationship (mirrors ScoreCalibrationWithScoreSetUrn) so internal PKs never leak, and carries record_type + audit fields. Extend the calibration view models with disease, controls_not_phi, and controls: write models take CalibrationControlCreate; the saved response returns SavedCalibrationControl (empty list by default). Exclude the controls relationship from the model_dump splat in _create_score_calibration and modify_score_calibration — passing controls=None to the ORM relationship raised "None is not list-like" and 500'd every create/modify. disease and controls_not_phi are real columns and flow through. Controls persistence and the CSV path remain #753. --- src/mavedb/lib/score_calibrations.py | 2 + src/mavedb/view_models/calibration_control.py | 86 ++++++++++ src/mavedb/view_models/score_calibration.py | 10 ++ tests/view_models/test_calibration_control.py | 147 ++++++++++++++++++ 4 files changed, 245 insertions(+) create mode 100644 src/mavedb/view_models/calibration_control.py create mode 100644 tests/view_models/test_calibration_control.py diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index 11e1b2e88..ad778e430 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -181,6 +181,7 @@ async def _create_score_calibration( by_alias=False, exclude={ "functional_classifications", + "controls", "threshold_sources", "evidence_sources", "method_sources", @@ -459,6 +460,7 @@ async def modify_score_calibration( for attr, value in calibration_update.model_dump().items(): if attr not in { "functional_classifications", + "controls", "threshold_sources", "evidence_sources", "method_sources", diff --git a/src/mavedb/view_models/calibration_control.py b/src/mavedb/view_models/calibration_control.py new file mode 100644 index 000000000..b6f48e020 --- /dev/null +++ b/src/mavedb/view_models/calibration_control.py @@ -0,0 +1,86 @@ +"""Pydantic view models for calibration controls. + +A calibration control pairs a MaveDB variant with its independently known clinical +significance, serving as empirical ground truth when a calibration's score thresholds +are derived. These models are the API-layer representation used when controls are +created, updated, and served through the calibration endpoints. +""" + +from datetime import date +from typing import Any + +from pydantic import model_validator + +from mavedb.lib.validation.exceptions import ValidationError +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus +from mavedb.view_models import record_type_validator, set_record_type +from mavedb.view_models.base.base import BaseModel +from mavedb.view_models.user import SavedUser + + +class CalibrationControlBase(BaseModel): + """Fields shared by every calibration control view model. + + ``variant_urn`` is the external identifier for the control's variant; the internal + primary key is never exposed. + """ + + variant_urn: str + clinical_status: CalibrationControlStatus + + +class CalibrationControlModify(CalibrationControlBase): + """Model used to modify an existing calibration control. + + Carries no additional fields — only the base fields can be updated. + """ + + pass + + +class CalibrationControlCreate(CalibrationControlModify): + """Model used to create a calibration control. + + Carries no additional fields — only the modifiable fields are required for creation. + """ + + pass + + +class SavedCalibrationControl(CalibrationControlBase): + """Persisted calibration control, including identifier and audit metadata.""" + + record_type: str = None # type: ignore + _record_type_factory = record_type_validator()(set_record_type) + + id: int + creation_date: date + modification_date: date + created_by: SavedUser + modified_by: SavedUser + + class Config: + """Pydantic configuration (ORM mode).""" + + from_attributes = True + + @model_validator(mode="before") + def generate_variant_urn(cls, data: Any): + """Expose the control's variant as ``variant_urn`` when building from an ORM object. + + The ORM row references a ``Variant`` relationship, not a bare URN. Mirrors the + synthetic-field pattern used by ``ScoreCalibrationWithScoreSetUrn``. + """ + if hasattr(data, "variant"): + try: + data.__setattr__("variant_urn", data.variant.urn) + except (AttributeError, KeyError) as exc: + raise ValidationError(f"Unable to coerce variant urn for {cls.__name__}: {exc}.") # type: ignore + + return data + + +class CalibrationControl(SavedCalibrationControl): + """Calibration control with its associated calibration and score set URNs.""" + + pass diff --git a/src/mavedb/view_models/score_calibration.py b/src/mavedb/view_models/score_calibration.py index 857d4e49f..87cde6f5a 100644 --- a/src/mavedb/view_models/score_calibration.py +++ b/src/mavedb/view_models/score_calibration.py @@ -26,6 +26,10 @@ SavedACMGClassification, ) from mavedb.view_models.base.base import BaseModel +from mavedb.view_models.calibration_control import ( + CalibrationControlCreate, + SavedCalibrationControl, +) from mavedb.view_models.publication_identifier import ( PublicationIdentifier, PublicationIdentifierBase, @@ -282,6 +286,9 @@ class ScoreCalibrationBase(BaseModel): baseline_score_description: Optional[str] = None notes: Optional[str] = None + disease: Optional[str] = None + controls_not_phi: Optional[bool] = None + functional_classifications: Optional[Sequence[FunctionalClassificationBase]] = None threshold_sources: Sequence[PublicationIdentifierBase] evidence_sources: Sequence[PublicationIdentifierBase] @@ -432,6 +439,8 @@ class ScoreCalibrationModify(ScoreCalibrationBase): score_set_urn: Optional[str] = None functional_classifications: Optional[Sequence[FunctionalClassificationModify]] = None + # None means "no change" on modify; an empty list clears all controls. + controls: Optional[Sequence[CalibrationControlCreate]] = None threshold_sources: Sequence[PublicationIdentifierCreate] evidence_sources: Sequence[PublicationIdentifierCreate] method_sources: Sequence[PublicationIdentifierCreate] @@ -498,6 +507,7 @@ class SavedScoreCalibration(ScoreCalibrationBase): private: bool = True functional_classifications: Optional[Sequence[SavedFunctionalClassification]] = None + controls: Sequence[SavedCalibrationControl] = [] threshold_sources: Sequence[SavedPublicationIdentifier] evidence_sources: Sequence[SavedPublicationIdentifier] method_sources: Sequence[SavedPublicationIdentifier] diff --git a/tests/view_models/test_calibration_control.py b/tests/view_models/test_calibration_control.py new file mode 100644 index 000000000..8b61d5120 --- /dev/null +++ b/tests/view_models/test_calibration_control.py @@ -0,0 +1,147 @@ +from copy import deepcopy +from datetime import date + +import pytest +from pydantic import ValidationError + +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus +from mavedb.view_models.calibration_control import ( + CalibrationControlCreate, + SavedCalibrationControl, +) +from mavedb.view_models.score_calibration import ScoreCalibration, ScoreCalibrationCreate +from tests.helpers.constants import ( + TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED, + TEST_SAVED_BRNICH_SCORE_CALIBRATION_RANGE_BASED, + TEST_USER, +) +from tests.helpers.util.common import dummy_attributed_object_from_dict + +TEST_CONTROL_VARIANT_URN = "urn:mavedb:00000001-a-1#1" + + +def _saved_user_like(): + """An attributed object standing in for a persisted User (audit field).""" + return dummy_attributed_object_from_dict({"username": TEST_USER["username"]}) + + +def _control_attributed_object(status=CalibrationControlStatus.pathogenic): + """An attributed object standing in for a persisted CalibrationControl ORM row.""" + return dummy_attributed_object_from_dict( + { + "id": 1, + "variant": dummy_attributed_object_from_dict({"urn": TEST_CONTROL_VARIANT_URN}), + "clinical_status": status, + "creation_date": date(2026, 9, 16), + "modification_date": date(2026, 9, 16), + "created_by": _saved_user_like(), + "modified_by": _saved_user_like(), + } + ) + + +############################################################################## +# CalibrationControl view models from dicts (request bodies) +############################################################################## + + +def test_calibration_control_create_from_dict(): + control = CalibrationControlCreate(**{"variantUrn": TEST_CONTROL_VARIANT_URN, "clinicalStatus": "pathogenic"}) + + assert control.variant_urn == TEST_CONTROL_VARIANT_URN + assert control.clinical_status is CalibrationControlStatus.pathogenic + assert control.model_dump(by_alias=True)["variantUrn"] == TEST_CONTROL_VARIANT_URN + + +@pytest.mark.parametrize("status", ["pathogenic", "benign"]) +def test_calibration_control_create_accepts_both_statuses(status): + control = CalibrationControlCreate(variant_urn=TEST_CONTROL_VARIANT_URN, clinical_status=status) + assert control.clinical_status is CalibrationControlStatus(status) + + +def test_calibration_control_create_rejects_unknown_status(): + with pytest.raises(ValidationError): + CalibrationControlCreate(variant_urn=TEST_CONTROL_VARIANT_URN, clinical_status="likely_pathogenic") + + +def test_calibration_control_create_requires_variant_urn(): + with pytest.raises(ValidationError): + CalibrationControlCreate(clinical_status="benign") + + +############################################################################## +# SavedCalibrationControl from attributed objects (ORM models) +############################################################################## + + +def test_saved_calibration_control_synthesizes_variant_urn_from_orm(): + saved = SavedCalibrationControl.model_validate(_control_attributed_object()) + + # variant_urn is not a column on the ORM row; it is derived from the variant relationship. + assert saved.variant_urn == TEST_CONTROL_VARIANT_URN + assert saved.clinical_status is CalibrationControlStatus.pathogenic + assert saved.id == 1 + assert saved.created_by.username == TEST_USER["username"] + assert saved.modified_by.username == TEST_USER["username"] + assert saved.record_type == "SavedCalibrationControl" + + dumped = saved.model_dump(by_alias=True) + assert dumped["variantUrn"] == TEST_CONTROL_VARIANT_URN + assert dumped["recordType"] == "SavedCalibrationControl" + + +############################################################################## +# ScoreCalibration view models extended with controls / disease / controls_not_phi +############################################################################## + + +def test_score_calibration_create_accepts_controls_disease_and_phi(): + payload = deepcopy(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + payload["disease"] = "Brugada syndrome" + payload["controls_not_phi"] = True + payload["controls"] = [ + {"variant_urn": TEST_CONTROL_VARIANT_URN, "clinical_status": "pathogenic"}, + {"variant_urn": "urn:mavedb:00000001-a-1#2", "clinical_status": "benign"}, + ] + + calibration = ScoreCalibrationCreate.model_validate(payload) + + assert calibration.disease == "Brugada syndrome" + assert calibration.controls_not_phi is True + assert len(calibration.controls) == 2 + assert calibration.controls[0].clinical_status is CalibrationControlStatus.pathogenic + assert calibration.controls[1].clinical_status is CalibrationControlStatus.benign + + +def test_saved_score_calibration_includes_controls(): + saved = deepcopy(TEST_SAVED_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + saved["disease"] = "Brugada syndrome" + saved["controlsNotPhi"] = True + saved["controls"] = [ + { + "id": 1, + "variantUrn": TEST_CONTROL_VARIANT_URN, + "clinicalStatus": "pathogenic", + "creationDate": date(2026, 9, 16), + "modificationDate": date(2026, 9, 16), + "createdBy": {"orcidId": TEST_USER["username"]}, + "modifiedBy": {"orcidId": TEST_USER["username"]}, + } + ] + + calibration = ScoreCalibration.model_validate(dummy_attributed_object_from_dict(saved)) + + assert calibration.disease == "Brugada syndrome" + assert calibration.controls_not_phi is True + assert len(calibration.controls) == 1 + assert calibration.controls[0].variant_urn == TEST_CONTROL_VARIANT_URN + assert calibration.controls[0].clinical_status is CalibrationControlStatus.pathogenic + + +def test_saved_score_calibration_controls_default_to_empty_list(): + saved = deepcopy(TEST_SAVED_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + saved.pop("controls", None) + + calibration = ScoreCalibration.model_validate(dummy_attributed_object_from_dict(saved)) + + assert calibration.controls == [] From 8fde78321cdccd57e2e1502fe3e6bc09447ef905 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 10:59:02 -0700 Subject: [PATCH 03/19] feat(calibration-controls): validate controls belong to the score set (#751) Add validate_calibration_controls_in_score_set to lib/score_calibrations.py: a batched, score-set-scoped lookup that rejects controls whose variant is unknown or belongs to a different score set (reported together, per the combined-error decision), plus a Counter-based duplicate check that fails with a readable message before the UNIQUE(calibration_id, variant_id) constraint. No-op for None/empty. Wiring the call into create/modify lands with the endpoint work in #753. --- src/mavedb/lib/score_calibrations.py | 53 +++++++++++++++++- tests/lib/test_calibration_control.py | 79 ++++++++++++++++++++++++++- 2 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index ad778e430..58b5b259f 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -1,7 +1,8 @@ """Utilities for building and mutating score calibration ORM objects.""" import math -from typing import Optional, Union +from collections import Counter +from typing import Optional, Sequence, Union import pandas as pd from sqlalchemy import Float, and_, select @@ -16,6 +17,7 @@ hgvs_nt_column, hgvs_pro_column, ) +from mavedb.lib.validation.exceptions import ValidationError from mavedb.lib.validation.utilities import inf_or_float from mavedb.models.enums.score_calibration_relation import ScoreCalibrationRelation from mavedb.models.score_calibration import ScoreCalibration @@ -27,6 +29,55 @@ from mavedb.view_models import score_calibration +def validate_calibration_controls_in_score_set( + db: Session, + score_set: ScoreSet, + controls: Optional[Sequence[score_calibration.CalibrationControlCreate]], +) -> None: + """Ensure every calibration control references a variant in the calibration's score set. + + A control earns its role as calibration evidence from a variant the assay actually scored, + so each control must reference a variant belonging to the calibration's own score set. A + single scoped query resolves all submitted URNs at once; any URN it does not return either + does not exist in MaveDB or belongs to a different score set, and both are rejected together. + + Duplicate variant URNs within one submission are caught here with a readable message rather + than deferred to the ``UNIQUE(calibration_id, variant_id)`` database constraint. + + Args: + db: Database session used for the lookup. + score_set: The score set the calibration belongs to. + controls: Controls submitted on create or modify. ``None`` or empty is a no-op. + + Raises: + ValidationError: If any control URN is duplicated within the submission, absent from + MaveDB, or belongs to a different score set. + """ + if not controls: + return + + submitted_urns = [control.variant_urn for control in controls] + + counts = Counter(submitted_urns) + duplicate_urns = {urn for urn, count in counts.items() if count > 1} + if duplicate_urns: + raise ValidationError( + f"Duplicate control variant URNs detected within the submission: {', '.join(sorted(duplicate_urns))}." + ) + + existing_urns = set( + db.scalars( + select(Variant.urn).where(Variant.score_set_id == score_set.id, Variant.urn.in_(submitted_urns)) + ).all() + ) + missing_urns = set(counts) - existing_urns + if missing_urns: + raise ValidationError( + "The following control variants do not belong to the calibration's score set: " + f"{', '.join(sorted(missing_urns))}." + ) + + def create_functional_classification( db: Session, functional_range_create: Union[ diff --git a/tests/lib/test_calibration_control.py b/tests/lib/test_calibration_control.py index 2296ae0c2..2c9f254fd 100644 --- a/tests/lib/test_calibration_control.py +++ b/tests/lib/test_calibration_control.py @@ -6,11 +6,16 @@ from sqlalchemy.exc import IntegrityError +from mavedb.lib.score_calibrations import validate_calibration_controls_in_score_set +from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.calibration_control_status import CalibrationControlStatus from mavedb.models.score_calibration import ScoreCalibration +from mavedb.models.score_set import ScoreSet from mavedb.models.user import User -from tests.helpers.constants import TEST_USER +from mavedb.models.variant import Variant +from mavedb.view_models.calibration_control import CalibrationControlCreate +from tests.helpers.constants import TEST_LICENSE, TEST_MINIMAL_VARIANT, TEST_SEQ_SCORESET, TEST_USER def _make_calibration(session, score_set_id: int, user: User) -> ScoreCalibration: @@ -106,3 +111,75 @@ def test_deleting_calibration_cascades_to_controls(session, setup_lib_db_with_va session.commit() assert session.get(CalibrationControl, control_id) is None + + +############################################################################## +# validate_calibration_controls_in_score_set (#751) +############################################################################## + + +def _variant_in_other_score_set(session, reference_score_set, user) -> Variant: + """Create a second score set (in the same experiment) with one variant, for cross-set tests.""" + scaffold = TEST_SEQ_SCORESET.copy() + scaffold.pop("target_genes") + other_score_set = ScoreSet( + **scaffold, + urn="urn:mavedb:00000002-a-1", + experiment_id=reference_score_set.experiment_id, + licence_id=TEST_LICENSE["id"], + ) + other_score_set.created_by = user + other_score_set.modified_by = user + session.add(other_score_set) + session.commit() + session.refresh(other_score_set) + + variant = Variant(**TEST_MINIMAL_VARIANT, urn=f"{other_score_set.urn}#1", score_set_id=other_score_set.id) + session.add(variant) + session.commit() + session.refresh(variant) + return variant + + +@pytest.mark.parametrize("controls", [None, []]) +def test_validate_controls_is_a_no_op_when_absent(session, setup_lib_db_with_variant, controls): + variant = setup_lib_db_with_variant + # Must not raise for None or an empty list. + validate_calibration_controls_in_score_set(session, variant.score_set, controls) + + +def test_validate_controls_accepts_variant_in_score_set(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + controls = [CalibrationControlCreate(variant_urn=variant.urn, clinical_status="pathogenic")] + + validate_calibration_controls_in_score_set(session, variant.score_set, controls) + + +def test_validate_controls_rejects_nonexistent_variant(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + controls = [CalibrationControlCreate(variant_urn="urn:mavedb:99999999-x-9#1", clinical_status="benign")] + + with pytest.raises(ValidationError, match="do not belong to the calibration's score set"): + validate_calibration_controls_in_score_set(session, variant.score_set, controls) + + +def test_validate_controls_rejects_variant_from_another_score_set(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + other_variant = _variant_in_other_score_set(session, variant.score_set, user) + + controls = [CalibrationControlCreate(variant_urn=other_variant.urn, clinical_status="pathogenic")] + + with pytest.raises(ValidationError, match="do not belong to the calibration's score set"): + validate_calibration_controls_in_score_set(session, variant.score_set, controls) + + +def test_validate_controls_rejects_duplicate_variant_urns(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + controls = [ + CalibrationControlCreate(variant_urn=variant.urn, clinical_status="pathogenic"), + CalibrationControlCreate(variant_urn=variant.urn, clinical_status="benign"), + ] + + with pytest.raises(ValidationError, match="Duplicate control variant URNs detected"): + validate_calibration_controls_in_score_set(session, variant.score_set, controls) From cefdc2607160dec06a9a7c5cf94aabb60352a597 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 11:21:23 -0700 Subject: [PATCH 04/19] feat(calibration-controls): gate publishing on PHI acknowledgment (#752) Block the private->public transition in publish_score_calibration_route when a calibration has controls but controls_not_phi is not True (None or False both block; only True clears). Zero-control calibrations are unaffected. The check lives in the router to keep HTTP semantics out of the lib layer. Tests attach controls at the ORM level since endpoint persistence is #753. --- src/mavedb/routers/score_calibrations.py | 25 ++++- tests/routers/test_calibration_controls.py | 111 +++++++++++++++++++++ 2 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 tests/routers/test_calibration_controls.py diff --git a/src/mavedb/routers/score_calibrations.py b/src/mavedb/routers/score_calibrations.py index e8b31478c..6e876c29d 100644 --- a/src/mavedb/routers/score_calibrations.py +++ b/src/mavedb/routers/score_calibrations.py @@ -569,7 +569,11 @@ async def promote_score_calibration_to_primary_route( Promote a score calibration to be the primary calibration for its associated score set. """ save_to_logging_context( - {"requested_resource": urn, "resource_property": "primary", "demote_existing_primary": demote_existing_primary} + { + "requested_resource": urn, + "resource_property": "primary", + "demote_existing_primary": demote_existing_primary, + } ) item = ( @@ -691,6 +695,19 @@ def publish_score_calibration_route( logger.debug("The requested score calibration is already public", extra=logging_context()) return item + # Control data must be affirmed free of PHI before it can be made public. The affirmation is only + # meaningful when controls exist, so zero-control calibrations publish regardless. controls_not_phi + # is tristate: only True clears the gate — None (unaddressed) and False (declined) both block. + if item.controls and item.controls_not_phi is not True: + logger.debug("Calibration controls have not been affirmed free of PHI", extra=logging_context()) + raise HTTPException( + status_code=422, + detail=( + "This calibration has controls that have not been affirmed to be free of protected health " + "information (PHI). Set controls_not_phi to true before publishing." + ), + ) + # XXX: desired? # if item.score_set.private: # logger.debug( @@ -729,7 +746,11 @@ def get_functional_classification_variants( a `variant_count` summary for performance. """ save_to_logging_context( - {"requested_resource": urn, "requested_classification": classification_id, "resource_property": "variants"} + { + "requested_resource": urn, + "requested_classification": classification_id, + "resource_property": "variants", + } ) calibration = ( diff --git a/tests/routers/test_calibration_controls.py b/tests/routers/test_calibration_controls.py new file mode 100644 index 000000000..eeab1bba1 --- /dev/null +++ b/tests/routers/test_calibration_controls.py @@ -0,0 +1,111 @@ +# ruff: noqa: E402 + +import pytest + +arq = pytest.importorskip("arq") +cdot = pytest.importorskip("cdot") +fastapi = pytest.importorskip("fastapi") + +from sqlalchemy import select + +from mavedb.models.calibration_control import CalibrationControl +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus +from mavedb.models.score_calibration import ScoreCalibration as CalibrationDbModel +from mavedb.models.variant import Variant +from tests.helpers.constants import ( + TEST_BIORXIV_IDENTIFIER, + TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED, + TEST_PUBMED_IDENTIFIER, +) +from tests.helpers.util.common import deepcamelize +from tests.helpers.util.experiment import create_experiment +from tests.helpers.util.score_calibration import create_test_score_calibration_in_score_set_via_client +from tests.helpers.util.score_set import create_seq_score_set_with_mapped_variants + +CALIBRATION_PUBLICATIONS = [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, +] + + +def _create_private_calibration(client, session, data_provider, data_files): + """Create a score set with variants and a private range-based calibration; return the calibration dict.""" + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, + session, + data_provider, + experiment["urn"], + data_files / "scores.csv", + ) + return create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + + +def _attach_control(session, calibration_urn, controls_not_phi): + """Attach one control to a calibration at the ORM level (endpoint wiring is #753) and set the PHI flag.""" + calibration = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration_urn).one() + variant = session.scalars(select(Variant).where(Variant.score_set_id == calibration.score_set_id)).first() + + session.add( + CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.pathogenic, + created_by=calibration.created_by, + modified_by=calibration.created_by, + ) + ) + calibration.controls_not_phi = controls_not_phi + session.add(calibration) + session.commit() + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +@pytest.mark.parametrize("controls_not_phi", [None, False]) +def test_cannot_publish_calibration_with_controls_when_phi_not_affirmed( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, controls_not_phi +): + calibration = _create_private_calibration(client, session, data_provider, data_files) + _attach_control(session, calibration["urn"], controls_not_phi=controls_not_phi) + + response = client.post(f"/api/v1/score-calibrations/{calibration['urn']}/publish") + + assert response.status_code == 422 + assert "protected health information" in response.json()["detail"].lower() + + # The gate must not have published the calibration. + refreshed = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]).one() + assert refreshed.private is True + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_can_publish_calibration_with_controls_when_phi_affirmed( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + calibration = _create_private_calibration(client, session, data_provider, data_files) + _attach_control(session, calibration["urn"], controls_not_phi=True) + + response = client.post(f"/api/v1/score-calibrations/{calibration['urn']}/publish") + + assert response.status_code == 200 + assert response.json()["private"] is False + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_can_publish_calibration_without_controls_regardless_of_phi_flag( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + calibration = _create_private_calibration(client, session, data_provider, data_files) + + # No controls attached, but an explicit (non-True) PHI flag must not block publishing. + item = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]).one() + item.controls_not_phi = False + session.add(item) + session.commit() + + response = client.post(f"/api/v1/score-calibrations/{calibration['urn']}/publish") + + assert response.status_code == 200 + assert response.json()["private"] is False From b3c1801b23fddc3a6d3f11d41b13d639c1e2969b Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 12:55:58 -0700 Subject: [PATCH 05/19] feat(calibration-controls): persist controls on create and modify (#753) Add build_calibration_controls (validate via validate_calibration_controls_in_score_set, then resolve URNs to variants and construct CalibrationControl rows) and wire it into create_score_calibration_in_score_set and modify_score_calibration. Modify uses replace semantics: controls=None leaves existing controls untouched, a list (including []) deletes and rebuilds them. validate_calibration_controls_in_score_set now returns the validated variant URNs so build reuses them instead of rebuilding the list. Controls are read via getattr on the update/create models so the existing minimal test mocks (which predate the field) still work. CSV upload, the controls_not_phi reset-on-change, and controlsCount on list responses are the remaining slices. --- src/mavedb/lib/score_calibrations.py | 74 +++++++++++++++++++++++++-- tests/lib/test_calibration_control.py | 45 ++++++++++++++-- 2 files changed, 111 insertions(+), 8 deletions(-) diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index 58b5b259f..cd36b12e3 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -19,6 +19,7 @@ ) from mavedb.lib.validation.exceptions import ValidationError from mavedb.lib.validation.utilities import inf_or_float +from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.score_calibration_relation import ScoreCalibrationRelation from mavedb.models.score_calibration import ScoreCalibration from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification @@ -33,8 +34,8 @@ def validate_calibration_controls_in_score_set( db: Session, score_set: ScoreSet, controls: Optional[Sequence[score_calibration.CalibrationControlCreate]], -) -> None: - """Ensure every calibration control references a variant in the calibration's score set. +) -> list[str]: + """Validate calibration controls against a score set and return their variant URNs. A control earns its role as calibration evidence from a variant the assay actually scored, so each control must reference a variant belonging to the calibration's own score set. A @@ -44,17 +45,23 @@ def validate_calibration_controls_in_score_set( Duplicate variant URNs within one submission are caught here with a readable message rather than deferred to the ``UNIQUE(calibration_id, variant_id)`` database constraint. + The validated URNs are returned so a caller that goes on to persist the controls can reuse + them (see :func:`build_calibration_controls`). + Args: db: Database session used for the lookup. score_set: The score set the calibration belongs to. - controls: Controls submitted on create or modify. ``None`` or empty is a no-op. + controls: Controls submitted on create or modify. ``None`` or empty returns ``[]``. + + Returns: + The submitted and validated variant URNs, in order; empty for ``None``/empty input. Raises: ValidationError: If any control URN is duplicated within the submission, absent from MaveDB, or belongs to a different score set. """ if not controls: - return + return [] submitted_urns = [control.variant_urn for control in controls] @@ -77,6 +84,50 @@ def validate_calibration_controls_in_score_set( f"{', '.join(sorted(missing_urns))}." ) + return submitted_urns + + +def build_calibration_controls( + db: Session, + score_set: ScoreSet, + controls: Optional[Sequence[score_calibration.CalibrationControlCreate]], + user: User, +) -> list[CalibrationControl]: + """Validate and construct transient ``CalibrationControl`` rows for a calibration. + + Validation (see :func:`validate_calibration_controls_in_score_set`) returns the validated + variant URNs, which this function resolves to their ``Variant`` rows in one query and turns + into unattached ``CalibrationControl`` rows with audit fields set. The caller assigns them to + the calibration's ``controls`` collection and commits. Returns an empty list for ``None`` or + empty input. + + Args: + db: Database session. + score_set: The score set the calibration belongs to; controls resolve within it. + controls: Submitted controls, or ``None``/empty for none. + user: The acting user, recorded on each control's audit fields. + """ + submitted_urns = validate_calibration_controls_in_score_set(db, score_set, controls) + if not controls: + return [] + + variants_by_urn = { + variant.urn: variant + for variant in db.scalars( + select(Variant).where(Variant.score_set_id == score_set.id, Variant.urn.in_(submitted_urns)) + ).all() + } + + return [ + CalibrationControl( + variant=variants_by_urn[control.variant_urn], + clinical_status=control.clinical_status, + created_by=user, + modified_by=user, + ) + for control in controls + ] + def create_functional_classification( db: Session, @@ -313,6 +364,10 @@ async def create_score_calibration_in_score_set( else: calibration.investigator_provided = False + calibration.controls = build_calibration_controls( + db, containing_score_set, getattr(calibration_create, "controls", None), user + ) + db.add(calibration) return calibration @@ -535,6 +590,17 @@ async def modify_score_calibration( db.add(persisted_functional_range) calibration.functional_classifications.append(persisted_functional_range) + # Replace semantics: a provided controls list (even empty) replaces all existing controls, while + # None leaves them untouched. TODO#752: reset controls_not_phi here once the re-acknowledgment + # behavior is settled against PUT semantics and the editor's same-request affirmation. + submitted_controls = getattr(calibration_update, "controls", None) + if submitted_controls is not None: + for control in list(calibration.controls): + db.delete(control) + calibration.controls.clear() + db.flush() + calibration.controls = build_calibration_controls(db, containing_score_set, submitted_controls, user) + db.add(calibration) return calibration diff --git a/tests/lib/test_calibration_control.py b/tests/lib/test_calibration_control.py index 2c9f254fd..050f780ca 100644 --- a/tests/lib/test_calibration_control.py +++ b/tests/lib/test_calibration_control.py @@ -6,7 +6,7 @@ from sqlalchemy.exc import IntegrityError -from mavedb.lib.score_calibrations import validate_calibration_controls_in_score_set +from mavedb.lib.score_calibrations import build_calibration_controls, validate_calibration_controls_in_score_set from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.calibration_control_status import CalibrationControlStatus @@ -144,15 +144,16 @@ def _variant_in_other_score_set(session, reference_score_set, user) -> Variant: @pytest.mark.parametrize("controls", [None, []]) def test_validate_controls_is_a_no_op_when_absent(session, setup_lib_db_with_variant, controls): variant = setup_lib_db_with_variant - # Must not raise for None or an empty list. - validate_calibration_controls_in_score_set(session, variant.score_set, controls) + # Must not raise for None or an empty list, and returns no URNs. + assert validate_calibration_controls_in_score_set(session, variant.score_set, controls) == [] def test_validate_controls_accepts_variant_in_score_set(session, setup_lib_db_with_variant): variant = setup_lib_db_with_variant controls = [CalibrationControlCreate(variant_urn=variant.urn, clinical_status="pathogenic")] - validate_calibration_controls_in_score_set(session, variant.score_set, controls) + # Returns the validated URNs so persistence can reuse them without rebuilding the list. + assert validate_calibration_controls_in_score_set(session, variant.score_set, controls) == [variant.urn] def test_validate_controls_rejects_nonexistent_variant(session, setup_lib_db_with_variant): @@ -183,3 +184,39 @@ def test_validate_controls_rejects_duplicate_variant_urns(session, setup_lib_db_ with pytest.raises(ValidationError, match="Duplicate control variant URNs detected"): validate_calibration_controls_in_score_set(session, variant.score_set, controls) + + +############################################################################## +# build_calibration_controls (#753 lib persistence) +############################################################################## + + +@pytest.mark.parametrize("controls", [None, []]) +def test_build_calibration_controls_empty_for_absent_input(session, setup_lib_db_with_variant, controls): + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + + assert build_calibration_controls(session, variant.score_set, controls, user) == [] + + +def test_build_calibration_controls_constructs_rows(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + controls = [CalibrationControlCreate(variant_urn=variant.urn, clinical_status="benign")] + + built = build_calibration_controls(session, variant.score_set, controls, user) + + assert len(built) == 1 + assert built[0].variant is variant + assert built[0].clinical_status is CalibrationControlStatus.benign + assert built[0].created_by is user + assert built[0].modified_by is user + + +def test_build_calibration_controls_propagates_validation_error(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + controls = [CalibrationControlCreate(variant_urn="urn:mavedb:99999999-x-9#1", clinical_status="benign")] + + with pytest.raises(ValidationError, match="do not belong to the calibration's score set"): + build_calibration_controls(session, variant.score_set, controls, user) From d5ab9bcfe3d142faf71d29046f1f26be4415661b Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 15:16:15 -0700 Subject: [PATCH 06/19] feat(calibration-controls): reset controls_not_phi when controls change on modify (#753) On modify, controls_not_phi is no longer set by the generic attribute loop. An explicit value in the request always wins; otherwise replacing the controls invalidates any prior affirmation (reset to None, forcing re-acknowledgment before republishing), while leaving the controls untouched preserves it. This keeps an unrelated edit from silently wiping an affirmation and supports the editor's change-controls-and-affirm-in-one-save flow. --- src/mavedb/lib/score_calibrations.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index cd36b12e3..73092fc69 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -567,6 +567,9 @@ async def modify_score_calibration( if attr not in { "functional_classifications", "controls", + # controls_not_phi carries re-acknowledgment semantics; set it explicitly so an + # unrelated edit cannot silently wipe a prior affirmation. + "controls_not_phi", "threshold_sources", "evidence_sources", "method_sources", @@ -591,8 +594,7 @@ async def modify_score_calibration( calibration.functional_classifications.append(persisted_functional_range) # Replace semantics: a provided controls list (even empty) replaces all existing controls, while - # None leaves them untouched. TODO#752: reset controls_not_phi here once the re-acknowledgment - # behavior is settled against PUT semantics and the editor's same-request affirmation. + # None leaves them untouched. submitted_controls = getattr(calibration_update, "controls", None) if submitted_controls is not None: for control in list(calibration.controls): @@ -601,6 +603,13 @@ async def modify_score_calibration( db.flush() calibration.controls = build_calibration_controls(db, containing_score_set, submitted_controls, user) + # Re-acknowledgment: an explicit controls_not_phi in the request always wins; otherwise, replacing the + # controls invalidates any prior affirmation, while leaving the controls untouched preserves it. + if "controls_not_phi" in calibration_update.model_fields_set: + calibration.controls_not_phi = calibration_update.controls_not_phi + elif submitted_controls is not None: + calibration.controls_not_phi = None + db.add(calibration) return calibration From 17a2b2d4776c365787b0bf92c8335899318119bf Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 15:16:30 -0700 Subject: [PATCH 07/19] feat(calibration-controls): CSV validation with 1:1 HGVS resolution (#753) Add validate_and_standardize_calibration_controls_dataframe (mirroring the class-based validator) plus the clinical_status column constant. The controls CSV accepts the same variant_urn / hgvs_nt / hgvs_pro columns as the class-based CSV and a clinical_status column validated against CalibrationControlStatus (case-insensitive), returning CalibrationControlCreate rows so the router feeds inline and CSV controls through one path. Because a control is a single variant while an HGVS can match many (hgvs_pro collapses distinct nucleotide variants in a nucleotide-indexed score set), resolution rejects any identifier that maps to more than one variant in the score set rather than silently choosing one. --- .../lib/validation/constants/general.py | 1 + .../lib/validation/dataframe/calibration.py | 146 ++++++++++++++++++ tests/lib/test_calibration_control.py | 107 +++++++++++++ 3 files changed, 254 insertions(+) diff --git a/src/mavedb/lib/validation/constants/general.py b/src/mavedb/lib/validation/constants/general.py index 22ca4cbf5..14a087fc7 100644 --- a/src/mavedb/lib/validation/constants/general.py +++ b/src/mavedb/lib/validation/constants/general.py @@ -46,6 +46,7 @@ calibration_variant_column_name = "variant_urn" calibration_class_column_name = "class_name" +calibration_control_status_column_name = "clinical_status" valid_dataset_columns = [score_columns, count_columns] valid_variant_columns = [variant_score_data, variant_count_data] diff --git a/src/mavedb/lib/validation/dataframe/calibration.py b/src/mavedb/lib/validation/dataframe/calibration.py index 1c46be46b..7594c36c5 100644 --- a/src/mavedb/lib/validation/dataframe/calibration.py +++ b/src/mavedb/lib/validation/dataframe/calibration.py @@ -4,6 +4,7 @@ from mavedb.lib.validation.constants.general import ( calibration_class_column_name, + calibration_control_status_column_name, calibration_variant_column_name, hgvs_nt_column, hgvs_pro_column, @@ -11,6 +12,7 @@ from mavedb.lib.validation.dataframe.column import validate_data_column, validate_variant_column from mavedb.lib.validation.dataframe.dataframe import standardize_dataframe, validate_no_null_rows from mavedb.lib.validation.exceptions import ValidationError +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus from mavedb.models.score_set import ScoreSet from mavedb.models.variant import Variant from mavedb.view_models import score_calibration @@ -22,6 +24,13 @@ hgvs_pro_column, ) +STANDARD_CALIBRATION_CONTROL_COLUMNS = ( + calibration_variant_column_name, + calibration_control_status_column_name, + hgvs_nt_column, + hgvs_pro_column, +) + def validate_and_standardize_calibration_classes_dataframe( db: Session, @@ -246,3 +255,140 @@ def validate_calibration_classes( unprovided_classes = defined_classes - provided_classes if unprovided_classes: raise ValidationError("Some defined classes in the calibration are missing from the classes file.") + + +def validate_and_standardize_calibration_controls_dataframe( + db: Session, + score_set: ScoreSet, + controls_df: pd.DataFrame, +) -> list[score_calibration.CalibrationControlCreate]: + """Validate a calibration controls CSV and convert it to ``CalibrationControlCreate`` rows. + + Mirrors :func:`validate_and_standardize_calibration_classes_dataframe`. The CSV must carry a + variant identifier column (``variant_urn``, ``hgvs_nt``, or ``hgvs_pro``) and a + ``clinical_status`` column. Every variant must belong to the score set, and every status must be + a ``CalibrationControlStatus`` value (case-insensitive). The chosen variant column is resolved to + MaveDB URNs so the returned controls carry ``variant_urn``, matching the inline-JSON contract. + + Args: + db: Database session for validation and URN resolution. + score_set: The score set the calibration belongs to. + controls_df: The parsed controls CSV. + + Returns: + One ``CalibrationControlCreate`` per row. + + Raises: + ValidationError: On unexpected/missing columns, invalid variants, or invalid status values. + """ + standardized_df = standardize_dataframe(controls_df, STANDARD_CALIBRATION_CONTROL_COLUMNS) + validate_calibration_control_df_column_names(standardized_df) + validate_no_null_rows(standardized_df) + + column_mapping = {c.lower(): c for c in standardized_df.columns} + index_column = choose_calibration_index_column(standardized_df) + + # Drop rows without a clinical status. + standardized_df = standardized_df.dropna( + subset=[column_mapping[calibration_control_status_column_name]] + ).reset_index(drop=True) + + for c in column_mapping: + if c in {calibration_variant_column_name, hgvs_nt_column, hgvs_pro_column}: + validate_variant_column(standardized_df[c], column_mapping[c] == index_column) + elif c == calibration_control_status_column_name: + validate_data_column(standardized_df[c], force_numeric=False) + validate_calibration_control_statuses(standardized_df[column_mapping[c]]) + + if column_mapping[c] == index_column: + validate_index_existence_in_score_set(db, score_set, standardized_df[column_mapping[c]], column_mapping[c]) + + return resolve_calibration_controls(db, score_set, standardized_df, index_column) + + +def validate_calibration_control_df_column_names(df: pd.DataFrame) -> None: + """Validate the column names of a calibration controls DataFrame. + + Requires a ``clinical_status`` column and at least one variant identifier column, and rejects + unexpected columns. Comparison is case-insensitive. Mirrors + :func:`validate_calibration_df_column_names`. + """ + if any(type(c) is not str for c in df.columns): + raise ValidationError("column names must be strings") + + if any(c.isspace() for c in df.columns) or any(len(c) == 0 for c in df.columns): + raise ValidationError("column names cannot be empty or whitespace") + + if len(df.columns) != len(set(c.lower() for c in df.columns)): + raise ValidationError("duplicate column names are not allowed (case-insensitive)") + + columns = [c.lower() for c in df.columns] + + if calibration_control_status_column_name not in columns: + raise ValidationError(f"missing required column: '{calibration_control_status_column_name}'") + + if set(columns).isdisjoint({hgvs_nt_column, hgvs_pro_column, calibration_variant_column_name}): + raise ValidationError( + f"at least one of {', '.join({hgvs_nt_column, hgvs_pro_column, calibration_variant_column_name})} must be present" + ) + + +def validate_calibration_control_statuses(statuses: pd.Series) -> None: + """Ensure every ``clinical_status`` value maps to a ``CalibrationControlStatus`` (case-insensitive).""" + valid_statuses = {status.value for status in CalibrationControlStatus} + invalid_statuses = { + str(status) for status in statuses.tolist() if str(status).strip().lower() not in valid_statuses + } + if invalid_statuses: + raise ValidationError( + f"Invalid clinical status value(s): {', '.join(sorted(invalid_statuses))}. " + f"Allowed values are: {', '.join(sorted(valid_statuses))}." + ) + + +def resolve_calibration_controls( + db: Session, + score_set: ScoreSet, + df: pd.DataFrame, + index_column: str, +) -> list[score_calibration.CalibrationControlCreate]: + """Convert a validated controls DataFrame to ``CalibrationControlCreate`` rows, resolving URNs. + + For a ``variant_urn`` index the URN is taken directly. For an HGVS index the values are mapped to + their variant URNs within the score set in a single query. A control is a single variant, so an + HGVS that matches more than one variant is ambiguous and rejected — whether a given HGVS column + collides depends on the score set's own index (``hgvs_pro`` is unique in a protein-indexed set but + not in a nucleotide-indexed one), so this is checked against the data rather than assumed. + """ + column_mapping = {c.lower(): c for c in df.columns} + status_column = column_mapping[calibration_control_status_column_name] + index_values = df[index_column].tolist() + + if index_column.lower() == calibration_variant_column_name: + urn_by_index_value = {value: value for value in index_values} + else: + hgvs_attribute = Variant.hgvs_nt if index_column.lower() == hgvs_nt_column else Variant.hgvs_pro + urns_by_hgvs: dict[str, list[str]] = {} + for urn, hgvs in db.execute( + select(Variant.urn, hgvs_attribute).where( + Variant.score_set_id == score_set.id, hgvs_attribute.in_(index_values) + ) + ).all(): + urns_by_hgvs.setdefault(hgvs, []).append(urn) + + ambiguous_identifiers = sorted(hgvs for hgvs, urns in urns_by_hgvs.items() if len(urns) > 1) + if ambiguous_identifiers: + raise ValidationError( + "These variant identifiers match more than one variant in the score set; " + f"use the variant URN instead: {', '.join(ambiguous_identifiers)}." + ) + + urn_by_index_value = {hgvs: urns[0] for hgvs, urns in urns_by_hgvs.items()} + + return [ + score_calibration.CalibrationControlCreate( + variant_urn=urn_by_index_value[row[index_column]], + clinical_status=CalibrationControlStatus(str(row[status_column]).strip().lower()), + ) + for _, row in df.iterrows() + ] diff --git a/tests/lib/test_calibration_control.py b/tests/lib/test_calibration_control.py index 050f780ca..5637f6286 100644 --- a/tests/lib/test_calibration_control.py +++ b/tests/lib/test_calibration_control.py @@ -4,9 +4,17 @@ pytest.importorskip("psycopg2") +import pandas as pd from sqlalchemy.exc import IntegrityError from mavedb.lib.score_calibrations import build_calibration_controls, validate_calibration_controls_in_score_set +from mavedb.lib.validation.constants.general import ( + calibration_control_status_column_name, + calibration_variant_column_name, + hgvs_nt_column, + hgvs_pro_column, +) +from mavedb.lib.validation.dataframe.calibration import validate_and_standardize_calibration_controls_dataframe from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.calibration_control_status import CalibrationControlStatus @@ -220,3 +228,102 @@ def test_build_calibration_controls_propagates_validation_error(session, setup_l with pytest.raises(ValidationError, match="do not belong to the calibration's score set"): build_calibration_controls(session, variant.score_set, controls, user) + + +############################################################################## +# validate_and_standardize_calibration_controls_dataframe (#753 CSV path) +############################################################################## + + +def _controls_df(variant_values, statuses): + return pd.DataFrame( + {calibration_variant_column_name: variant_values, calibration_control_status_column_name: statuses} + ) + + +def test_controls_csv_validates_and_converts_to_create_rows(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + # Status is case-insensitive. + df = _controls_df([variant.urn], ["Pathogenic"]) + + controls = validate_and_standardize_calibration_controls_dataframe(session, variant.score_set, df) + + assert len(controls) == 1 + assert controls[0].variant_urn == variant.urn + assert controls[0].clinical_status is CalibrationControlStatus.pathogenic + + +def test_controls_csv_rejects_invalid_status(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + df = _controls_df([variant.urn], ["likely_pathogenic"]) + + with pytest.raises(ValidationError, match="Invalid clinical status"): + validate_and_standardize_calibration_controls_dataframe(session, variant.score_set, df) + + +def test_controls_csv_rejects_variant_not_in_score_set(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + df = _controls_df(["urn:mavedb:99999999-x-9#1"], ["benign"]) + + with pytest.raises(ValidationError, match="do not exist in the score set"): + validate_and_standardize_calibration_controls_dataframe(session, variant.score_set, df) + + +def test_controls_csv_rejects_missing_status_column(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + df = pd.DataFrame({calibration_variant_column_name: [variant.urn]}) + + with pytest.raises(ValidationError, match=calibration_control_status_column_name): + validate_and_standardize_calibration_controls_dataframe(session, variant.score_set, df) + + +def test_controls_csv_accepts_hgvs_nt_index(session, setup_lib_db_with_variant): + variant = setup_lib_db_with_variant + # hgvs_nt is unique within the score set, so it resolves 1:1 to the variant URN. + df = pd.DataFrame({hgvs_nt_column: [variant.hgvs_nt], calibration_control_status_column_name: ["benign"]}) + + controls = validate_and_standardize_calibration_controls_dataframe(session, variant.score_set, df) + + assert len(controls) == 1 + assert controls[0].variant_urn == variant.urn + assert controls[0].clinical_status is CalibrationControlStatus.benign + + +@pytest.mark.parametrize( + "index_column, shared_value, distinct_values", + [ + # hgvs_pro collision: two nucleotide variants collapsing to the same protein consequence + # (the expected case in a nucleotide-indexed score set). + (hgvs_pro_column, "p.Met1Val", ("c.1A>G", "c.3G>T")), + # hgvs_nt collision: not expected in practice, but the detection must still fire. + (hgvs_nt_column, "c.99A>G", ("p.Lys33Arg", "p.Lys33Gln")), + ], +) +def test_controls_csv_rejects_ambiguous_hgvs_index( + session, setup_lib_db_with_variant, index_column, shared_value, distinct_values +): + variant = setup_lib_db_with_variant + score_set = variant.score_set + + # Two variants that share the submitted index value but differ on the other HGVS field, so the + # index resolves to more than one variant in the score set. + for suffix, distinct_value in enumerate(distinct_values, start=2): + hgvs_nt = shared_value if index_column == hgvs_nt_column else distinct_value + hgvs_pro = shared_value if index_column == hgvs_pro_column else distinct_value + session.add( + Variant( + data=TEST_MINIMAL_VARIANT["data"], + hgvs_nt=hgvs_nt, + hgvs_pro=hgvs_pro, + creation_date=TEST_MINIMAL_VARIANT["creation_date"], + modification_date=TEST_MINIMAL_VARIANT["modification_date"], + urn=f"{score_set.urn}#{suffix}", + score_set_id=score_set.id, + ) + ) + session.commit() + + df = pd.DataFrame({index_column: [shared_value], calibration_control_status_column_name: ["pathogenic"]}) + + with pytest.raises(ValidationError, match="match more than one variant"): + validate_and_standardize_calibration_controls_dataframe(session, score_set, df) From bc47a8be42a57dbbd7ebdccb0f05bd07461109b2 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 16:57:30 -0700 Subject: [PATCH 08/19] feat(calibration-controls): wire controls into create/update/read endpoints (#753) Add an optional controls_file CSV upload to the create and modify routes alongside the existing classes_file, documented in the multipart OpenAPI schema and route docstrings. Providing inline controls and a controls_file together returns 422; a controls_file is parsed and validated (URN or HGVS, 1:1) and fed through the same create/modify path as inline controls. GET returns controls; list responses are addressed separately (#753 slice 4). Integration tests cover inline create, CSV create, inline+file conflict, GET, modify replace/clear/omit, and the controls_not_phi re-acknowledgment behavior. --- src/mavedb/routers/score_calibrations.py | 95 ++++++++- tests/routers/test_calibration_controls.py | 225 +++++++++++++++++++++ 2 files changed, 317 insertions(+), 3 deletions(-) diff --git a/src/mavedb/routers/score_calibrations.py b/src/mavedb/routers/score_calibrations.py index 6e876c29d..d585dd45c 100644 --- a/src/mavedb/routers/score_calibrations.py +++ b/src/mavedb/routers/score_calibrations.py @@ -25,8 +25,17 @@ ) from mavedb.lib.score_sets import csv_data_to_df from mavedb.lib.types.authentication import UserData -from mavedb.lib.validation.constants.general import calibration_class_column_name, calibration_variant_column_name -from mavedb.lib.validation.dataframe.calibration import validate_and_standardize_calibration_classes_dataframe +from mavedb.lib.validation.constants.general import ( + calibration_class_column_name, + calibration_control_status_column_name, + calibration_variant_column_name, + hgvs_nt_column, + hgvs_pro_column, +) +from mavedb.lib.validation.dataframe.calibration import ( + validate_and_standardize_calibration_classes_dataframe, + validate_and_standardize_calibration_controls_dataframe, +) from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.score_calibration import ScoreCalibration from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification @@ -224,6 +233,11 @@ async def get_primary_score_calibrations_for_score_set( "format": "binary", "description": "CSV file containing variant classifications", }, + "controls_file": { + "type": "string", + "format": "binary", + "description": "CSV file containing calibration controls", + }, }, } }, @@ -239,6 +253,10 @@ async def create_score_calibration_route( None, description=f"CSV file containing variant classifications. This file must contain two columns: '{calibration_variant_column_name}' and '{calibration_class_column_name}'.", ), + controls_file: Optional[UploadFile] = File( + None, + description=f"CSV file containing calibration controls. This file must contain a variant column (one of '{calibration_variant_column_name}', '{hgvs_nt_column}', '{hgvs_pro_column}') and a '{calibration_control_status_column_name}' column.", + ), db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user_with_email), ) -> ScoreCalibration: @@ -272,6 +290,7 @@ async def create_score_calibration_route( **Form Fields**: - `calibration_json` (string, required): JSON string containing the calibration data - `classes_file` (file, optional): CSV file containing variant classifications + - `controls_file` (file, optional): CSV file containing calibration controls **Example**: ```bash @@ -287,11 +306,16 @@ async def create_score_calibration_route( - If uploading a classes_file, it must be a valid CSV with variant classification data - User must have ADD_CALIBRATION permission on the score set (any authenticated user for published sets; contributors/owners/admins for private sets) - + ## File Upload Details The `classes_file` parameter accepts CSV files containing variant classification data. The file should have appropriate headers and contain columns for variant urns and class names. + The `controls_file` parameter accepts a CSV of calibration controls with a variant column + (one of `variant_urn`, `hgvs_nt`, `hgvs_pro`) and a `clinical_status` column (`pathogenic` or + `benign`, case-insensitive). Controls may be supplied either via this file or inline in + `calibration_json`, but not both. + ## Response Returns the created score calibration with its generated URN and associated score set information. """ @@ -338,6 +362,29 @@ async def create_score_calibration_route( detail=[{"loc": [e.custom_loc or "classesFile"], "msg": str(e), "type": "value_error"}], ) + # Controls may be supplied inline in the JSON payload or as a CSV, but not both. + if calibration.controls is not None and controls_file: + raise HTTPException( + status_code=422, + detail="Provide calibration controls either inline or via controls_file, not both.", + ) + + if controls_file: + try: + controls_df = csv_data_to_df(controls_file.file, induce_hgvs_cols=False) + except UnicodeDecodeError as e: + raise HTTPException( + status_code=400, detail=f"Error decoding file: {e}. Ensure the file has correct values." + ) + + try: + calibration.controls = validate_and_standardize_calibration_controls_dataframe(db, score_set, controls_df) + except ValidationError as e: + raise HTTPException( + status_code=422, + detail=[{"loc": [e.custom_loc or "controlsFile"], "msg": str(e), "type": "value_error"}], + ) + created_calibration = await create_score_calibration_in_score_set( db, calibration, user_data.user, variant_classes if classes_file else None ) @@ -372,6 +419,11 @@ async def create_score_calibration_route( "format": "binary", "description": "CSV file containing updated variant classifications", }, + "controls_file": { + "type": "string", + "format": "binary", + "description": "CSV file containing updated calibration controls (replaces existing)", + }, }, } }, @@ -388,6 +440,10 @@ async def modify_score_calibration_route( None, description=f"CSV file containing variant classifications. This file must contain two columns: '{calibration_variant_column_name}' and '{calibration_class_column_name}'.", ), + controls_file: Optional[UploadFile] = File( + None, + description=f"CSV file containing calibration controls. This file must contain a variant column (one of '{calibration_variant_column_name}', '{hgvs_nt_column}', '{hgvs_pro_column}') and a '{calibration_control_status_column_name}' column. Replaces existing controls.", + ), db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user_with_email), ) -> ScoreCalibration: @@ -421,6 +477,7 @@ async def modify_score_calibration_route( **Form Fields**: - `calibration_json` (string, required): JSON string containing the calibration update data - `classes_file` (file, optional): CSV file containing updated variant classifications + - `controls_file` (file, optional): CSV file containing calibration controls (replaces existing) **Example**: ```bash @@ -442,6 +499,12 @@ async def modify_score_calibration_route( The file should have appropriate headers and follow the expected format for variant classifications within the associated score set. + The `controls_file` parameter accepts a CSV of calibration controls with a variant column + (one of `variant_urn`, `hgvs_nt`, `hgvs_pro`) and a `clinical_status` column (`pathogenic` or + `benign`, case-insensitive). If provided, it replaces the calibration's existing controls. + Controls may be supplied either via this file or inline in `calibration_json`, but not both; + omitting both leaves existing controls unchanged. + ## Response Returns the updated score calibration with all modifications applied and any new classification data from the uploaded file. @@ -506,6 +569,32 @@ async def modify_score_calibration_route( detail=[{"loc": [e.custom_loc or "classesFile"], "msg": str(e), "type": "value_error"}], ) + # Controls may be supplied inline in the JSON payload or as a CSV, but not both. A controls_file + # replaces existing controls; omitting both leaves them unchanged (see modify replace semantics). + if calibration_update.controls is not None and controls_file: + raise HTTPException( + status_code=422, + detail="Provide calibration controls either inline or via controls_file, not both.", + ) + + if controls_file: + try: + controls_df = csv_data_to_df(controls_file.file, induce_hgvs_cols=False) + except UnicodeDecodeError as e: + raise HTTPException( + status_code=400, detail=f"Error decoding file: {e}. Ensure the file has correct values." + ) + + try: + calibration_update.controls = validate_and_standardize_calibration_controls_dataframe( + db, score_set, controls_df + ) + except ValidationError as e: + raise HTTPException( + status_code=422, + detail=[{"loc": [e.custom_loc or "controlsFile"], "msg": str(e), "type": "value_error"}], + ) + updated_calibration = await modify_score_calibration( db, item, calibration_update, user_data.user, variant_classes if classes_file else None ) diff --git a/tests/routers/test_calibration_controls.py b/tests/routers/test_calibration_controls.py index eeab1bba1..6d7fc55ba 100644 --- a/tests/routers/test_calibration_controls.py +++ b/tests/routers/test_calibration_controls.py @@ -6,11 +6,18 @@ cdot = pytest.importorskip("cdot") fastapi = pytest.importorskip("fastapi") +import json + from sqlalchemy import select +from mavedb.lib.validation.constants.general import ( + calibration_control_status_column_name, + calibration_variant_column_name, +) from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.calibration_control_status import CalibrationControlStatus from mavedb.models.score_calibration import ScoreCalibration as CalibrationDbModel +from mavedb.models.score_set import ScoreSet as ScoreSetDbModel from mavedb.models.variant import Variant from tests.helpers.constants import ( TEST_BIORXIV_IDENTIFIER, @@ -28,6 +35,27 @@ ] +def _score_set_with_variant_urns(client, session, data_provider, data_files): + """Create a score set with mapped variants; return (score_set dict, list of its variant URNs).""" + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, session, data_provider, experiment["urn"], data_files / "scores.csv" + ) + score_set_orm = session.query(ScoreSetDbModel).where(ScoreSetDbModel.urn == score_set["urn"]).one() + variant_urns = [ + variant.urn + for variant in session.scalars(select(Variant).where(Variant.score_set_id == score_set_orm.id)).all() + ] + return score_set, variant_urns + + +def _calibration_payload(score_set_urn, **extra): + return {**deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED), "scoreSetUrn": score_set_urn, **extra} + + +CONTROLS_CSV_HEADER = f"{calibration_variant_column_name},{calibration_control_status_column_name}" + + def _create_private_calibration(client, session, data_provider, data_files): """Create a score set with variants and a private range-based calibration; return the calibration dict.""" experiment = create_experiment(client) @@ -109,3 +137,200 @@ def test_can_publish_calibration_without_controls_regardless_of_phi_flag( assert response.status_code == 200 assert response.json()["private"] is False + + +########################################################### +# Controls create / update / read wiring (#753) +########################################################### + + +def _create_with_controls(client, score_set_urn, controls, **extra): + response = client.post( + "/api/v1/score-calibrations/", json=_calibration_payload(score_set_urn, controls=controls, **extra) + ) + assert response.status_code == 200, response.text + return response.json() + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_create_calibration_with_inline_controls( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + + calibration = _create_with_controls( + client, + score_set["urn"], + [ + {"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}, + {"variantUrn": variant_urns[1], "clinicalStatus": "benign"}, + ], + ) + + assert {(c["variantUrn"], c["clinicalStatus"]) for c in calibration["controls"]} == { + (variant_urns[0], "pathogenic"), + (variant_urns[1], "benign"), + } + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_create_calibration_with_controls_file( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + csv = f"{CONTROLS_CSV_HEADER}\n{variant_urns[0]},pathogenic\n{variant_urns[1]},Benign\n" + + response = client.post( + "/api/v1/score-calibrations/", + data={"calibration_json": json.dumps(_calibration_payload(score_set["urn"]))}, + files={"controls_file": ("controls.csv", csv, "text/csv")}, + ) + + assert response.status_code == 200, response.text + assert {(c["variantUrn"], c["clinicalStatus"]) for c in response.json()["controls"]} == { + (variant_urns[0], "pathogenic"), + (variant_urns[1], "benign"), + } + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_create_calibration_rejects_inline_controls_and_controls_file_together( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + csv = f"{CONTROLS_CSV_HEADER}\n{variant_urns[0]},pathogenic\n" + + response = client.post( + "/api/v1/score-calibrations/", + data={ + "calibration_json": json.dumps( + _calibration_payload( + score_set["urn"], controls=[{"variantUrn": variant_urns[0], "clinicalStatus": "benign"}] + ) + ) + }, + files={"controls_file": ("controls.csv", csv, "text/csv")}, + ) + + assert response.status_code == 422 + assert "not both" in str(response.json()["detail"]) + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_get_calibration_returns_controls( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, score_set["urn"], [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}] + ) + + response = client.get(f"/api/v1/score-calibrations/{calibration['urn']}") + + assert response.status_code == 200 + assert response.json()["controls"][0]["variantUrn"] == variant_urns[0] + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_modify_replaces_controls(client, setup_router_db, mock_publication_fetch, session, data_provider, data_files): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, score_set["urn"], [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}] + ) + + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload( + score_set["urn"], controls=[{"variantUrn": variant_urns[1], "clinicalStatus": "benign"}] + ), + ) + + assert response.status_code == 200, response.text + controls = response.json()["controls"] + assert len(controls) == 1 + assert controls[0]["variantUrn"] == variant_urns[1] + assert controls[0]["clinicalStatus"] == "benign" + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_modify_with_empty_controls_clears_them( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, score_set["urn"], [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}] + ) + + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload(score_set["urn"], controls=[]), + ) + + assert response.status_code == 200, response.text + assert response.json()["controls"] == [] + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_modify_without_controls_leaves_them_unchanged( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, score_set["urn"], [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}] + ) + + # The payload omits controls entirely, so existing controls must be preserved. + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", json=_calibration_payload(score_set["urn"]) + ) + + assert response.status_code == 200, response.text + assert len(response.json()["controls"]) == 1 + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_modify_controls_without_affirmation_resets_phi_flag( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, + score_set["urn"], + [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}], + controlsNotPhi=True, + ) + assert calibration["controlsNotPhi"] is True + + # Changing controls without re-affirming invalidates the prior affirmation. + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload( + score_set["urn"], controls=[{"variantUrn": variant_urns[1], "clinicalStatus": "benign"}] + ), + ) + + assert response.status_code == 200, response.text + assert response.json()["controlsNotPhi"] is None + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_modify_controls_with_affirmation_keeps_phi_flag( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, score_set["urn"], [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}] + ) + + # Affirming in the same request that changes controls keeps the flag set. + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload( + score_set["urn"], + controls=[{"variantUrn": variant_urns[1], "clinicalStatus": "benign"}], + controlsNotPhi=True, + ), + ) + + assert response.status_code == 200, response.text + assert response.json()["controlsNotPhi"] is True From 11ca9a265e090c8741d23d58daeac85a04493406 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 16 Sep 2026 17:27:34 -0700 Subject: [PATCH 09/19] feat(calibration-controls): report controlsCount on list responses (#753) Add a controls_count correlated-COUNT column_property to ScoreCalibration (mirroring FunctionalClassification.variant_count). Move the full controls list off SavedScoreCalibration onto the detail models (the full ScoreCalibration and a new ScoreCalibrationDetailWithScoreSetUrn), and add controls_count to the base. The single-item calibration endpoints return the detail model (full controls + count); the two list endpoints return count-only. Net: calibration list responses and score-set-embedded SavedScoreCalibration carry controlsCount, while single-calibration detail, score-set full detail, and the public dump keep the full controls list. --- src/mavedb/models/score_calibration.py | 16 +++++++++++--- src/mavedb/routers/score_calibrations.py | 14 ++++++------- src/mavedb/view_models/score_calibration.py | 17 +++++++++++++-- tests/helpers/constants.py | 2 ++ tests/routers/test_calibration_controls.py | 23 +++++++++++++++++++++ 5 files changed, 60 insertions(+), 12 deletions(-) diff --git a/src/mavedb/models/score_calibration.py b/src/mavedb/models/score_calibration.py index 08f4b3a0f..d32b3ed9a 100644 --- a/src/mavedb/models/score_calibration.py +++ b/src/mavedb/models/score_calibration.py @@ -5,18 +5,18 @@ from datetime import date from typing import TYPE_CHECKING -from sqlalchemy import Boolean, Column, Date, Float, ForeignKey, Integer, String +from sqlalchemy import Boolean, Column, Date, Float, ForeignKey, Integer, String, func, select from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.ext.associationproxy import AssociationProxy, association_proxy -from sqlalchemy.orm import Mapped, relationship +from sqlalchemy.orm import Mapped, column_property, relationship from mavedb.db.base import Base from mavedb.lib.urns import generate_calibration_urn +from mavedb.models.calibration_control import CalibrationControl from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification from mavedb.models.score_calibration_publication_identifier import ScoreCalibrationPublicationIdentifierAssociation if TYPE_CHECKING: - from mavedb.models.calibration_control import CalibrationControl from mavedb.models.publication_identifier import PublicationIdentifier from mavedb.models.score_set import ScoreSet from mavedb.models.user import User @@ -67,6 +67,16 @@ class ScoreCalibration(Base): cascade="all, delete-orphan", ) + # Efficient count via correlated subquery — lets list/collection responses report how many + # controls a calibration has without loading the control rows. Mirrors + # ``ScoreCalibrationFunctionalClassification.variant_count``. + controls_count: Mapped[int] = column_property( + select(func.count(CalibrationControl.id)) + .where(CalibrationControl.calibration_id == id) + .correlate_except(CalibrationControl) + .scalar_subquery() + ) + publication_identifier_associations: Mapped[list[ScoreCalibrationPublicationIdentifierAssociation]] = relationship( "ScoreCalibrationPublicationIdentifierAssociation", back_populates="score_calibration", diff --git a/src/mavedb/routers/score_calibrations.py b/src/mavedb/routers/score_calibrations.py index d585dd45c..bc5c48e8d 100644 --- a/src/mavedb/routers/score_calibrations.py +++ b/src/mavedb/routers/score_calibrations.py @@ -87,7 +87,7 @@ def list_my_calibrations( @router.get( "/{urn}", - response_model=score_calibration.ScoreCalibrationWithScoreSetUrn, + response_model=score_calibration.ScoreCalibrationDetailWithScoreSetUrn, responses={404: {}}, ) def get_score_calibration( @@ -157,7 +157,7 @@ async def get_score_calibrations_for_score_set( @router.get( "/score-set/{score_set_urn}/primary", - response_model=score_calibration.ScoreCalibrationWithScoreSetUrn, + response_model=score_calibration.ScoreCalibrationDetailWithScoreSetUrn, responses={404: {}}, ) async def get_primary_score_calibrations_for_score_set( @@ -211,7 +211,7 @@ async def get_primary_score_calibrations_for_score_set( @router.post( "/", - response_model=score_calibration.ScoreCalibrationWithScoreSetUrn, + response_model=score_calibration.ScoreCalibrationDetailWithScoreSetUrn, responses={404: {}, 422: {"description": "Validation Error"}}, openapi_extra={ "requestBody": { @@ -397,7 +397,7 @@ async def create_score_calibration_route( @router.put( "/{urn}", - response_model=score_calibration.ScoreCalibrationWithScoreSetUrn, + response_model=score_calibration.ScoreCalibrationDetailWithScoreSetUrn, responses={404: {}, 422: {"description": "Validation Error"}}, openapi_extra={ "requestBody": { @@ -642,7 +642,7 @@ async def delete_score_calibration_route( @router.post( "/{urn}/promote-to-primary", - response_model=score_calibration.ScoreCalibrationWithScoreSetUrn, + response_model=score_calibration.ScoreCalibrationDetailWithScoreSetUrn, responses={404: {}}, ) async def promote_score_calibration_to_primary_route( @@ -715,7 +715,7 @@ async def promote_score_calibration_to_primary_route( @router.post( "/{urn}/demote-from-primary", - response_model=score_calibration.ScoreCalibrationWithScoreSetUrn, + response_model=score_calibration.ScoreCalibrationDetailWithScoreSetUrn, responses={404: {}}, ) def demote_score_calibration_from_primary_route( @@ -754,7 +754,7 @@ def demote_score_calibration_from_primary_route( @router.post( "/{urn}/publish", - response_model=score_calibration.ScoreCalibrationWithScoreSetUrn, + response_model=score_calibration.ScoreCalibrationDetailWithScoreSetUrn, responses={404: {}}, ) def publish_score_calibration_route( diff --git a/src/mavedb/view_models/score_calibration.py b/src/mavedb/view_models/score_calibration.py index 87cde6f5a..3406e2b6f 100644 --- a/src/mavedb/view_models/score_calibration.py +++ b/src/mavedb/view_models/score_calibration.py @@ -507,7 +507,9 @@ class SavedScoreCalibration(ScoreCalibrationBase): private: bool = True functional_classifications: Optional[Sequence[SavedFunctionalClassification]] = None - controls: Sequence[SavedCalibrationControl] = [] + # The full controls list lives on the detail models; the base (and any list/collection response) + # carries only the count to keep those payloads small. See ScoreCalibrationDetailWithScoreSetUrn. + controls_count: int = 0 threshold_sources: Sequence[SavedPublicationIdentifier] evidence_sources: Sequence[SavedPublicationIdentifier] method_sources: Sequence[SavedPublicationIdentifier] @@ -577,6 +579,7 @@ class ScoreCalibration(SavedScoreCalibration): """Complete score calibration model returned by the API.""" functional_classifications: Optional[Sequence[FunctionalClassification]] = None + controls: Sequence[SavedCalibrationControl] = [] threshold_sources: Sequence[PublicationIdentifier] evidence_sources: Sequence[PublicationIdentifier] method_sources: Sequence[PublicationIdentifier] @@ -585,7 +588,11 @@ class ScoreCalibration(SavedScoreCalibration): class ScoreCalibrationWithScoreSetUrn(SavedScoreCalibration): - """Complete score calibration model returned by the API, with score_set_urn.""" + """Score calibration model with score_set_urn, used for list/collection responses. + + Carries ``controls_count`` (from the base) but not the full controls list — see + ``ScoreCalibrationDetailWithScoreSetUrn`` for the single-item detail representation. + """ score_set_urn: str @@ -599,3 +606,9 @@ def generate_score_set_urn(cls, data: Any): f"Unable to coerce score set urn for {cls.__name__}: {exc}." # type: ignore ) return data + + +class ScoreCalibrationDetailWithScoreSetUrn(ScoreCalibrationWithScoreSetUrn): + """Single-calibration detail response: adds the full controls list to the list model.""" + + controls: Sequence[SavedCalibrationControl] = [] diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index faabe6d18..fb36eee2b 100644 --- a/tests/helpers/constants.py +++ b/tests/helpers/constants.py @@ -1692,6 +1692,8 @@ "primary": True, "private": False, "scoreSetId": 1, + "controls": [], + "controlsCount": 0, "createdBy": { "recordType": "User", "firstName": TEST_USER["first_name"], diff --git a/tests/routers/test_calibration_controls.py b/tests/routers/test_calibration_controls.py index 6d7fc55ba..660be6e97 100644 --- a/tests/routers/test_calibration_controls.py +++ b/tests/routers/test_calibration_controls.py @@ -231,6 +231,29 @@ def test_get_calibration_returns_controls( assert response.json()["controls"][0]["variantUrn"] == variant_urns[0] +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_list_endpoint_reports_controls_count_without_full_controls( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + _create_with_controls( + client, + score_set["urn"], + [ + {"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}, + {"variantUrn": variant_urns[1], "clinicalStatus": "benign"}, + ], + ) + + response = client.get(f"/api/v1/score-calibrations/score-set/{score_set['urn']}") + + assert response.status_code == 200, response.text + item = response.json()[0] + # List responses carry the count, not the full controls list. + assert item["controlsCount"] == 2 + assert "controls" not in item + + @pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) def test_modify_replaces_controls(client, setup_router_db, mock_publication_fetch, session, data_provider, data_files): score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) From 8eee8e9b99191feeeef72722108ef420b4833fad Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Thu, 17 Sep 2026 09:54:16 -0700 Subject: [PATCH 10/19] feat(calibration): model disease as a MONDO-coded concept (#754) Replace the free-text calibration disease column with a MONDO-coded disease_term FK into a new mondo_terms table, served on the wire as a GA4GH MappableConcept. The API input is the bare MONDO code (e.g. "MONDO:0015263"); the server resolves the canonical label and system against EBI OLS4 and get-or-creates the term on save. A null code resolves to the seeded generic "disease or disorder" root (MONDO:0000001), so the FK is never null. Validation against OLS is mandatory: an unknown code is rejected and an OLS outage raises MondoServiceError (503) rather than persisting an unvalidated label. Add a /diseases/search proxy over OLS4's /select endpoint to back the disease typeahead. The migration seeds the generic term and backfills existing calibrations before enforcing NOT NULL. Covered by unit tests that mock OLS, plus a network-marked canary suite exercising the real OLS4 service so upstream shape changes surface. --- ...add_calibration_controls_table_and_phi_.py | 54 ++++- src/mavedb/lib/exceptions.py | 6 + src/mavedb/lib/mondo.py | 203 ++++++++++++++++++ src/mavedb/lib/score_calibrations.py | 7 + src/mavedb/models/__init__.py | 1 + src/mavedb/models/mondo_term.py | 39 ++++ src/mavedb/models/score_calibration.py | 9 +- src/mavedb/routers/diseases.py | 42 ++++ src/mavedb/server_main.py | 12 ++ src/mavedb/view_models/score_calibration.py | 28 ++- tests/helpers/constants.py | 13 ++ tests/helpers/mocks/factories.py | 12 +- tests/lib/annotation/test_contribution.py | 2 + tests/lib/mondo/__init__.py | 0 tests/lib/mondo/network/__init__.py | 0 tests/lib/mondo/network/test_mondo.py | 54 +++++ tests/lib/mondo/test_mondo.py | 199 +++++++++++++++++ tests/lib/test_calibration_control.py | 2 + tests/routers/test_diseases.py | 51 +++++ tests/routers/test_score_calibrations.py | 67 ++++++ tests/routers/test_variant.py | 2 + tests/scripts/conftest_optional.py | 2 + tests/view_models/test_calibration_control.py | 15 +- 23 files changed, 806 insertions(+), 14 deletions(-) create mode 100644 src/mavedb/lib/mondo.py create mode 100644 src/mavedb/models/mondo_term.py create mode 100644 src/mavedb/routers/diseases.py create mode 100644 tests/lib/mondo/__init__.py create mode 100644 tests/lib/mondo/network/__init__.py create mode 100644 tests/lib/mondo/network/test_mondo.py create mode 100644 tests/lib/mondo/test_mondo.py create mode 100644 tests/routers/test_diseases.py diff --git a/alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py b/alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py index 1743d8ff6..b0999f4ef 100644 --- a/alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py +++ b/alembic/versions/024370c4bca7_add_calibration_controls_table_and_phi_.py @@ -1,4 +1,4 @@ -"""add calibration controls table and phi/disease columns +"""add calibration controls, mondo terms, and phi/disease columns Revision ID: 024370c4bca7 Revises: a7f3c2e9b104 @@ -16,6 +16,11 @@ branch_labels = None depends_on = None +# The generic "disease or disorder" MONDO term seeded so the non-nullable calibration FK always resolves. +MONDO_SYSTEM = "https://purl.obolibrary.org/obo/mondo.owl" +MONDO_GENERIC_CODE = "MONDO:0000001" +MONDO_GENERIC_LABEL = "disease or disorder" + def upgrade(): op.create_table( @@ -50,13 +55,56 @@ def upgrade(): op.f("ix_calibration_controls_modified_by_id"), "calibration_controls", ["modified_by_id"], unique=False ) - op.add_column("score_calibrations", sa.Column("disease", sa.String(), nullable=True)) + # MONDO disease terms, with the generic root seeded as the default for calibrations naming no disease. + op.create_table( + "mondo_terms", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("code", sa.String(), nullable=False), + sa.Column("system", sa.String(), nullable=False), + sa.Column("system_version", sa.String(), nullable=True), + sa.Column("label", sa.String(), nullable=False), + sa.Column("creation_date", sa.Date(), nullable=False), + sa.Column("modification_date", sa.Date(), nullable=False), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("system", "code", name="uq_mondo_terms_system_code"), + ) + op.execute( + sa.text( + "INSERT INTO mondo_terms (code, system, label, creation_date, modification_date) " + "VALUES (:code, :system, :label, CURRENT_DATE, CURRENT_DATE)" + ).bindparams(code=MONDO_GENERIC_CODE, system=MONDO_SYSTEM, label=MONDO_GENERIC_LABEL) + ) + + # Disease FK: add nullable, backfill existing calibrations to the generic term, then enforce NOT NULL. + op.add_column("score_calibrations", sa.Column("disease_term_id", sa.Integer(), nullable=True)) + op.execute( + sa.text( + "UPDATE score_calibrations SET disease_term_id = " + "(SELECT id FROM mondo_terms WHERE system = :system AND code = :code)" + ).bindparams(system=MONDO_SYSTEM, code=MONDO_GENERIC_CODE) + ) + op.alter_column("score_calibrations", "disease_term_id", nullable=False) + op.create_index( + op.f("ix_score_calibrations_disease_term_id"), "score_calibrations", ["disease_term_id"], unique=False + ) + op.create_foreign_key( + "fk_score_calibrations_disease_term_id_mondo_terms", + "score_calibrations", + "mondo_terms", + ["disease_term_id"], + ["id"], + ) + op.add_column("score_calibrations", sa.Column("controls_not_phi", sa.Boolean(), nullable=True)) def downgrade(): op.drop_column("score_calibrations", "controls_not_phi") - op.drop_column("score_calibrations", "disease") + + op.drop_constraint("fk_score_calibrations_disease_term_id_mondo_terms", "score_calibrations", type_="foreignkey") + op.drop_index(op.f("ix_score_calibrations_disease_term_id"), table_name="score_calibrations") + op.drop_column("score_calibrations", "disease_term_id") + op.drop_table("mondo_terms") op.drop_index(op.f("ix_calibration_controls_modified_by_id"), table_name="calibration_controls") op.drop_index(op.f("ix_calibration_controls_created_by_id"), table_name="calibration_controls") diff --git a/src/mavedb/lib/exceptions.py b/src/mavedb/lib/exceptions.py index 8b8633f1f..e0c079a67 100644 --- a/src/mavedb/lib/exceptions.py +++ b/src/mavedb/lib/exceptions.py @@ -222,6 +222,12 @@ class HGNCServiceError(Exception): pass +class MondoServiceError(Exception): + """Raised when the MONDO/OLS disease ontology service cannot provide search results.""" + + pass + + class LDHSubmissionFailureError(Exception): """Raised when submission to ClinGen Linked Data Hub (LDH) fails for all submissions.""" diff --git a/src/mavedb/lib/mondo.py b/src/mavedb/lib/mondo.py new file mode 100644 index 000000000..dbabbf01d --- /dev/null +++ b/src/mavedb/lib/mondo.py @@ -0,0 +1,203 @@ +"""MONDO disease-term resolution and search. + +Calibrations carry a disease/disorder context drawn from the Monarch Disease Ontology (MONDO). This +module owns the MONDO vocabulary integration: get-or-create of :class:`MondoTerm` rows (mirroring +:func:`mavedb.lib.taxonomies.find_or_create_taxonomy`), a typeahead search + validation against the +EBI OLS4 service, and conversion of a stored term to the GA4GH ``MappableConcept`` served on the wire. + +The generic term (``MONDO:0000001``, "disease or disorder") is the write-time default for a calibration +with no specific disease, so ``disease_term_id`` is never null. The annotation layer serializes whatever +term a calibration carries — generic or specific — as its VA-Spec disease condition (see +:func:`mavedb.lib.annotation.condition.calibration_disease_condition`). +""" + +import logging +from typing import Any, Optional, TypedDict + +import httpx +from ga4gh.core.models import Coding, MappableConcept, iriReference +from sqlalchemy.orm import Session + +from mavedb.lib.exceptions import MondoServiceError +from mavedb.lib.logging.context import format_raised_exception_info_as_dict, logging_context, save_to_logging_context +from mavedb.lib.validation.exceptions import ValidationError +from mavedb.models.mondo_term import MondoTerm + +logger = logging.getLogger(__name__) + +# The Monarch Disease Ontology (MONDO), the controlled vocabulary for disease terms used across MaveDB. +MONDO_SYSTEM = "https://purl.obolibrary.org/obo/mondo.owl" +# EBI Ontology Lookup Service (OLS4) — the source for MONDO term search and validation. +OLS_BASE_URL = "https://www.ebi.ac.uk/ols4/api" +MONDO_ONTOLOGY = "mondo" +# OLS can be slow; the default httpx 5s timeout is too tight for its autocomplete on short queries. +OLS_TIMEOUT = 10.0 + +# The generic "disease or disorder" root, used when a calibration names no specific disease. +MONDO_GENERIC_CODE = "MONDO:0000001" +MONDO_GENERIC_LABEL = "disease or disorder" + +DEFAULT_SEARCH_LIMIT = 20 + + +class MondoSuggestion(TypedDict): + """A single MONDO term resolved from OLS: the CURIE code, its label, and its resolvable IRI.""" + + code: str + label: str + iri: str + + +def mondo_iri(code: str) -> str: + """The resolvable OBO IRI for a MONDO CURIE (e.g. ``MONDO:0015263`` → ``.../obo/MONDO_0015263``).""" + return f"https://purl.obolibrary.org/obo/{code.replace(':', '_')}" + + +def mondo_term_to_mappable_concept(term: MondoTerm) -> MappableConcept: + """Serialize a stored :class:`MondoTerm` as a GA4GH disease ``MappableConcept``.""" + return MappableConcept( + conceptType="Disease", + name=term.label, + primaryCoding=Coding( + code=term.code, + system=term.system, + systemVersion=term.system_version, + iris=[iriReference(root=mondo_iri(term.code))], + ), + ) + + +def mondo_suggestion_to_mappable_concept(suggestion: MondoSuggestion) -> MappableConcept: + """Build a disease ``MappableConcept`` from an OLS search suggestion (for typeahead results).""" + return MappableConcept( + conceptType="Disease", + name=suggestion["label"], + primaryCoding=Coding( + code=suggestion["code"], + system=MONDO_SYSTEM, + iris=[iriReference(root=suggestion["iri"])], + ), + ) + + +def generic_disease_mappable_concept() -> MappableConcept: + """The generic "disease or disorder" concept, MaveDB's single unspecified-disease sentinel.""" + return MappableConcept( + conceptType="Disease", + name=MONDO_GENERIC_LABEL, + primaryCoding=Coding( + code=MONDO_GENERIC_CODE, + system=MONDO_SYSTEM, + iris=[iriReference(root=mondo_iri(MONDO_GENERIC_CODE))], + ), + ) + + +async def search_mondo(query: str, limit: int = DEFAULT_SEARCH_LIMIT) -> list[MondoSuggestion]: + """Search MONDO disease classes by free text via OLS, for the disease typeahead.""" + if not query or not query.strip(): + return [] + + # OLS4's /select endpoint is the autocomplete-optimized path; /search times out on broad prefixes + # (e.g. a two-letter query), which is often what a typeahead sends. + params: dict[str, Any] = { + "q": query.strip(), + "ontology": MONDO_ONTOLOGY, + "rows": limit, + "fieldList": "iri,label,obo_id", + } + save_to_logging_context({"mondo_search_query": query, "mondo_search_limit": limit}) + try: + async with httpx.AsyncClient(timeout=OLS_TIMEOUT) as client: + response = await client.get(f"{OLS_BASE_URL}/select", params=params) + response.raise_for_status() + docs = response.json().get("response", {}).get("docs", []) + except httpx.HTTPError as exc: + save_to_logging_context(format_raised_exception_info_as_dict(exc)) + logger.error(msg="MONDO/OLS search request failed.", exc_info=exc, extra=logging_context()) + raise MondoServiceError("Disease ontology service temporarily unavailable") from exc + + return [ + MondoSuggestion(code=doc["obo_id"], label=doc["label"], iri=doc.get("iri", mondo_iri(doc["obo_id"]))) + for doc in docs + if doc.get("obo_id") and doc.get("label") + ] + + +async def fetch_mondo_term(code: str) -> Optional[MondoSuggestion]: + """Resolve one MONDO CURIE to its canonical label + IRI via OLS, or None if it does not exist.""" + params: dict[str, Any] = { + "q": code, + "ontology": MONDO_ONTOLOGY, + "queryFields": "obo_id", + "exact": "true", + "rows": 1, + "fieldList": "iri,label,obo_id", + } + try: + async with httpx.AsyncClient(timeout=OLS_TIMEOUT) as client: + response = await client.get(f"{OLS_BASE_URL}/search", params=params) + response.raise_for_status() + docs = response.json().get("response", {}).get("docs", []) + except httpx.HTTPError as exc: + save_to_logging_context(format_raised_exception_info_as_dict(exc)) + logger.error(msg="MONDO/OLS term lookup failed.", exc_info=exc, extra=logging_context()) + raise MondoServiceError("Disease ontology service temporarily unavailable") from exc + + if not docs: + return None + + doc = docs[0] + return MondoSuggestion(code=doc["obo_id"], label=doc["label"], iri=doc.get("iri", mondo_iri(code))) + + +def get_generic_disease_term(db: Session) -> MondoTerm: + """Get-or-create the generic disease term. Known-canonical, so it needs no OLS round trip.""" + term = ( + db.query(MondoTerm).filter(MondoTerm.system == MONDO_SYSTEM, MondoTerm.code == MONDO_GENERIC_CODE).one_or_none() + ) + if term is None: + term = MondoTerm(code=MONDO_GENERIC_CODE, system=MONDO_SYSTEM, label=MONDO_GENERIC_LABEL) # type: ignore[call-arg] + db.add(term) + db.flush() + + return term + + +async def find_or_create_mondo_term(db: Session, code: str) -> MondoTerm: + """Find an existing MONDO term by code, or create one after validating it against OLS. + + A new code is validated against OLS and its canonical label stored. An unknown code raises + ``ValidationError``; an OLS outage raises ``MondoServiceError`` (via :func:`fetch_mondo_term`) so a + term is never persisted without a validated label — a controlled vocabulary must stay validated. + """ + # The generic term is canonical and seeded — resolve it directly, never via OLS. + if code == MONDO_GENERIC_CODE: + return get_generic_disease_term(db) + + term = db.query(MondoTerm).filter(MondoTerm.system == MONDO_SYSTEM, MondoTerm.code == code).one_or_none() + if term is not None: + return term + + resolved = await fetch_mondo_term(code) + if resolved is None: + raise ValidationError(f"'{code}' is not a valid MONDO disease term.", custom_loc=["body", "disease"]) + + term = MondoTerm(code=resolved["code"], system=MONDO_SYSTEM, label=resolved["label"]) # type: ignore[call-arg] + db.add(term) + db.flush() + + return term + + +async def resolve_disease_term(db: Session, code: Optional[str]) -> MondoTerm: + """Resolve a calibration's submitted disease code to a stored MONDO term. + + ``None`` (no disease specified) resolves to the generic "disease or disorder" term; any other code + is validated/created via :func:`find_or_create_mondo_term`. Shared by the score calibration create + and update paths so the write-time default lives in one place. + """ + if code is None: + return get_generic_disease_term(db) + + return await find_or_create_mondo_term(db, code) diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index 73092fc69..5d270e043 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -10,6 +10,7 @@ from mavedb.lib.acmg import find_or_create_acmg_classification from mavedb.lib.identifiers import find_or_create_publication_identifier +from mavedb.lib.mondo import resolve_disease_term from mavedb.lib.types.score_calibrations import ClassificationDict from mavedb.lib.validation.constants.general import ( calibration_class_column_name, @@ -284,6 +285,7 @@ async def _create_score_calibration( exclude={ "functional_classifications", "controls", + "disease", "threshold_sources", "evidence_sources", "method_sources", @@ -296,6 +298,8 @@ async def _create_score_calibration( modified_by=user, ) # type: ignore[call-arg] + calibration.disease_term = await resolve_disease_term(db, getattr(calibration_create, "disease", None)) + if containing_score_set: calibration.score_set = containing_score_set calibration.score_set_id = containing_score_set.id @@ -567,6 +571,7 @@ async def modify_score_calibration( if attr not in { "functional_classifications", "controls", + "disease", # controls_not_phi carries re-acknowledgment semantics; set it explicitly so an # unrelated edit cannot silently wipe a prior affirmation. "controls_not_phi", @@ -581,6 +586,8 @@ async def modify_score_calibration( }: setattr(calibration, attr, value) + calibration.disease_term = await resolve_disease_term(db, getattr(calibration_update, "disease", None)) + calibration.score_set = containing_score_set calibration.score_set_id = containing_score_set.id calibration.publication_identifier_associations = updated_assocs diff --git a/src/mavedb/models/__init__.py b/src/mavedb/models/__init__.py index ba5334e99..2da3e40b7 100644 --- a/src/mavedb/models/__init__.py +++ b/src/mavedb/models/__init__.py @@ -17,6 +17,7 @@ "legacy_keyword", "license", "mapped_variant", + "mondo_term", "pipeline", "publication_identifier", "published_variant", diff --git a/src/mavedb/models/mondo_term.py b/src/mavedb/models/mondo_term.py new file mode 100644 index 000000000..7059e1b0a --- /dev/null +++ b/src/mavedb/models/mondo_term.py @@ -0,0 +1,39 @@ +"""SQLAlchemy model for MONDO disease terms. + +A ``MondoTerm`` is a single controlled-vocabulary entry (a MONDO code + its label) referenced by +calibrations as their disease/disorder context. The columns are deliberately the generic coding +columns (``system``/``code``/``system_version``/``label``) rather than MONDO-specific ones: this table +is the narrow, first consumer of a shared concept model, and matching the column shape of +``controlled_keywords`` (and the eventual ``mappable_concepts`` table) keeps a later promotion additive +rather than a reshape. On the wire a term is served as a GA4GH ``MappableConcept``. +""" + +from datetime import date +from typing import Optional + +from sqlalchemy import Date, Integer, String, UniqueConstraint +from sqlalchemy.orm import Mapped, mapped_column + +from mavedb.db.base import Base + + +class MondoTerm(Base): + __tablename__ = "mondo_terms" + + id: Mapped[int] = mapped_column(Integer, primary_key=True) + + # The coding: ``code`` is the MONDO CURIE (e.g. "MONDO:0015263"), ``system`` its terminology + # identifier. ``system`` is constant for MONDO today but stored explicitly so a promotion to a + # multi-system concept table needs no backfill. ``UNIQUE(system, code)`` dedupes shared terms. + code: Mapped[str] = mapped_column(String, nullable=False) + system: Mapped[str] = mapped_column(String, nullable=False) + system_version: Mapped[Optional[str]] = mapped_column(String, nullable=True) + label: Mapped[str] = mapped_column(String, nullable=False) + + creation_date: Mapped[date] = mapped_column(Date, nullable=False, default=date.today) + modification_date: Mapped[date] = mapped_column(Date, nullable=False, default=date.today, onupdate=date.today) + + __table_args__ = (UniqueConstraint("system", "code", name="uq_mondo_terms_system_code"),) + + def __repr__(self) -> str: # pragma: no cover - repr utility + return f"" diff --git a/src/mavedb/models/score_calibration.py b/src/mavedb/models/score_calibration.py index d32b3ed9a..e2c1fec7e 100644 --- a/src/mavedb/models/score_calibration.py +++ b/src/mavedb/models/score_calibration.py @@ -17,6 +17,7 @@ from mavedb.models.score_calibration_publication_identifier import ScoreCalibrationPublicationIdentifierAssociation if TYPE_CHECKING: + from mavedb.models.mondo_term import MondoTerm from mavedb.models.publication_identifier import PublicationIdentifier from mavedb.models.score_set import ScoreSet from mavedb.models.user import User @@ -42,10 +43,10 @@ class ScoreCalibration(Base): baseline_score = Column(Float, nullable=True) baseline_score_description = Column(String, nullable=True) - # Optional free-text disease/disorder context for the controls, at the calibration level. Deliberately not a - # controlled vocabulary and never recorded per-control: a per-control disease label could let a phenotype be - # inferred for an individual variant, which risks PHI. - disease = Column(String, nullable=True) + # MONDO-coded disease/disorder context. Non-nullable and defaulted to the generic "disease or disorder" term, + # so every calibration carries a concept and consumers never branch on null. + disease_term_id = Column(Integer, ForeignKey("mondo_terms.id"), nullable=False, index=True) + disease_term: Mapped["MondoTerm"] = relationship("MondoTerm") # Submitter's affirmation that the control data contains no PHI. Tristate on purpose: # None = not yet addressed, # False = explicitly declined, True = affirmed. Publishing a calibration that has controls is gated on this being diff --git a/src/mavedb/routers/diseases.py b/src/mavedb/routers/diseases.py new file mode 100644 index 000000000..6f2d5e70c --- /dev/null +++ b/src/mavedb/routers/diseases.py @@ -0,0 +1,42 @@ +from typing import Any + +from fastapi import APIRouter, Query +from ga4gh.core.models import MappableConcept + +from mavedb.lib.logging.context import save_to_logging_context +from mavedb.lib.logging.logged_route import LoggedRoute +from mavedb.lib.mondo import DEFAULT_SEARCH_LIMIT, mondo_suggestion_to_mappable_concept, search_mondo +from mavedb.routers.shared import GATEWAY_ERROR_RESPONSES, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX + +TAG_NAME = "Diseases" + +router = APIRouter( + prefix=f"{ROUTER_BASE_PREFIX}/diseases", + tags=[TAG_NAME], + responses={**PUBLIC_ERROR_RESPONSES}, + route_class=LoggedRoute, +) + +metadata = { + "name": TAG_NAME, + "description": "Search disease terms from the Monarch Disease Ontology (MONDO).", +} + + +@router.get( + "/search", + status_code=200, + response_model=list[MappableConcept], + summary="Search MONDO disease terms", + responses={**GATEWAY_ERROR_RESPONSES}, +) +async def search_diseases( + *, + q: str = Query(..., description="Free-text query for a MONDO disease term."), + limit: int = Query(DEFAULT_SEARCH_LIMIT, ge=1, le=100, description="Maximum number of results."), +) -> Any: + """Typeahead search for MONDO disease terms, returned as GA4GH ``MappableConcept`` suggestions.""" + save_to_logging_context({"query": q, "limit": limit}) + + suggestions = await search_mondo(q, limit) + return [mondo_suggestion_to_mappable_concept(suggestion) for suggestion in suggestions] diff --git a/src/mavedb/server_main.py b/src/mavedb/server_main.py index 880bfcfe1..be83c0d6f 100644 --- a/src/mavedb/server_main.py +++ b/src/mavedb/server_main.py @@ -25,6 +25,7 @@ HGNCGeneNotFoundError, HGNCServiceError, MixedTargetError, + MondoServiceError, NonexistentIdentifierError, ) from mavedb.lib.logging.canonical import log_request @@ -44,6 +45,7 @@ api_information, collections, controlled_keywords, + diseases, doi_identifiers, experiment_sets, experiments, @@ -100,6 +102,7 @@ app.include_router(api_information.router) app.include_router(collections.router) app.include_router(controlled_keywords.router) +app.include_router(diseases.router) app.include_router(doi_identifiers.router) app.include_router(experiment_sets.router) app.include_router(experiments.router) @@ -178,6 +181,14 @@ async def hgnc_service_error_exception_handler(request: Request, exc: HGNCServic return response +@app.exception_handler(MondoServiceError) +async def mondo_service_error_exception_handler(request: Request, exc: MondoServiceError): + response = JSONResponse(status_code=503, content={"message": "Disease ontology service temporarily unavailable"}) + save_to_logging_context(format_raised_exception_info_as_dict(exc)) + log_request(request, response, time.time_ns()) + return response + + @app.exception_handler(EutilsRequestError) async def nonexistent_pmid_error_exception_handler(request: Request, exc: EutilsRequestError): response = JSONResponse(status_code=404, content={"message": str(exc)}) @@ -248,6 +259,7 @@ def customize_openapi_schema(): api_information.metadata, collections.metadata, controlled_keywords.metadata, + diseases.metadata, doi_identifiers.metadata, experiment_sets.metadata, experiments.metadata, diff --git a/src/mavedb/view_models/score_calibration.py b/src/mavedb/view_models/score_calibration.py index 3406e2b6f..5d5e6337e 100644 --- a/src/mavedb/view_models/score_calibration.py +++ b/src/mavedb/view_models/score_calibration.py @@ -7,8 +7,10 @@ from datetime import date from typing import TYPE_CHECKING, Any, Collection, Optional, Sequence, Union +from ga4gh.core.models import MappableConcept from pydantic import Field, field_validator, model_validator +from mavedb.lib.mondo import generic_disease_mappable_concept, mondo_term_to_mappable_concept from mavedb.lib.oddspaths import oddspaths_evidence_strength_equivalent from mavedb.lib.validation.exceptions import ValidationError from mavedb.lib.validation.transform import ( @@ -286,7 +288,6 @@ class ScoreCalibrationBase(BaseModel): baseline_score_description: Optional[str] = None notes: Optional[str] = None - disease: Optional[str] = None controls_not_phi: Optional[bool] = None functional_classifications: Optional[Sequence[FunctionalClassificationBase]] = None @@ -441,6 +442,13 @@ class ScoreCalibrationModify(ScoreCalibrationBase): functional_classifications: Optional[Sequence[FunctionalClassificationModify]] = None # None means "no change" on modify; an empty list clears all controls. controls: Optional[Sequence[CalibrationControlCreate]] = None + disease: Optional[str] = Field( + None, + description=( + 'The MONDO code (e.g. "MONDO:0015263") for this calibration\'s disease context; validated ' + 'against OLS. Omitted or null resolves to the generic "disease or disorder" term.' + ), + ) threshold_sources: Sequence[PublicationIdentifierCreate] evidence_sources: Sequence[PublicationIdentifierCreate] method_sources: Sequence[PublicationIdentifierCreate] @@ -510,6 +518,9 @@ class SavedScoreCalibration(ScoreCalibrationBase): # The full controls list lives on the detail models; the base (and any list/collection response) # carries only the count to keep those payloads small. See ScoreCalibrationDetailWithScoreSetUrn. controls_count: int = 0 + # The FK is non-nullable, so this is always populated by ``generate_disease_concept`` when building + # from an ORM object; an unspecified disease resolves to the generic "disease or disorder" concept. + disease: MappableConcept threshold_sources: Sequence[SavedPublicationIdentifier] evidence_sources: Sequence[SavedPublicationIdentifier] method_sources: Sequence[SavedPublicationIdentifier] @@ -574,6 +585,21 @@ def generate_threshold_classification_and_method_sources(cls, data: Any): # typ ) return data + @model_validator(mode="before") + def generate_disease_concept(cls, data: Any): + """Serialize the stored MONDO term as a ``disease`` MappableConcept when building from an ORM object. + + Falls back to the generic concept if the relationship is unexpectedly empty, so the non-nullable + field is always satisfied. + """ + if hasattr(data, "disease_term"): + term = data.disease_term + data.__setattr__( + "disease", + mondo_term_to_mappable_concept(term) if term is not None else generic_disease_mappable_concept(), + ) + return data + class ScoreCalibration(SavedScoreCalibration): """Complete score calibration model returned by the API.""" diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index fb36eee2b..53e0a4e5e 100644 --- a/tests/helpers/constants.py +++ b/tests/helpers/constants.py @@ -1652,6 +1652,17 @@ } +SAVED_GENERIC_DISEASE_CONCEPT = { + "conceptType": "Disease", + "name": "disease or disorder", + "primaryCoding": { + "system": "https://purl.obolibrary.org/obo/mondo.owl", + "code": "MONDO:0000001", + "iris": ["https://purl.obolibrary.org/obo/MONDO_0000001"], + }, +} + + TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED = { "title": "Test BRNICH Score Calibration", "research_use_only": False, @@ -1694,6 +1705,7 @@ "scoreSetId": 1, "controls": [], "controlsCount": 0, + "disease": SAVED_GENERIC_DISEASE_CONCEPT, "createdBy": { "recordType": "User", "firstName": TEST_USER["first_name"], @@ -1764,6 +1776,7 @@ "private": False, "urn": VALID_CALIBRATION_URN, "scoreSetId": 1, + "disease": SAVED_GENERIC_DISEASE_CONCEPT, "createdBy": { "recordType": "User", "firstName": TEST_USER["first_name"], diff --git a/tests/helpers/mocks/factories.py b/tests/helpers/mocks/factories.py index 6e83d089c..c78532695 100644 --- a/tests/helpers/mocks/factories.py +++ b/tests/helpers/mocks/factories.py @@ -8,6 +8,7 @@ from datetime import date, datetime from unittest.mock import MagicMock +from mavedb.lib.mondo import MONDO_GENERIC_CODE, MONDO_GENERIC_LABEL, MONDO_SYSTEM from mavedb.models.enums.acmg_criterion import ACMGCriterion from mavedb.models.enums.functional_classification import FunctionalClassification as FunctionalClassificationOptions from mavedb.models.enums.strength_of_evidence import StrengthOfEvidenceProvided @@ -169,11 +170,19 @@ def create_mock_score_set( ) +def create_mock_mondo_term( + code=MONDO_GENERIC_CODE, label=MONDO_GENERIC_LABEL, system=MONDO_SYSTEM, system_version=None +): + """Create a mock MondoTerm, defaulting to the generic "disease or disorder" term.""" + return create_sealed_mock(code=code, label=label, system=system, system_version=system_version) + + def create_mock_score_calibration(functional_classifications=None, primary=True, **kwargs): """Create a mock ScoreCalibration for both Pydantic validation and direct attribute access. When ``functional_classifications`` is not provided a minimal default list is - created (single not_specified classification with no ACMG). + created (single not_specified classification with no ACMG). ``disease_term`` defaults to + the generic MONDO term (calibrations always carry a non-null disease FK). """ user = kwargs.get("created_by") or create_mock_user() @@ -207,6 +216,7 @@ def create_mock_score_calibration(functional_classifications=None, primary=True, "primary": primary, "private": kwargs.get("private", not primary), "researchUseOnly": kwargs.get("research_use_only", False), + "diseaseTerm": kwargs.get("disease_term", create_mock_mondo_term()), "thresholdSources": kwargs.get("threshold_sources", []), "classificationSources": kwargs.get("classification_sources", []), "methodSources": kwargs.get("method_sources", []), diff --git a/tests/lib/annotation/test_contribution.py b/tests/lib/annotation/test_contribution.py index 51597db37..986d86f15 100644 --- a/tests/lib/annotation/test_contribution.py +++ b/tests/lib/annotation/test_contribution.py @@ -23,6 +23,7 @@ mavedb_score_calibration_contribution, mavedb_vrs_contribution, ) +from mavedb.lib.mondo import get_generic_disease_term from mavedb.models.score_calibration import ScoreCalibration from mavedb.models.user import User from tests.helpers.mocks.factories import ( @@ -326,6 +327,7 @@ def test_contributions_with_real_db_objects(self, session, setup_lib_db_with_map primary=True, investigator_provided=True, private=False, + disease_term=get_generic_disease_term(session), created_by_id=creator.id, modified_by_id=creator.id, ) diff --git a/tests/lib/mondo/__init__.py b/tests/lib/mondo/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/lib/mondo/network/__init__.py b/tests/lib/mondo/network/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/lib/mondo/network/test_mondo.py b/tests/lib/mondo/network/test_mondo.py new file mode 100644 index 000000000..0daaf2866 --- /dev/null +++ b/tests/lib/mondo/network/test_mondo.py @@ -0,0 +1,54 @@ +# ruff: noqa: E402 +"""Network tests for the MONDO/OLS integration. Require a live connection to www.ebi.ac.uk (OLS4). + +The unit tests in ``tests/lib/mondo/test_mondo.py`` mock OLS, so these are the only exposure to real upstream +behavior — a canary for changes to OLS4's endpoints, response shape, or term identity. Sockets are +blocked for unmarked tests, so these run only under the ``network`` marker and are not part of the +default suite. +""" + +import pytest + +# starlette is required for logging context functionality pulled in by mavedb.lib.mondo. +pytest.importorskip("starlette") + +from mavedb.lib.mondo import MONDO_GENERIC_CODE, fetch_mondo_term, search_mondo + +# Brugada syndrome: a stable, long-established MONDO disease term used as a known-good fixture. +BRUGADA_CODE = "MONDO:0015263" + + +@pytest.mark.network +@pytest.mark.asyncio +class TestMondoOlsNetwork: + """Canary tests exercising the real OLS4 service.""" + + async def test_search_returns_known_term_with_expected_shape(self): + # /select endpoint: a known query surfaces the expected code, in the {code, label, iri} shape. + results = await search_mondo("brugada syndrome") + assert results, "OLS returned no results for a known disease query." + assert BRUGADA_CODE in {result["code"] for result in results} + first = results[0] + assert first["code"].startswith("MONDO:") + assert first["label"] + assert first["iri"].startswith("http") + + async def test_search_broad_prefix_returns_results(self): + # A two-letter prefix must still return via /select — /search timed out on these (the #754 switch). + assert await search_mondo("ca") + + async def test_fetch_known_code_returns_canonical_term(self): + # /search exact-obo_id lookup: a known code resolves to its canonical label. + resolved = await fetch_mondo_term(BRUGADA_CODE) + assert resolved is not None + assert resolved["code"] == BRUGADA_CODE + assert "brugada" in resolved["label"].lower() + + async def test_fetch_generic_root_resolves(self): + resolved = await fetch_mondo_term(MONDO_GENERIC_CODE) + assert resolved is not None + assert resolved["code"] == MONDO_GENERIC_CODE + + async def test_fetch_unknown_code_returns_none(self): + # The not-found path must stay a clean None so validation can reject unknown codes. + assert await fetch_mondo_term("MONDO:9999999") is None diff --git a/tests/lib/mondo/test_mondo.py b/tests/lib/mondo/test_mondo.py new file mode 100644 index 000000000..b50550970 --- /dev/null +++ b/tests/lib/mondo/test_mondo.py @@ -0,0 +1,199 @@ +"""Tests for mavedb.lib.mondo: MONDO term resolution, search, and MappableConcept serialization.""" + +# ruff: noqa: E402 + +import pytest + +pytest.importorskip("psycopg2") + +import httpx + +from mavedb.lib.exceptions import MondoServiceError +from mavedb.lib.mondo import ( + MONDO_GENERIC_CODE, + MONDO_GENERIC_LABEL, + MONDO_SYSTEM, + fetch_mondo_term, + find_or_create_mondo_term, + generic_disease_mappable_concept, + get_generic_disease_term, + mondo_iri, + mondo_suggestion_to_mappable_concept, + mondo_term_to_mappable_concept, + search_mondo, +) +from mavedb.lib.validation.exceptions import ValidationError +from mavedb.models.mondo_term import MondoTerm + + +class _FakeResponse: + def __init__(self, payload): + self._payload = payload + + def raise_for_status(self): + pass + + def json(self): + return self._payload + + +class _FakeAsyncClient: + """Minimal stand-in for httpx.AsyncClient returning a canned OLS payload.""" + + def __init__(self, payload): + self._payload = payload + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def get(self, url, params=None): + return _FakeResponse(self._payload) + + +class _FailingAsyncClient: + """Stand-in for httpx.AsyncClient whose request fails, as when OLS is unreachable.""" + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def get(self, url, params=None): + raise httpx.ConnectError("ols down") + + +def _ols_payload(docs): + return {"response": {"docs": docs}} + + +@pytest.mark.unit +class TestMondoSerialization: + def test_mondo_iri_replaces_curie_colon(self): + assert mondo_iri("MONDO:0015263") == "https://purl.obolibrary.org/obo/MONDO_0015263" + + def test_term_to_mappable_concept(self): + term = MondoTerm( + code="MONDO:0015263", system=MONDO_SYSTEM, system_version="2026-01-01", label="Brugada syndrome" + ) + concept = mondo_term_to_mappable_concept(term) + assert concept.conceptType == "Disease" + assert concept.name == "Brugada syndrome" + assert concept.primaryCoding.code.root == "MONDO:0015263" + assert concept.primaryCoding.system == MONDO_SYSTEM + assert concept.primaryCoding.iris[0].root == "https://purl.obolibrary.org/obo/MONDO_0015263" + + def test_generic_concept_is_the_mondo_root(self): + concept = generic_disease_mappable_concept() + assert concept.primaryCoding.code.root == MONDO_GENERIC_CODE + assert concept.name == MONDO_GENERIC_LABEL + + def test_suggestion_to_mappable_concept(self): + concept = mondo_suggestion_to_mappable_concept( + {"code": "MONDO:0015263", "label": "Brugada syndrome", "iri": "https://example.org/MONDO_0015263"} + ) + assert concept.primaryCoding.code.root == "MONDO:0015263" + assert concept.primaryCoding.iris[0].root == "https://example.org/MONDO_0015263" + + +@pytest.mark.asyncio +class TestMondoOlsSearch: + async def test_search_mondo_parses_ols_docs(self, monkeypatch): + payload = _ols_payload( + [ + { + "obo_id": "MONDO:0015263", + "label": "Brugada syndrome", + "iri": "https://purl.obolibrary.org/obo/MONDO_0015263", + }, + {"obo_id": "MONDO:0000001", "label": "disease or disorder"}, # missing iri → derived + ] + ) + monkeypatch.setattr(httpx, "AsyncClient", lambda *a, **k: _FakeAsyncClient(payload)) + + results = await search_mondo("brugada") + assert results[0] == { + "code": "MONDO:0015263", + "label": "Brugada syndrome", + "iri": "https://purl.obolibrary.org/obo/MONDO_0015263", + } + assert results[1]["iri"] == mondo_iri("MONDO:0000001") + + async def test_search_mondo_empty_query_skips_request(self, monkeypatch): + # No network expected; if a request were made the fake would still be needed, so assert on the shortcut. + assert await search_mondo(" ") == [] + + async def test_fetch_mondo_term_returns_none_when_absent(self, monkeypatch): + monkeypatch.setattr(httpx, "AsyncClient", lambda *a, **k: _FakeAsyncClient(_ols_payload([]))) + assert await fetch_mondo_term("MONDO:9999999") is None + + async def test_search_mondo_wraps_http_errors_in_service_error(self, monkeypatch): + monkeypatch.setattr(httpx, "AsyncClient", lambda *a, **k: _FailingAsyncClient()) + + with pytest.raises(MondoServiceError): + await search_mondo("brugada") + + async def test_fetch_mondo_term_wraps_http_errors_in_service_error(self, monkeypatch): + monkeypatch.setattr(httpx, "AsyncClient", lambda *a, **k: _FailingAsyncClient()) + + with pytest.raises(MondoServiceError): + await fetch_mondo_term("MONDO:0015263") + + +@pytest.mark.asyncio +class TestFindOrCreateMondoTerm: + async def test_creates_from_ols_and_is_idempotent(self, session, monkeypatch): + async def fake_fetch(code): + return {"code": code, "label": "Brugada syndrome", "iri": mondo_iri(code)} + + monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + + term = await find_or_create_mondo_term(session, "MONDO:0015263") + assert term.code == "MONDO:0015263" + assert term.label == "Brugada syndrome" + assert term.system == MONDO_SYSTEM + + # A second call reuses the row rather than creating a duplicate. + again = await find_or_create_mondo_term(session, "MONDO:0015263") + assert again.id == term.id + assert session.query(MondoTerm).filter(MondoTerm.code == "MONDO:0015263").count() == 1 + + async def test_generic_code_resolves_without_ols(self, session, monkeypatch): + async def fail_fetch(code): + raise AssertionError("OLS should not be consulted for the generic term") + + monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fail_fetch) + term = await find_or_create_mondo_term(session, MONDO_GENERIC_CODE) + assert term.code == MONDO_GENERIC_CODE + assert term.label == MONDO_GENERIC_LABEL + + async def test_rejects_unknown_code(self, session, monkeypatch): + async def fake_fetch(code): + return None + + monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + with pytest.raises(ValidationError): + await find_or_create_mondo_term(session, "MONDO:9999999") + + async def test_ols_outage_propagates_service_error(self, session, monkeypatch): + # A controlled term is never persisted unvalidated: an OLS outage surfaces as MondoServiceError. + async def fake_fetch(code): + raise MondoServiceError("ols down") + + monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + with pytest.raises(MondoServiceError): + await find_or_create_mondo_term(session, "MONDO:0015263") + + +@pytest.mark.unit +class TestGenericDiseaseTerm: + def test_get_generic_disease_term_is_idempotent(self, session): + first = get_generic_disease_term(session) + assert first.code == MONDO_GENERIC_CODE + assert first.system == MONDO_SYSTEM + second = get_generic_disease_term(session) + assert second.id == first.id + assert session.query(MondoTerm).filter(MondoTerm.code == MONDO_GENERIC_CODE).count() == 1 diff --git a/tests/lib/test_calibration_control.py b/tests/lib/test_calibration_control.py index 5637f6286..7a04a1f28 100644 --- a/tests/lib/test_calibration_control.py +++ b/tests/lib/test_calibration_control.py @@ -18,6 +18,7 @@ from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.calibration_control_status import CalibrationControlStatus +from mavedb.lib.mondo import get_generic_disease_term from mavedb.models.score_calibration import ScoreCalibration from mavedb.models.score_set import ScoreSet from mavedb.models.user import User @@ -31,6 +32,7 @@ def _make_calibration(session, score_set_id: int, user: User) -> ScoreCalibratio calibration = ScoreCalibration( title="Calibration with controls", score_set_id=score_set_id, + disease_term=get_generic_disease_term(session), created_by=user, modified_by=user, ) diff --git a/tests/routers/test_diseases.py b/tests/routers/test_diseases.py new file mode 100644 index 000000000..19c3c2bf9 --- /dev/null +++ b/tests/routers/test_diseases.py @@ -0,0 +1,51 @@ +"""Tests for the /diseases/search MONDO typeahead endpoint.""" + +# ruff: noqa: E402 + +import pytest + +pytest.importorskip("arq") +pytest.importorskip("cdot") +pytest.importorskip("fastapi") + +from mavedb.lib.exceptions import MondoServiceError + + +@pytest.fixture +def mock_mondo_search(monkeypatch): + async def fake_search(query, limit=20): + return [ + { + "code": "MONDO:0015263", + "label": "Brugada syndrome", + "iri": "https://purl.obolibrary.org/obo/MONDO_0015263", + } + ] + + monkeypatch.setattr("mavedb.routers.diseases.search_mondo", fake_search) + + +def test_search_diseases_returns_mappable_concepts(client, mock_mondo_search): + response = client.get("/api/v1/diseases/search", params={"q": "brugada"}) + + assert response.status_code == 200 + data = response.json() + assert len(data) == 1 + assert data[0]["conceptType"] == "Disease" + assert data[0]["name"] == "Brugada syndrome" + assert data[0]["primaryCoding"]["code"] == "MONDO:0015263" + + +def test_search_diseases_requires_a_query(client): + assert client.get("/api/v1/diseases/search").status_code == 422 + + +def test_search_diseases_surfaces_ontology_outage(client, monkeypatch): + async def failing_search(query, limit=20): + raise MondoServiceError("Disease ontology service temporarily unavailable") + + monkeypatch.setattr("mavedb.routers.diseases.search_mondo", failing_search) + + response = client.get("/api/v1/diseases/search", params={"q": "brugada"}) + assert response.status_code == 503 + assert "Disease ontology service temporarily unavailable" in response.json()["message"] diff --git a/tests/routers/test_score_calibrations.py b/tests/routers/test_score_calibrations.py index fe1aeba7c..e5bb5c486 100644 --- a/tests/routers/test_score_calibrations.py +++ b/tests/routers/test_score_calibrations.py @@ -14,6 +14,7 @@ from arq import ArqRedis from sqlalchemy import select +from mavedb.models.mondo_term import MondoTerm as MondoTermDbModel from mavedb.models.score_calibration import ScoreCalibration as CalibrationDbModel from mavedb.models.score_set import ScoreSet as ScoreSetDbModel from tests.helpers.constants import ( @@ -148,6 +149,72 @@ def test_creating_user_can_get_score_calibration_when_private( assert calibration_response["private"] is True +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_create_score_calibration_without_disease_defaults_to_generic( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, session, data_provider, experiment["urn"], data_files / "scores.csv" + ) + calibration = create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + + disease = client.get(f"/api/v1/score-calibrations/{calibration['urn']}").json()["disease"] + assert disease["conceptType"] == "Disease" + assert disease["primaryCoding"]["code"] == "MONDO:0000001" + + +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_create_score_calibration_with_disease_resolves_mondo_concept( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, monkeypatch +): + async def fake_fetch(code): + return { + "code": code, + "label": "Brugada syndrome", + "iri": f"https://purl.obolibrary.org/obo/{code.replace(':', '_')}", + } + + monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, session, data_provider, experiment["urn"], data_files / "scores.csv" + ) + payload = { + **deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED), + "disease": "MONDO:0015263", + } + calibration = create_test_score_calibration_in_score_set_via_client(client, score_set["urn"], payload) + + disease = client.get(f"/api/v1/score-calibrations/{calibration['urn']}").json()["disease"] + assert disease["primaryCoding"]["code"] == "MONDO:0015263" + assert disease["name"] == "Brugada syndrome" + + stored = session.query(MondoTermDbModel).filter(MondoTermDbModel.code == "MONDO:0015263").one() + assert stored.label == "Brugada syndrome" + + @pytest.mark.parametrize( "mock_publication_fetch", [ diff --git a/tests/routers/test_variant.py b/tests/routers/test_variant.py index 48f3fbd99..d865f7c9c 100644 --- a/tests/routers/test_variant.py +++ b/tests/routers/test_variant.py @@ -176,6 +176,7 @@ class TestPrivateCalibrationsAreNotServedOverHttp: """ def _private_calibration(self, session, score_set_urn): + from mavedb.lib.mondo import get_generic_disease_term from mavedb.models.score_calibration import ScoreCalibration score_set = session.scalars(select(ScoreSetDbModel).where(ScoreSetDbModel.urn == score_set_urn)).one() @@ -188,6 +189,7 @@ def _private_calibration(self, session, score_set_urn): primary=False, private=True, calibration_metadata={}, + disease_term=get_generic_disease_term(session), created_by_id=score_set.created_by_id, modified_by_id=score_set.modified_by_id, ) diff --git a/tests/scripts/conftest_optional.py b/tests/scripts/conftest_optional.py index c2216f6d5..65b16e8a7 100644 --- a/tests/scripts/conftest_optional.py +++ b/tests/scripts/conftest_optional.py @@ -12,6 +12,7 @@ from mavedb.models.acmg_classification import ACMGClassification from mavedb.models.enums.acmg_criterion import ACMGCriterion from mavedb.models.enums.functional_classification import FunctionalClassification +from mavedb.lib.mondo import get_generic_disease_term from mavedb.models.score_calibration import ScoreCalibration from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification @@ -65,6 +66,7 @@ def _make(score_set, *, private=False, research_use_only=False, title=None): primary=not (private or research_use_only), private=private, calibration_metadata={}, + disease_term=get_generic_disease_term(session), created_by_id=sample_user.id, modified_by_id=sample_user.id, ) diff --git a/tests/view_models/test_calibration_control.py b/tests/view_models/test_calibration_control.py index 8b61d5120..3f1bf5b5b 100644 --- a/tests/view_models/test_calibration_control.py +++ b/tests/view_models/test_calibration_control.py @@ -4,6 +4,7 @@ import pytest from pydantic import ValidationError +from mavedb.lib.mondo import MONDO_SYSTEM from mavedb.models.enums.calibration_control_status import CalibrationControlStatus from mavedb.view_models.calibration_control import ( CalibrationControlCreate, @@ -97,7 +98,7 @@ def test_saved_calibration_control_synthesizes_variant_urn_from_orm(): def test_score_calibration_create_accepts_controls_disease_and_phi(): payload = deepcopy(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) - payload["disease"] = "Brugada syndrome" + payload["disease"] = "MONDO:0015263" payload["controls_not_phi"] = True payload["controls"] = [ {"variant_urn": TEST_CONTROL_VARIANT_URN, "clinical_status": "pathogenic"}, @@ -106,16 +107,18 @@ def test_score_calibration_create_accepts_controls_disease_and_phi(): calibration = ScoreCalibrationCreate.model_validate(payload) - assert calibration.disease == "Brugada syndrome" + assert calibration.disease == "MONDO:0015263" assert calibration.controls_not_phi is True assert len(calibration.controls) == 2 assert calibration.controls[0].clinical_status is CalibrationControlStatus.pathogenic assert calibration.controls[1].clinical_status is CalibrationControlStatus.benign -def test_saved_score_calibration_includes_controls(): +def test_saved_score_calibration_includes_controls_and_disease_concept(): saved = deepcopy(TEST_SAVED_BRNICH_SCORE_CALIBRATION_RANGE_BASED) - saved["disease"] = "Brugada syndrome" + saved["disease_term"] = dummy_attributed_object_from_dict( + {"code": "MONDO:0015263", "system": MONDO_SYSTEM, "system_version": None, "label": "Brugada syndrome"} + ) saved["controlsNotPhi"] = True saved["controls"] = [ { @@ -131,7 +134,9 @@ def test_saved_score_calibration_includes_controls(): calibration = ScoreCalibration.model_validate(dummy_attributed_object_from_dict(saved)) - assert calibration.disease == "Brugada syndrome" + assert calibration.disease.conceptType == "Disease" + assert calibration.disease.primaryCoding.code.root == "MONDO:0015263" + assert calibration.disease.name == "Brugada syndrome" assert calibration.controls_not_phi is True assert len(calibration.controls) == 1 assert calibration.controls[0].variant_urn == TEST_CONTROL_VARIANT_URN From 8c132aa4b0c36c1d4027bb9d79777f3682d85a4b Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Thu, 17 Sep 2026 09:55:46 -0700 Subject: [PATCH 11/19] refactor(annotation): derive the disease condition from the calibration term (#754) With every calibration now carrying a non-null MONDO disease term, build the VA-Spec Condition directly from it instead of the hardcoded generic mappable-concept helper. The clinical-impact proposition takes the authoritative (strongest) calibration and uses its disease as the single objectCondition every evidence line on the statement targets. Removes the now-unused annotation disease constants module. --- src/mavedb/lib/annotation/annotate.py | 12 +- src/mavedb/lib/annotation/condition.py | 24 +-- src/mavedb/lib/annotation/constants.py | 2 - src/mavedb/lib/annotation/proposition.py | 6 +- tests/lib/annotation/test_condition.py | 163 +++++---------------- tests/lib/annotation/test_constants.py | 40 ----- tests/lib/annotation/test_evidence_line.py | 17 ++- tests/lib/annotation/test_proposition.py | 20 ++- 8 files changed, 85 insertions(+), 199 deletions(-) delete mode 100644 src/mavedb/lib/annotation/constants.py delete mode 100644 tests/lib/annotation/test_constants.py diff --git a/src/mavedb/lib/annotation/annotate.py b/src/mavedb/lib/annotation/annotate.py index 83b13b714..70f926425 100644 --- a/src/mavedb/lib/annotation/annotate.py +++ b/src/mavedb/lib/annotation/annotate.py @@ -120,7 +120,6 @@ def variant_pathogenicity_statement( study_result = mapped_variant_to_experimental_variant_impact_study_result(mapped_variant) functional_proposition = mapped_variant_to_experimental_variant_functional_impact_proposition(mapped_variant) - clinical_proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition(mapped_variant) eligible_calibrations = calibrations_available_for_annotation( mapped_variant, @@ -137,6 +136,17 @@ def variant_pathogenicity_statement( if not strongest_calibration: return None + # The statement carries one proposition (targeted by every evidence line), so its disease context + # is the strongest calibration's — the same calibration that anchors the ACMG classification. + # + # TODO#XXX - Pooling calibrations with different disease contexts is not supported by the current VA-Spec model. If a + # variant has multiple calibrations with different disease contexts, the strongest calibration is used for the + # statement's disease context and we should consider whether to filter out calibrations with different disease contexts + # from the evidence lines. + clinical_proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition( + mapped_variant, strongest_calibration + ) + # Get the classification from the strongest range (used for the functional statement within clinical evidence) # If strongest_range is None, the variant is not in any range, so classification will be INDETERMINATE _, classification = functional_classification_of_variant(mapped_variant, strongest_calibration) diff --git a/src/mavedb/lib/annotation/condition.py b/src/mavedb/lib/annotation/condition.py index cbd5fabe0..ae1f1b7af 100644 --- a/src/mavedb/lib/annotation/condition.py +++ b/src/mavedb/lib/annotation/condition.py @@ -1,21 +1,13 @@ -from ga4gh.core.models import Coding, iriReference as IRI, MappableConcept from ga4gh.va_spec.base.domain_entities import Condition -from mavedb.lib.annotation.constants import GENERIC_DISEASE_MEDGEN_CODE, MEDGEN_SYSTEM +from mavedb.lib.mondo import mondo_term_to_mappable_concept +from mavedb.models.score_calibration import ScoreCalibration -def generic_disease_condition_iri() -> IRI: - return IRI(root=f"http://identifiers.org/medgen/{GENERIC_DISEASE_MEDGEN_CODE}") +def calibration_disease_condition(score_calibration: ScoreCalibration) -> Condition: + """The disease/disorder a calibration applies to, as a VA-Spec ``Condition``. - -def generic_disease_condition() -> Condition: - return Condition( - root=MappableConcept( - conceptType="Disease", - primaryCoding=Coding( - code=GENERIC_DISEASE_MEDGEN_CODE, - system=MEDGEN_SYSTEM, - iris=[generic_disease_condition_iri()], - ), - ) - ) + Every calibration carries a non-null MONDO disease term (often the generic "disease or + disorder" (``MONDO:0000001``)) which is serialized directly to a condition. + """ + return Condition(root=mondo_term_to_mappable_concept(score_calibration.disease_term)) diff --git a/src/mavedb/lib/annotation/constants.py b/src/mavedb/lib/annotation/constants.py deleted file mode 100644 index 90b7dfecf..000000000 --- a/src/mavedb/lib/annotation/constants.py +++ /dev/null @@ -1,2 +0,0 @@ -GENERIC_DISEASE_MEDGEN_CODE = "C0012634" -MEDGEN_SYSTEM = "https://www.ncbi.nlm.nih.gov/medgen/" diff --git a/src/mavedb/lib/annotation/proposition.py b/src/mavedb/lib/annotation/proposition.py index 963327c73..59213d66e 100644 --- a/src/mavedb/lib/annotation/proposition.py +++ b/src/mavedb/lib/annotation/proposition.py @@ -1,14 +1,16 @@ from ga4gh.core.models import Coding, MappableConcept from ga4gh.va_spec.base.core import ExperimentalVariantFunctionalImpactProposition, VariantPathogenicityProposition -from mavedb.lib.annotation.condition import generic_disease_condition +from mavedb.lib.annotation.condition import calibration_disease_condition from mavedb.lib.annotation.document import experiment_to_document from mavedb.lib.annotation.util import sequence_feature_for_mapped_variant, variation_from_mapped_variant from mavedb.models.mapped_variant import MappedVariant +from mavedb.models.score_calibration import ScoreCalibration def mapped_variant_to_experimental_variant_clinical_impact_proposition( mapped_variant: MappedVariant, + score_calibration: ScoreCalibration, ) -> VariantPathogenicityProposition: coding, system = sequence_feature_for_mapped_variant(mapped_variant) sequence_feature = MappableConcept( @@ -19,7 +21,7 @@ def mapped_variant_to_experimental_variant_clinical_impact_proposition( description=f"Variant pathogenicity proposition for {mapped_variant.variant.urn}.", subjectVariant=variation_from_mapped_variant(mapped_variant), predicate="isCausalFor", - objectCondition=generic_disease_condition(), + objectCondition=calibration_disease_condition(score_calibration), geneContextQualifier=sequence_feature if system == "https://www.genenames.org/" else None, # only include gene context if we have a gene identifier diff --git a/tests/lib/annotation/test_condition.py b/tests/lib/annotation/test_condition.py index 817628794..2619f1abd 100644 --- a/tests/lib/annotation/test_condition.py +++ b/tests/lib/annotation/test_condition.py @@ -1,12 +1,14 @@ """ -Tests for mavedb.lib.annotation.condition module. +Tests for mavedb.lib.annotation.condition. -This module tests functions for creating GA4GH Condition objects and IRIs, -specifically for generic disease conditions used in variant annotations. +A calibration's disease condition is serialized directly from its stored MONDO term (see +mavedb.lib.mondo); an unspecified disease resolves to the generic term at write time. """ # ruff: noqa: E402 +from types import SimpleNamespace + import pytest pytest.importorskip("psycopg2") @@ -15,142 +17,43 @@ from ga4gh.core.models import iriReference as IRI from ga4gh.va_spec.base.domain_entities import Condition -from mavedb.lib.annotation.condition import generic_disease_condition, generic_disease_condition_iri -from mavedb.lib.annotation.constants import GENERIC_DISEASE_MEDGEN_CODE, MEDGEN_SYSTEM - - -@pytest.mark.unit -class TestGenericDiseaseConditionIriUnit: - """Unit tests for generic_disease_condition_iri function.""" - - def test_returns_correct_iri_structure(self): - """Test that function returns proper IRI object with expected root.""" - iri = generic_disease_condition_iri() - - assert isinstance(iri, IRI) - expected_root = f"http://identifiers.org/medgen/{GENERIC_DISEASE_MEDGEN_CODE}" - assert iri.root == expected_root - - def test_uses_correct_medgen_code(self): - """Test that function uses the correct MedGen code from constants.""" - iri = generic_disease_condition_iri() - - assert GENERIC_DISEASE_MEDGEN_CODE in iri.root - assert iri.root.endswith(GENERIC_DISEASE_MEDGEN_CODE) - - def test_uses_correct_iri_format(self): - """Test that IRI follows the expected identifiers.org format.""" - iri = generic_disease_condition_iri() +from mavedb.lib.annotation.condition import calibration_disease_condition +from mavedb.lib.mondo import MONDO_GENERIC_CODE, MONDO_GENERIC_LABEL, MONDO_SYSTEM, mondo_iri +from mavedb.models.mondo_term import MondoTerm - assert iri.root.startswith("http://identifiers.org/medgen/") - assert "identifiers.org" in iri.root - def test_iri_consistency(self): - """Test that multiple calls return consistent IRI values.""" - iri1 = generic_disease_condition_iri() - iri2 = generic_disease_condition_iri() - - assert iri1.root == iri2.root +def _calibration_with_disease(term: MondoTerm) -> SimpleNamespace: + """A stand-in calibration exposing only the disease_term the condition builder reads.""" + return SimpleNamespace(disease_term=term) @pytest.mark.unit -class TestGenericDiseaseConditionUnit: - """Unit tests for generic_disease_condition function.""" - - def test_returns_correct_condition_structure(self): - """Test that function returns proper Condition object.""" - condition = generic_disease_condition() - +class TestCalibrationDiseaseConditionUnit: + """Unit tests for the calibration disease condition.""" + + def test_builds_condition_from_the_calibration_disease_term(self): + term = MondoTerm( + code="MONDO:0015263", system=MONDO_SYSTEM, system_version="2026-01-01", label="Brugada syndrome" + ) + condition = calibration_disease_condition(_calibration_with_disease(term)) assert isinstance(condition, Condition) - assert hasattr(condition, "root") assert isinstance(condition.root, MappableConcept) - - def test_concept_type_is_disease(self): - """Test that condition has correct conceptType.""" - condition = generic_disease_condition() - assert condition.root.conceptType == "Disease" + assert condition.root.name == "Brugada syndrome" - def test_primary_coding_structure(self): - """Test that primary coding has correct structure and values.""" - condition = generic_disease_condition() - coding = condition.root.primaryCoding - + def test_primary_coding_reflects_the_stored_term(self): + term = MondoTerm(code="MONDO:0015263", system=MONDO_SYSTEM, label="Brugada syndrome") + coding = calibration_disease_condition(_calibration_with_disease(term)).root.primaryCoding assert isinstance(coding, Coding) - assert coding.code.root == GENERIC_DISEASE_MEDGEN_CODE - assert coding.system == MEDGEN_SYSTEM - - def test_coding_iris_structure(self): - """Test that coding includes correct IRI list.""" - condition = generic_disease_condition() - coding = condition.root.primaryCoding - + assert coding.code.root == "MONDO:0015263" + assert coding.system == MONDO_SYSTEM assert isinstance(coding.iris, list) assert len(coding.iris) == 1 - - iri = coding.iris[0] - assert isinstance(iri, IRI) - - def test_coding_iri_matches_helper_function(self): - """Test that coding IRI matches the output of helper function.""" - condition = generic_disease_condition() - coding = condition.root.primaryCoding - expected_iri = generic_disease_condition_iri() - - assert len(coding.iris) == 1 - assert coding.iris[0].root == expected_iri.root - - def test_uses_correct_constants(self): - """Test that condition uses the correct constant values.""" - condition = generic_disease_condition() - coding = condition.root.primaryCoding - - assert coding.code.root == GENERIC_DISEASE_MEDGEN_CODE - assert coding.system == MEDGEN_SYSTEM - - def test_iri_contains_medgen_code(self): - """Test that IRI contains the correct MedGen code.""" - condition = generic_disease_condition() - coding = condition.root.primaryCoding - iri_root = coding.iris[0].root - - expected_iri_content = f"http://identifiers.org/medgen/{GENERIC_DISEASE_MEDGEN_CODE}" - assert iri_root == expected_iri_content - - def test_condition_consistency(self): - """Test that multiple calls return consistent condition structures.""" - condition1 = generic_disease_condition() - condition2 = generic_disease_condition() - - assert condition1.root.conceptType == condition2.root.conceptType - assert condition1.root.primaryCoding.code == condition2.root.primaryCoding.code - assert condition1.root.primaryCoding.system == condition2.root.primaryCoding.system - assert condition1.root.primaryCoding.iris[0].root == condition2.root.primaryCoding.iris[0].root - - -@pytest.mark.unit -class TestConditionConsistencyUnit: - """Unit tests for cross-function consistency in condition helpers.""" - - def test_iri_function_integration_with_condition(self): - """Test that standalone IRI function produces same result as condition's IRI.""" - standalone_iri = generic_disease_condition_iri() - condition = generic_disease_condition() - condition_iri = condition.root.primaryCoding.iris[0] - - assert standalone_iri.root == condition_iri.root - - def test_complete_condition_structure_integration(self): - """Test the complete condition structure matches expected GA4GH format.""" - condition = generic_disease_condition() - - # Verify complete structure - assert condition.root.conceptType == "Disease" - assert condition.root.primaryCoding.code.root == GENERIC_DISEASE_MEDGEN_CODE - assert condition.root.primaryCoding.system == MEDGEN_SYSTEM - assert len(condition.root.primaryCoding.iris) == 1 - - # Verify IRI structure - iri = condition.root.primaryCoding.iris[0] - expected_iri_root = f"http://identifiers.org/medgen/{GENERIC_DISEASE_MEDGEN_CODE}" - assert iri.root == expected_iri_root + assert isinstance(coding.iris[0], IRI) + assert coding.iris[0].root == mondo_iri("MONDO:0015263") + + def test_generic_disease_term_yields_the_mondo_root(self): + term = MondoTerm(code=MONDO_GENERIC_CODE, system=MONDO_SYSTEM, label=MONDO_GENERIC_LABEL) + condition = calibration_disease_condition(_calibration_with_disease(term)) + assert condition.root.primaryCoding.code.root == MONDO_GENERIC_CODE + assert condition.root.name == MONDO_GENERIC_LABEL diff --git a/tests/lib/annotation/test_constants.py b/tests/lib/annotation/test_constants.py deleted file mode 100644 index 558cc42e2..000000000 --- a/tests/lib/annotation/test_constants.py +++ /dev/null @@ -1,40 +0,0 @@ -""" -Tests for mavedb.lib.annotation.constants module. - -This module tests constant values used throughout the annotation system. -""" - -# ruff: noqa: E402 - -import pytest - -pytest.importorskip("psycopg2") - -from mavedb.lib.annotation.constants import ( - GENERIC_DISEASE_MEDGEN_CODE, - MEDGEN_SYSTEM, -) - - -@pytest.mark.unit -class TestAnnotationConstants: - """Unit tests for annotation constants.""" - - def test_generic_disease_medgen_code(self): - """Test generic disease MedGen code constant.""" - assert GENERIC_DISEASE_MEDGEN_CODE == "C0012634" - assert isinstance(GENERIC_DISEASE_MEDGEN_CODE, str) - - def test_medgen_system(self): - """Test MedGen system URL constant.""" - expected_url = "https://www.ncbi.nlm.nih.gov/medgen/" - assert MEDGEN_SYSTEM == expected_url - assert isinstance(MEDGEN_SYSTEM, str) - assert MEDGEN_SYSTEM.startswith("https://") - assert MEDGEN_SYSTEM.endswith("/") - - def test_constants_immutability(self): - """Test that constants are properly defined as strings.""" - # These should be string constants, not mutable objects - assert isinstance(GENERIC_DISEASE_MEDGEN_CODE, str) - assert isinstance(MEDGEN_SYSTEM, str) diff --git a/tests/lib/annotation/test_evidence_line.py b/tests/lib/annotation/test_evidence_line.py index fb6f33b74..d983793d0 100644 --- a/tests/lib/annotation/test_evidence_line.py +++ b/tests/lib/annotation/test_evidence_line.py @@ -25,6 +25,7 @@ mapped_variant_to_experimental_variant_functional_impact_proposition, ) from mavedb.lib.annotation.statement import mapped_variant_to_functional_statement +from tests.helpers.mocks.factories import create_mock_score_calibration @pytest.mark.unit @@ -62,7 +63,9 @@ def test_acmg_evidence_line_with_met_valid_clinical_classification( "mavedb.lib.annotation.evidence_line.pathogenicity_classification_of_variant", return_value=(MagicMock(label="Test Range"), expected_outcome, expected_strength), ): - proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition(mapped_variant) + proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition( + mapped_variant, score_calibration + ) study_result = variant_study_result(mapped_variant) evidence = functional_evidence_line(mapped_variant, score_calibration, [study_result]) result = acmg_evidence_line(mapped_variant, score_calibration, proposition, [evidence]) @@ -100,7 +103,9 @@ def test_acmg_evidence_line_with_not_met_clinical_classification( "mavedb.lib.annotation.evidence_line.pathogenicity_classification_of_variant", return_value=(MagicMock(label="Test Range"), expected_outcome, expected_strength), ): - proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition(mapped_variant) + proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition( + mapped_variant, score_calibration + ) study_result = variant_study_result(mapped_variant) evidence = functional_evidence_line(mapped_variant, score_calibration, [study_result]) result = acmg_evidence_line(mapped_variant, score_calibration, proposition, [evidence]) @@ -123,7 +128,9 @@ def test_acmg_evidence_line_with_no_calibrations_raises_error(self, mock_mapped_ score_calibration = MagicMock() with pytest.raises(ValueError, match="does not have a score set with score calibrations"): - proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition(mock_mapped_variant) + proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition( + mock_mapped_variant, create_mock_score_calibration() + ) study_result = variant_study_result(mock_mapped_variant) acmg_evidence_line(mock_mapped_variant, score_calibration, proposition, [study_result]) @@ -151,7 +158,9 @@ def test_acmg_evidence_line_accepts_statement_evidence_without_serialization_err score_calibration, ExperimentalVariantFunctionalImpactClassification.NORMAL, ) - clinical_proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition(mapped_variant) + clinical_proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition( + mapped_variant, score_calibration + ) result = acmg_evidence_line(mapped_variant, score_calibration, clinical_proposition, [functional_statement]) diff --git a/tests/lib/annotation/test_proposition.py b/tests/lib/annotation/test_proposition.py index cb883e0bc..14563f343 100644 --- a/tests/lib/annotation/test_proposition.py +++ b/tests/lib/annotation/test_proposition.py @@ -21,6 +21,7 @@ mapped_variant_to_experimental_variant_clinical_impact_proposition, mapped_variant_to_experimental_variant_functional_impact_proposition, ) +from tests.helpers.mocks.factories import create_mock_mondo_term, create_mock_score_calibration @pytest.mark.unit @@ -28,16 +29,27 @@ class TestExperimentalVariantClinicalImpactProposition: """Unit tests for experimental variant clinical impact proposition creation.""" def test_mapped_variant_to_experimental_variant_clinical_impact_proposition(self, mock_mapped_variant): - """Test creation of clinical impact proposition from mapped variant.""" - result = mapped_variant_to_experimental_variant_clinical_impact_proposition(mock_mapped_variant) + """The proposition's condition defaults to the calibration's generic disease term.""" + calibration = create_mock_score_calibration() + result = mapped_variant_to_experimental_variant_clinical_impact_proposition(mock_mapped_variant, calibration) assert isinstance(result, VariantPathogenicityProposition) assert result.description == f"Variant pathogenicity proposition for {mock_mapped_variant.variant.urn}." assert isinstance(result.subjectVariant, MolecularVariation) assert result.predicate == "isCausalFor" assert result.objectCondition.root.conceptType == "Disease" - assert result.objectCondition.root.primaryCoding.code.root == "C0012634" - assert result.objectCondition.root.primaryCoding.system == "https://www.ncbi.nlm.nih.gov/medgen/" + assert result.objectCondition.root.primaryCoding.code.root == "MONDO:0000001" + assert result.objectCondition.root.primaryCoding.system == "https://purl.obolibrary.org/obo/mondo.owl" + + def test_clinical_impact_proposition_reflects_the_calibration_disease(self, mock_mapped_variant): + """A calibration with a specific disease term drives the proposition's condition.""" + calibration = create_mock_score_calibration( + disease_term=create_mock_mondo_term(code="MONDO:0015263", label="Brugada syndrome") + ) + result = mapped_variant_to_experimental_variant_clinical_impact_proposition(mock_mapped_variant, calibration) + + assert result.objectCondition.root.primaryCoding.code.root == "MONDO:0015263" + assert result.objectCondition.root.name == "Brugada syndrome" @pytest.mark.unit From 18b1c8a8d25a6ccdeab870a642d5a83769bf885c Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Thu, 17 Sep 2026 09:56:31 -0700 Subject: [PATCH 12/19] feat(calibration-controls): reject controls on score set creation (#754) Controls resolve each variant URN against the score set's own variants, which are minted asynchronously from the scores file after the score set row exists, so none are resolvable at creation time. Reject a non-empty controls list with a 409, mirroring the existing class-based-range guard, rather than silently dropping the controls (the nested-create path calls standalone create_score_calibration, which never builds them). --- src/mavedb/routers/score_sets.py | 11 +++++++++++ tests/routers/test_score_set.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index aff713429..66fda2d15 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -1919,6 +1919,17 @@ async def create_score_set( detail="Class-based calibrations are not supported on score set creation. Please create class-based calibrations after creating the score set.", ) + # TODO#592: Support for controls on score set creation + if calibration_create.controls: + logger.info( + msg="Failed to create score set; Calibration controls are not supported on score set creation.", + extra=logging_context(), + ) + raise HTTPException( + status_code=409, + detail="Calibration controls are not supported on score set creation. Please add controls after creating the score set.", + ) + created_calibration_item = await create_score_calibration( db, calibration_create, user_data.user, variant_classes=None ) diff --git a/tests/routers/test_score_set.py b/tests/routers/test_score_set.py index 7d9290a9a..d91bdea13 100644 --- a/tests/routers/test_score_set.py +++ b/tests/routers/test_score_set.py @@ -65,6 +65,7 @@ TEST_SAVED_TAXONOMY, TEST_USER, VALID_CLINGEN_CA_ID, + VALID_VARIANT_URN, ) from tests.helpers.dependency_overrider import DependencyOverrider from tests.helpers.mocks.factories import create_mock_mapped_variant @@ -286,6 +287,33 @@ def test_cannot_create_score_set_with_class_based_calibration(client, mock_publi assert "Class-based calibrations are not supported on score set creation" in response_data["detail"] +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + ( + [ + {"dbName": "PubMed", "identifier": f"{TEST_PUBMED_IDENTIFIER}"}, + {"dbName": "bioRxiv", "identifier": f"{TEST_BIORXIV_IDENTIFIER}"}, + ] + ) + ], + indirect=["mock_publication_fetch"], +) +def test_cannot_create_score_set_with_calibration_controls(client, mock_publication_fetch, setup_router_db): + experiment = create_experiment(client) + score_set = deepcopy(TEST_MINIMAL_SEQ_SCORESET) + score_set["experimentUrn"] = experiment["urn"] + calibration = deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + # Controls reference variants that don't exist until the scores file is processed after creation. + calibration["controls"] = [{"variantUrn": VALID_VARIANT_URN, "clinicalStatus": "pathogenic"}] + score_set.update({"scoreCalibrations": [calibration]}) + + response = client.post("/api/v1/score-sets/", json=score_set) + assert response.status_code == 409 + response_data = response.json() + assert "Calibration controls are not supported on score set creation" in response_data["detail"] + + @pytest.mark.parametrize( "mock_publication_fetch", [ From 6b387fc87b633d9590eadcd709ba3a8075c6b993 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Thu, 17 Sep 2026 11:49:07 -0700 Subject: [PATCH 13/19] fix(calibration-controls): re-enforce PHI gate on modify, not just publish (#753) Publishing already blocked a calibration from going public with unaffirmed controls, but the modify route never re-checked the invariant afterward. An admin (the only user who can reach this route on an already-public calibration) could add or replace controls without re-affirming, since replacing controls resets the affirmation flag. Apply the same PHI acknowledgment gate on modify that publish already enforces. --- src/mavedb/routers/score_calibrations.py | 16 ++ tests/routers/test_calibration_controls.py | 165 ++++++++++++++++++++- 2 files changed, 180 insertions(+), 1 deletion(-) diff --git a/src/mavedb/routers/score_calibrations.py b/src/mavedb/routers/score_calibrations.py index bc5c48e8d..62a5d88d4 100644 --- a/src/mavedb/routers/score_calibrations.py +++ b/src/mavedb/routers/score_calibrations.py @@ -599,6 +599,22 @@ async def modify_score_calibration_route( db, item, calibration_update, user_data.user, variant_classes if classes_file else None ) + # A public calibration may not carry unacknowledged controls, on this route the same as at + # publish time. controls_not_phi is tristate: only True clears the gate — None (unaddressed) and + # False (declined) both block. Only meaningful when controls exist. + if ( + not updated_calibration.private + and updated_calibration.controls + and updated_calibration.controls_not_phi is not True + ): + raise HTTPException( + status_code=422, + detail=( + "This calibration has controls that have not been affirmed to be free of protected health " + "information (PHI). Set controls_not_phi to true." + ), + ) + db.commit() db.refresh(updated_calibration) diff --git a/tests/routers/test_calibration_controls.py b/tests/routers/test_calibration_controls.py index 660be6e97..cf47e45c1 100644 --- a/tests/routers/test_calibration_controls.py +++ b/tests/routers/test_calibration_controls.py @@ -24,9 +24,13 @@ TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED, TEST_PUBMED_IDENTIFIER, ) +from tests.helpers.dependency_overrider import DependencyOverrider from tests.helpers.util.common import deepcamelize from tests.helpers.util.experiment import create_experiment -from tests.helpers.util.score_calibration import create_test_score_calibration_in_score_set_via_client +from tests.helpers.util.score_calibration import ( + create_test_score_calibration_in_score_set_via_client, + publish_test_score_calibration_via_client, +) from tests.helpers.util.score_set import create_seq_score_set_with_mapped_variants CALIBRATION_PUBLICATIONS = [ @@ -357,3 +361,162 @@ def test_modify_controls_with_affirmation_keeps_phi_flag( assert response.status_code == 200, response.text assert response.json()["controlsNotPhi"] is True + + +########################################################### +# PHI re-affirmation invariant on published calibrations +########################################################### + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_create_calibration_ignores_client_supplied_private_field( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + # The create schema has no `private` field — creation always starts private, and publishing + # (which independently gates PHI-bearing controls) is the only path to making one public. + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + + response = client.post( + "/api/v1/score-calibrations/", + json={ + **_calibration_payload(score_set["urn"]), + "private": False, + "controls": [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}], + }, + ) + + assert response.status_code == 200, response.text + assert response.json()["private"] is True + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_cannot_add_controls_to_published_calibration_without_phi_affirmation_as_admin( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, admin_app_overrides +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + publish_test_score_calibration_via_client(client, calibration["urn"]) + + with DependencyOverrider(admin_app_overrides): + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload( + score_set["urn"], controls=[{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}] + ), + ) + + assert response.status_code == 422 + assert "protected health information" in response.json()["detail"].lower() + + # The gate must not have attached the controls. + refreshed = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]).one() + assert refreshed.controls == [] + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_can_add_controls_to_published_calibration_with_phi_affirmation_as_admin( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, admin_app_overrides +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + publish_test_score_calibration_via_client(client, calibration["urn"]) + + with DependencyOverrider(admin_app_overrides): + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload( + score_set["urn"], + controls=[{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}], + controlsNotPhi=True, + ), + ) + + assert response.status_code == 200, response.text + assert response.json()["controlsNotPhi"] is True + assert len(response.json()["controls"]) == 1 + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_cannot_replace_controls_on_published_calibration_without_reaffirming_as_admin( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, admin_app_overrides +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, + score_set["urn"], + [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}], + controlsNotPhi=True, + ) + publish_test_score_calibration_via_client(client, calibration["urn"]) + + # Replacing controls invalidates the prior affirmation just as it does on a private calibration, + # so an admin who doesn't re-affirm in the same request must be rejected here too. + with DependencyOverrider(admin_app_overrides): + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload( + score_set["urn"], controls=[{"variantUrn": variant_urns[1], "clinicalStatus": "benign"}] + ), + ) + + assert response.status_code == 422 + assert "protected health information" in response.json()["detail"].lower() + + # The gate must not have replaced the controls or reset the affirmation. + refreshed = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]).one() + assert refreshed.controls_not_phi is True + assert len(refreshed.controls) == 1 + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_cannot_decline_phi_affirmation_on_published_calibration_with_existing_controls_as_admin( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, admin_app_overrides +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, + score_set["urn"], + [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}], + controlsNotPhi=True, + ) + publish_test_score_calibration_via_client(client, calibration["urn"]) + + # Controls are left unchanged; only the affirmation is walked back to False. + with DependencyOverrider(admin_app_overrides): + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload(score_set["urn"], controlsNotPhi=False), + ) + + assert response.status_code == 422 + assert "protected health information" in response.json()["detail"].lower() + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_can_update_published_calibration_leaving_affirmed_controls_untouched_as_admin( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, admin_app_overrides +): + score_set, variant_urns = _score_set_with_variant_urns(client, session, data_provider, data_files) + calibration = _create_with_controls( + client, + score_set["urn"], + [{"variantUrn": variant_urns[0], "clinicalStatus": "pathogenic"}], + controlsNotPhi=True, + ) + publish_test_score_calibration_via_client(client, calibration["urn"]) + + # An edit that neither submits controls nor touches controlsNotPhi must leave the standing + # affirmation alone, so it should not trip the gate. + with DependencyOverrider(admin_app_overrides): + response = client.put( + f"/api/v1/score-calibrations/{calibration['urn']}", + json=_calibration_payload(score_set["urn"], notes="Updated notes"), + ) + + assert response.status_code == 200, response.text + assert response.json()["controlsNotPhi"] is True + assert len(response.json()["controls"]) == 1 From c29253ba9ceabc4c182d0ce97dd7871271d31186 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 18 Sep 2026 12:43:56 -0700 Subject: [PATCH 14/19] fix(calibration): make MONDO term resolution idempotent find_or_create_mondo_term looked up an existing term by the submitted code but inserted the canonical code returned by OLS. When OLS normalizes the input (case, or an alias/obsolete id), a resubmission missed the lookup and tried to insert the canonical code again, tripping UNIQUE(system, code) with an unhandled 500. Re-check for an existing row by the resolved canonical code before inserting, so lookup and insert key on the same value and the get-or-create stays idempotent. --- src/mavedb/lib/mondo.py | 11 ++++++++++- tests/lib/mondo/test_mondo.py | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/mavedb/lib/mondo.py b/src/mavedb/lib/mondo.py index dbabbf01d..71a9c1223 100644 --- a/src/mavedb/lib/mondo.py +++ b/src/mavedb/lib/mondo.py @@ -183,7 +183,16 @@ async def find_or_create_mondo_term(db: Session, code: str) -> MondoTerm: if resolved is None: raise ValidationError(f"'{code}' is not a valid MONDO disease term.", custom_loc=["body", "disease"]) - term = MondoTerm(code=resolved["code"], system=MONDO_SYSTEM, label=resolved["label"]) # type: ignore[call-arg] + # OLS may normalize the submitted code (case, or an alias/obsolete id) to a different canonical code, + # so re-check existence by the resolved code before inserting: keying the lookup on the same value we + # insert under keeps this idempotent and avoids a UNIQUE(system, code) violation when the canonical + # row already exists under a different submitted spelling. + canonical_code = resolved["code"] + term = db.query(MondoTerm).filter(MondoTerm.system == MONDO_SYSTEM, MondoTerm.code == canonical_code).one_or_none() + if term is not None: + return term + + term = MondoTerm(code=canonical_code, system=MONDO_SYSTEM, label=resolved["label"]) # type: ignore[call-arg] db.add(term) db.flush() diff --git a/tests/lib/mondo/test_mondo.py b/tests/lib/mondo/test_mondo.py index b50550970..10d2a22f9 100644 --- a/tests/lib/mondo/test_mondo.py +++ b/tests/lib/mondo/test_mondo.py @@ -161,6 +161,22 @@ async def fake_fetch(code): assert again.id == term.id assert session.query(MondoTerm).filter(MondoTerm.code == "MONDO:0015263").count() == 1 + async def test_non_canonical_code_reuses_existing_canonical_row(self, session, monkeypatch): + # OLS normalizes both spellings to the same canonical code; the lookup must key on the resolved + # code so a second submission reuses the row instead of tripping UNIQUE(system, code). + async def fake_fetch(code): + return {"code": "MONDO:0015263", "label": "Brugada syndrome", "iri": mondo_iri("MONDO:0015263")} + + monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + + canonical = await find_or_create_mondo_term(session, "MONDO:0015263") + # A differently-spelled submission (here lowercase) resolves to the same canonical term. + aliased = await find_or_create_mondo_term(session, "mondo:0015263") + + assert aliased.id == canonical.id + assert aliased.code == "MONDO:0015263" + assert session.query(MondoTerm).filter(MondoTerm.code == "MONDO:0015263").count() == 1 + async def test_generic_code_resolves_without_ols(self, session, monkeypatch): async def fail_fetch(code): raise AssertionError("OLS should not be consulted for the generic term") From cbb0c1a0010523f9f33054fb0c05507bd63ffecc Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 18 Sep 2026 12:44:06 -0700 Subject: [PATCH 15/19] feat(score-set): freeze scores after publish A published score set's scores are the record other work cites; re-uploading them would silently change results already in circulation and anything derived from the variants they recreate (mappings, calibration bin membership). Restrict SET_SCORES so owners and contributors may only set scores while the set is private; admins keep the ability as an operational escape hatch. Corrections go through a superseding set. The combined update-with-variants endpoint checks the permission before applying the update, since it commits the metadata half before enqueuing variant creation. --- src/mavedb/lib/permissions/score_set.py | 14 ++++-- src/mavedb/routers/score_sets.py | 43 ++++++++++------ tests/lib/permissions/test_score_set.py | 8 +-- tests/routers/test_score_set.py | 67 +++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 24 deletions(-) diff --git a/src/mavedb/lib/permissions/score_set.py b/src/mavedb/lib/permissions/score_set.py index ba35eaf91..cb61a6ad6 100644 --- a/src/mavedb/lib/permissions/score_set.py +++ b/src/mavedb/lib/permissions/score_set.py @@ -234,6 +234,12 @@ def _handle_set_scores_action( Only owners, contributors, and admins can modify the scores data within a ScoreSet. This is a critical operation that affects the scientific data. + Publishing freezes that data: a published score set's scores are the record other work cites, and + re-uploading them would silently change results already in circulation, along with anything + derived from the variants they create (mappings, and the bin membership of any calibration built + on the score set). Corrections to published data go through a superseding score set instead. + Admins keep the ability as an operational escape hatch, mirroring DELETE. + Args: user_data: The user's authentication data. entity: The ScoreSet entity whose scores are being modified. @@ -246,12 +252,12 @@ def _handle_set_scores_action( PermissionResponse: Permission result with appropriate HTTP status. """ ## Allow set scores access under the following conditions: - # The owner or contributors may set scores. - if user_is_owner or user_is_contributor: - return PermissionResponse(True) - # Users with these specific roles may set scores. + # Users with these specific roles may set scores on any score set. if roles_permitted(active_roles, [UserRole.admin]): return PermissionResponse(True) + # The owner or contributors may set scores only while the score set is still private. + if (user_is_owner or user_is_contributor) and private: + return PermissionResponse(True) return deny_action_for_entity(entity, private, user_data, user_is_contributor or user_is_owner, "score set") diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index 66fda2d15..a4dca8edb 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -2318,6 +2318,27 @@ async def update_score_set_with_variants( logger.info(msg="Failed to update score set; The requested score set does not exist.", extra=logging_context()) raise HTTPException(status_code=404, detail=f"score set with URN '{urn}' not found") + existing_score_columns_metadata = (existing_item.dataset_columns or {}).get("score_columns_metadata", {}) + existing_count_columns_metadata = (existing_item.dataset_columns or {}).get("count_columns_metadata", {}) + + did_score_columns_metadata_change = ( + dataset_column_metadata.get("score_columns_metadata", {}) != existing_score_columns_metadata + ) + did_count_columns_metadata_change = ( + dataset_column_metadata.get("count_columns_metadata", {}) != existing_count_columns_metadata + ) + request_sets_scores = ( + did_score_columns_metadata_change + or did_count_columns_metadata_change + or any([val is not None for val in score_set_variants_data.values()]) + ) + + # Checked before the update is applied, not just before the job is enqueued: score_set_update + # commits the metadata half of this request, so refusing afterwards would leave a caller who may + # not set scores with a half-applied update and no way to tell which half landed. + if request_sets_scores: + assert_permission(user_data, existing_item, Action.SET_SCORES) + itemUpdateResult = await score_set_update( db=db, urn=urn, @@ -2329,24 +2350,9 @@ async def update_score_set_with_variants( updatedItem = itemUpdateResult["item"] should_create_variants = itemUpdateResult.get("should_create_variants", False) - existing_score_columns_metadata = (existing_item.dataset_columns or {}).get("score_columns_metadata", {}) - existing_count_columns_metadata = (existing_item.dataset_columns or {}).get("count_columns_metadata", {}) - - did_score_columns_metadata_change = ( - dataset_column_metadata.get("score_columns_metadata", {}) != existing_score_columns_metadata - ) - did_count_columns_metadata_change = ( - dataset_column_metadata.get("count_columns_metadata", {}) != existing_count_columns_metadata - ) - # run variant creation job only if targets have changed (indicated by "should_create_variants"), new score # or count files were uploaded, or dataset column metadata has changed - if ( - should_create_variants - or did_score_columns_metadata_change - or did_count_columns_metadata_change - or any([val is not None for val in score_set_variants_data.values()]) - ): + if should_create_variants or request_sets_scores: assert_permission(user_data, updatedItem, Action.SET_SCORES) updatedItem.processing_state = ProcessingState.processing @@ -2431,6 +2437,11 @@ async def update_score_set( should_create_variants = itemUpdateResult["should_create_variants"] if should_create_variants: + # Structurally unreachable for a published score set, since score_set_update only sets this + # flag while the score set is private. Asserted anyway so every path that recreates variants + # states the same requirement, and a future change to that flag cannot open a hole here. + assert_permission(user_data, updatedItem, Action.SET_SCORES) + # Although this is also updated within the variant creation job, update it here # as well so that we can display the proper UI components (queue invocation delay # races the score set GET request). diff --git a/tests/lib/permissions/test_score_set.py b/tests/lib/permissions/test_score_set.py index 7198fba97..2d5a0c497 100644 --- a/tests/lib/permissions/test_score_set.py +++ b/tests/lib/permissions/test_score_set.py @@ -245,12 +245,12 @@ class TestScoreSetSetScoresActionHandler: # Admins can set scores on any ScoreSet PermissionTest("ScoreSet", "private", "admin", Action.SET_SCORES, True), PermissionTest("ScoreSet", "published", "admin", Action.SET_SCORES, True), - # Owners can set scores on any ScoreSet they own + # Owners can only set scores on ScoreSets they own that are still unpublished PermissionTest("ScoreSet", "private", "owner", Action.SET_SCORES, True), - PermissionTest("ScoreSet", "published", "owner", Action.SET_SCORES, True), - # Contributors can set scores on any ScoreSet they contribute to + PermissionTest("ScoreSet", "published", "owner", Action.SET_SCORES, False, 403), + # Contributors are likewise limited to unpublished ScoreSets PermissionTest("ScoreSet", "private", "contributor", Action.SET_SCORES, True), - PermissionTest("ScoreSet", "published", "contributor", Action.SET_SCORES, True), + PermissionTest("ScoreSet", "published", "contributor", Action.SET_SCORES, False, 403), # Mappers cannot set scores on ScoreSets PermissionTest("ScoreSet", "private", "mapper", Action.SET_SCORES, False, 404), PermissionTest("ScoreSet", "published", "mapper", Action.SET_SCORES, False, 403), diff --git a/tests/routers/test_score_set.py b/tests/routers/test_score_set.py index d91bdea13..4f3f08946 100644 --- a/tests/routers/test_score_set.py +++ b/tests/routers/test_score_set.py @@ -1222,6 +1222,73 @@ def test_add_score_set_variants_scores_only_endpoint(client, setup_router_db, da assert score_set == response_data +def test_add_score_set_variants_is_refused_on_a_published_score_set( + session, data_provider, client, setup_router_db, data_files, mock_s3_client +): + """Publishing freezes a score set's scores; the upload endpoint must refuse them afterwards. + + The UI only offers score editing while a score set is private, but that is not a guarantee: the + endpoint is reachable directly, and a re-upload would replace variants other records already + point at. + """ + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + score_set = mock_worker_variant_insertion(client, session, data_provider, score_set, data_files / "scores.csv") + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, score_set["urn"]) + + scores_csv_path = data_files / "scores.csv" + with ( + open(scores_csv_path, "rb") as scores_file, + patch.object(arq.ArqRedis, "enqueue_job", return_value=None) as queue, + patch.object(mock_s3_client, "upload_fileobj", return_value=None), + ): + response = client.post( + f"/api/v1/score-sets/{published['urn']}/variants/data", + files={"scores_file": (scores_csv_path.name, scores_file, "text/csv")}, + ) + # The refusal must land before any work is queued or uploaded. + queue.assert_not_called() + + assert response.status_code == 403 + + +def test_patch_published_score_set_with_scores_file_is_refused_without_applying_the_update( + session, data_provider, client, setup_router_db, data_files, mock_s3_client +): + """A refused score upload must not leave the request's metadata half committed. + + The combined endpoint commits metadata before it reaches the enqueue step, so the SET_SCORES + check has to run before the update is applied rather than only before the job is queued. + """ + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + score_set = mock_worker_variant_insertion(client, session, data_provider, score_set, data_files / "scores.csv") + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, score_set["urn"]) + + scores_csv_path = data_files / "scores.csv" + with ( + open(scores_csv_path, "rb") as scores_file, + patch.object(arq.ArqRedis, "enqueue_job", return_value=None) as queue, + patch.object(mock_s3_client, "upload_fileobj", return_value=None), + ): + response = client.patch( + f"/api/v1/score-sets-with-variants/{published['urn']}", + data={"title": "Retitled after publication"}, + files={"scores_file": (scores_csv_path.name, scores_file, "text/csv")}, + ) + queue.assert_not_called() + + assert response.status_code == 403 + + # The title edit rode along with the refused upload, so it must not have been applied either. + refreshed = client.get(f"/api/v1/score-sets/{published['urn']}").json() + assert refreshed["title"] == published["title"] + + def test_add_score_set_variants_scores_and_counts_endpoint( session, client, setup_router_db, data_files, mock_s3_client ): From ed1debee593ea2e976c042a269a612cd35bb6b66 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 18 Sep 2026 12:44:34 -0700 Subject: [PATCH 16/19] feat(calibration-controls): serve each control's functional-classification placement Expose which of the calibration's own functional classifications a control's variant falls into by bin membership, so the concordance view can cross a control's clinical status against where its score landed. Compute it as a correlated-subquery column_property on CalibrationControl rather than storing it, so it can never drift from the bin assignments it reports. Classified bins never overlap, so at most one applies; a classified bin is preferred over a 'not_specified' one, then lowest id breaks ties, for a deterministic result. --- src/mavedb/models/calibration_control.py | 46 ++++++- src/mavedb/view_models/calibration_control.py | 5 +- tests/lib/test_calibration_control.py | 115 +++++++++++++++++ tests/routers/test_calibration_controls.py | 118 +++++++++++++++++- tests/view_models/test_calibration_control.py | 2 + 5 files changed, 281 insertions(+), 5 deletions(-) diff --git a/src/mavedb/models/calibration_control.py b/src/mavedb/models/calibration_control.py index 363756616..144a1aeb0 100644 --- a/src/mavedb/models/calibration_control.py +++ b/src/mavedb/models/calibration_control.py @@ -10,13 +10,18 @@ from __future__ import annotations from datetime import date -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Optional -from sqlalchemy import Column, Date, Enum, ForeignKey, Integer, UniqueConstraint -from sqlalchemy.orm import Mapped, relationship +from sqlalchemy import Column, Date, Enum, ForeignKey, Integer, UniqueConstraint, case, select +from sqlalchemy.orm import Mapped, column_property, relationship from mavedb.db.base import Base from mavedb.models.enums.calibration_control_status import CalibrationControlStatus +from mavedb.models.enums.functional_classification import FunctionalClassification +from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification +from mavedb.models.score_calibration_functional_classification_variant_association import ( + score_calibration_functional_classification_variants_association_table, +) if TYPE_CHECKING: from mavedb.models.score_calibration import ScoreCalibration @@ -58,6 +63,41 @@ class CalibrationControl(Base): variant_id = Column(Integer, ForeignKey("variants.id"), nullable=False, index=True) variant: Mapped["Variant"] = relationship("Variant") + # Which of the calibration's own functional classifications contains this control's variant by bin + # membership, or NULL when it lands under none. Computed live from the membership association rather + # than stored, so it can never drift from the clinical bin assignments it reports. Mirrors the + # ``variant_count`` correlated-subquery pattern on ScoreCalibrationFunctionalClassification. Ranges may + # overlap when one is 'not_specified' (see ScoreCalibrationBase.ranges_do_not_overlap), so a variant can + # fall in several bins. Classified bins never overlap each other, so at most one applies; it is + # preferred over a 'not_specified' bin (which asserts no classification), then the lowest id breaks + # any remaining tie, for a deterministic result. + functional_classification_id: Mapped[Optional[int]] = column_property( + select(ScoreCalibrationFunctionalClassification.id) + .where( + ScoreCalibrationFunctionalClassification.calibration_id == calibration_id, + score_calibration_functional_classification_variants_association_table.c.functional_classification_id + == ScoreCalibrationFunctionalClassification.id, + score_calibration_functional_classification_variants_association_table.c.variant_id == variant_id, + ) + .order_by( + case( + ( + ScoreCalibrationFunctionalClassification.functional_classification + == FunctionalClassification.not_specified, + 1, + ), + else_=0, + ), + ScoreCalibrationFunctionalClassification.id, + ) + .limit(1) + .correlate_except( + ScoreCalibrationFunctionalClassification, + score_calibration_functional_classification_variants_association_table, + ) + .scalar_subquery() + ) + clinical_status = Column( Enum(CalibrationControlStatus, native_enum=False, validate_strings=True, length=32), nullable=False, diff --git a/src/mavedb/view_models/calibration_control.py b/src/mavedb/view_models/calibration_control.py index b6f48e020..9d1871ba6 100644 --- a/src/mavedb/view_models/calibration_control.py +++ b/src/mavedb/view_models/calibration_control.py @@ -7,7 +7,7 @@ """ from datetime import date -from typing import Any +from typing import Any, Optional from pydantic import model_validator @@ -54,6 +54,9 @@ class SavedCalibrationControl(CalibrationControlBase): _record_type_factory = record_type_validator()(set_record_type) id: int + # The calibration's own functional classification (range or class) which contains this control's + # variant, or None when the variant lands under none of them. + functional_classification_id: Optional[int] = None creation_date: date modification_date: date created_by: SavedUser diff --git a/tests/lib/test_calibration_control.py b/tests/lib/test_calibration_control.py index 7a04a1f28..4abc3ae6d 100644 --- a/tests/lib/test_calibration_control.py +++ b/tests/lib/test_calibration_control.py @@ -18,8 +18,10 @@ from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.calibration_control_status import CalibrationControlStatus +from mavedb.models.enums.functional_classification import FunctionalClassification from mavedb.lib.mondo import get_generic_disease_term from mavedb.models.score_calibration import ScoreCalibration +from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification from mavedb.models.score_set import ScoreSet from mavedb.models.user import User from mavedb.models.variant import Variant @@ -123,6 +125,119 @@ def test_deleting_calibration_cascades_to_controls(session, setup_lib_db_with_va assert session.get(CalibrationControl, control_id) is None +############################################################################## +# functional_classification_id placement (computed column_property) +############################################################################## + + +def _classification(calibration, label, functional_classification, score_range): + return ScoreCalibrationFunctionalClassification( + calibration=calibration, + label=label, + functional_classification=functional_classification, + range=score_range, + inclusive_lower_bound=True, + inclusive_upper_bound=False, + ) + + +def test_control_placement_prefers_classified_bin_over_not_specified(session, setup_lib_db_with_variant): + """A control in a not_specified-vs-classified overlap resolves to the classified bin. + + Overlapping ranges are only possible when one bin is 'not_specified' (see + ScoreCalibrationBase.ranges_do_not_overlap), and membership is built per-range, so the control's + variant lands in both bins. The placement must surface the clinically meaningful classified bin even + when the not_specified bin has the lower id — the tiebreak a plain id ordering would wrongly pick. + """ + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + calibration = _make_calibration(session, variant.score_set_id, user) + + # Create the not_specified bin first so it takes the lower id: this is the value a plain + # ``ORDER BY id`` would select, so asserting against the classified bin proves classified wins. + not_specified_bin = _classification( + calibration, "unclassified", FunctionalClassification.not_specified, [-2.0, 2.0] + ) + session.add(not_specified_bin) + session.commit() + + classified_bin = _classification(calibration, "abnormal", FunctionalClassification.abnormal, [0.0, 1.0]) + session.add(classified_bin) + session.commit() + + assert not_specified_bin.id < classified_bin.id + + # The variant sits in the overlap [0.0, 1.0], so it is a member of both bins. + not_specified_bin.variants = [variant] + classified_bin.variants = [variant] + session.commit() + + control = CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.pathogenic, + created_by=user, + modified_by=user, + ) + session.add(control) + session.commit() + session.refresh(control) + + assert control.functional_classification_id == classified_bin.id + + +def test_control_placement_falls_back_to_not_specified_when_no_classified_bin(session, setup_lib_db_with_variant): + """With no classified bin containing the variant, placement honestly reports the not_specified bin.""" + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + calibration = _make_calibration(session, variant.score_set_id, user) + + not_specified_bin = _classification( + calibration, "unclassified", FunctionalClassification.not_specified, [-2.0, 2.0] + ) + session.add(not_specified_bin) + session.commit() + not_specified_bin.variants = [variant] + session.commit() + + control = CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.benign, + created_by=user, + modified_by=user, + ) + session.add(control) + session.commit() + session.refresh(control) + + assert control.functional_classification_id == not_specified_bin.id + + +def test_control_placement_is_null_when_variant_in_no_bin(session, setup_lib_db_with_variant): + """A control whose variant falls under none of the calibration's classifications has no placement.""" + variant = setup_lib_db_with_variant + user = session.query(User).filter(User.username == TEST_USER["username"]).first() + calibration = _make_calibration(session, variant.score_set_id, user) + + classified_bin = _classification(calibration, "abnormal", FunctionalClassification.abnormal, [0.0, 1.0]) + session.add(classified_bin) + session.commit() # bin exists but the variant is never added to its membership + + control = CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=CalibrationControlStatus.pathogenic, + created_by=user, + modified_by=user, + ) + session.add(control) + session.commit() + session.refresh(control) + + assert control.functional_classification_id is None + + ############################################################################## # validate_calibration_controls_in_score_set (#751) ############################################################################## diff --git a/tests/routers/test_calibration_controls.py b/tests/routers/test_calibration_controls.py index cf47e45c1..d75be1af3 100644 --- a/tests/routers/test_calibration_controls.py +++ b/tests/routers/test_calibration_controls.py @@ -8,7 +8,7 @@ import json -from sqlalchemy import select +from sqlalchemy import delete, select from mavedb.lib.validation.constants.general import ( calibration_control_status_column_name, @@ -17,6 +17,9 @@ from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.calibration_control_status import CalibrationControlStatus from mavedb.models.score_calibration import ScoreCalibration as CalibrationDbModel +from mavedb.models.score_calibration_functional_classification_variant_association import ( + score_calibration_functional_classification_variants_association_table, +) from mavedb.models.score_set import ScoreSet as ScoreSetDbModel from mavedb.models.variant import Variant from tests.helpers.constants import ( @@ -520,3 +523,116 @@ def test_can_update_published_calibration_leaving_affirmed_controls_untouched_as assert response.status_code == 200, response.text assert response.json()["controlsNotPhi"] is True assert len(response.json()["controls"]) == 1 + + +########################################################### +# Control placement (functional_classification_id) — Option A +########################################################### + + +def _add_control_on_variant(session, calibration, variant, status=CalibrationControlStatus.pathogenic): + """Attach one control to ``calibration`` on a chosen ``variant`` at the ORM level.""" + session.add( + CalibrationControl( + calibration=calibration, + variant=variant, + clinical_status=status, + created_by=calibration.created_by, + modified_by=calibration.created_by, + ) + ) + session.commit() + + +def _placement_by_variant_id(calibration): + """Ground-truth ``{variant_id: functional_classification_id}`` from stored bin membership.""" + placement = {} + for classification in calibration.functional_classifications: + for variant in classification.variants: + placement[variant.id] = classification.id + return placement + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_get_calibration_populates_control_placement( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + """A control whose variant a range files reports that range's id as its placement.""" + calibration = _create_private_calibration(client, session, data_provider, data_files) + calibration_orm = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]).one() + + placement = _placement_by_variant_id(calibration_orm) + assert placement, "fixture calibration should file at least one variant into a range" + + variant_id, expected_classification_id = next(iter(placement.items())) + variant = session.get(Variant, variant_id) + _add_control_on_variant(session, calibration_orm, variant) + + response = client.get(f"/api/v1/score-calibrations/{calibration['urn']}") + assert response.status_code == 200, response.text + + control = next(c for c in response.json()["controls"] if c["variantUrn"] == variant.urn) + assert control["functionalClassificationId"] == expected_classification_id + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_control_placement_is_none_when_variant_has_no_bin( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + """A control whose variant is filed under none of the calibration's ranges reports null placement.""" + calibration = _create_private_calibration(client, session, data_provider, data_files) + calibration_orm = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]).one() + + variant = session.scalars(select(Variant).where(Variant.score_set_id == calibration_orm.score_set_id)).first() + _add_control_on_variant(session, calibration_orm, variant) + + # Strip any bin membership for this variant so its placement must resolve to None. + association = score_calibration_functional_classification_variants_association_table + session.execute(delete(association).where(association.c.variant_id == variant.id)) + session.commit() + + response = client.get(f"/api/v1/score-calibrations/{calibration['urn']}") + assert response.status_code == 200, response.text + + control = next(c for c in response.json()["controls"] if c["variantUrn"] == variant.urn) + assert control["functionalClassificationId"] is None + + +@pytest.mark.parametrize("mock_publication_fetch", [CALIBRATION_PUBLICATIONS], indirect=["mock_publication_fetch"]) +def test_control_placement_scoped_to_own_calibration( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + """Placement reflects the control's own calibration, not another that happens to bin the variant.""" + calibration = _create_private_calibration(client, session, data_provider, data_files) + calibration_orm = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]).one() + + # Capture plain ids/urns up front; creating the second calibration commits and expires ORM objects. + score_set_urn = calibration_orm.score_set.urn + placement = _placement_by_variant_id(calibration_orm) + assert placement + binned_variant_id = next(iter(placement)) + binned_variant_urn = session.get(Variant, binned_variant_id).urn + + # A second calibration on the same score set bins the same variant identically; clear only the + # second's membership for it, so the first calibration's binning must not leak into the second's + # placement. + second = create_test_score_calibration_in_score_set_via_client( + client, score_set_urn, deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + second_orm = session.query(CalibrationDbModel).where(CalibrationDbModel.urn == second["urn"]).one() + + association = score_calibration_functional_classification_variants_association_table + second_classification_ids = [fc.id for fc in second_orm.functional_classifications] + session.execute( + delete(association).where( + association.c.variant_id == binned_variant_id, + association.c.functional_classification_id.in_(second_classification_ids), + ) + ) + _add_control_on_variant(session, second_orm, session.get(Variant, binned_variant_id)) + + response = client.get(f"/api/v1/score-calibrations/{second['urn']}") + assert response.status_code == 200, response.text + + control = next(c for c in response.json()["controls"] if c["variantUrn"] == binned_variant_urn) + assert control["functionalClassificationId"] is None diff --git a/tests/view_models/test_calibration_control.py b/tests/view_models/test_calibration_control.py index 3f1bf5b5b..587782846 100644 --- a/tests/view_models/test_calibration_control.py +++ b/tests/view_models/test_calibration_control.py @@ -141,6 +141,8 @@ def test_saved_score_calibration_includes_controls_and_disease_concept(): assert len(calibration.controls) == 1 assert calibration.controls[0].variant_urn == TEST_CONTROL_VARIANT_URN assert calibration.controls[0].clinical_status is CalibrationControlStatus.pathogenic + # Placement is a computed column_property; an in-memory object never carries it, so it stays unset. + assert calibration.controls[0].functional_classification_id is None def test_saved_score_calibration_controls_default_to_empty_list(): From 0f6098947fc0a35898359d78ddaea38cbbef10cb Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 18 Sep 2026 12:44:46 -0700 Subject: [PATCH 17/19] feat(calibration-controls): preserve controls and class bins across a score re-upload Re-uploading a score set deletes and recreates its variants, breaking the foreign keys calibration controls and class-based bin membership hold into them (RESTRICT, so the delete would otherwise fail). Snapshot the irreproducible references by HGVS identity before the delete, then re-resolve them against the new variants; range-based bins are recomputed from the new scores instead. A dropped control resets controls_not_phi, and provenance (author, date) is carried across rather than reattributed to the re-uploader. Also consolidates control-variant validation and construction onto a single scoped variant query instead of one lookup to validate and another to build. --- src/mavedb/lib/score_calibrations.py | 366 ++++++++++++-- src/mavedb/lib/types/score_calibrations.py | 81 ++- .../jobs/variant_processing/creation.py | 46 ++ .../jobs/variant_processing/test_creation.py | 460 ++++++++++++++++++ 4 files changed, 906 insertions(+), 47 deletions(-) diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index 5d270e043..627d607b7 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -1,8 +1,10 @@ """Utilities for building and mutating score calibration ORM objects.""" +import logging import math from collections import Counter -from typing import Optional, Sequence, Union +from datetime import date +from typing import Optional, Sequence, Union, cast import pandas as pd from sqlalchemy import Float, and_, select @@ -11,7 +13,13 @@ from mavedb.lib.acmg import find_or_create_acmg_classification from mavedb.lib.identifiers import find_or_create_publication_identifier from mavedb.lib.mondo import resolve_disease_term -from mavedb.lib.types.score_calibrations import ClassificationDict +from mavedb.lib.types.score_calibrations import ( + CalibrationControlSnapshot, + CalibrationVariantLinkSnapshot, + CalibrationVariantRelinkReport, + ClassificationDict, + VariantIdentity, +) from mavedb.lib.validation.constants.general import ( calibration_class_column_name, calibration_variant_column_name, @@ -24,12 +32,71 @@ from mavedb.models.enums.score_calibration_relation import ScoreCalibrationRelation from mavedb.models.score_calibration import ScoreCalibration from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification +from mavedb.models.score_calibration_functional_classification_variant_association import ( + score_calibration_functional_classification_variants_association_table, +) from mavedb.models.score_calibration_publication_identifier import ScoreCalibrationPublicationIdentifierAssociation from mavedb.models.score_set import ScoreSet from mavedb.models.user import User from mavedb.models.variant import Variant from mavedb.view_models import score_calibration +logger = logging.getLogger(__name__) + + +def _fetch_control_variants_by_urn( + db: Session, + score_set: ScoreSet, + controls: Sequence[score_calibration.CalibrationControlCreate], +) -> dict[str, Variant]: + """Fetch the ``Variant`` rows for a submission's control URNs, scoped to the score set, keyed by URN. + + A single scoped query serves both membership validation (a URN absent from the result does not + belong to the score set) and row construction, so validating and building a submission share one + lookup rather than each issuing its own. + """ + submitted_urns = [control.variant_urn for control in controls] + return { + variant.urn: variant + for variant in db.scalars( + select(Variant).where(Variant.score_set_id == score_set.id, Variant.urn.in_(submitted_urns)) + ).all() + # Every returned row matched an IN over the submitted (non-null) URNs, so its urn is non-null; + # the guard both states that and narrows the key type from Optional[str]. + if variant.urn is not None + } + + +def _validate_control_submission( + controls: Sequence[score_calibration.CalibrationControlCreate], + variants_by_urn: dict[str, Variant], +) -> None: + """Duplicate and score-set-membership checks for a control submission, given its resolved variants. + + A control earns its role as calibration evidence from a variant the assay actually scored, so each + must reference a variant belonging to the calibration's own score set. Duplicate URNs within one + submission are caught here with a readable message rather than deferred to the + ``UNIQUE(calibration_id, variant_id)`` database constraint. + + Raises: + ValidationError: If any control URN is duplicated within the submission, absent from MaveDB, + or belongs to a different score set. + """ + counts = Counter(control.variant_urn for control in controls) + + duplicate_urns = {urn for urn, count in counts.items() if count > 1} + if duplicate_urns: + raise ValidationError( + f"Duplicate control variant URNs detected within the submission: {', '.join(sorted(duplicate_urns))}." + ) + + missing_urns = counts.keys() - variants_by_urn.keys() + if missing_urns: + raise ValidationError( + "The following control variants do not belong to the calibration's score set: " + f"{', '.join(sorted(missing_urns))}." + ) + def validate_calibration_controls_in_score_set( db: Session, @@ -38,16 +105,9 @@ def validate_calibration_controls_in_score_set( ) -> list[str]: """Validate calibration controls against a score set and return their variant URNs. - A control earns its role as calibration evidence from a variant the assay actually scored, - so each control must reference a variant belonging to the calibration's own score set. A - single scoped query resolves all submitted URNs at once; any URN it does not return either - does not exist in MaveDB or belongs to a different score set, and both are rejected together. - - Duplicate variant URNs within one submission are caught here with a readable message rather - than deferred to the ``UNIQUE(calibration_id, variant_id)`` database constraint. - - The validated URNs are returned so a caller that goes on to persist the controls can reuse - them (see :func:`build_calibration_controls`). + A thin wrapper over :func:`_fetch_control_variants_by_urn` and :func:`_validate_control_submission` + for callers that only need the validated URNs; :func:`build_calibration_controls` shares the same + two steps to avoid re-fetching the variants. Args: db: Database session used for the lookup. @@ -64,28 +124,9 @@ def validate_calibration_controls_in_score_set( if not controls: return [] - submitted_urns = [control.variant_urn for control in controls] - - counts = Counter(submitted_urns) - duplicate_urns = {urn for urn, count in counts.items() if count > 1} - if duplicate_urns: - raise ValidationError( - f"Duplicate control variant URNs detected within the submission: {', '.join(sorted(duplicate_urns))}." - ) - - existing_urns = set( - db.scalars( - select(Variant.urn).where(Variant.score_set_id == score_set.id, Variant.urn.in_(submitted_urns)) - ).all() - ) - missing_urns = set(counts) - existing_urns - if missing_urns: - raise ValidationError( - "The following control variants do not belong to the calibration's score set: " - f"{', '.join(sorted(missing_urns))}." - ) - - return submitted_urns + variants_by_urn = _fetch_control_variants_by_urn(db, score_set, controls) + _validate_control_submission(controls, variants_by_urn) + return [control.variant_urn for control in controls] def build_calibration_controls( @@ -96,11 +137,12 @@ def build_calibration_controls( ) -> list[CalibrationControl]: """Validate and construct transient ``CalibrationControl`` rows for a calibration. - Validation (see :func:`validate_calibration_controls_in_score_set`) returns the validated - variant URNs, which this function resolves to their ``Variant`` rows in one query and turns - into unattached ``CalibrationControl`` rows with audit fields set. The caller assigns them to - the calibration's ``controls`` collection and commits. Returns an empty list for ``None`` or - empty input. + Fetches the submission's ``Variant`` rows once, validates them (duplicates + score-set membership, + see :func:`_validate_control_submission`), then turns each control into an unattached + ``CalibrationControl`` with audit fields set. The single fetch is reused for validation and + construction, so this issues one variant query rather than one to validate and another to build. + The caller assigns the returned rows to the calibration's ``controls`` collection and commits. + Returns an empty list for ``None`` or empty input. Args: db: Database session. @@ -108,16 +150,11 @@ def build_calibration_controls( controls: Submitted controls, or ``None``/empty for none. user: The acting user, recorded on each control's audit fields. """ - submitted_urns = validate_calibration_controls_in_score_set(db, score_set, controls) if not controls: return [] - variants_by_urn = { - variant.urn: variant - for variant in db.scalars( - select(Variant).where(Variant.score_set_id == score_set.id, Variant.urn.in_(submitted_urns)) - ).all() - } + variants_by_urn = _fetch_control_variants_by_urn(db, score_set, controls) + _validate_control_submission(controls, variants_by_urn) return [ CalibrationControl( @@ -964,3 +1001,240 @@ def variant_classification_df_to_dict( classifications[functional_class].add(index_element) return {"indexed_by": index_column, "classifications": classifications} + + +def snapshot_calibration_variant_links(db: Session, score_set: ScoreSet) -> list[CalibrationVariantLinkSnapshot]: + """Record the calibration variant references a re-upload cannot reconstruct on its own. + + Re-uploading a score set's data deletes and recreates all of its ``Variant`` rows, which breaks + the foreign keys calibrations hold into them: ``calibration_controls.variant_id`` and the + functional-classification membership association. Neither carries an ``ON DELETE`` action, on + purpose — the resulting ``RESTRICT`` protects hand-entered controls from being destroyed by an + unrelated delete. Capturing the irreproducible references as :data:`VariantIdentity` tuples before + the delete lets :func:`restore_calibration_variant_links` re-point the survivors afterwards. + Variant URNs cannot serve as the key because they are renumbered on every upload. + + The rule for what to capture is whether the depositor asserted the reference or MaveDB derived it: + + * **Controls** carry clinical significance sourced from outside MaveDB, submitted inline or as a + ``controls_file``. Nothing in a score upload can regenerate them, so they are remembered. + * **Class-based bin membership** comes from the ``classes_file`` required of a class-based + calibration. That file is not part of a score upload either, so it too survives only by identity. + * **Range-based bin membership** is derived — purely a function of the variants' scores. It is + left out and recomputed from the new upload instead, which avoids holding an entry per variant + in memory and keeps membership honest when a re-upload moves a score across a threshold. + + The queries scope to variants in ``score_set``, which keeps the snapshot aligned with exactly the + rows the caller deletes. Every reference is already so scoped — controls are validated against the + calibration's own score set and bin membership is only ever populated from it — so the condition + holds the two halves together rather than filtering anything out. Were they to diverge, restore + would re-create a row whose original was never deleted and trip the unique constraint on + ``(calibration_id, variant_id)``. + + This function only reads. The caller performs the deletes. + + Args: + db: Database session. + score_set: The score set whose variants are about to be replaced. + + Returns: + One snapshot per calibration holding at least one irreproducible reference; empty when no + calibration has one. A score set whose calibrations use only range-based bins yields an empty + list and still needs :func:`restore_calibration_variant_links` called to re-bin them. + """ + calibration_ids = [calibration.id for calibration in score_set.score_calibrations] + if not calibration_ids: + return [] + + snapshots: dict[int, CalibrationVariantLinkSnapshot] = {} + + control_rows = db.execute( + select( + CalibrationControl.calibration_id, + CalibrationControl.clinical_status, + CalibrationControl.created_by_id, + CalibrationControl.creation_date, + Variant.hgvs_nt, + Variant.hgvs_pro, + Variant.hgvs_splice, + ) + .join(Variant, Variant.id == CalibrationControl.variant_id) + .where(CalibrationControl.calibration_id.in_(calibration_ids), Variant.score_set_id == score_set.id) + ).all() + + for calibration_id, clinical_status, created_by_id, creation_date, hgvs_nt, hgvs_pro, hgvs_splice in control_rows: + snapshot = snapshots.setdefault(calibration_id, CalibrationVariantLinkSnapshot(calibration_id=calibration_id)) + snapshot.controls.append( + CalibrationControlSnapshot( + identity=(hgvs_nt, hgvs_pro, hgvs_splice), + clinical_status=clinical_status, + created_by_id=created_by_id, + creation_date=creation_date, + ) + ) + + association = score_calibration_functional_classification_variants_association_table + membership_rows = db.execute( + select( + ScoreCalibrationFunctionalClassification.calibration_id, + ScoreCalibrationFunctionalClassification.id, + Variant.hgvs_nt, + Variant.hgvs_pro, + Variant.hgvs_splice, + ) + .join(association, association.c.functional_classification_id == ScoreCalibrationFunctionalClassification.id) + .join(Variant, Variant.id == association.c.variant_id) + .where( + ScoreCalibrationFunctionalClassification.calibration_id.in_(calibration_ids), + ScoreCalibrationFunctionalClassification.class_.is_not(None), + Variant.score_set_id == score_set.id, + ) + ).all() + + for calibration_id, classification_id, hgvs_nt, hgvs_pro, hgvs_splice in membership_rows: + snapshot = snapshots.setdefault(calibration_id, CalibrationVariantLinkSnapshot(calibration_id=calibration_id)) + snapshot.classification_members.setdefault(classification_id, []).append((hgvs_nt, hgvs_pro, hgvs_splice)) + + return list(snapshots.values()) + + +def restore_calibration_variant_links( + db: Session, + score_set: ScoreSet, + snapshots: Sequence[CalibrationVariantLinkSnapshot], + updater: User, +) -> CalibrationVariantRelinkReport: + """Re-establish a score set's calibration variant references after its variants are recreated. + + Each calibration is handled by the two mechanisms its references call for: + + * **Controls and class-based bin membership** are carried across by + :data:`VariantIdentity`. A reference whose identity is absent from the new upload describes a + variant the assay no longer scores, so it is dropped rather than guessed at; an identity + matching more than one new variant is ambiguous and dropped for the same reason. Dropping + rather than failing is deliberate: a re-upload that aborted on a vanished control would leave + depositors unable to correct their own data. + * **Range-based bin membership** is recomputed from the new scores via + :func:`variants_for_functional_classification`, the same helper that populated it originally. + Carrying the old membership across might leave a variant filed under a range its new score no + longer falls in, so the recorded membership would simply be wrong. + + A classification always has exactly one of ``range`` or ``class_`` set (enforced by the view + models), so every classification falls squarely into one branch or the other. + + Dropping a control changes the control set the submitter affirmed as free of protected health + information, so ``controls_not_phi`` is reset to ``None`` on that calibration — the same + re-acknowledgment rule applied when controls are replaced through the API (see + :func:`modify_score_calibration`). A calibration whose controls all relink keeps its affirmation. + Re-binning does not reset it: bin membership carries no clinical annotation and so no PHI. + + Changes are staged on the session; the caller commits. + + Args: + db: Database session. + score_set: The score set whose variants have just been recreated. + snapshots: Output of :func:`snapshot_calibration_variant_links`, taken before the delete. May + be empty while calibrations still need re-binning. + updater: The user who triggered the re-upload, recorded on the rows this relink touches. + + Returns: + Counts of relinked, dropped and re-binned references, for the caller's job log. + """ + report = CalibrationVariantRelinkReport() + if not score_set.score_calibrations: + return report + + # Only controls and class-based bin membership are re-resolved by identity; range-based bins are + # recomputed from the new scores below and never touch this map. Building it materializes every + # variant of the score set, so skip that entirely when nothing needs identity resolution — the + # common case of a re-upload whose calibrations use only range-based bins. + variants_by_identity: dict[VariantIdentity, Optional[Variant]] = {} + if any(snapshot.controls or snapshot.classification_members for snapshot in snapshots): + for new_variant in db.scalars(select(Variant).where(Variant.score_set_id == score_set.id)).all(): + identity = (new_variant.hgvs_nt, new_variant.hgvs_pro, new_variant.hgvs_splice) + # Upstream validation rejects duplicate variants, but a collision here would otherwise bind + # every reference to whichever row happened to be seen first. Mark it unresolvable instead. + variants_by_identity[identity] = None if identity in variants_by_identity else new_variant + + snapshots_by_calibration_id = {snapshot.calibration_id: snapshot for snapshot in snapshots} + + for calibration in score_set.score_calibrations: + calibration_id = cast(int, calibration.id) + snapshot = snapshots_by_calibration_id.get( + calibration_id, CalibrationVariantLinkSnapshot(calibration_id=calibration_id) + ) + + # The linkage rows were removed with Core deletes, which leave any loaded collections holding + # rows that no longer exist. Expire them so the ORM rebuilds from the post-delete state rather + # than issuing deletes for rows that are already gone. + db.expire(calibration, ["controls"]) + + dropped_controls = 0 + relinked_controls: list[CalibrationControl] = [] + for control in snapshot.controls: + control_variant = variants_by_identity.get(control.identity) + if control_variant is None: + dropped_controls += 1 + continue + + relinked_controls.append( + CalibrationControl( + calibration_id=calibration.id, + variant_id=control_variant.id, + clinical_status=control.clinical_status, + created_by_id=control.created_by_id, + creation_date=control.creation_date, + modified_by_id=updater.id, + modification_date=date.today(), + ) + ) + + db.add_all(relinked_controls) + report.controls_relinked += len(relinked_controls) + report.controls_dropped += dropped_controls + + dropped_members = 0 + for classification in calibration.functional_classifications: + db.expire(classification, ["variants"]) + + if classification.class_ is not None: + members: list[Variant] = [] + for identity in snapshot.classification_members.get(cast(int, classification.id), []): + member_variant = variants_by_identity.get(identity) + if member_variant is None: + dropped_members += 1 + continue + + members.append(member_variant) + + report.classification_members_relinked += len(members) + else: + members = variants_for_functional_classification(db, classification, use_sql=True) + report.classifications_rebinned += 1 + report.classification_members_rebinned += len(members) + + classification.variants = members + db.add(classification) + + report.classification_members_dropped += dropped_members + + if dropped_controls: + calibration.controls_not_phi = None + report.calibrations_pending_phi_reaffirmation.append(calibration_id) + + # Re-binning is the expected mechanical consequence of new scores, so it does not count as an + # edit. Losing a hand-entered reference does. + if dropped_controls or dropped_members: + calibration.modified_by = updater + db.add(calibration) + + if report.controls_dropped or report.classification_members_dropped: + logger.warning( + "Dropped %s calibration control(s) and %s class-based bin membership(s) from score set %s: their " + "variants are absent from the new upload.", + report.controls_dropped, + report.classification_members_dropped, + score_set.urn, + ) + + return report diff --git a/src/mavedb/lib/types/score_calibrations.py b/src/mavedb/lib/types/score_calibrations.py index d40edaf27..b6b4753bc 100644 --- a/src/mavedb/lib/types/score_calibrations.py +++ b/src/mavedb/lib/types/score_calibrations.py @@ -1,6 +1,85 @@ -from typing import TypedDict +from __future__ import annotations + +from dataclasses import dataclass, field +from datetime import date +from typing import Any, Optional, TypedDict + +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus class ClassificationDict(TypedDict): indexed_by: str classifications: dict[str, set[str]] + + +VariantIdentity = tuple[Optional[str], Optional[str], Optional[str]] +"""A variant's ``(hgvs_nt, hgvs_pro, hgvs_splice)`` tuple. + +Variant URNs are positional within a score set (``{score_set.urn}#{n}``, renumbered from one on every +upload), so ``#5`` can name a different biological variant after a re-upload. The HGVS tuple is the +identity that survives one, and so is what calibration variant references are re-resolved against. +""" + + +@dataclass(frozen=True) +class CalibrationControlSnapshot: + """One calibration control, recorded by variant identity instead of variant id. + + Audit provenance is carried alongside the identity so a relinked control keeps the authorship and + date of the original submission rather than appearing to have been entered by whoever re-uploaded + the scores. + """ + + identity: VariantIdentity + clinical_status: CalibrationControlStatus + created_by_id: int + creation_date: date + + +@dataclass +class CalibrationVariantLinkSnapshot: + """The variant references held by one calibration that a re-upload cannot reconstruct. + + Only irreproducible references are captured. Range-based bin membership is omitted on purpose: it + is a function of the variants' scores, so it is recomputed from the new upload rather than + remembered (see :func:`~mavedb.lib.score_calibrations.restore_calibration_variant_links`). + """ + + calibration_id: int + controls: list[CalibrationControlSnapshot] = field(default_factory=list) + + # Class-based bin membership keyed by functional classification id. Classification rows survive a + # re-upload, so their ids remain valid addresses when the membership is restored. + classification_members: dict[int, list[VariantIdentity]] = field(default_factory=dict) + + +@dataclass +class CalibrationVariantRelinkReport: + """Outcome of re-establishing a score set's calibration variant references after a re-upload. + + Relinked and dropped counts describe references carried across by identity (controls and + class-based bin membership). Re-binned counts describe range-based membership recomputed from the + new scores, where "dropped" has no meaning — the bin simply holds whatever now falls in it. + """ + + controls_relinked: int = 0 + controls_dropped: int = 0 + classification_members_relinked: int = 0 + classification_members_dropped: int = 0 + classifications_rebinned: int = 0 + classification_members_rebinned: int = 0 + + # Calibrations whose ``controls_not_phi`` affirmation was cleared because a control was dropped. + calibrations_pending_phi_reaffirmation: list[int] = field(default_factory=list) + + def to_dict(self) -> dict[str, Any]: + """Return a JSON-serializable summary suitable for a job's logging context.""" + return { + "calibration_controls_relinked": self.controls_relinked, + "calibration_controls_dropped": self.controls_dropped, + "calibration_classification_members_relinked": self.classification_members_relinked, + "calibration_classification_members_dropped": self.classification_members_dropped, + "calibration_classifications_rebinned": self.classifications_rebinned, + "calibration_classification_members_rebinned": self.classification_members_rebinned, + "calibrations_pending_phi_reaffirmation": list(self.calibrations_pending_phi_reaffirmation), + } diff --git a/src/mavedb/worker/jobs/variant_processing/creation.py b/src/mavedb/worker/jobs/variant_processing/creation.py index a519bda07..480705ecb 100644 --- a/src/mavedb/worker/jobs/variant_processing/creation.py +++ b/src/mavedb/worker/jobs/variant_processing/creation.py @@ -13,14 +13,23 @@ from mavedb.data_providers.services import CSV_UPLOAD_S3_BUCKET_NAME, RESTDataProvider, s3_client from mavedb.lib.logging.context import format_raised_exception_info_as_dict +from mavedb.lib.score_calibrations import ( + restore_calibration_variant_links, + snapshot_calibration_variant_links, +) from mavedb.lib.score_sets import columns_for_dataset, create_variants, create_variants_data +from mavedb.lib.types.score_calibrations import CalibrationVariantLinkSnapshot from mavedb.lib.types.workflow import JobExecutionOutcome from mavedb.lib.validation.dataframe.dataframe import validate_and_standardize_dataframe_pair from mavedb.lib.validation.exceptions import ValidationError +from mavedb.models.calibration_control import CalibrationControl from mavedb.models.enums.job_pipeline import FailureCategory from mavedb.models.enums.mapping_state import MappingState from mavedb.models.enums.processing_state import ProcessingState from mavedb.models.mapped_variant import MappedVariant +from mavedb.models.score_calibration_functional_classification_variant_association import ( + score_calibration_functional_classification_variants_association_table, +) from mavedb.models.score_set import ScoreSet from mavedb.models.user import User from mavedb.models.variant import Variant @@ -180,10 +189,30 @@ async def create_variants_for_score_set(ctx: dict, job_id: int, job_manager: Job } # Delete variants after validation occurs so we don't overwrite them in the case of a bad update. + calibration_link_snapshot: list[CalibrationVariantLinkSnapshot] = [] + relink_calibrations = False if score_set.variants: existing_variants = job_manager.db.scalars( select(Variant.id).where(Variant.score_set_id == score_set.id) ).all() + + # Calibration controls and functional classification bin membership reference variants + # without an ON DELETE action, so the variant delete below fails outright while those + # links stand. Record what the new upload cannot regenerate, clear the links, and + # re-establish them once the new variants exist. + relink_calibrations = bool(score_set.score_calibrations) + calibration_link_snapshot = snapshot_calibration_variant_links(job_manager.db, score_set) + job_manager.db.execute( + delete(CalibrationControl).where(CalibrationControl.variant_id.in_(existing_variants)) + ) + job_manager.db.execute( + delete(score_calibration_functional_classification_variants_association_table).where( + score_calibration_functional_classification_variants_association_table.c.variant_id.in_( + existing_variants + ) + ) + ) + job_manager.db.execute(delete(MappedVariant).where(MappedVariant.variant_id.in_(existing_variants))) job_manager.db.execute(delete(Variant).where(Variant.id.in_(existing_variants))) @@ -198,6 +227,23 @@ async def create_variants_for_score_set(ctx: dict, job_id: int, job_manager: Job variants_data = create_variants_data(validated_scores, validated_counts, None) create_variants(job_manager.db, score_set, variants_data) + # Scoped to the replacement path above: only links the delete broke need re-establishing, so a + # first upload leaves calibrations alone. An empty snapshot is not the same as nothing to do, + # though, since range-based bins are recomputed from the new scores rather than remembered. + if relink_calibrations: + # The new variants need ids and scores on record before anything can point at them. + job_manager.db.flush() + + relink_report = restore_calibration_variant_links( + job_manager.db, score_set, calibration_link_snapshot, updated_by + ) + job_manager.save_to_context(relink_report.to_dict()) + + logger.info( + msg="Re-established calibration variant references against the newly created variants.", + extra=job_manager.logging_context(), + ) + except ValidationError as e: job_manager.db.rollback() diff --git a/tests/worker/jobs/variant_processing/test_creation.py b/tests/worker/jobs/variant_processing/test_creation.py index d0d37562a..4c937eb6e 100644 --- a/tests/worker/jobs/variant_processing/test_creation.py +++ b/tests/worker/jobs/variant_processing/test_creation.py @@ -5,13 +5,19 @@ pytest.importorskip("arq") import math +from datetime import date, timedelta from unittest.mock import ANY, MagicMock, call, patch +from mavedb.lib.mondo import get_generic_disease_term +from mavedb.models.calibration_control import CalibrationControl +from mavedb.models.enums.calibration_control_status import CalibrationControlStatus from mavedb.models.enums.job_pipeline import JobStatus, PipelineStatus from mavedb.models.enums.mapping_state import MappingState from mavedb.models.enums.processing_state import ProcessingState from mavedb.models.job_run import JobRun from mavedb.models.pipeline import Pipeline +from mavedb.models.score_calibration import ScoreCalibration +from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification from mavedb.models.variant import Variant from mavedb.worker.jobs.variant_processing.creation import create_variants_for_score_set from mavedb.worker.lib.managers.job_manager import JobManager @@ -1188,3 +1194,457 @@ async def test_create_variants_for_score_set_with_arq_context_generic_exception_ ) for run in other_runs: assert run.status == JobStatus.SKIPPED + + +def _make_calibration(session, score_set, user, controls_not_phi=None): + """Persist a bare calibration on a score set; controls and bin membership need a parent.""" + calibration = ScoreCalibration( + title="Relink test calibration", + score_set_id=score_set.id, + disease_term=get_generic_disease_term(session), + controls_not_phi=controls_not_phi, + created_by=user, + modified_by=user, + ) + session.add(calibration) + session.commit() + session.refresh(calibration) + return calibration + + +def _variants_by_hgvs_nt(session, score_set): + return {variant.hgvs_nt: variant for variant in session.query(Variant).filter_by(score_set_id=score_set.id).all()} + + +def _without_variant(scores_df, counts_df, hgvs_nt): + """Return the score/count pair with one variant removed, simulating a corrected re-upload.""" + return ( + scores_df[scores_df["hgvs_nt"] != hgvs_nt].copy(), + counts_df[counts_df["hgvs_nt"] != hgvs_nt].copy(), + ) + + +async def _run_creation_job(mock_worker_ctx, mock_s3_client, job_run, scores_df, counts_df): + with ( + patch.object(mock_s3_client, "download_fileobj", return_value=None), + patch( + "mavedb.worker.jobs.variant_processing.creation.pd.read_csv", + side_effect=[scores_df, counts_df], + ), + ): + await create_variants_for_score_set(mock_worker_ctx, job_run.id) + + +@pytest.mark.integration +@pytest.mark.asyncio +class TestCreateVariantsForScoreSetCalibrationRelinking: + """Re-upload behavior for score sets whose calibrations reference variants. + + Calibration controls and functional classification bin membership hold plain foreign keys into + ``variants``, which a re-upload deletes and recreates wholesale. These tests pin both halves of + the fix: the delete no longer trips those constraints, and surviving references are re-resolved + against the new rows by HGVS identity rather than by the positional variant URN. + """ + + async def test_reupload_with_calibration_controls_does_not_violate_foreign_key( + self, + session, + with_independent_processing_runs, + with_populated_domain_data, + mock_worker_ctx, + mock_s3_client, + sample_score_dataframe, + sample_count_dataframe, + sample_score_set, + sample_user, + sample_independent_variant_creation_run, + ): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + calibration = _make_calibration(session, sample_score_set, sample_user) + session.add( + CalibrationControl( + calibration_id=calibration.id, + variant_id=_variants_by_hgvs_nt(session, sample_score_set)["c.1A>T"].id, + clinical_status=CalibrationControlStatus.pathogenic, + created_by=sample_user, + modified_by=sample_user, + ) + ) + sample_independent_variant_creation_run.status = JobStatus.PENDING + session.commit() + + # Regression: this second upload raised ForeignKeyViolation on calibration_controls_variant_id_fkey. + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + session.refresh(sample_score_set) + assert sample_score_set.processing_state == ProcessingState.success + assert session.query(CalibrationControl).filter_by(calibration_id=calibration.id).count() == 1 + + async def test_controls_relink_to_new_variants_when_every_identity_persists( + self, + session, + with_independent_processing_runs, + with_populated_domain_data, + mock_worker_ctx, + mock_s3_client, + sample_score_dataframe, + sample_count_dataframe, + sample_score_set, + sample_user, + sample_extra_user, + sample_independent_variant_creation_run, + ): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + original_variant_ids = { + hgvs: variant.id for hgvs, variant in _variants_by_hgvs_nt(session, sample_score_set).items() + } + calibration = _make_calibration(session, sample_score_set, sample_user, controls_not_phi=True) + + # An older submission date proves the relinked row keeps its own provenance rather than + # inheriting the re-upload's. + submitted_on = date.today() - timedelta(days=30) + for hgvs_nt, status in ( + ("c.1A>T", CalibrationControlStatus.pathogenic), + ("c.2T>A", CalibrationControlStatus.benign), + ): + session.add( + CalibrationControl( + calibration_id=calibration.id, + variant_id=original_variant_ids[hgvs_nt], + clinical_status=status, + created_by=sample_extra_user, + modified_by=sample_extra_user, + creation_date=submitted_on, + modification_date=submitted_on, + ) + ) + + updated_scores = sample_score_dataframe.copy() + updated_scores["score"] += 10 + + sample_independent_variant_creation_run.status = JobStatus.PENDING + session.commit() + + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + updated_scores, + sample_count_dataframe.copy(), + ) + + new_variants = _variants_by_hgvs_nt(session, sample_score_set) + assert set(new_variants) == set(original_variant_ids) + # Every variant row really was replaced, so a stale link would be visible below. + assert all(new_variants[hgvs].id != variant_id for hgvs, variant_id in original_variant_ids.items()) + + controls = session.query(CalibrationControl).filter_by(calibration_id=calibration.id).all() + assert len(controls) == 2 + assert {control.variant_id for control in controls} == { + new_variants["c.1A>T"].id, + new_variants["c.2T>A"].id, + } + assert {control.clinical_status for control in controls} == { + CalibrationControlStatus.pathogenic, + CalibrationControlStatus.benign, + } + assert all(control.created_by_id == sample_extra_user.id for control in controls) + assert all(control.creation_date == submitted_on for control in controls) + assert all(control.modified_by_id == sample_user.id for control in controls) + + # Nothing was dropped, so the submitter's PHI affirmation still describes this control set. + session.refresh(calibration) + assert calibration.controls_not_phi is True + + async def test_controls_absent_from_new_upload_are_dropped_and_reset_phi_affirmation( + self, + session, + with_independent_processing_runs, + with_populated_domain_data, + mock_worker_ctx, + mock_s3_client, + sample_score_dataframe, + sample_count_dataframe, + sample_score_set, + sample_user, + sample_independent_variant_creation_run, + ): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + original_variants = _variants_by_hgvs_nt(session, sample_score_set) + calibration = _make_calibration(session, sample_score_set, sample_user, controls_not_phi=True) + for hgvs_nt in ("c.1A>T", "c.4C>G"): + session.add( + CalibrationControl( + calibration_id=calibration.id, + variant_id=original_variants[hgvs_nt].id, + clinical_status=CalibrationControlStatus.pathogenic, + created_by=sample_user, + modified_by=sample_user, + ) + ) + + sample_independent_variant_creation_run.status = JobStatus.PENDING + session.commit() + + trimmed_scores, trimmed_counts = _without_variant(sample_score_dataframe, sample_count_dataframe, "c.4C>G") + + recorded_context: dict = {} + original_save_to_context = JobManager.save_to_context + + def _capture_context(self, ctx): + recorded_context.update(ctx) + return original_save_to_context(self, ctx) + + with patch.object(JobManager, "save_to_context", _capture_context): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + trimmed_scores, + trimmed_counts, + ) + + new_variants = _variants_by_hgvs_nt(session, sample_score_set) + assert "c.4C>G" not in new_variants + + controls = session.query(CalibrationControl).filter_by(calibration_id=calibration.id).all() + assert len(controls) == 1 + assert controls[0].variant_id == new_variants["c.1A>T"].id + + # A control set that lost a member is no longer the one the submitter affirmed as PHI-free. + session.refresh(calibration) + assert calibration.controls_not_phi is None + + assert recorded_context["calibration_controls_relinked"] == 1 + assert recorded_context["calibration_controls_dropped"] == 1 + assert recorded_context["calibrations_pending_phi_reaffirmation"] == [calibration.id] + + async def test_range_based_classification_membership_is_rebinned_against_new_scores( + self, + session, + with_independent_processing_runs, + with_populated_domain_data, + mock_worker_ctx, + mock_s3_client, + sample_score_dataframe, + sample_count_dataframe, + sample_score_set, + sample_user, + sample_independent_variant_creation_run, + ): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + # Sample scores: c.1A>T 0.3, c.2T>A 0.0, c.3G>C -1.65, c.4C>G unscored. + original_variants = _variants_by_hgvs_nt(session, sample_score_set) + calibration = _make_calibration(session, sample_score_set, sample_user) + normal = ScoreCalibrationFunctionalClassification( + calibration_id=calibration.id, + label="Normal function", + range=[0.0, None], + inclusive_lower_bound=True, + ) + normal.variants = [original_variants["c.1A>T"], original_variants["c.2T>A"]] + abnormal = ScoreCalibrationFunctionalClassification( + calibration_id=calibration.id, + label="Abnormal function", + range=[None, 0.0], + inclusive_upper_bound=False, + ) + abnormal.variants = [original_variants["c.3G>C"]] + session.add_all([normal, abnormal]) + + original_variant_ids = {variant.id for variant in original_variants.values()} + + sample_independent_variant_creation_run.status = JobStatus.PENDING + session.commit() + normal_id, abnormal_id = normal.id, abnormal.id + + # Shifting every score up by two moves c.3G>C across the threshold, so recorded membership and + # recomputed membership disagree and the test can tell which one was applied. + updated_scores = sample_score_dataframe.copy() + updated_scores["score"] += 2 + + recorded_context: dict = {} + original_save_to_context = JobManager.save_to_context + + def _capture_context(self, ctx): + recorded_context.update(ctx) + return original_save_to_context(self, ctx) + + with patch.object(JobManager, "save_to_context", _capture_context): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + updated_scores, + sample_count_dataframe.copy(), + ) + + session.expire_all() + normal = session.get(ScoreCalibrationFunctionalClassification, normal_id) + abnormal = session.get(ScoreCalibrationFunctionalClassification, abnormal_id) + + # Membership follows the new scores rather than what was recorded: c.3G>C (-1.65 -> 0.35) moves + # into the normal bin, emptying the abnormal one. c.4C>G stays out of both, having no score. + assert {variant.hgvs_nt for variant in normal.variants} == {"c.1A>T", "c.2T>A", "c.3G>C"} + assert normal.variant_count == 3 + assert abnormal.variants == [] + assert abnormal.variant_count == 0 + + # Re-binned membership points at the new variant rows, not the deleted ones. + new_variants = _variants_by_hgvs_nt(session, sample_score_set) + assert {variant.id for variant in normal.variants} <= {variant.id for variant in new_variants.values()} + assert {variant.id for variant in normal.variants}.isdisjoint(original_variant_ids) + + assert recorded_context["calibration_classifications_rebinned"] == 2 + assert recorded_context["calibration_classification_members_rebinned"] == 3 + # Range bins are recomputed, never relinked, so nothing is reported as carried across. + assert recorded_context["calibration_classification_members_relinked"] == 0 + + async def test_class_based_classification_membership_relinks_by_hgvs_identity( + self, + session, + with_independent_processing_runs, + with_populated_domain_data, + mock_worker_ctx, + mock_s3_client, + sample_score_dataframe, + sample_count_dataframe, + sample_score_set, + sample_user, + sample_independent_variant_creation_run, + ): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + original_variants = _variants_by_hgvs_nt(session, sample_score_set) + calibration = _make_calibration(session, sample_score_set, sample_user) + + # A class-based bin comes from an uploaded classes file that a score upload does not carry, so + # its membership can only survive by identity. + classification = ScoreCalibrationFunctionalClassification( + calibration_id=calibration.id, + label="Loss of function", + class_="loss_of_function", + ) + classification.variants = [original_variants["c.1A>T"], original_variants["c.4C>G"]] + session.add(classification) + + sample_independent_variant_creation_run.status = JobStatus.PENDING + session.commit() + classification_id = classification.id + + trimmed_scores, trimmed_counts = _without_variant(sample_score_dataframe, sample_count_dataframe, "c.4C>G") + + recorded_context: dict = {} + original_save_to_context = JobManager.save_to_context + + def _capture_context(self, ctx): + recorded_context.update(ctx) + return original_save_to_context(self, ctx) + + with patch.object(JobManager, "save_to_context", _capture_context): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + trimmed_scores, + trimmed_counts, + ) + + new_variants = _variants_by_hgvs_nt(session, sample_score_set) + assert "c.4C>G" not in new_variants + + session.expire_all() + classification = session.get(ScoreCalibrationFunctionalClassification, classification_id) + + # The surviving member is relinked to its new row; the one absent from the upload is dropped. + assert [variant.id for variant in classification.variants] == [new_variants["c.1A>T"].id] + assert classification.variant_count == 1 + + assert recorded_context["calibration_classification_members_relinked"] == 1 + assert recorded_context["calibration_classification_members_dropped"] == 1 + assert recorded_context["calibration_classifications_rebinned"] == 0 + + async def test_reupload_without_calibrations_is_unaffected( + self, + session, + with_independent_processing_runs, + with_populated_domain_data, + mock_worker_ctx, + mock_s3_client, + sample_score_dataframe, + sample_count_dataframe, + sample_score_set, + sample_independent_variant_creation_run, + ): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + sample_independent_variant_creation_run.status = JobStatus.PENDING + session.commit() + + recorded_context: dict = {} + original_save_to_context = JobManager.save_to_context + + def _capture_context(self, ctx): + recorded_context.update(ctx) + return original_save_to_context(self, ctx) + + with patch.object(JobManager, "save_to_context", _capture_context): + await _run_creation_job( + mock_worker_ctx, + mock_s3_client, + sample_independent_variant_creation_run, + sample_score_dataframe, + sample_count_dataframe, + ) + + session.refresh(sample_score_set) + assert sample_score_set.processing_state == ProcessingState.success + assert sample_score_set.num_variants == sample_score_dataframe.shape[0] + # The relink path stays out of the way entirely when there is nothing to relink. + assert "calibration_controls_relinked" not in recorded_context From e085ba4eb79a8349ee09ad6bc70f4743911c1f80 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Mon, 21 Sep 2026 10:20:21 -0700 Subject: [PATCH 18/19] fix(calibration): give directly-constructed test calibrations a disease term score_calibrations.disease_term_id is now NOT NULL; two helpers in tests/lib/csv/test_variant.py built ScoreCalibration rows without one, tripping the constraint the first time they were exercised against a real session. --- tests/lib/csv/test_variant.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/lib/csv/test_variant.py b/tests/lib/csv/test_variant.py index 6b475d6fd..13b7f5de2 100644 --- a/tests/lib/csv/test_variant.py +++ b/tests/lib/csv/test_variant.py @@ -18,6 +18,7 @@ available_variant_csv_namespaces, get_variant_csv, ) +from mavedb.lib.mondo import get_generic_disease_term from mavedb.lib.permissions.principal import Principal from mavedb.lib.permissions.score_calibration import ScoreCalibrationViewer from mavedb.models.acmg_classification import ACMGClassification @@ -63,6 +64,7 @@ def _add_pathogenicity_calibration(db, score_set, variants_in_abnormal_range, ur primary=True, private=False, calibration_metadata={}, + disease_term=get_generic_disease_term(db), created_by_id=score_set.created_by_id, modified_by_id=score_set.modified_by_id, ) @@ -120,6 +122,7 @@ def _add_rangeless_calibration(db, score_set, urn, title): primary=True, private=False, calibration_metadata={}, + disease_term=get_generic_disease_term(db), created_by_id=score_set.created_by_id, modified_by_id=score_set.modified_by_id, ) From e1732e7c02fa8ea1649a3994fd8285877f400d7c Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Mon, 21 Sep 2026 10:21:01 -0700 Subject: [PATCH 19/19] refactor(mondo): split OLS integration out of mondo.py for core-dependency imports mavedb.lib.mondo imported mavedb.lib.logging.context at module level, which pulls in fastapi/starlette (the optional "server" extra) via mavedb.lib.logging.logged_route. tests/helpers/mocks/factories.py imports mondo.py for its generic-term constants and is itself hard-imported by tests/lib/conftest.py, so this broke collection for all of tests/lib/ plus every view_models test that reaches ScoreCalibration, under the "Pytest on Core Dependencies" CI job (no server extras installed). logging.context is only actually needed by the two OLS-network functions (search_mondo, fetch_mondo_term); everything else in the module (constants, MappableConcept serialization, get_generic_disease_term) has no such dependency. Moved the network functions, plus find_or_create_mondo_term and resolve_disease_term (which call them), to a new mondo_ols.py; mondo.py keeps the vocabulary surface and stays importable without server extras. See #459 for the deeper fix (making logging.context itself core-compatible), which this intentionally does not attempt. --- src/mavedb/lib/mondo.py | 143 +++-------------------- src/mavedb/lib/mondo_ols.py | 138 ++++++++++++++++++++++ src/mavedb/lib/score_calibrations.py | 2 +- src/mavedb/routers/diseases.py | 3 +- tests/lib/mondo/network/test_mondo.py | 5 +- tests/lib/mondo/test_mondo.py | 17 ++- tests/routers/test_score_calibrations.py | 2 +- 7 files changed, 168 insertions(+), 142 deletions(-) create mode 100644 src/mavedb/lib/mondo_ols.py diff --git a/src/mavedb/lib/mondo.py b/src/mavedb/lib/mondo.py index 71a9c1223..7906146c8 100644 --- a/src/mavedb/lib/mondo.py +++ b/src/mavedb/lib/mondo.py @@ -1,44 +1,37 @@ -"""MONDO disease-term resolution and search. +"""MONDO disease-term vocabulary: constants, MappableConcept serialization, and the generic term. Calibrations carry a disease/disorder context drawn from the Monarch Disease Ontology (MONDO). This -module owns the MONDO vocabulary integration: get-or-create of :class:`MondoTerm` rows (mirroring -:func:`mavedb.lib.taxonomies.find_or_create_taxonomy`), a typeahead search + validation against the -EBI OLS4 service, and conversion of a stored term to the GA4GH ``MappableConcept`` served on the wire. +module owns the vocabulary's static surface: the generic "disease or disorder" root, conversion of a +stored :class:`MondoTerm` (or an OLS suggestion) to the GA4GH ``MappableConcept`` served on the wire, and +get-or-create of the generic term (mirroring :func:`mavedb.lib.taxonomies.find_or_create_taxonomy`). -The generic term (``MONDO:0000001``, "disease or disorder") is the write-time default for a calibration -with no specific disease, so ``disease_term_id`` is never null. The annotation layer serializes whatever -term a calibration carries — generic or specific — as its VA-Spec disease condition (see +The generic term (``MONDO:0000001``) is the write-time default for a calibration with no specific +disease, so ``disease_term_id`` is never null. The annotation layer serializes whatever term a +calibration carries — generic or specific — as its VA-Spec disease condition (see :func:`mavedb.lib.annotation.condition.calibration_disease_condition`). + +Typeahead search and OLS-backed validation of a submitted code live in :mod:`mavedb.lib.mondo_ols`, +kept separate because they need :mod:`mavedb.lib.logging.context` (an optional "server" dependency, +tracked for a core-compatible rework in +`#459 `_); this module has no such dependency, +so callers that only need the constants or serialization (e.g. the score calibration view model) stay +importable under core dependencies. """ -import logging -from typing import Any, Optional, TypedDict +from typing import TypedDict -import httpx from ga4gh.core.models import Coding, MappableConcept, iriReference from sqlalchemy.orm import Session -from mavedb.lib.exceptions import MondoServiceError -from mavedb.lib.logging.context import format_raised_exception_info_as_dict, logging_context, save_to_logging_context -from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.mondo_term import MondoTerm -logger = logging.getLogger(__name__) - # The Monarch Disease Ontology (MONDO), the controlled vocabulary for disease terms used across MaveDB. MONDO_SYSTEM = "https://purl.obolibrary.org/obo/mondo.owl" -# EBI Ontology Lookup Service (OLS4) — the source for MONDO term search and validation. -OLS_BASE_URL = "https://www.ebi.ac.uk/ols4/api" -MONDO_ONTOLOGY = "mondo" -# OLS can be slow; the default httpx 5s timeout is too tight for its autocomplete on short queries. -OLS_TIMEOUT = 10.0 # The generic "disease or disorder" root, used when a calibration names no specific disease. MONDO_GENERIC_CODE = "MONDO:0000001" MONDO_GENERIC_LABEL = "disease or disorder" -DEFAULT_SEARCH_LIMIT = 20 - class MondoSuggestion(TypedDict): """A single MONDO term resolved from OLS: the CURIE code, its label, and its resolvable IRI.""" @@ -93,64 +86,6 @@ def generic_disease_mappable_concept() -> MappableConcept: ) -async def search_mondo(query: str, limit: int = DEFAULT_SEARCH_LIMIT) -> list[MondoSuggestion]: - """Search MONDO disease classes by free text via OLS, for the disease typeahead.""" - if not query or not query.strip(): - return [] - - # OLS4's /select endpoint is the autocomplete-optimized path; /search times out on broad prefixes - # (e.g. a two-letter query), which is often what a typeahead sends. - params: dict[str, Any] = { - "q": query.strip(), - "ontology": MONDO_ONTOLOGY, - "rows": limit, - "fieldList": "iri,label,obo_id", - } - save_to_logging_context({"mondo_search_query": query, "mondo_search_limit": limit}) - try: - async with httpx.AsyncClient(timeout=OLS_TIMEOUT) as client: - response = await client.get(f"{OLS_BASE_URL}/select", params=params) - response.raise_for_status() - docs = response.json().get("response", {}).get("docs", []) - except httpx.HTTPError as exc: - save_to_logging_context(format_raised_exception_info_as_dict(exc)) - logger.error(msg="MONDO/OLS search request failed.", exc_info=exc, extra=logging_context()) - raise MondoServiceError("Disease ontology service temporarily unavailable") from exc - - return [ - MondoSuggestion(code=doc["obo_id"], label=doc["label"], iri=doc.get("iri", mondo_iri(doc["obo_id"]))) - for doc in docs - if doc.get("obo_id") and doc.get("label") - ] - - -async def fetch_mondo_term(code: str) -> Optional[MondoSuggestion]: - """Resolve one MONDO CURIE to its canonical label + IRI via OLS, or None if it does not exist.""" - params: dict[str, Any] = { - "q": code, - "ontology": MONDO_ONTOLOGY, - "queryFields": "obo_id", - "exact": "true", - "rows": 1, - "fieldList": "iri,label,obo_id", - } - try: - async with httpx.AsyncClient(timeout=OLS_TIMEOUT) as client: - response = await client.get(f"{OLS_BASE_URL}/search", params=params) - response.raise_for_status() - docs = response.json().get("response", {}).get("docs", []) - except httpx.HTTPError as exc: - save_to_logging_context(format_raised_exception_info_as_dict(exc)) - logger.error(msg="MONDO/OLS term lookup failed.", exc_info=exc, extra=logging_context()) - raise MondoServiceError("Disease ontology service temporarily unavailable") from exc - - if not docs: - return None - - doc = docs[0] - return MondoSuggestion(code=doc["obo_id"], label=doc["label"], iri=doc.get("iri", mondo_iri(code))) - - def get_generic_disease_term(db: Session) -> MondoTerm: """Get-or-create the generic disease term. Known-canonical, so it needs no OLS round trip.""" term = ( @@ -162,51 +97,3 @@ def get_generic_disease_term(db: Session) -> MondoTerm: db.flush() return term - - -async def find_or_create_mondo_term(db: Session, code: str) -> MondoTerm: - """Find an existing MONDO term by code, or create one after validating it against OLS. - - A new code is validated against OLS and its canonical label stored. An unknown code raises - ``ValidationError``; an OLS outage raises ``MondoServiceError`` (via :func:`fetch_mondo_term`) so a - term is never persisted without a validated label — a controlled vocabulary must stay validated. - """ - # The generic term is canonical and seeded — resolve it directly, never via OLS. - if code == MONDO_GENERIC_CODE: - return get_generic_disease_term(db) - - term = db.query(MondoTerm).filter(MondoTerm.system == MONDO_SYSTEM, MondoTerm.code == code).one_or_none() - if term is not None: - return term - - resolved = await fetch_mondo_term(code) - if resolved is None: - raise ValidationError(f"'{code}' is not a valid MONDO disease term.", custom_loc=["body", "disease"]) - - # OLS may normalize the submitted code (case, or an alias/obsolete id) to a different canonical code, - # so re-check existence by the resolved code before inserting: keying the lookup on the same value we - # insert under keeps this idempotent and avoids a UNIQUE(system, code) violation when the canonical - # row already exists under a different submitted spelling. - canonical_code = resolved["code"] - term = db.query(MondoTerm).filter(MondoTerm.system == MONDO_SYSTEM, MondoTerm.code == canonical_code).one_or_none() - if term is not None: - return term - - term = MondoTerm(code=canonical_code, system=MONDO_SYSTEM, label=resolved["label"]) # type: ignore[call-arg] - db.add(term) - db.flush() - - return term - - -async def resolve_disease_term(db: Session, code: Optional[str]) -> MondoTerm: - """Resolve a calibration's submitted disease code to a stored MONDO term. - - ``None`` (no disease specified) resolves to the generic "disease or disorder" term; any other code - is validated/created via :func:`find_or_create_mondo_term`. Shared by the score calibration create - and update paths so the write-time default lives in one place. - """ - if code is None: - return get_generic_disease_term(db) - - return await find_or_create_mondo_term(db, code) diff --git a/src/mavedb/lib/mondo_ols.py b/src/mavedb/lib/mondo_ols.py new file mode 100644 index 000000000..c6a58bcec --- /dev/null +++ b/src/mavedb/lib/mondo_ols.py @@ -0,0 +1,138 @@ +"""MONDO disease-term search and validation against the EBI Ontology Lookup Service (OLS4). + +Split out from :mod:`mavedb.lib.mondo`: this module resolves and validates a submitted MONDO code +against the live OLS4 service, which needs :mod:`mavedb.lib.logging.context` for structured request +logging — and that, in turn, needs ``starlette``, an optional "server" dependency (see +`#459 `_ for making logging context available to +core dependencies directly). Keeping the OLS calls here, separate from :mod:`mavedb.lib.mondo`'s +constants and serialization, lets callers that only need the static vocabulary stay importable without +the server extras. +""" + +import logging +from typing import Any, Optional + +import httpx +from sqlalchemy.orm import Session + +from mavedb.lib.exceptions import MondoServiceError +from mavedb.lib.logging.context import format_raised_exception_info_as_dict, logging_context, save_to_logging_context +from mavedb.lib.mondo import MONDO_GENERIC_CODE, MONDO_SYSTEM, MondoSuggestion, get_generic_disease_term, mondo_iri +from mavedb.lib.validation.exceptions import ValidationError +from mavedb.models.mondo_term import MondoTerm + +logger = logging.getLogger(__name__) + +# EBI Ontology Lookup Service (OLS4) — the source for MONDO term search and validation. +OLS_BASE_URL = "https://www.ebi.ac.uk/ols4/api" +MONDO_ONTOLOGY = "mondo" +# OLS can be slow; the default httpx 5s timeout is too tight for its autocomplete on short queries. +OLS_TIMEOUT = 10.0 + +DEFAULT_SEARCH_LIMIT = 20 + + +async def search_mondo(query: str, limit: int = DEFAULT_SEARCH_LIMIT) -> list[MondoSuggestion]: + """Search MONDO disease classes by free text via OLS, for the disease typeahead.""" + if not query or not query.strip(): + return [] + + # OLS4's /select endpoint is the autocomplete-optimized path; /search times out on broad prefixes + # (e.g. a two-letter query), which is often what a typeahead sends. + params: dict[str, Any] = { + "q": query.strip(), + "ontology": MONDO_ONTOLOGY, + "rows": limit, + "fieldList": "iri,label,obo_id", + } + save_to_logging_context({"mondo_search_query": query, "mondo_search_limit": limit}) + try: + async with httpx.AsyncClient(timeout=OLS_TIMEOUT) as client: + response = await client.get(f"{OLS_BASE_URL}/select", params=params) + response.raise_for_status() + docs = response.json().get("response", {}).get("docs", []) + except httpx.HTTPError as exc: + save_to_logging_context(format_raised_exception_info_as_dict(exc)) + logger.error(msg="MONDO/OLS search request failed.", exc_info=exc, extra=logging_context()) + raise MondoServiceError("Disease ontology service temporarily unavailable") from exc + + return [ + MondoSuggestion(code=doc["obo_id"], label=doc["label"], iri=doc.get("iri", mondo_iri(doc["obo_id"]))) + for doc in docs + if doc.get("obo_id") and doc.get("label") + ] + + +async def fetch_mondo_term(code: str) -> Optional[MondoSuggestion]: + """Resolve one MONDO CURIE to its canonical label + IRI via OLS, or None if it does not exist.""" + params: dict[str, Any] = { + "q": code, + "ontology": MONDO_ONTOLOGY, + "queryFields": "obo_id", + "exact": "true", + "rows": 1, + "fieldList": "iri,label,obo_id", + } + try: + async with httpx.AsyncClient(timeout=OLS_TIMEOUT) as client: + response = await client.get(f"{OLS_BASE_URL}/search", params=params) + response.raise_for_status() + docs = response.json().get("response", {}).get("docs", []) + except httpx.HTTPError as exc: + save_to_logging_context(format_raised_exception_info_as_dict(exc)) + logger.error(msg="MONDO/OLS term lookup failed.", exc_info=exc, extra=logging_context()) + raise MondoServiceError("Disease ontology service temporarily unavailable") from exc + + if not docs: + return None + + doc = docs[0] + return MondoSuggestion(code=doc["obo_id"], label=doc["label"], iri=doc.get("iri", mondo_iri(code))) + + +async def find_or_create_mondo_term(db: Session, code: str) -> MondoTerm: + """Find an existing MONDO term by code, or create one after validating it against OLS. + + A new code is validated against OLS and its canonical label stored. An unknown code raises + ``ValidationError``; an OLS outage raises ``MondoServiceError`` (via :func:`fetch_mondo_term`) so a + term is never persisted without a validated label — a controlled vocabulary must stay validated. + """ + # The generic term is canonical and seeded — resolve it directly, never via OLS. + if code == MONDO_GENERIC_CODE: + return get_generic_disease_term(db) + + term = db.query(MondoTerm).filter(MondoTerm.system == MONDO_SYSTEM, MondoTerm.code == code).one_or_none() + if term is not None: + return term + + resolved = await fetch_mondo_term(code) + if resolved is None: + raise ValidationError(f"'{code}' is not a valid MONDO disease term.", custom_loc=["body", "disease"]) + + # OLS may normalize the submitted code (case, or an alias/obsolete id) to a different canonical code, + # so re-check existence by the resolved code before inserting: keying the lookup on the same value we + # insert under keeps this idempotent and avoids a UNIQUE(system, code) violation when the canonical + # row already exists under a different submitted spelling. + canonical_code = resolved["code"] + term = db.query(MondoTerm).filter(MondoTerm.system == MONDO_SYSTEM, MondoTerm.code == canonical_code).one_or_none() + if term is not None: + return term + + term = MondoTerm(code=canonical_code, system=MONDO_SYSTEM, label=resolved["label"]) # type: ignore[call-arg] + db.add(term) + db.flush() + + return term + + +async def resolve_disease_term(db: Session, code: Optional[str]) -> MondoTerm: + """Resolve a calibration's submitted disease code to a stored MONDO term. + + ``None`` (no disease specified) resolves to the generic "disease or disorder" term; any other code + is validated/created via :func:`find_or_create_mondo_term`. Shared by the score calibration create + and update paths so the write-time default lives in one place. + """ + if code is None: + return get_generic_disease_term(db) + + return await find_or_create_mondo_term(db, code) diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index 627d607b7..4e9a7b5e8 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -12,7 +12,7 @@ from mavedb.lib.acmg import find_or_create_acmg_classification from mavedb.lib.identifiers import find_or_create_publication_identifier -from mavedb.lib.mondo import resolve_disease_term +from mavedb.lib.mondo_ols import resolve_disease_term from mavedb.lib.types.score_calibrations import ( CalibrationControlSnapshot, CalibrationVariantLinkSnapshot, diff --git a/src/mavedb/routers/diseases.py b/src/mavedb/routers/diseases.py index 6f2d5e70c..f0f2f9daa 100644 --- a/src/mavedb/routers/diseases.py +++ b/src/mavedb/routers/diseases.py @@ -5,7 +5,8 @@ from mavedb.lib.logging.context import save_to_logging_context from mavedb.lib.logging.logged_route import LoggedRoute -from mavedb.lib.mondo import DEFAULT_SEARCH_LIMIT, mondo_suggestion_to_mappable_concept, search_mondo +from mavedb.lib.mondo import mondo_suggestion_to_mappable_concept +from mavedb.lib.mondo_ols import DEFAULT_SEARCH_LIMIT, search_mondo from mavedb.routers.shared import GATEWAY_ERROR_RESPONSES, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX TAG_NAME = "Diseases" diff --git a/tests/lib/mondo/network/test_mondo.py b/tests/lib/mondo/network/test_mondo.py index 0daaf2866..8e056927f 100644 --- a/tests/lib/mondo/network/test_mondo.py +++ b/tests/lib/mondo/network/test_mondo.py @@ -9,10 +9,11 @@ import pytest -# starlette is required for logging context functionality pulled in by mavedb.lib.mondo. +# starlette is required for logging context functionality pulled in by mavedb.lib.mondo_ols. pytest.importorskip("starlette") -from mavedb.lib.mondo import MONDO_GENERIC_CODE, fetch_mondo_term, search_mondo +from mavedb.lib.mondo import MONDO_GENERIC_CODE +from mavedb.lib.mondo_ols import fetch_mondo_term, search_mondo # Brugada syndrome: a stable, long-established MONDO disease term used as a known-good fixture. BRUGADA_CODE = "MONDO:0015263" diff --git a/tests/lib/mondo/test_mondo.py b/tests/lib/mondo/test_mondo.py index 10d2a22f9..409fe8102 100644 --- a/tests/lib/mondo/test_mondo.py +++ b/tests/lib/mondo/test_mondo.py @@ -1,4 +1,5 @@ -"""Tests for mavedb.lib.mondo: MONDO term resolution, search, and MappableConcept serialization.""" +"""Tests for mavedb.lib.mondo and mavedb.lib.mondo_ols: MONDO term resolution, search, and +MappableConcept serialization.""" # ruff: noqa: E402 @@ -13,15 +14,13 @@ MONDO_GENERIC_CODE, MONDO_GENERIC_LABEL, MONDO_SYSTEM, - fetch_mondo_term, - find_or_create_mondo_term, generic_disease_mappable_concept, get_generic_disease_term, mondo_iri, mondo_suggestion_to_mappable_concept, mondo_term_to_mappable_concept, - search_mondo, ) +from mavedb.lib.mondo_ols import fetch_mondo_term, find_or_create_mondo_term, search_mondo from mavedb.lib.validation.exceptions import ValidationError from mavedb.models.mondo_term import MondoTerm @@ -149,7 +148,7 @@ async def test_creates_from_ols_and_is_idempotent(self, session, monkeypatch): async def fake_fetch(code): return {"code": code, "label": "Brugada syndrome", "iri": mondo_iri(code)} - monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + monkeypatch.setattr("mavedb.lib.mondo_ols.fetch_mondo_term", fake_fetch) term = await find_or_create_mondo_term(session, "MONDO:0015263") assert term.code == "MONDO:0015263" @@ -167,7 +166,7 @@ async def test_non_canonical_code_reuses_existing_canonical_row(self, session, m async def fake_fetch(code): return {"code": "MONDO:0015263", "label": "Brugada syndrome", "iri": mondo_iri("MONDO:0015263")} - monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + monkeypatch.setattr("mavedb.lib.mondo_ols.fetch_mondo_term", fake_fetch) canonical = await find_or_create_mondo_term(session, "MONDO:0015263") # A differently-spelled submission (here lowercase) resolves to the same canonical term. @@ -181,7 +180,7 @@ async def test_generic_code_resolves_without_ols(self, session, monkeypatch): async def fail_fetch(code): raise AssertionError("OLS should not be consulted for the generic term") - monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fail_fetch) + monkeypatch.setattr("mavedb.lib.mondo_ols.fetch_mondo_term", fail_fetch) term = await find_or_create_mondo_term(session, MONDO_GENERIC_CODE) assert term.code == MONDO_GENERIC_CODE assert term.label == MONDO_GENERIC_LABEL @@ -190,7 +189,7 @@ async def test_rejects_unknown_code(self, session, monkeypatch): async def fake_fetch(code): return None - monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + monkeypatch.setattr("mavedb.lib.mondo_ols.fetch_mondo_term", fake_fetch) with pytest.raises(ValidationError): await find_or_create_mondo_term(session, "MONDO:9999999") @@ -199,7 +198,7 @@ async def test_ols_outage_propagates_service_error(self, session, monkeypatch): async def fake_fetch(code): raise MondoServiceError("ols down") - monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + monkeypatch.setattr("mavedb.lib.mondo_ols.fetch_mondo_term", fake_fetch) with pytest.raises(MondoServiceError): await find_or_create_mondo_term(session, "MONDO:0015263") diff --git a/tests/routers/test_score_calibrations.py b/tests/routers/test_score_calibrations.py index e5bb5c486..62a7660db 100644 --- a/tests/routers/test_score_calibrations.py +++ b/tests/routers/test_score_calibrations.py @@ -195,7 +195,7 @@ async def fake_fetch(code): "iri": f"https://purl.obolibrary.org/obo/{code.replace(':', '_')}", } - monkeypatch.setattr("mavedb.lib.mondo.fetch_mondo_term", fake_fetch) + monkeypatch.setattr("mavedb.lib.mondo_ols.fetch_mondo_term", fake_fetch) experiment = create_experiment(client) score_set = create_seq_score_set_with_mapped_variants(