diff --git a/cli/src/pixl_cli/_database.py b/cli/src/pixl_cli/_database.py index a430b398f..0a2fafc48 100644 --- a/cli/src/pixl_cli/_database.py +++ b/cli/src/pixl_cli/_database.py @@ -39,10 +39,11 @@ def filter_exported_or_add_to_db(messages_df: pd.DataFrame) -> pd.DataFrame: """ - Filter exported images for multiple projects, and adds missing extract and images to database. + Filter exported or previously skipped images for multiple projects, and adds missing + extract and images to database. :param messages: Initial messages to filter if they already exist - :return DataFrame of messages that have not been exported + :return DataFrame of messages that have not been exported or previously skipped """ PixlSession = sessionmaker(engine) with PixlSession() as pixl_session, pixl_session.begin(): @@ -70,7 +71,7 @@ def _filter_exported_or_add_to_db_for_project( if extract: db_images_df = all_images_for_project(project_slug) missing_images_df = _filter_existing_images(messages_df, db_images_df) - messages_df = _filter_exported_messages(messages_df, db_images_df) + messages_df = _filter_exported_or_skipped_messages(messages_df, db_images_df) else: # We need to add the extract to the database and retrive it again so # we can access extract.extract_id (needed by session.bulk_save_objects(images)) @@ -95,10 +96,11 @@ def _filter_existing_images( return messages_df[keep_indices] -def _filter_exported_messages( +def _filter_exported_or_skipped_messages( messages_df: pd.DataFrame, images_df: pd.DataFrame, ) -> pd.DataFrame: + """Exclude messages already exported, or that previously failed anonymisation.""" merged = messages_df.merge( images_df, on=["accession_number", "mrn", "study_uid"], @@ -106,7 +108,7 @@ def _filter_exported_messages( validate="one_to_one", suffixes=(None, None), ) - keep_indices = merged["exported_at"].isna().to_numpy() + keep_indices = (merged["exported_at"].isna() & merged["skip_reasons"].isna()).to_numpy() return merged[keep_indices][messages_df.columns] @@ -131,7 +133,13 @@ def all_images_for_project(project_slug: str) -> pd.DataFrame: PixlSession = sessionmaker(engine) query = ( - select(Image.accession_number, Image.study_uid, Image.mrn, Image.exported_at) + select( + Image.accession_number, + Image.study_uid, + Image.mrn, + Image.exported_at, + Image.skip_reasons, + ) .join(Extract) .where(Extract.slug == project_slug) ) diff --git a/cli/tests/test_database_cli_interaction.py b/cli/tests/test_database_cli_interaction.py index efd4f147b..d6a6f8ca5 100644 --- a/cli/tests/test_database_cli_interaction.py +++ b/cli/tests/test_database_cli_interaction.py @@ -74,3 +74,42 @@ def test_processed_images_for_project(rows_in_session): processed = exported_images_for_project("i-am-a-project") assert len(processed) == 1 assert processed[0].accession_number == "123" + + +def test_reimport_of_previously_skipped_image(example_messages_df, rows_in_session): + """ + GIVEN an image that previously had all instances skipped during anonymisation and has + skip_reasons recorded against it, but has not been exported + WHEN the same messages are re-imported (filtered again for the same project) + THEN no duplicate row should be added for that image (row-level filtering) + and the image should not be returned for reprocessing, as it previously failed + anonymisation (export-status filtering), with its recorded skip_reasons left untouched + """ + extract = rows_in_session.query(Extract).one() + previously_skipped_image = ( + rows_in_session.query(Image) + .filter(Image.extract == extract, Image.accession_number == "234") + .one() + ) + skip_reasons = {"DICOM instance discarded as series has too few instances": 3} + previously_skipped_image.skip_reasons = skip_reasons + rows_in_session.commit() + + output = filter_exported_or_add_to_db(example_messages_df) + + # Row-level: re-importing must not create a duplicate row for the existing image + images = rows_in_session.query(Image).filter(Image.extract == extract).all() + assert len(images) == len(example_messages_df) + + # Filtering-level: previously skipped images are not queued again, + # nor are already-exported images + assert "234" not in output.accession_number.to_numpy() + assert "123" not in output.accession_number.to_numpy() + + # The recorded skip reasons must survive the re-import untouched + reloaded_image = ( + rows_in_session.query(Image) + .filter(Image.extract == extract, Image.accession_number == "234") + .one() + ) + assert reloaded_image.skip_reasons == skip_reasons diff --git a/docs/services/pixl_database.md b/docs/services/pixl_database.md index 24058d9b5..a67720cc9 100644 --- a/docs/services/pixl_database.md +++ b/docs/services/pixl_database.md @@ -5,4 +5,6 @@ PIXL uses a [postgres database](../../postgres/README.md) to - Add pseudo identifiers along with the originals for DICOM images (in `pixl_dcmd`) - Keep track of the export status of imaging (in `core.uploader`) studies and the projects they are used in -Note that the pipeline will not process any studies for a project that have already been exported. +Note that the pipeline will not process any studies for a project that have already been exported, +or that previously failed anonymisation (recorded in the `skip_reasons` column of the `image` +table). diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 9050fa56b..686fdd5ef 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -53,6 +53,7 @@ anonymise_dicom_and_update_db, get_series_to_skip, parse_validation_results, + update_db_with_skip_failure_reason, write_dataset_to_bytes, ) from pydicom import dcmread @@ -492,6 +493,11 @@ def _anonymise_study_instances( if not anonymised_instances_bytes: message = f"All instances have been skipped for study: {dict(skipped_instance_counts)}" + update_db_with_skip_failure_reason( + project_name=project_name, + study_info=study_info, + skip_reasons=dict(skipped_instance_counts), + ) raise PixlDiscardError(message) with logger.contextualize(pseudo_study_uid=anonymised_study_uid): diff --git a/pixl_core/src/core/db/models.py b/pixl_core/src/core/db/models.py index 76b131979..c40748712 100644 --- a/pixl_core/src/core/db/models.py +++ b/pixl_core/src/core/db/models.py @@ -17,9 +17,10 @@ from __future__ import annotations from sqlalchemy import MetaData +from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column, relationship from sqlalchemy.schema import ForeignKey -from sqlalchemy.types import Date, DateTime +from sqlalchemy.types import JSON, Date, DateTime class Base(DeclarativeBase): @@ -56,6 +57,9 @@ class Image(Base): extract: Mapped[Extract] = relationship() extract_id: Mapped[int] = mapped_column(ForeignKey("extract.extract_id")) pseudo_patient_id: Mapped[str | None] + skip_reasons: Mapped[dict[str, int] | None] = mapped_column( + JSONB().with_variant(JSON(), "sqlite"), nullable=True + ) def __repr__(self) -> str: """Nice representation for printing.""" diff --git a/pixl_dcmd/src/pixl_dcmd/_database.py b/pixl_dcmd/src/pixl_dcmd/_database.py index 4563388d0..fcec66d1f 100644 --- a/pixl_dcmd/src/pixl_dcmd/_database.py +++ b/pixl_dcmd/src/pixl_dcmd/_database.py @@ -39,6 +39,20 @@ engine = create_engine(url) +def record_skip_reasons_for_study( + project_slug: str, study_info: StudyInfo, skip_reasons: dict[str, int] +) -> None: + """ + Record the reasons (and instance counts) a study's instances were skipped for, + against the existing image record for that study. + """ + PixlSession = sessionmaker(engine) + with PixlSession() as pixl_session, pixl_session.begin(): + existing_image = get_unexported_image(project_slug, study_info, pixl_session) + existing_image.skip_reasons = skip_reasons + pixl_session.add(existing_image) + + def get_uniq_pseudo_study_uid_and_update_db( project_slug: str, original_study_info: StudyInfo ) -> UID: diff --git a/pixl_dcmd/src/pixl_dcmd/main.py b/pixl_dcmd/src/pixl_dcmd/main.py index 8fbbfad31..b33d40b2f 100644 --- a/pixl_dcmd/src/pixl_dcmd/main.py +++ b/pixl_dcmd/src/pixl_dcmd/main.py @@ -18,33 +18,34 @@ from io import BytesIO from zipfile import ZipFile +import pydicom import requests from core.exceptions import PixlSkipInstanceError from core.project_config import ( - load_tag_operations, load_image_operations, + load_tag_operations, ) +from core.project_config.pixl_config_model import PixlConfig from decouple import config +from deid.config import DeidRecipe +from deid.dicom.pixels import clean_pixel_data, has_burned_pixels from dicomanonymizer.simpledicomanonymizer import ( ActionsMapNameFunctions, anonymize_dataset, ) from loguru import logger from pydicom import DataElement, Dataset, dcmread, dcmwrite -import pydicom -from core.project_config.pixl_config_model import PixlConfig from pixl_dcmd._database import ( - get_uniq_pseudo_study_uid_and_update_db, get_pseudo_patient_id_and_update_db, + get_uniq_pseudo_study_uid_and_update_db, + record_skip_reasons_for_study, ) +from pixl_dcmd._tag_schemes import _scheme_list_to_dict, merge_tag_schemes from pixl_dcmd.dicom_helpers import ( DicomValidator, get_study_info, ) -from pixl_dcmd._tag_schemes import _scheme_list_to_dict, merge_tag_schemes -from deid.config import DeidRecipe -from deid.dicom.pixels import clean_pixel_data, has_burned_pixels if typing.TYPE_CHECKING: from pixl_dcmd.dicom_helpers import StudyInfo @@ -149,6 +150,23 @@ def anonymise_dicom_and_update_db( return validation_errors +def update_db_with_skip_failure_reason( + project_name: str, + study_info: StudyInfo, + skip_reasons: dict[str, int], +) -> None: + """ + Record a study de-identification failure in the database. + + Args: + project_name: The name of the project for which the de-identification failure occurred. + study_info: Identifiable study info, used to look up the existing image record. + skip_reasons: Mapping of skip-reason message to the number of instances skipped for it. + + """ + record_skip_reasons_for_study(project_name, study_info, skip_reasons) + + def anonymise_and_validate_dicom( dataset: Dataset, *, diff --git a/pixl_dcmd/tests/test_database.py b/pixl_dcmd/tests/test_database.py index a2db83aae..dae0212e7 100644 --- a/pixl_dcmd/tests/test_database.py +++ b/pixl_dcmd/tests/test_database.py @@ -25,6 +25,7 @@ get_unexported_image, get_uniq_pseudo_study_uid_and_update_db, get_pseudo_patient_id_and_update_db, + record_skip_reasons_for_study, ) from pixl_dcmd.dicom_helpers import StudyInfo from sqlalchemy.orm import Session @@ -170,6 +171,24 @@ def test_get_pseudo_patient_id_and_update_db(rows_for_database_testing, db_sessi assert result.pseudo_patient_id == PSEUDO_IDS_STUDY.db.pseudo_patient_id +def test_record_skip_reasons_for_study(rows_for_database_testing, db_session): + """ + GIVEN an existing, unexported image + WHEN record_skip_reasons_for_study is called with a mapping of skip reasons to counts + THEN the image record should be updated with those skip reasons. + """ + skip_reasons = {"Instance discarded due to its manufacturer": 3} + + record_skip_reasons_for_study( + TEST_PROJECT_SLUG, UNPROCESSED_STUDY.input, skip_reasons + ) + + result = get_unexported_image( + TEST_PROJECT_SLUG, UNPROCESSED_STUDY.input, db_session + ) + assert result.skip_reasons == skip_reasons + + def test_get_unexported_image_fallback(rows_for_database_testing, db_session): """ GIVEN a database entry with a non-exported image diff --git a/pixl_dcmd/tests/test_main.py b/pixl_dcmd/tests/test_main.py index 47b425975..405fbfd39 100644 --- a/pixl_dcmd/tests/test_main.py +++ b/pixl_dcmd/tests/test_main.py @@ -41,12 +41,14 @@ from pixl_dcmd.main import ( anonymise_dicom_and_update_db, _anonymise_dicom_from_scheme, + _clean_dicom_image_pixels, anonymise_and_validate_dicom, anonymise_dicom, get_series_to_skip, _enforce_allowlist, _should_exclude_series, _should_exclude_manufacturer, + update_db_with_skip_failure_reason, ) from pytest_pixl.dicom import generate_dicom_dataset from pytest_pixl.helpers import run_subprocess @@ -263,6 +265,51 @@ def test_anonymise_with_clean_dicom_image_pixels( assert np.all(compare_clean_region_with_zeros) +def test_clean_dicom_image_pixels_encapsulates_compressed_pixel_data( + monkeypatch, ultrasound_project_config +): + """ + GIVEN a DICOM dataset with a compressed transfer syntax + WHEN pixel cleaning is applied + THEN the cleaned pixel data should be written back in encapsulated (fragmented) form + rather than as a raw, native pixel data byte string + """ + dataset = generate_dicom_dataset(Modality="US") + dataset.file_meta.TransferSyntaxUID = pydicom.uid.JPEGBaseline8Bit + + cleaned_pixels = np.zeros((2, 2), dtype=np.uint8) + monkeypatch.setattr( + "pixl_dcmd.main.has_burned_pixels", lambda *args, **kwargs: object() + ) + monkeypatch.setattr( + "pixl_dcmd.main.clean_pixel_data", lambda *args, **kwargs: cleaned_pixels + ) + + _clean_dicom_image_pixels(dataset, ultrasound_project_config) + + assert dataset.PixelData == pydicom.encaps.encapsulate([cleaned_pixels.tobytes()]) + + +def test_update_db_with_skip_failure_reason(monkeypatch): + """ + GIVEN a study that failed de-identification with some skip reasons + WHEN update_db_with_skip_failure_reason is called + THEN the skip reasons should be recorded against the study in the database + """ + recorded_calls = [] + monkeypatch.setattr( + "pixl_dcmd.main.record_skip_reasons_for_study", + lambda *args: recorded_calls.append(args), + ) + + study_info = get_study_info(generate_dicom_dataset()) + skip_reasons = {"Instance discarded due to its manufacturer": 2} + + update_db_with_skip_failure_reason("test-project", study_info, skip_reasons) + + assert recorded_calls == [("test-project", study_info, skip_reasons)] + + @pytest.fixture def dummy_manufacturer() -> Manufacturer: return Manufacturer(regex="^company", exclude_series_numbers=[]) diff --git a/pixl_imaging/alembic/versions/4226124af0db_add_skip_reasons_column_to_image_table.py b/pixl_imaging/alembic/versions/4226124af0db_add_skip_reasons_column_to_image_table.py new file mode 100644 index 000000000..9a6e9dac7 --- /dev/null +++ b/pixl_imaging/alembic/versions/4226124af0db_add_skip_reasons_column_to_image_table.py @@ -0,0 +1,55 @@ +# Copyright (c) University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Add skip reasons column to image table + +Revision ID: 4226124af0db +Revises: d947cc715eb1 +Create Date: 2026-08-12 15:15:01.560015 + +""" + +from collections.abc import Sequence +from typing import Union + +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "4226124af0db" +down_revision: Union[str, None] = "d947cc715eb1" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.add_column( + "image", + sa.Column( + "skip_reasons", + postgresql.JSONB(astext_type=sa.Text()).with_variant(sa.JSON(), "sqlite"), + nullable=True, + ), + schema="pixl_pipeline", + ) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("image", "skip_reasons", schema="pixl_pipeline") + # ### end Alembic commands ###