fix: raise exception when LoadImage reader is specified but not installed#8768
Open
abishop1990 wants to merge 13 commits intoProject-MONAI:devfrom
Open
fix: raise exception when LoadImage reader is specified but not installed#8768abishop1990 wants to merge 13 commits intoProject-MONAI:devfrom
abishop1990 wants to merge 13 commits intoProject-MONAI:devfrom
Conversation
…lled Previously, when a user explicitly specified a reader (e.g. LoadImage(reader='ITKReader')) but the required optional package was not installed, MONAI would silently warn and fall back to the next available reader. This silent fallback is surprising and hard to debug. This change raises a RuntimeError instead, giving the user a clear actionable message that explains what happened and how to fix it (install the package or omit the reader argument to use automatic fallback). Backward compatibility is preserved: if no reader is specified, the existing warn-and- skip behavior for missing optional packages is unchanged. Fixes Project-MONAI#7437 Signed-off-by: Alan Bishop <alanbishop@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLoadImage.init behavior changed: when a string reader or a provided reader class requires a missing/incompatible optional dependency, the code now raises RuntimeError with guidance to install the dependency or pick an alternative. TypeError cases for class readers still register a fallback instance but now emit warnings with stacklevel=2. Tests were added to assert RuntimeError for explicit missing readers (string and class), to check unspecified-reader fallback, and to validate successful explicit-reader loading; a duplicate test class was accidentally introduced in the test file. 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.
Actionable comments posted: 3
🤖 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-217: The try/except currently only wraps string-based reader
imports but not explicit reader classes, causing OptionalImportError to leak
when someone passes a class like ITKReader to LoadImage; update the code path in
LoadImage where the reader is instantiated (the branch handling the `reader`
argument and any use of `_r`/`reader` to create reader instances) to catch
OptionalImportError around the instantiation and re-raise the same RuntimeError
message used for string readers so both `LoadImage(reader="ITKReader")` and
`LoadImage(reader=ITKReader)` produce the consistent RuntimeError; ensure you
reference the same message and `from e` chaining when catching
OptionalImportError raised during explicit class construction.
In `@tests/transforms/test_load_image.py`:
- Around line 503-529: Add a unit test in TestLoadImageMissingReader to cover
the reader-as-class path: mimic the existing string-case test by patching
ITKReader.__init__ to raise monai.utils.OptionalImportError("itk") and assert
that LoadImage(reader=ITKReader) raises a RuntimeError containing "ITKReader"
and "not installed"; also add the complementary case to ensure
LoadImage(reader=NibabelReader) still succeeds when passing the class object
(assert instance), referencing the LoadImage, ITKReader, and NibabelReader
symbols so the class-based reader API is exercised alongside the string-based
tests.
- Around line 519-523: The test test_unspecified_reader_falls_back_silently
currently only constructs LoadImage() and may pass in CI when optional deps
exist; change it to force the "missing optional readers" path by temporarily
simulating ImportError(s) for the reader modules LoadImage tries to import
(e.g., patch importlib.import_module or the specific module lookup used inside
LoadImage to raise ImportError for PIL/torchvision/imageio), then instantiate
LoadImage() and assert it returns an instance without raising; restore the
original import behavior afterward and optionally assert the warn-and-skip
logging was invoked to prove the fallback path was exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20a87e52-ae6a-446a-beb1-fec7a663885a
📒 Files selected for processing (2)
monai/transforms/io/array.pytests/transforms/test_load_image.py
Signed-off-by: Cipher <cipher@openclaw.ai>
…onment When LoadImage is initialized with an explicitly-specified reader that is not installed, it now raises RuntimeError (instead of silently falling back). This is the correct behavior per issue Project-MONAI#7437, but breaks existing tests that assume the old fallback behavior. Update test_load_image() to only test with reader=None (auto-select path) which works in all environments. Specific readers with @SkipIfNoModule decorators are tested in their own dedicated test methods. Signed-off-by: Cipher <cipher@openclaw.ai>
…ise fallback path The previous test only verified LoadImage() succeeds in the happy path where optional dependencies exist. This does not validate the critical warn-and-skip fallback behavior when packages are missing. Update the test to: 1. Patch SUPPORTED_READERS to simulate missing optional package (ITKReader) 2. Trigger LoadImage() to exercise the exception-catching, warn, and skip path 3. Verify the debug log about the missing package was invoked 4. Restore original reader entries afterward This ensures the auto-select fallback path is actually tested, not just the happy path. Signed-off-by: Cipher <cipher@openclaw.ai>
The exception handling for missing optional packages only wrapped string-based reader imports (LoadImage(reader='ITKReader')). When users passed a class directly (LoadImage(reader=ITKReader)), OptionalImportError would leak through. Update the class instantiation path (elif inspect.isclass(_r):) to: 1. Catch OptionalImportError during _r(*args, **kwargs) call 2. Re-raise as RuntimeError with consistent message 3. Use exception chaining (from e) for debugging 4. Handle TypeError with warn-and-retry (same as string path) 5. Extract class name safely for error message Now both LoadImage(reader='ITKReader') and LoadImage(reader=ITKReader) produce consistent RuntimeError when the optional package is missing. Add test_explicit_class_reader_not_installed_raises_runtime_error to verify the class path is properly handled. Signed-off-by: Cipher <cipher@openclaw.ai>
for more information, see https://pre-commit.ci
…ently Pre-commit ci caught unused variable original_readers that was defined but never used. Remove it to pass ruff check. Signed-off-by: Cipher <cipher@openclaw.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/transforms/io/array.py (1)
212-220: String-reader path looks correct.Catches
OptionalImportError, chains it properly withfrom e, and provides actionable guidance. The TypeError fallback preserves existing behavior.Static analysis flags missing
stacklevel=2on line 219's warning—minor but worth fixing for clearer tracebacks.Proposed fix
except TypeError: # the reader doesn't have the corresponding args/kwargs - warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.") + warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.", stacklevel=2) self.register(the_reader())🤖 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 - 220, The TypeError except block in monai.transforms.io.array (the except handling where it warns that "{_r} is not supported with the given parameters {args} {kwargs}" and then calls self.register(the_reader())) should include stacklevel=2 on the warnings.warn call to point the warning at the caller; update the warnings.warn invocation to pass stacklevel=2 while keeping the existing message and the subsequent self.register(the_reader()) behavior unchanged.
🤖 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 538-558: The test is patching SUPPORTED_READERS with the wrong key
("itk") so the fallback path in LoadImage isn't exercised; change the test to
use the actual key used in SUPPORTED_READERS ("itkreader") when saving
original_itk, replacing it with failing_reader (that raises
OptionalImportError), and restoring original_itk afterwards; ensure you still
capture logs with self.assertLogs("LoadImage", level="DEBUG") so the
warn-and-skip behavior is asserted, referencing SUPPORTED_READERS, LoadImage,
failing_reader, and OptionalImportError to locate the relevant lines to update.
---
Nitpick comments:
In `@monai/transforms/io/array.py`:
- Around line 212-220: The TypeError except block in monai.transforms.io.array
(the except handling where it warns that "{_r} is not supported with the given
parameters {args} {kwargs}" and then calls self.register(the_reader())) should
include stacklevel=2 on the warnings.warn call to point the warning at the
caller; update the warnings.warn invocation to pass stacklevel=2 while keeping
the existing message and the subsequent self.register(the_reader()) behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2655f31-305d-401d-99da-990c69bf5f15
📒 Files selected for processing (3)
monai/transforms/io/array.pytests/data/test_init_reader.pytests/transforms/test_load_image.py
1. Add stacklevel=2 to all warnings.warn() calls for better tracebacks - Helps developers see where the warning originates 2. Fix SUPPORTED_READERS key in fallback test from 'itk' to 'itkreader' - Ensures test actually exercises the warn-and-skip fallback path - Was using wrong key, preventing the patch from working These fixes improve warning clarity and test effectiveness per CodeRabbit review. Signed-off-by: Cipher <cipher@openclaw.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/transforms/test_load_image.py (1)
526-558: Remove redundant import at line 531.
OptionalImportErroris already imported at the module level (line 32). The local import is unnecessary.Proposed fix
def test_unspecified_reader_falls_back_silently(self): """When no reader is specified, missing optional readers should be silently skipped (no exception).""" # Force the fallback path by simulating missing optional dependencies. # Patch the constructor to raise OptionalImportError for some readers, # then verify LoadImage still instantiates and logs warnings. - from monai.utils import OptionalImportError - # Patch SUPPORTED_READERS entries to raise OptionalImportError # This simulates optional packages not being installed from monai.transforms.io.array import SUPPORTED_READERS🤖 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 526 - 558, Remove the redundant local import of OptionalImportError inside test_unspecified_reader_falls_back_silently; instead reuse the module-level OptionalImportError already imported, so delete the line "from monai.utils import OptionalImportError" in the test, leaving the rest of the test (including use of SUPPORTED_READERS and LoadImage) unchanged.monai/transforms/io/array.py (1)
246-248: stacklevel may be off when called via LoadImaged.register().
stacklevel=2correctly points to user code for directLoadImage.register()calls. However, when called throughLoadImaged.register()(which delegates toself._loader.register()), the warning will point toLoadImaged.register()instead of the actual user code. This is minor and doesn't break anything.🤖 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 246 - 248, The warning in the reader-registration block uses a fixed stacklevel=2 which misattributes the call when LoadImaged.register() delegates to self._loader.register(); update the registration API and implementation so register() can accept an optional stacklevel or caller_depth parameter and pass that through from LoadImaged.register() into the reader-check (the block that does "if not isinstance(reader, ImageReader): warnings.warn(..., stacklevel=...)"), or compute the correct stacklevel dynamically (e.g., via inspect) to skip internal frames; change the relevant symbols LoadImage.register / LoadImaged.register / self._loader.register and the reader check in array.py to propagate or compute the correct stacklevel so the warning points to the actual user code.
🤖 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 246-248: The warning in the reader-registration block uses a fixed
stacklevel=2 which misattributes the call when LoadImaged.register() delegates
to self._loader.register(); update the registration API and implementation so
register() can accept an optional stacklevel or caller_depth parameter and pass
that through from LoadImaged.register() into the reader-check (the block that
does "if not isinstance(reader, ImageReader): warnings.warn(...,
stacklevel=...)"), or compute the correct stacklevel dynamically (e.g., via
inspect) to skip internal frames; change the relevant symbols LoadImage.register
/ LoadImaged.register / self._loader.register and the reader check in array.py
to propagate or compute the correct stacklevel so the warning points to the
actual user code.
In `@tests/transforms/test_load_image.py`:
- Around line 526-558: Remove the redundant local import of OptionalImportError
inside test_unspecified_reader_falls_back_silently; instead reuse the
module-level OptionalImportError already imported, so delete the line "from
monai.utils import OptionalImportError" in the test, leaving the rest of the
test (including use of SUPPORTED_READERS and LoadImage) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 818a6f12-c4bf-4779-9d1b-df9b5e64622b
📒 Files selected for processing (2)
monai/transforms/io/array.pytests/transforms/test_load_image.py
OptionalImportError and logging were imported both at module level and again inside the test_unspecified_reader_falls_back_silently() method. Remove the duplicate local imports since they're already available at the module level (where we moved them earlier per MONAI style). Keep the local import of SUPPORTED_READERS since it's only needed in this one test method. This fixes isort formatting check failure. Signed-off-by: Cipher <cipher@openclaw.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/transforms/test_load_image.py (1)
551-554: Cleanup may leave stale key if original was missing.If
SUPPORTED_READERSdidn't have"itkreader"originally,original_itkwould beNoneand this block would skip restoration, leaving the failing_reader in place. In practice,itkreaderis always defined, so this is safe. Consider usingpopwithdelfor stricter test isolation.Optional stricter cleanup
finally: # Restore original reader - if original_itk is not None: - SUPPORTED_READERS["itkreader"] = original_itk + if original_itk is not None: + SUPPORTED_READERS["itkreader"] = original_itk + elif "itkreader" in SUPPORTED_READERS: + del SUPPORTED_READERS["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 551 - 554, The test cleanup currently restores SUPPORTED_READERS["itkreader"] only when original_itk is not None, which leaves the test-installed reader in place if the key was absent originally; change the finally block to remove the test entry when original_itk is None and restore it when present: check original_itk (from the setup where you saved the initial value), and if original_itk is None use SUPPORTED_READERS.pop("itkreader", None) or del SUPPORTED_READERS["itkreader"] to remove the key, otherwise set SUPPORTED_READERS["itkreader"] = original_itk to restore the original reader (reference SUPPORTED_READERS and original_itk in your fix).
🤖 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 551-554: The test cleanup currently restores
SUPPORTED_READERS["itkreader"] only when original_itk is not None, which leaves
the test-installed reader in place if the key was absent originally; change the
finally block to remove the test entry when original_itk is None and restore it
when present: check original_itk (from the setup where you saved the initial
value), and if original_itk is None use SUPPORTED_READERS.pop("itkreader", None)
or del SUPPORTED_READERS["itkreader"] to remove the key, otherwise set
SUPPORTED_READERS["itkreader"] = original_itk to restore the original reader
(reference SUPPORTED_READERS and original_itk in your fix).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd591be8-ba29-446b-8d48-7538cd717224
📒 Files selected for processing (1)
tests/transforms/test_load_image.py
added 4 commits
March 7, 2026 18:47
- Break long warning messages across multiple lines to keep individual lines ≤ 120 chars - Shorten verbose docstring in test_explicit_class_reader_not_installed_raises_runtime_error - Ensures codeformat CI check passes This complies with MONAI's ruff/black/isort configuration which enforces line length limits. Signed-off-by: Cipher <cipher@openclaw.ai>
Black reformatted code according to MONAI's style requirements: - Add blank line after module docstring - Remove extra blank lines before class definition - Reformat long assertions to Black's multi-line style Note: Some lines exceed 120 chars due to f-string messages, which are allowed per setup.cfg (E501 ignored for strings). Signed-off-by: Cipher <cipher@openclaw.ai>
The finally block now correctly handles both cases: - If 'itkreader' existed originally: restore the original reader - If 'itkreader' didn't exist originally: remove the test entry This prevents leaving the patched reader in place when the key was absent originally, ensuring clean test isolation. Signed-off-by: Cipher <cipher@openclaw.ai>
The register() method's warning about non-ImageReader instances now uses stacklevel=3 instead of stacklevel=2. This correctly points to the LoadImage() call site where the reader is being instantiated, rather than internal frames. Stack trace: - User code: LoadImage(reader=...) - LoadImage.__init__() - self.register() - warnings.warn(stacklevel=3) → points to LoadImage() call This improves debugging experience for users who pass invalid readers. Signed-off-by: Cipher <cipher@openclaw.ai>
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
2 participants
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.
Description
Fixes #7437
Problem
When a user explicitly specifies a reader via but the required optional package (e.g. ) is not installed, MONAI currently:
This silent fallback is surprising and hard to debug — the user asked for a specific reader and got a different one without an error.
Fix
When an explicitly-specified reader raises
OptionalImportErrorduring initialization, raise aRuntimeErrorwith a clear, actionable message:Backward Compatibility
Tests Added
Three new tests in
TestLoadImageMissingReader:test_explicit_reader_not_installed_raises_runtime_error— mocks OptionalImportError to verify RuntimeError is raised with helpful messagetest_unspecified_reader_falls_back_silently— verifies no-reader case still workstest_explicit_reader_available_succeeds— verifies happy path still worksChecklist
git commit -s)