Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions monai/transforms/io/array.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,16 @@
"nibabelreader": NibabelReader,
}

# Maps reader names (lower-cased) to pip install commands so error messages
# can tell users exactly how to fix a missing-package error.
_READER_INSTALL_HINTS: dict[str, str] = {
"nibabelreader": "pip install nibabel",
"itkreader": "pip install itk",
"pilreader": "pip install Pillow",
"pydicomreader": "pip install pydicom",
"nrrdreader": "pip install pynrrd",
}


def switch_endianness(data, new="<"):
"""
Expand Down Expand Up @@ -209,10 +219,10 @@ def __init__(
the_reader = look_up_option(_r.lower(), SUPPORTED_READERS)
try:
self.register(the_reader(*args, **kwargs))
except OptionalImportError:
warnings.warn(
f"required package for reader {_r} is not installed, or the version doesn't match requirement."
)
except OptionalImportError as e:
hint = _READER_INSTALL_HINTS.get(_r.lower(), "")
install_msg = f" Install with: {hint}" if hint else ""
raise OptionalImportError(f"{e}{install_msg}") from e
except TypeError: # the reader doesn't have the corresponding args/kwargs
warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.")
self.register(the_reader())
Expand Down
10 changes: 8 additions & 2 deletions tests/data/test_init_reader.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

from monai.data import ITKReader, NibabelReader, NrrdReader, NumpyReader, PILReader, PydicomReader
from monai.transforms import LoadImage, LoadImaged
from monai.utils import OptionalImportError
from tests.test_utils import SkipIfNoModule


Expand All @@ -26,8 +27,13 @@ def test_load_image(self):
self.assertIsInstance(instance2, LoadImage)

for r in ["NibabelReader", "PILReader", "ITKReader", "NumpyReader", "NrrdReader", "PydicomReader", None]:
inst = LoadImaged("image", reader=r)
self.assertIsInstance(inst, LoadImaged)
try:
inst = LoadImaged("image", reader=r)
self.assertIsInstance(inst, LoadImaged)
except OptionalImportError:
# Reader's backend package is not installed — expected in
# minimal-dependency environments after the fix for #7437.
pass
Comment on lines +30 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid swallowing OptionalImportError without assertions.

This except ...: pass weakens the test and can mask regressions. Assert the expected branch instead of silently continuing.

Suggested test-hardening patch
         for r in ["NibabelReader", "PILReader", "ITKReader", "NumpyReader", "NrrdReader", "PydicomReader", None]:
-            try:
-                inst = LoadImaged("image", reader=r)
-                self.assertIsInstance(inst, LoadImaged)
-            except OptionalImportError:
-                # Reader's backend package is not installed — expected in
-                # minimal-dependency environments after the fix for `#7437`.
-                pass
+            with self.subTest(reader=r):
+                try:
+                    inst = LoadImaged("image", reader=r)
+                except OptionalImportError as err:
+                    self.assertIsNotNone(r)
+                    self.assertIn(str(r), str(err))
+                else:
+                    self.assertIsInstance(inst, LoadImaged)

As per coding guidelines, "Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Ensure new or modified definitions will be covered by existing or new unit tests."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/data/test_init_reader.py` around lines 30 - 36, The test currently
swallows OptionalImportError in the LoadImaged instantiation which can mask
regressions; change the test to explicitly assert the expected behavior instead
of pass: either use self.assertRaises(OptionalImportError, LoadImaged, "image",
reader=r) to assert the error is raised in minimal-dependency environments, or
keep the try block and in the except OptionalImportError as e branch assert
something specific about e (e.g., error message contains the backend name) so
the failing branch is tested rather than silently ignored; reference the
LoadImaged constructor and OptionalImportError in your change.


@SkipIfNoModule("nibabel")
@SkipIfNoModule("cupy")
Expand Down
42 changes: 42 additions & 0 deletions tests/transforms/test_load_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -498,5 +498,47 @@ def test_correct(self, input_param, expected_shape, track_meta):
self.assertFalse(hasattr(r, "affine"))


class TestLoadImageReaderNotInstalled(unittest.TestCase):
"""Test that specifying a reader whose required package is not installed raises an error.

Addresses https://github.com/Project-MONAI/MONAI/issues/7437
"""

@unittest.skipIf(has_itk, "test requires itk to NOT be installed")
def test_specified_reader_not_installed_raises(self):
"""When a user explicitly specifies a reader that is not installed, LoadImage should raise
an OptionalImportError with an install hint instead of silently falling back."""
from monai.utils import OptionalImportError

with self.assertRaises(OptionalImportError) as ctx:
LoadImage(reader="ITKReader")
self.assertIn("pip install itk", str(ctx.exception))

def test_specified_reader_not_installed_raises_mocked(self):
"""Mock test to verify OptionalImportError is raised with original message preserved
when a user-specified reader's required package is not installed."""
from unittest.mock import patch

from monai.utils import OptionalImportError

_original = __import__("monai.transforms.io.array", fromlist=["optional_import"]).optional_import

def _mock_optional_import(module, name="", *args, **kwargs):
if name == "MockMissingReader":

class _Unavailable:
def __init__(self, *a, **kw):
raise OptionalImportError("mock package is not installed")

return _Unavailable, True
return _original(module, *args, name=name, **kwargs)

with patch("monai.transforms.io.array.optional_import", side_effect=_mock_optional_import):
with self.assertRaises(OptionalImportError) as ctx:
LoadImage(reader="MockMissingReader")
# Original error message should be preserved in the re-raised exception
self.assertIn("mock package is not installed", str(ctx.exception))

Comment on lines +501 to +541
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Docstring guideline is only partially satisfied for newly added definitions.

New definitions in this block are missing full Google-style docstrings (notably nested _mock_optional_import, _Unavailable, and _Unavailable.__init__), including explicit raised-exception documentation.

As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/transforms/test_load_image.py` around lines 501 - 541, Add Google-style
docstrings for the newly introduced nested definitions: the function
_mock_optional_import, the class _Unavailable, and its __init__ method so they
document purpose, parameters, return values, and the raised OptionalImportError;
update _mock_optional_import to include a short description, args (module, name,
*args, **kwargs), and returns (callable, bool) and _Unavailable.__init__ to
include an Args section and a Raises section noting OptionalImportError("mock
package is not installed"); reference these exact names (_mock_optional_import,
_Unavailable, and _Unavailable.__init__) when adding the docstrings.


if __name__ == "__main__":
unittest.main()
Loading