Fix: raise OptionalImportError when user-specified reader package is not installed#8796
Fix: raise OptionalImportError when user-specified reader package is not installed#8796Talhax55z wants to merge 5 commits intoProject-MONAI:devfrom
Conversation
|
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLoadImage.init now re-raises an OptionalImportError (using exception chaining) if a user-specified reader registration raises OptionalImportError; previously it only warned and continued. The TypeError path still warns and falls back to registering the reader instance. Tests added in tests/data/test_image_rw.py (TestLoadImageReaderError) assert that providing a reader whose constructor raises OptionalImportError causes LoadImage to raise that error; an additional test simulates a missing backend when the reader is specified by string. The module-level unittest main block was removed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 (1)
monai/transforms/io/array.py (1)
213-216: Document the new raised exception inLoadImage.__init__docstring.
__init__now explicitly raisesOptionalImportErrorfor user-specified readers with missing dependencies; add aRaises:section.Proposed docstring update
def __init__(...): """ Args: ... + Raises: + OptionalImportError: If a user-specified reader requires an optional package + that is not installed or has an incompatible version. Note: ... """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 `@monai/transforms/io/array.py` around lines 213 - 216, The LoadImage.__init__ docstring must be updated to document that it can raise OptionalImportError when a user-specified reader's dependency is missing; add a Raises: section to the Google-style docstring for LoadImage.__init__ that cites OptionalImportError and explains it is raised when the required package for a requested reader (referenced as _r in the exception) is not installed or version-mismatched. Ensure the wording matches existing docstring style and appears under the __init__ docstring for the LoadImage class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@monai/transforms/io/array.py`:
- Around line 213-216: The LoadImage.__init__ docstring must be updated to
document that it can raise OptionalImportError when a user-specified reader's
dependency is missing; add a Raises: section to the Google-style docstring for
LoadImage.__init__ that cites OptionalImportError and explains it is raised when
the required package for a requested reader (referenced as _r in the exception)
is not installed or version-mismatched. Ensure the wording matches existing
docstring style and appears under the __init__ docstring for the LoadImage
class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12f8bd6a-2a3b-4cde-9c52-4b5bd0ab90f2
📒 Files selected for processing (1)
monai/transforms/io/array.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/data/test_image_rw.py (2)
192-197: Test covers class-based reader; consider also testing string-based reader path.The PR objective references
LoadImage(reader="ITKReader")(string). This test uses a class (FakeReader), which exercises a different code path inLoadImage.__init__. Both paths are valid, but the string path (lines 217-219 inarray.py) has the explicit re-raise with exception chaining, while the class path just propagates naturally.The current test is still useful coverage—just noting the distinction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/test_image_rw.py` around lines 192 - 197, Current test exercises the class-based reader path (FakeReader) but not the string-based path exercised by LoadImage(reader="ITKReader"); add a new test that passes a string reader name (e.g., "ITKReader") to LoadImage and asserts it raises OptionalImportError, so the code path in LoadImage.__init__ that looks up string readers and re-raises with exception chaining is covered; reference the LoadImage class and the string reader name ("ITKReader") in your new test to ensure the specific branch in array.py is executed.
190-197: Missing docstrings for class and test method.Per coding guidelines, docstrings should be present for all definitions using Google-style format.
📝 Proposed docstrings
class TestLoadImageReaderError(unittest.TestCase): + """Tests for LoadImage error handling when reader initialization fails.""" + def test_missing_reader_package_raises_error(self): + """Verify OptionalImportError is raised when reader package is unavailable. + + Raises: + OptionalImportError: When FakeReader is passed and its backend is missing. + """ class FakeReader: def __init__(self): raise OptionalImportError("fake_package is not installed") with self.assertRaises(OptionalImportError): LoadImage(reader=FakeReader)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/data/test_image_rw.py` around lines 190 - 197, Add Google-style docstrings to the test class TestLoadImageReaderError and its test method test_missing_reader_package_raises_error: for the class include a brief description of the test suite purpose, and for the method document the test scenario, expected behavior, raised exception (OptionalImportError) and any relevant setup (FakeReader raising OptionalImportError and calling LoadImage). Ensure the docstrings follow Google-style sections (Args, Raises) and reference the FakeReader and LoadImage behavior.
🤖 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/data/test_image_rw.py`:
- Around line 192-197: Current test exercises the class-based reader path
(FakeReader) but not the string-based path exercised by
LoadImage(reader="ITKReader"); add a new test that passes a string reader name
(e.g., "ITKReader") to LoadImage and asserts it raises OptionalImportError, so
the code path in LoadImage.__init__ that looks up string readers and re-raises
with exception chaining is covered; reference the LoadImage class and the string
reader name ("ITKReader") in your new test to ensure the specific branch in
array.py is executed.
- Around line 190-197: Add Google-style docstrings to the test class
TestLoadImageReaderError and its test method
test_missing_reader_package_raises_error: for the class include a brief
description of the test suite purpose, and for the method document the test
scenario, expected behavior, raised exception (OptionalImportError) and any
relevant setup (FakeReader raising OptionalImportError and calling LoadImage).
Ensure the docstrings follow Google-style sections (Args, Raises) and reference
the FakeReader and LoadImage behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b829b25e-69b4-413e-b57b-74f5f53d469e
📒 Files selected for processing (2)
monai/transforms/io/array.pytests/data/test_image_rw.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/transforms/io/array.py
|
Hi @Talhax55z thanks for the contribution, but I think there's three other PRs working on this with similar solutions. If you refer to my most recent comment on #7437 I was coordinating with the other contributors, if there's something specific in this PR you want to add to that conversation we can keep it here or close and integrate into someone else's work. |
|
Thanks for understanding @Talhax55z. I almost certainly commented in the issue after you'd already started work on this PR so a little bad timing there. I'll leave this PR open and when we have progress with the others we'll circle back to this one. |
2 participants
Fixes #7437
Description
When a user specifies a reader in LoadImage that requires a package
which is not installed, the code previously only issued a warnings.warn()
which could easily be missed.
This change raises an OptionalImportError instead, giving the user
a clear, visible error message telling them exactly which reader
package needs to be installed.
Types of changes