fix(LoadImage): raise OptionalImportError when user-specified reader is not installed#8823
Open
KamayaniR wants to merge 3 commits intoProject-MONAI:devfrom
Open
Conversation
…package is not installed Previously, if a user explicitly passed a reader name (e.g. reader="ITKReader") and the required package was not installed, LoadImage would only emit a warning and silently continue with fallback readers. This masked real configuration errors. Now an OptionalImportError is raised immediately, making the failure explicit. The auto-select path (reader=None) is unchanged and still skips missing readers silently. Fixes Project-MONAI#7437 Signed-off-by: Kamayani Rai <kamayanirai78@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe change makes LoadImage.init re-raise an OptionalImportError (using Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/transforms/test_load_image.py (2)
509-511: Tighten the exception assertion.
assertRaises(OptionalImportError)is broad. PreferassertRaisesRegex(...)to verify the intended failure path/message.Proposed test hardening
- with self.assertRaises(OptionalImportError): + with self.assertRaisesRegex(OptionalImportError, "required package for reader ITKReader|itk not installed"): LoadImage(reader="ITKReader")🤖 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 509 - 511, The test currently catches any OptionalImportError broadly; tighten it by using assertRaisesRegex to assert the specific error message from ITKReader's __init__ side-effect. Replace the nested assertRaises(OptionalImportError) around LoadImage(reader="ITKReader") with self.assertRaisesRegex(OptionalImportError, r"itk not installed") (or an appropriate regex matching the exact message thrown) so the test verifies the intended failure path from ITKReader.__init__.
501-503: Add Google-style docstrings for new definitions.Please add a class docstring and make the method docstring Google-style (including
Raises), per repo guideline expectations.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 - 503, Add a Google-style class docstring to TestLoadImageReaderNotInstalled describing the test class purpose, and convert the test_raises_when_user_specified_reader_not_installed docstring to a Google-style docstring that includes Args (if any), a short description, and a Raises section documenting that OptionalImportError is expected to be raised; update the docstrings for the class TestLoadImageReaderNotInstalled and its method test_raises_when_user_specified_reader_not_installed accordingly so they follow the repository's Google-style docstring conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/transforms/test_load_image.py`:
- Around line 509-511: The test currently catches any OptionalImportError
broadly; tighten it by using assertRaisesRegex to assert the specific error
message from ITKReader's __init__ side-effect. Replace the nested
assertRaises(OptionalImportError) around LoadImage(reader="ITKReader") with
self.assertRaisesRegex(OptionalImportError, r"itk not installed") (or an
appropriate regex matching the exact message thrown) so the test verifies the
intended failure path from ITKReader.__init__.
- Around line 501-503: Add a Google-style class docstring to
TestLoadImageReaderNotInstalled describing the test class purpose, and convert
the test_raises_when_user_specified_reader_not_installed docstring to a
Google-style docstring that includes Args (if any), a short description, and a
Raises section documenting that OptionalImportError is expected to be raised;
update the docstrings for the class TestLoadImageReaderNotInstalled and its
method test_raises_when_user_specified_reader_not_installed accordingly so they
follow the repository's Google-style docstring conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea3fe7f5-762b-476f-8de4-72874842db42
📒 Files selected for processing (2)
monai/transforms/io/array.pytests/transforms/test_load_image.py
…eaders - tests/data/test_init_reader.py: wrap LoadImaged init in try/except OptionalImportError since packages may not be installed in min-dep envs - tests/transforms/test_load_image.py: tighten assertion using assertRaisesRegex, add Google-style class and method docstrings Signed-off-by: Kamayani Rai <kamayanirai78@gmail.com>
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-34: The test currently silences OptionalImportError around the
LoadImaged instantiation, hiding real failures (e.g., reader=None); change the
handler so you do not silently swallow all errors: when creating
LoadImaged("image", reader=r) catch OptionalImportError as e and call
self.skipTest(f"missing optional dependency: {e}") or re-raise the exception for
any other unexpected condition (or assert r is not None before instantiation),
ensuring LoadImaged failures due to logic bugs are not hidden while still
skipping tests when a dependency truly isn't installed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 948aae05-0d26-4453-adab-fb0162baad6f
📒 Files selected for processing (2)
tests/data/test_init_reader.pytests/transforms/test_load_image.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/transforms/test_load_image.py
Use subTest per reader and explicitly fail if reader=None raises OptionalImportError, preventing silent swallowing of real logic bugs. Signed-off-by: Kamayani Rai <kamayanirai78@gmail.com>
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.
What does this PR do?
Fixes #7437
When a user explicitly specifies a reader in
LoadImage(e.g.LoadImage(reader="ITKReader")),and the required package is not installed, the previous behavior was to silently warn and
continue with fallback readers. This masked real configuration errors.
Changes
monai/transforms/io/array.py: Changedwarnings.warntoraise OptionalImportErrorin the user-specified reader path inside
LoadImage.__init__tests/transforms/test_load_image.py: AddedTestLoadImageReaderNotInstalledto verifythe exception is raised correctly
Behavior
Before:
LoadImage(reader="ITKReader") # itk not installed
UserWarning: silently falls back to another reader — user has no idea!
After:
LoadImage(reader="ITKReader") # itk not installed
OptionalImportError: required package for reader ITKReader is not installed
Checklist