Repository: DIAGNijmegen/rse-panimg (language: python)

Issue:
TIFF importer mixes DZI creation from source
The TIFF importer mixes DZI and TIFF imports in the same round. They should be split, generating the DZIs from the generated TIFFs in the target directory. Doing this import should be optional.


Discussion / hints:

Fix patch:
diff --git a/README.md b/README.md
index 0039e92..59f253e 100644
--- a/README.md
+++ b/README.md
@@ -40,24 +40,32 @@ result = convert(
 
 ### Supported Formats
 
-| Input                               | Output           | Strategy   | Notes                      |
-| ----------------------------------- | ---------------- | ---------- | -------------------------- |
-| `.mha`                              | `.mha`           | `metaio`   |                            |
-| `.mhd` with `.raw` or `.zraw`       | `.mha`           | `metaio`   |                            |
-| `.dcm`                              | `.mha`           | `dicom`    | <sup>[1](#footnote1)</sup> |
-| `.nii`                              | `.mha`           | `nifti`    |                            |
-| `.nii.gz`                           | `.mha`           | `nifti`    |                            |
-| `.png`                              | `.mha`           | `fallback` | <sup>[2](#footnote2)</sup> |
-| `.jpeg`                             | `.mha`           | `fallback` | <sup>[2](#footnote2)</sup> |
-| `.tiff`                             | `.tiff` & `.dzi` | `tiff`     | <sup>[3](#footnote3)</sup> |
-| `.svs` (Aperio)                     | `.tiff` & `.dzi` | `tiff`     | <sup>[3](#footnote3)</sup> |
-| `.vms`, `.vmu`, `.ndpi` (Hamamatsu) | `.tiff` & `.dzi` | `tiff`     | <sup>[3](#footnote3)</sup> |
-| `.scn` (Leica)                      | `.tiff` & `.dzi` | `tiff`     | <sup>[3](#footnote3)</sup> |
-| `.mrxs` (MIRAX)                     | `.tiff` & `.dzi` | `tiff`     | <sup>[3](#footnote3)</sup> |
-| `.biff` (Ventana)                   | `.tiff` & `.dzi` | `tiff`     | <sup>[3](#footnote3)</sup> |
+| Input                               | Output  | Strategy   | Notes                      |
+| ----------------------------------- | --------| ---------- | -------------------------- |
+| `.mha`                              | `.mha`  | `metaio`   |                            |
+| `.mhd` with `.raw` or `.zraw`       | `.mha`  | `metaio`   |                            |
+| `.dcm`                              | `.mha`  | `dicom`    | <sup>[1](#footnote1)</sup> |
+| `.nii`                              | `.mha`  | `nifti`    |                            |
+| `.nii.gz`                           | `.mha`  | `nifti`    |                            |
+| `.png`                              | `.mha`  | `fallback` | <sup>[2](#footnote2)</sup> |
+| `.jpeg`                             | `.mha`  | `fallback` | <sup>[2](#footnote2)</sup> |
+| `.tiff`                             | `.tiff` | `tiff`     |                            |
+| `.svs` (Aperio)                     | `.tiff` | `tiff`     |                            |
+| `.vms`, `.vmu`, `.ndpi` (Hamamatsu) | `.tiff` | `tiff`     |                            |
+| `.scn` (Leica)                      | `.tiff` | `tiff`     |                            |
+| `.mrxs` (MIRAX)                     | `.tiff` | `tiff`     |                            |
+| `.biff` (Ventana)                   | `.tiff` | `tiff`     |                            |
 
 <a name="footnote1">1</a>: Compressed DICOM requires `gdcm`
 
 <a name="footnote2">2</a>: 2D only, unitary dimensions
 
-<a name="footnote3">3</a>: DZI only created if possible
+#### Post Processors
+
+You can also define a set of post processors that will operate on each output file.
+We provide a `dzi_to_tiff` post processor that is enabled by default, which will produce a DZI file if it is able to.
+To customise the post processors that run you can do this with
+
+```python
+result = convert(..., post_processors=[...])
+```
diff --git a/panimg/image_builders/tiff.py b/panimg/image_builders/tiff.py
index ffde985..4e41fbd 100644
--- a/panimg/image_builders/tiff.py
+++ b/panimg/image_builders/tiff.py
@@ -5,7 +5,7 @@
 from dataclasses import dataclass, field
 from pathlib import Path
 from tempfile import TemporaryDirectory
-from typing import Callable, Dict, List, Optional, Set, Tuple
+from typing import Callable, Dict, List, Optional, Set
 from uuid import UUID, uuid4
 
 import openslide
@@ -18,10 +18,8 @@
     ImageType,
     PanImg,
     PanImgFile,
-    PanImgFolder,
     PanImgResult,
 )
-from panimg.settings import DZI_TILE_SIZE
 
 
 @dataclass
@@ -35,7 +33,6 @@ class GrandChallengeTiffFile:
     voxel_width_mm: float = 0
     voxel_height_mm: float = 0
     voxel_depth_mm: Optional[float] = None
-    source_files: List[Path] = field(default_factory=list)
     associated_files: List[Path] = field(default_factory=list)
 
     def validate(self) -> None:
@@ -197,16 +194,6 @@ def _get_color_space(*, color_space_string) -> Optional[ColorSpace]:
     return color_space
 
 
-def _create_image_file(*, path: Path, image: PanImg) -> PanImgFile:
-    return PanImgFile(
-        image_id=image.pk,
-        image_type=ImageType.DZI
-        if path.suffix.lower() == ".dzi"
-        else ImageType.TIFF,
-        file=path,
-    )
-
-
 def _load_with_tiff(
     *, gc_file: GrandChallengeTiffFile
 ) -> GrandChallengeTiffFile:
@@ -225,35 +212,6 @@ def _load_with_openslide(
     return gc_file
 
 
-def _new_image_files(
-    *, gc_file: GrandChallengeTiffFile, image: PanImg
-) -> Set[PanImgFile]:
-    new_image_files = {
-        _create_image_file(path=gc_file.path.absolute(), image=image,)
-    }
-
-    if gc_file.source_files:
-        for s in gc_file.source_files:
-            new_image_files.add(_create_image_file(path=s, image=image))
-
-    return new_image_files
-
-
-def _new_folder_uploads(
-    *, dzi_output: Optional[Path], image: PanImg,
-) -> Set[PanImgFolder]:
-    new_folder_upload = set()
-
-    if dzi_output:
-        dzi_folder_upload = PanImgFolder(
-            folder=dzi_output.parent / (dzi_output.name + "_files"),
-            image_id=image.pk,
-        )
-        new_folder_upload.add(dzi_folder_upload)
-
-    return new_folder_upload
-
-
 mirax_pattern = r"INDEXFILE\s?=\s?.|FILE_\d+\s?=\s?."
 # vms (and vmu) files contain key value pairs, where the ImageFile keys
 # can have the following format:
@@ -433,7 +391,6 @@ def image_builder_tiff(  # noqa: C901
     new_image_files: Set[PanImgFile] = set()
     consumed_files: Set[Path] = set()
     file_errors: Dict[Path, List[str]] = defaultdict(list)
-    new_folders: Set[PanImgFolder] = set()
 
     loaded_files = _load_gc_files(
         files=files,
@@ -443,8 +400,6 @@ def image_builder_tiff(  # noqa: C901
     )
 
     for gc_file in loaded_files:
-        dzi_output = None
-
         # try and load image with tiff file
         try:
             gc_file = _load_with_tiff(gc_file=gc_file)
@@ -467,27 +422,25 @@ def image_builder_tiff(  # noqa: C901
         image = _create_tiff_image_entry(tiff_file=gc_file)
         new_images.add(image)
 
-        try:
-            dzi_output, gc_file = _create_dzi_images(
-                gc_file=gc_file, output_directory=output_directory
+        new_image_files.add(
+            PanImgFile(
+                image_id=image.pk,
+                image_type=ImageType.TIFF,
+                file=gc_file.path.absolute(),
             )
-        except ValidationError as e:
-            file_errors[gc_file.path].append(f"DZI error: {e}.")
-
-        new_image_files |= _new_image_files(gc_file=gc_file, image=image)
-        new_folders |= _new_folder_uploads(dzi_output=dzi_output, image=image)
+        )
 
         if gc_file.associated_files:
-            consumed_files |= {f for f in gc_file.associated_files}
+            consumed_files |= {f.absolute() for f in gc_file.associated_files}
         else:
-            consumed_files.add(gc_file.path)
+            consumed_files.add(gc_file.path.absolute())
 
     return PanImgResult(
         consumed_files=consumed_files,
         file_errors=file_errors,
         new_images=new_images,
         new_image_files=new_image_files,
-        new_folders=new_folders,
+        new_folders=set(),
     )
 
 
@@ -513,22 +466,3 @@ def _create_tiff_image_entry(*, tiff_file: GrandChallengeTiffFile) -> PanImg:
         window_center=None,
         window_width=None,
     )
-
-
-def _create_dzi_images(
-    *, gc_file: GrandChallengeTiffFile, output_directory: Path
-) -> Tuple[Path, GrandChallengeTiffFile]:
-    # Creates a dzi file(out.dzi) and corresponding tiles in folder {pk}_files
-    dzi_output = output_directory / str(gc_file.path.name) / str(gc_file.pk)
-    try:
-        image = pyvips.Image.new_from_file(
-            str(gc_file.path.absolute()), access="sequential"
-        )
-        pyvips.Image.dzsave(image, str(dzi_output), tile_size=DZI_TILE_SIZE)
-        gc_file.source_files.append(
-            dzi_output.parent / (dzi_output.name + ".dzi")
-        )
-    except Exception as e:
-        raise ValidationError(f"Image can't be converted to dzi: {e}")
-
-    return dzi_output, gc_file
diff --git a/panimg/models.py b/panimg/models.py
index 7965d4e..b72a5e0 100644
--- a/panimg/models.py
+++ b/panimg/models.py
@@ -56,3 +56,9 @@ class PanImgResult:
     new_folders: Set[PanImgFolder]
     consumed_files: Set[Path]
     file_errors: Dict[Path, List[str]]
+
+
+@dataclass
+class PostProcessorResult:
+    new_image_files: Set[PanImgFile]
+    new_folders: Set[PanImgFolder]
diff --git a/panimg/panimg.py b/panimg/panimg.py
index 1f84ef9..4b22a8c 100644
--- a/panimg/panimg.py
+++ b/panimg/panimg.py
@@ -3,8 +3,15 @@
 from typing import DefaultDict, Iterable, List, Optional, Set
 
 from panimg.image_builders import DEFAULT_IMAGE_BUILDERS
-from panimg.models import PanImg, PanImgFile, PanImgFolder, PanImgResult
-from panimg.types import ImageBuilder
+from panimg.models import (
+    PanImg,
+    PanImgFile,
+    PanImgFolder,
+    PanImgResult,
+    PostProcessorResult,
+)
+from panimg.post_processors import DEFAULT_POST_PROCESSORS
+from panimg.types import ImageBuilder, PostProcessor
 
 
 def convert(
@@ -12,6 +19,7 @@ def convert(
     input_directory: Path,
     output_directory: Path,
     builders: Optional[Iterable[ImageBuilder]] = None,
+    post_processors: Optional[Iterable[PostProcessor]] = None,
     created_image_prefix: str = "",
 ) -> PanImgResult:
     new_images: Set[PanImg] = set()
@@ -32,6 +40,15 @@ def convert(
         created_image_prefix=created_image_prefix,
     )
 
+    result = _post_process(
+        image_files=new_image_files,
+        post_processors=post_processors
+        if post_processors is not None
+        else DEFAULT_POST_PROCESSORS,
+    )
+    new_image_files |= result.new_image_files
+    new_folders |= result.new_folders
+
     return PanImgResult(
         new_images=new_images,
         new_image_files=new_image_files,
@@ -89,3 +106,19 @@ def _convert_directory(
 
         for filepath, errors in builder_result.file_errors.items():
             file_errors[filepath].extend(errors)
+
+
+def _post_process(
+    *, image_files: Set[PanImgFile], post_processors: Iterable[PostProcessor]
+) -> PostProcessorResult:
+    new_image_files: Set[PanImgFile] = set()
+    new_folders: Set[PanImgFolder] = set()
+
+    for processor in post_processors:
+        result = processor(image_files=image_files)
+        new_image_files |= result.new_image_files
+        new_folders |= result.new_folders
+
+    return PostProcessorResult(
+        new_image_files=new_image_files, new_folders=new_folders
+    )
diff --git a/panimg/post_processors/__init__.py b/panimg/post_processors/__init__.py
new file mode 100644
index 0000000..0447fc3
--- /dev/null
+++ b/panimg/post_processors/__init__.py
@@ -0,0 +1,3 @@
+from panimg.post_processors.tiff_to_dzi import tiff_to_dzi
+
+DEFAULT_POST_PROCESSORS = [tiff_to_dzi]
diff --git a/panimg/post_processors/tiff_to_dzi.py b/panimg/post_processors/tiff_to_dzi.py
new file mode 100644
index 0000000..9063603
--- /dev/null
+++ b/panimg/post_processors/tiff_to_dzi.py
@@ -0,0 +1,60 @@
+import logging
+from typing import Set
+
+import pyvips
+
+from panimg.models import (
+    ImageType,
+    PanImgFile,
+    PanImgFolder,
+    PostProcessorResult,
+)
+from panimg.settings import DZI_TILE_SIZE
+
+logger = logging.getLogger(__name__)
+
+
+def tiff_to_dzi(*, image_files: Set[PanImgFile]) -> PostProcessorResult:
+    new_image_files: Set[PanImgFile] = set()
+    new_folders: Set[PanImgFolder] = set()
+
+    for file in image_files:
+        if file.image_type == ImageType.TIFF:
+            try:
+                result = _create_dzi_image(tiff_file=file)
+            except Exception as e:
+                logger.warning(f"Could not create DZI for {file}: {e}")
+                continue
+
+            new_image_files |= result.new_image_files
+            new_folders |= result.new_folders
+
+    return PostProcessorResult(
+        new_image_files=new_image_files, new_folders=new_folders
+    )
+
+
+def _create_dzi_image(*, tiff_file: PanImgFile) -> PostProcessorResult:
+    # Creates a dzi file and corresponding tiles in folder {pk}_files
+    dzi_output = tiff_file.file.parent / str(tiff_file.image_id)
+
+    image = pyvips.Image.new_from_file(
+        str(tiff_file.file.absolute()), access="sequential"
+    )
+
+    pyvips.Image.dzsave(image, str(dzi_output), tile_size=DZI_TILE_SIZE)
+
+    new_file = PanImgFile(
+        image_id=tiff_file.image_id,
+        image_type=ImageType.DZI,
+        file=(dzi_output.parent / f"{dzi_output.name}.dzi").absolute(),
+    )
+
+    new_folder = PanImgFolder(
+        image_id=tiff_file.image_id,
+        folder=(dzi_output.parent / f"{dzi_output.name}_files").absolute(),
+    )
+
+    return PostProcessorResult(
+        new_image_files={new_file}, new_folders={new_folder}
+    )
diff --git a/panimg/types.py b/panimg/types.py
index 08ba575..da28a35 100644
--- a/panimg/types.py
+++ b/panimg/types.py
@@ -3,7 +3,7 @@
 
 from typing_extensions import Protocol  # for py37 support
 
-from panimg.models import PanImgResult
+from panimg.models import PanImgFile, PanImgResult, PostProcessorResult
 
 
 class ImageBuilder(Protocol):
@@ -15,3 +15,8 @@ def __call__(
         created_image_prefix: str,
     ) -> PanImgResult:
         ...
+
+
+class PostProcessor(Protocol):
+    def __call__(self, *, image_files: Set[PanImgFile]) -> PostProcessorResult:
+        ...


Test patch:
diff --git a/tests/test_post_processors.py b/tests/test_post_processors.py
new file mode 100644
index 0000000..d620ace
--- /dev/null
+++ b/tests/test_post_processors.py
@@ -0,0 +1,37 @@
+import shutil
+from pathlib import Path
+
+import pytest
+
+from panimg import convert
+from tests import RESOURCE_PATH
+
+
+@pytest.mark.parametrize("post_processors", (None, []))
+def test_dzi_creation(tmpdir_factory, post_processors):
+    filenames = [
+        "valid_tiff.tif",
+        "no_dzi.tif",
+        "invalid_resolutions_tiff.tif",
+    ]
+
+    input_dir = Path(tmpdir_factory.mktemp("input"))
+    output_dir = Path(tmpdir_factory.mktemp("output"))
+
+    for f in filenames:
+        shutil.copy(RESOURCE_PATH / f, input_dir / f)
+
+    result = convert(
+        input_directory=input_dir,
+        output_directory=output_dir,
+        post_processors=post_processors,
+    )
+
+    assert len(result.new_images) == 2
+
+    if post_processors is None:
+        assert len(result.new_image_files) == 3
+        assert len(result.new_folders) == 1
+    else:
+        assert len(result.new_image_files) == 2
+        assert len(result.new_folders) == 0
diff --git a/tests/test_tiff.py b/tests/test_tiff.py
index 3154bc4..e8bc723 100644
--- a/tests/test_tiff.py
+++ b/tests/test_tiff.py
@@ -15,7 +15,6 @@
 from panimg.image_builders.tiff import (
     GrandChallengeTiffFile,
     _convert,
-    _create_dzi_images,
     _create_tiff_image_entry,
     _extract_tags,
     _get_color_space,
@@ -175,20 +174,10 @@ def test_load_with_tiff(
 
 
 @pytest.mark.parametrize(
-    "source_dir, filename, expected_error_message",
-    [
-        (RESOURCE_PATH, "valid_tiff.tif", ""),
-        (
-            RESOURCE_PATH,
-            "no_dzi.tif",
-            "Image can't be converted to dzi: unable to call dzsave",
-        ),
-    ],
+    "source_dir, filename",
+    [(RESOURCE_PATH, "valid_tiff.tif"), (RESOURCE_PATH, "no_dzi.tif")],
 )
-def test_load_with_open_slide(
-    source_dir, filename, expected_error_message, tmpdir_factory
-):
-    error_message = ""
+def test_load_with_open_slide(source_dir, filename, tmpdir_factory):
     # Copy resource file to writable temp folder
     temp_file = Path(tmpdir_factory.mktemp("temp") / filename)
     shutil.copy(source_dir / filename, temp_file)
@@ -197,52 +186,9 @@ def test_load_with_open_slide(
     output_dir = Path(tmpdir_factory.mktemp("output"))
     (output_dir / filename).mkdir()
 
-    try:
-        gc_file = _load_with_tiff(gc_file=gc_file)
-        _create_dzi_images(
-            gc_file=gc_file, output_directory=output_dir,
-        )
-    except Exception as e:
-        error_message = str(e)
+    gc_file = _load_with_tiff(gc_file=gc_file)
 
-    assert expected_error_message in error_message
-    if not expected_error_message:
-        assert not error_message
-
-
-@pytest.mark.parametrize(
-    "source_dir, filename, expected_error_message",
-    [
-        (RESOURCE_PATH, "valid_tiff.tif", ""),
-        (
-            RESOURCE_PATH,
-            "no_dzi.tif",
-            "Image can't be converted to dzi: unable to call dzsave",
-        ),
-    ],
-)
-def test_dzi_creation(
-    source_dir, filename, expected_error_message, tmpdir_factory
-):
-    error_message = ""
-    # Copy resource file to writable temp folder
-    temp_file = Path(tmpdir_factory.mktemp("temp") / filename)
-    shutil.copy(source_dir / filename, temp_file)
-    gc_file = GrandChallengeTiffFile(temp_file)
-
-    output_dir = Path(tmpdir_factory.mktemp("output"))
-    (output_dir / filename).mkdir()
-
-    try:
-        _create_dzi_images(
-            gc_file=gc_file, output_directory=output_dir,
-        )
-    except ValidationError as e:
-        error_message = str(e)
-
-    assert expected_error_message in error_message
-    if not expected_error_message:
-        assert not error_message
+    assert gc_file.validate() is None
 
 
 @pytest.mark.parametrize(
@@ -320,32 +266,8 @@ def test_image_builder_tiff(tmpdir_factory,):
         pk = file_to_pk[file.name]
         assert os.path.isfile(output_dir / file.name / f"{pk}.tif")
 
-    valid_tiff_pk = [
-        new_image.pk
-        for new_image in image_builder_result.new_images
-        if new_image.name == "valid_tiff.tif"
-    ][0]
-
-    # Assert the valid tif results in 2 new image file objects
-    assert (
-        len(
-            [
-                imagefile
-                for imagefile in image_builder_result.new_image_files
-                if imagefile.image_id == valid_tiff_pk
-            ]
-        )
-        == 2
-    )
-
-    # Asserts successful creation of dzi files
-    assert os.path.isfile(
-        output_dir / "valid_tiff.tif" / f"{valid_tiff_pk}.dzi"
-    )
-
-    dzi_file_dir = output_dir / "valid_tiff.tif" / f"{valid_tiff_pk}_files"
-    assert os.path.isdir(dzi_file_dir)
-    assert len(list((dzi_file_dir).rglob("*.jpeg"))) == 9
+    # Assert that both tiff images are imported
+    assert len(image_builder_result.new_image_files) == 2
 
 
 def test_handle_complex_files(tmpdir_factory):
@@ -404,8 +326,7 @@ def test_convert_to_tiff(resource, tmpdir_factory):
     result = image_builder_tiff(files=input_files, output_directory=output_dir)
 
     assert len(result.new_images) == 1
-    # DZI and TIFF should be created
-    assert len(result.new_image_files) == 2
+    assert len(result.new_image_files) == 1
 
 
 def test_error_handling(tmpdir_factory):
diff --git a/tests/test_tiff_to_dzi.py b/tests/test_tiff_to_dzi.py
new file mode 100644
index 0000000..81d5ad3
--- /dev/null
+++ b/tests/test_tiff_to_dzi.py
@@ -0,0 +1,82 @@
+import shutil
+from pathlib import Path
+from uuid import uuid4
+
+import pytest
+
+from panimg.models import ImageType, PanImgFile
+from panimg.post_processors.tiff_to_dzi import tiff_to_dzi
+from tests import RESOURCE_PATH
+
+
+def test_dzi_creation(tmpdir_factory):
+    filename = "valid_tiff.tif"
+
+    temp_file = Path(tmpdir_factory.mktemp("temp") / filename)
+    shutil.copy(RESOURCE_PATH / filename, temp_file)
+
+    image_file = PanImgFile(
+        image_id=uuid4(), image_type=ImageType.TIFF, file=temp_file
+    )
+
+    result = tiff_to_dzi(image_files={image_file})
+
+    assert len(result.new_image_files) == 1
+
+    new_file = result.new_image_files.pop()
+
+    assert new_file.image_id == image_file.image_id
+    assert new_file.image_type == ImageType.DZI
+    assert (
+        new_file.file == image_file.file.parent / f"{image_file.image_id}.dzi"
+    )
+
+    assert len(result.new_folders) == 1
+
+    new_folder = result.new_folders.pop()
+
+    assert new_folder.image_id == image_file.image_id
+    assert (
+        new_folder.folder
+        == image_file.file.parent / f"{image_file.image_id}_files"
+    )
+
+    assert len(list((new_folder.folder).rglob("*.jpeg"))) == 9
+
+
+def test_no_exception_when_failed(tmpdir_factory, caplog):
+    filename = "no_dzi.tif"
+
+    temp_file = Path(tmpdir_factory.mktemp("temp") / filename)
+    shutil.copy(RESOURCE_PATH / filename, temp_file)
+
+    image_file = PanImgFile(
+        image_id=uuid4(), image_type=ImageType.TIFF, file=temp_file
+    )
+
+    result = tiff_to_dzi(image_files={image_file})
+
+    assert len(result.new_image_files) == 0
+    assert len(result.new_folders) == 0
+
+    # The last warning should be from our logger
+    last_log = caplog.records[-1]
+    assert last_log.name == "panimg.post_processors.tiff_to_dzi"
+    assert last_log.levelname == "WARNING"
+    assert "Could not create DZI for" in last_log.message
+
+
+@pytest.mark.parametrize("image_type", (ImageType.DZI, ImageType.MHD))
+def test_non_tiff_skipped(tmpdir_factory, image_type):
+    filename = "valid_tiff.tif"
+
+    temp_file = Path(tmpdir_factory.mktemp("temp") / filename)
+    shutil.copy(RESOURCE_PATH / filename, temp_file)
+
+    image_file = PanImgFile(
+        image_id=uuid4(), image_type=image_type, file=temp_file
+    )
+
+    result = tiff_to_dzi(image_files={image_file})
+
+    assert len(result.new_image_files) == 0