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
📝 WalkthroughWalkthroughLoadImage was changed to raise OptionalImportError when a user-specified reader backend (provided as a string) cannot be imported, with an installation-instruction message instead of only warning and falling back. Tests were added: two new tests in tests/transforms/test_load_image.py assert that specifying an unavailable reader (real or mocked) raises OptionalImportError. tests/data/test_init_reader.py was updated to catch OptionalImportError when instantiating LoadImaged and skip assertions if the backend is missing. 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
from
1b6730f to
255db19
Compare
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
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.
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