Pix 49 log skip error in db

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,18 +96,19 @@ 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"],
         how="left",
         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)
     )

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

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).

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):

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."""

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:

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,
     *,

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

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=[])

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 ###