Raise exception when user-specified reader package is not installed#8771
Conversation
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>
📝 WalkthroughWalkthroughChanges LoadImage to raise OptionalImportError instead of warning when a specified reader package is unavailable, with error messages directing users to install the required package. Adds test coverage for this new exception-based behavior through direct and mocked reader scenarios. 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)
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}
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
| except OptionalImportError: | ||
| warnings.warn( | ||
| f"required package for reader {_r} is not installed, or the version doesn't match requirement." | ||
| raise OptionalImportError( | ||
| f"Required package for reader {_r} is not installed, or the version doesn't match requirement. " | ||
| f"Please install the required package to use {_r}." | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n monai/transforms/io/array.py | sed -n '210,220p'Repository: Project-MONAI/MONAI
Length of output: 813
🏁 Script executed:
cat -n monai/utils/module.py | sed -n '395,415p'Repository: Project-MONAI/MONAI
Length of output: 1076
🏁 Script executed:
cat -n monai/transforms/io/array.py | sed -n '190,230p'Repository: Project-MONAI/MONAI
Length of output: 2458
🏁 Script executed:
# Check for similar exception patterns in the file
rg -n "except OptionalImportError" monai/transforms/io/array.py
# Check if there are other exception re-raise patterns in the file with chaining
rg -n "raise.*from" monai/transforms/io/array.pyRepository: Project-MONAI/MONAI
Length of output: 154
🏁 Script executed:
# Check if there are other OptionalImportError re-raises in the codebase with chaining
rg -A 3 "except OptionalImportError" monai/ -t py | head -40
# Check if exception chaining is used elsewhere in the file
rg "raise.*from" monai/transforms/io/array.py -B 2 -A 2Repository: Project-MONAI/MONAI
Length of output: 1379
🏁 Script executed:
# Check Python version targeted by the project to confirm exception chaining is supported
rg "python_requires" setup.py setup.cfg pyproject.toml -A 1
# Look at the OptionalImportError definition
rg -A 5 "class OptionalImportError" monai/
# Check if there are tests for this specific code path
fd test.*\.py -x rg -l "ArrayReader\|register.*OptionalImportError" {} +Repository: Project-MONAI/MONAI
Length of output: 33546
🏁 Script executed:
# Check if there are tests for the ArrayReader register functionality
find . -name "*test*array*" -o -name "*test*reader*" | grep -i transform | head -10Repository: Project-MONAI/MONAI
Length of output: 111
🏁 Script executed:
# Search for test files related to array I/O or ArrayReader
rg -l "ArrayReader\|register.*reader" tests/ -t py | head -5
# Also check if there are any existing tests for exception chaining patterns
rg "from err\|from e\>" tests/ -t py | head -20Repository: Project-MONAI/MONAI
Length of output: 163
🏁 Script executed:
# Search for exception chaining patterns in tests
rg "raise.*from" tests/ -t py | head -20
# Look for ArrayReader tests specifically
rg "ArrayReader" tests/ -t py -lRepository: Project-MONAI/MONAI
Length of output: 712
Add exception chaining to preserve the original error context.
The caught OptionalImportError at line 212 is replaced without preserving its cause. Use exception chaining to maintain the original traceback for debugging.
Patch
- except OptionalImportError:
- raise OptionalImportError(
+ except OptionalImportError as err:
+ raise OptionalImportError(
f"Required package for reader {_r} is not installed, or the version doesn't match requirement. "
f"Please install the required package to use {_r}."
- )
+ ) from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except OptionalImportError: | |
| warnings.warn( | |
| f"required package for reader {_r} is not installed, or the version doesn't match requirement." | |
| raise OptionalImportError( | |
| f"Required package for reader {_r} is not installed, or the version doesn't match requirement. " | |
| f"Please install the required package to use {_r}." | |
| ) | |
| except OptionalImportError as err: | |
| raise OptionalImportError( | |
| f"Required package for reader {_r} is not installed, or the version doesn't match requirement. " | |
| f"Please install the required package to use {_r}." | |
| ) from err |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 213-216: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 213-216: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/transforms/io/array.py` around lines 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.
1 participant
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