Raise exception when user-specified reader package is not installed#8771
Open
haoyu-haoyu wants to merge 1 commit intoProject-MONAI:devfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a mapping Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/transforms/test_load_image.py (2)
513-514: Assert the error text too.These pass on any
OptionalImportError, including one bubbling up from deeper in reader init.assertRaisesRegexon the reader name or install hint would lock in the new contract.Patch
- with self.assertRaises(OptionalImportError): + with self.assertRaisesRegex(OptionalImportError, r"ITKReader|install"): LoadImage(reader="ITKReader") @@ - with self.assertRaises(OptionalImportError): + with self.assertRaisesRegex(OptionalImportError, r"MockMissingReader|install"): LoadImage(reader="MockMissingReader")Also applies to: 537-538
🤖 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 513 - 514, The test currently uses assertRaises(OptionalImportError) when constructing LoadImage(reader="ITKReader"); replace it with assertRaisesRegex(OptionalImportError, r"(ITKReader|install.*ITK|itk)") to ensure the raised OptionalImportError specifically mentions the reader name or the install hint; do the same change for the other occurrence (LoadImage(reader="ITKReader") around lines 537-538) so the tests fail only when the expected import hint is present rather than any unrelated OptionalImportError.
501-518: Use Google-style docstrings on the new test definitions.The docstrings are present, but they do not follow the required Google-style sections. 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 - 518, Update the docstrings in class TestLoadImageReaderNotInstalled for the methods test_specified_reader_not_installed_raises and test_specified_reader_not_installed_raises_mocked to use Google-style docstrings: include a short summary, an Args: section if the test takes parameters (omit if none), a Returns: section (state that it returns None), and a Raises: section documenting that OptionalImportError is expected when calling LoadImage(reader="ITKReader"); reference the test names and the LoadImage/OptionalImportError symbols so the intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/transforms/io/array.py`:
- Around line 212-216: The except block currently discards the original
OptionalImportError context; change the handler to capture the exception (e.g.,
except OptionalImportError as e:) and re-raise the new OptionalImportError with
exception chaining (raise OptionalImportError(... ) from e) so the original
traceback is preserved; reference the caught OptionalImportError and the reader
identifier _r when updating the except block in monai/transforms/io/array.py.
---
Nitpick comments:
In `@tests/transforms/test_load_image.py`:
- Around line 513-514: The test currently uses assertRaises(OptionalImportError)
when constructing LoadImage(reader="ITKReader"); replace it with
assertRaisesRegex(OptionalImportError, r"(ITKReader|install.*ITK|itk)") to
ensure the raised OptionalImportError specifically mentions the reader name or
the install hint; do the same change for the other occurrence
(LoadImage(reader="ITKReader") around lines 537-538) so the tests fail only when
the expected import hint is present rather than any unrelated
OptionalImportError.
- Around line 501-518: Update the docstrings in class
TestLoadImageReaderNotInstalled for the methods
test_specified_reader_not_installed_raises and
test_specified_reader_not_installed_raises_mocked to use Google-style
docstrings: include a short summary, an Args: section if the test takes
parameters (omit if none), a Returns: section (state that it returns None), and
a Raises: section documenting that OptionalImportError is expected when calling
LoadImage(reader="ITKReader"); reference the test names and the
LoadImage/OptionalImportError symbols so the intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1553a00f-91e7-46a5-9318-e9d99eafc257
📒 Files selected for processing (2)
monai/transforms/io/array.pytests/transforms/test_load_image.py
haoyu-haoyu
force-pushed
the
fix/raise-exception-when-specified-reader-not-installed
branch
2 times, most recently
from
255db19 to
1e6dd49
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/data/test_init_reader.py`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0e4bb13-0db3-43af-a6f6-a6ee036899fe
📒 Files selected for processing (3)
monai/transforms/io/array.pytests/data/test_init_reader.pytests/transforms/test_load_image.py
Comment on lines
+30
to
+36
| 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 |
Contributor
There was a problem hiding this comment.
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.
When a user explicitly specifies a reader (e.g., reader="ITKReader") but the required package is not installed, LoadImage now raises an OptionalImportError instead of silently falling back to another reader with only a warning. This makes the behavior explicit: if the user specifically requested a reader, they should be informed immediately that it cannot be used, rather than having the system silently use a different reader which may produce unexpected results. Default behavior (reader=None) remains unchanged — missing optional packages are still handled gracefully with debug logging. Fixes Project-MONAI#7437 Signed-off-by: haoyu-haoyu <haoyu-haoyu@users.noreply.github.com> Signed-off-by: SexyERIC0723 <haoyuwang144@gmail.com>
haoyu-haoyu
force-pushed
the
fix/raise-exception-when-specified-reader-not-installed
branch
from
1e6dd49 to
8024900
Compare
Author
|
Updated with improved error messages: Before (v1): The user knows which reader failed but not which pip package to install. After (v2): Changes:
This also partially addresses #7980 (which requests install hints for writers) — the same pattern could be applied there. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/transforms/test_load_image.py`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccc0f226-bec7-40fb-85a8-aaafd6f376d9
📒 Files selected for processing (3)
monai/transforms/io/array.pytests/data/test_init_reader.pytests/transforms/test_load_image.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/data/test_init_reader.py
- monai/transforms/io/array.py
Comment on lines
+501
to
+541
| 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)) | ||
|
|
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7437
Description
When a user explicitly specifies a reader (e.g.,
reader="ITKReader") but the required package is not installed,LoadImagenow raises anOptionalImportErrorinstead of silently falling back to another reader with only a warning.Before (current behavior):
After (new behavior):
Default behavior (reader=None) remains unchanged — missing optional packages are still handled gracefully with debug logging, and the system falls back to available readers as before.
Changes
monai/transforms/io/array.py: Changedwarnings.warn()toraise OptionalImportError()in the user-specified reader registration path (lines 212-216).tests/transforms/test_load_image.py: AddedTestLoadImageReaderNotInstalledtest class with:test_specified_reader_not_installed_raises: Direct test (runs when itk is NOT installed)test_specified_reader_not_installed_raises_mocked: Mock-based test that always runs, simulating a reader whose package is not installedTesting
reader=Nonepath unchanged)Signed-off-by: haoyu-haoyu haoyu-haoyu@users.noreply.github.com