diff --git a/docker/orthanc/Dockerfile b/docker/orthanc/Dockerfile index e54f00b14..0ade50478 100644 --- a/docker/orthanc/Dockerfile +++ b/docker/orthanc/Dockerfile @@ -32,7 +32,7 @@ FROM pixl_orthanc_uv AS pixl_orthanc_with_spec # Do it in dead end build stage to discard this environment afterwards, # and because the spec is only needed in orthanc-anon. RUN uv venv -RUN uv pip install dicom-validator==0.7.3 +RUN uv pip install dicom-validator==0.9.0 COPY ./orthanc/orthanc-anon/plugin/download_dicom_spec.py /etc/orthanc/download_dicom_spec.py RUN --mount=type=cache,target=/root/.cache,id=dlspec \ python3 /etc/orthanc/download_dicom_spec.py diff --git a/orthanc/orthanc-anon/plugin/download_dicom_spec.py b/orthanc/orthanc-anon/plugin/download_dicom_spec.py index 04e4f3f4d..99fcee6ba 100644 --- a/orthanc/orthanc-anon/plugin/download_dicom_spec.py +++ b/orthanc/orthanc-anon/plugin/download_dicom_spec.py @@ -20,6 +20,4 @@ edition = "2024e" download_path = str(Path.home() / "dicom-validator") edition_reader = EditionReader(download_path) -destination = edition_reader.get_revision(edition, recreate_json=False) -json_path = Path(destination, "json") -EditionReader.load_dicom_info(json_path) +edition_reader.get_edition_path(edition) diff --git a/pixl_dcmd/pyproject.toml b/pixl_dcmd/pyproject.toml index 17f8cf8ef..f14a03855 100644 --- a/pixl_dcmd/pyproject.toml +++ b/pixl_dcmd/pyproject.toml @@ -11,7 +11,7 @@ dependencies = [ "arrow==1.4.0", "deid==0.4.12", "dicom-anonymizer==2.0.0", - "dicom-validator==0.7.3", + "dicom-validator==0.9.0", "logger==1.4", "pydicom==3.0.2", "pydicom-data", diff --git a/pixl_dcmd/src/pixl_dcmd/dicom_helpers.py b/pixl_dcmd/src/pixl_dcmd/dicom_helpers.py index e7bea3ed7..b9a61216e 100644 --- a/pixl_dcmd/src/pixl_dcmd/dicom_helpers.py +++ b/pixl_dcmd/src/pixl_dcmd/dicom_helpers.py @@ -19,7 +19,6 @@ import typing from contextlib import contextmanager, redirect_stdout from dataclasses import dataclass -import logging from io import StringIO from pathlib import Path from typing import Generator @@ -27,7 +26,19 @@ from loguru import logger from dicom_validator.spec_reader.edition_reader import EditionReader +from dicom_validator.tag_tools import tag_name_from_id +from dicom_validator.validator.error_handler import ( + NullValidationResultHandler, + ValidationResultFormatter, +) from dicom_validator.validator.iod_validator import IODValidator +from dicom_validator.validator.validation_result import ( + DicomTag, + ModuleErrors, + Status, + TagError, + ValidationResult, +) from pydicom import Dataset from core.exceptions import PixlSkipInstanceError @@ -44,64 +55,71 @@ def __init__(self, edition: str = "current"): standard_path = str(Path.home() / "dicom-validator") with _redirect_stdout_to_debug(logger): edition_reader = EditionReader(standard_path) - destination = edition_reader.get_revision(self.edition, False) - json_path = Path(destination, "json") - self.dicom_info = EditionReader.load_dicom_info(json_path) + self.dicom_info = edition_reader.dicom_info_for_edition(self.edition) - def validate_original(self, dataset: Dataset) -> dict | None: + # Used to format errors introduced by de-identification + self.formatter = ValidationResultFormatter(self.dicom_info.dictionary) + + def _validate(self, dataset: Dataset) -> ValidationResult: + """Validate a pydicom Dataset using dicom-validator.""" + return IODValidator( + dataset, + self.dicom_info, + error_handler=NullValidationResultHandler(), + ).validate() + + def _describe_error(self, tag: DicomTag, error: TagError) -> str: + tag_name = tag_name_from_id(tag.tag, self.dicom_info.dictionary) + return f"Tag {tag_name}{self.formatter.error_message(error)}" + + def validate_original(self, dataset: Dataset) -> ModuleErrors | None: """Check pre-existing validation errors in a dataset. Returns: - validation_errors: a dictionary of validation errors, or None - if dicom-validator raised a RuntimeError during validation. + module_errors: pre-existing validation errors, keyed by module + name then DICOM tag, or None if dicom-validator could not + validate the dataset at all (e.g. missing or unrecognised + SOP Class UID). """ - validator = IODValidator( - dataset, - self.dicom_info, - log_level=logging.ERROR, - ) - try: - errors: dict | None = validator.validate() - except RuntimeError as error: + result = self._validate(dataset) + if result.status not in (Status.Passed, Status.Failed): logger.warning( "Cannot check for pre-existing validation errors. " - "dicom-validator raised a RuntimeError during validation: {}", - error, + "dicom-validator returned status: {}", + result.status, ) - errors = None + return None - return errors + return result.module_errors def validate_anonymised( - self, dataset: Dataset, original_errors: dict | None - ) -> dict: + self, dataset: Dataset, original_errors: ModuleErrors | None + ) -> dict[str, set[str]]: """Check validation errors introduced during de-identification. Args: - original_errors: dict of errors returned by validate_original for - dataset before anonymisation, or None if the dataset hasn't - been validated for pre-existing errors. + original_errors: module_errors returned by validate_original for + the dataset before anonymisation, or None if the dataset + hasn't been validated for pre-existing errors. Returns: - new_errors: dict of errors introduced by anonymisation. If - original_errors is None, all errors found after - anonymisation are returned, as it's not possible to tell - which of them pre-existed. + new_errors: human-readable errors introduced by anonymisation, + keyed by module name. If original_errors is None, all errors + found after anonymisation are returned, as it's not possible + to tell which of them pre-existed. Raises: - PixlSkipInstanceError: If dicom-validator raises a RuntimeError - during validation. + PixlSkipInstanceError: If dicom-validator could not validate the + anonymised dataset at all (e.g. missing SOP Class UID). """ - validator = IODValidator( - dataset, - self.dicom_info, - log_level=logging.ERROR, - ) - try: - anon_errors: dict = validator.validate() - except RuntimeError as error: - msg = f"dicom-validator raised a RuntimeError when validating the anonymised dataset: {error}" - raise PixlSkipInstanceError(msg) from error + result = self._validate(dataset) + if result.status not in (Status.Passed, Status.Failed): + msg = ( + "Cannot validate the anonymised dataset. " + f"dicom-validator returned status: {result.status}" + ) + raise PixlSkipInstanceError(msg) + anon_errors = result.module_errors if original_errors is None: logger.warning( @@ -110,18 +128,26 @@ def validate_anonymised( "Errors found after anonymisation: {}", anon_errors, ) - return anon_errors - - diff_errors: dict = {} - for key in anon_errors: - if key in original_errors: - # Keep only errors introduced after the anonymisation - # The keys of the dictionary containt the actual errors - diff = set(anon_errors[key]) - set(original_errors[key]) - if diff: - diff_errors[key] = diff + original_errors = ModuleErrors() + + diff_errors: dict[str, set[str]] = {} + for module_name, anon_tag_errors in anon_errors.items(): + if module_name in original_errors: + # keep tags with new errors or errors that have changed + original_tag_errors = original_errors[module_name] + new_tag_errors = { + tag: error + for tag, error in anon_tag_errors.items() + if (tag, error) not in original_tag_errors.items() + } else: - diff_errors[key] = anon_errors[key] + new_tag_errors = anon_tag_errors + + if new_tag_errors: + diff_errors[module_name] = { + self._describe_error(tag, error) + for tag, error in new_tag_errors.items() + } return diff_errors diff --git a/pixl_dcmd/tests/test_dicom_validator.py b/pixl_dcmd/tests/test_dicom_validator.py index 112f2e483..121ad9471 100644 --- a/pixl_dcmd/tests/test_dicom_validator.py +++ b/pixl_dcmd/tests/test_dicom_validator.py @@ -16,6 +16,7 @@ import pytest from core.exceptions import PixlSkipInstanceError +from dicom_validator.validator.validation_result import ErrorCode from pixl_dcmd.dicom_helpers import DicomValidator from pixl_dcmd.main import anonymise_dicom from pydicom import Dataset @@ -85,10 +86,7 @@ def test_validation_fails_after_invalid_tag_modification( assert len(validation_result) == 1 assert "Patient" in validation_result.keys() assert len(validation_result["Patient"]) == 1 - assert ( - "Tag (0010,0010) (Patient's Name) is missing" - in validation_result["Patient"].keys() - ) + assert "Tag (0010,0010) (Patient's Name) is missing" in validation_result["Patient"] @pytest.fixture() @@ -103,21 +101,27 @@ def dicom_with_malformed_sequence_tag(vanilla_dicom_image_DX: Dataset) -> Datase return vanilla_dicom_image_DX -def test_validate_original_survives_runtime_error( +def test_validate_original_reports_malformed_sequence( dicom_with_malformed_sequence_tag: Dataset, ) -> None: """ - GIVEN a DICOM dataset that makes dicom-validator raise a RuntimeError + GIVEN a DICOM dataset with a malformed sequence tag WHEN the original dataset is validated - THEN None is returned rather than a dictionary of pre-existing errors + THEN an InvalidSequence error is returned """ validator = DicomValidator() original_errors = validator.validate_original(dicom_with_malformed_sequence_tag) - assert original_errors is None + + error_codes = { + error.code + for tag_errors in original_errors.values() + for error in tag_errors.values() + } + assert ErrorCode.InvalidSequence in error_codes def test_validate_anonymised_returns_all_errors_when_original_unknown( - dicom_with_malformed_sequence_tag: Dataset, + vanilla_dicom_image_DX: Dataset, ) -> None: """ GIVEN an anonymised dataset that has not been validated for pre-existing errors @@ -125,32 +129,41 @@ def test_validate_anonymised_returns_all_errors_when_original_unknown( THEN all errors found are returned """ validator = DicomValidator() - original_errors = validator.validate_original(dicom_with_malformed_sequence_tag) - assert original_errors is None - - # delete problematic element - del dicom_with_malformed_sequence_tag.DerivationCodeSequence - # delete a required element to introduce an error - del dicom_with_malformed_sequence_tag.PatientName + del vanilla_dicom_image_DX.PatientName - validation_result = validator.validate_anonymised( - dicom_with_malformed_sequence_tag, original_errors - ) + validation_result = validator.validate_anonymised(vanilla_dicom_image_DX, None) assert "Patient" in validation_result.keys() -def test_validate_anonymised_raises_skip_instance_error_on_runtime_error( - dicom_with_malformed_sequence_tag: Dataset, +@pytest.fixture() +def dicom_missing_sop_class_uid(vanilla_dicom_image_DX: Dataset) -> Dataset: + """A DICOM dataset with no SOP Class UID, which dicom-validator cannot validate.""" + del vanilla_dicom_image_DX.SOPClassUID + return vanilla_dicom_image_DX + + +def test_validate_original_returns_none_when_dataset_cannot_be_validated( + dicom_missing_sop_class_uid: Dataset, +) -> None: + """ + GIVEN a DICOM dataset that dicom-validator cannot validate at all + WHEN the original dataset is validated + THEN None is returned + """ + validator = DicomValidator() + original_errors = validator.validate_original(dicom_missing_sop_class_uid) + assert original_errors is None + + +def test_validate_anonymised_raises_skip_instance_error_when_dataset_cannot_be_validated( + dicom_missing_sop_class_uid: Dataset, ) -> None: """ - GIVEN an anonymised dataset that causes dicom-validator to raise a RuntimeError + GIVEN an anonymised DICOM dataset that dicom-validator cannot validate at all WHEN the anonymised dataset is validated THEN a PixlSkipInstanceError is raised """ validator = DicomValidator() - original_errors = validator.validate_original(dicom_with_malformed_sequence_tag) with pytest.raises(PixlSkipInstanceError): - validator.validate_anonymised( - dicom_with_malformed_sequence_tag, original_errors - ) + validator.validate_anonymised(dicom_missing_sop_class_uid, None) diff --git a/uv.lock b/uv.lock index cdaba1933..a57d8841b 100644 --- a/uv.lock +++ b/uv.lock @@ -735,16 +735,16 @@ wheels = [ [[package]] name = "dicom-validator" -version = "0.7.3" +version = "0.9.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "lxml" }, { name = "pydicom" }, { name = "pyparsing" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/a8/db/922c5d3c662aff47e1ab1a64fe9203b5ea813ea493b7b216c03b9bfb4676/dicom_validator-0.7.3.tar.gz", hash = "sha256:4ee8376688fb94ca33c48af34bc5a93dd3e3743abefdc1d16b70eba41c4aa542", size = 63526, upload-time = "2025-10-13T17:32:49.564Z" } +sdist = { url = "https://files.pythonhosted.org/packages/59/bd/467a5f6a2758bf87a78568a632207fc8788962b31f09300a9c329d5bd4eb/dicom_validator-0.9.0.tar.gz", hash = "sha256:1c9dc12da7646b8c4698253a80abb0151ce50d2b49bc483a3000c9a0acf835c8", size = 78818, upload-time = "2026-09-09T17:36:47.012Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/6e/8d/b3401e93c7301e96315d4d816f5643815f5e6797a8c511e56e91e237b546/dicom_validator-0.7.3-py3-none-any.whl", hash = "sha256:2bffcdf2ae774ef18c521774561987098bfe8c157fdd1fa5bf04f552a195499f", size = 72200, upload-time = "2025-10-13T17:32:48.204Z" }, + { url = "https://files.pythonhosted.org/packages/d1/c4/790dd16e8fec78a7c6b7bf541414a29ba9ad526b4ee091b71a5082aa6434/dicom_validator-0.9.0-py3-none-any.whl", hash = "sha256:a89db814114a339c4ebe07e4c4e848b118078cc016169147fac051620d3cdae1", size = 90871, upload-time = "2026-09-09T17:36:45.46Z" }, ] [[package]] @@ -2150,7 +2150,7 @@ requires-dist = [ { name = "core", extras = ["test"], marker = "extra == 'test'", editable = "pixl_core" }, { name = "deid", specifier = "==0.4.12" }, { name = "dicom-anonymizer", specifier = "==2.0.0" }, - { name = "dicom-validator", specifier = "==0.7.3" }, + { name = "dicom-validator", specifier = "==0.9.0" }, { name = "logger", specifier = "==1.4" }, { name = "pydicom", specifier = "==3.0.2" }, { name = "pydicom-data" },