Fix LoadImage to raise OptionalImportError when specified reader is not available #8522
Conversation
WalkthroughAdded a new constructor parameter Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
|
Hi @its-serah thanks for the contribution. As you see from the failed tests the expectation of the reader is to not raise an exception if an optional package isn't found, and this is correct in the cases when a fallback reader does exist for some formats. If we raise an exception whenever a reader can't be loaded then this fallback behaviour can't happen. I would suggest that we add a flag as a member to the class to enable the exception behaviour, but whose default state retains the existing behaviour. We would also need tests to check that turning this on correctly raises exceptions. What do you think? There's interest in the associated issue so I'm keen to find a solution that works for everyone. Thanks! |
- Add raise_on_missing_reader parameter (defaults to False for backward compatibility) - When True, raises OptionalImportError if specified reader is not available - When False (default), issues warning and uses fallback readers - Update tests to verify new behavior - Addresses reviewer feedback on PR Project-MONAI#8522
|
Hi @ericspod thanks for the excellent feedback! I've implemented exactly what you suggested. I added a raise_on_missing_reader flag to both LoadImage and LoadImaged classes that defaults to False to maintain existing behavior and backward compatibility. When set to True, it raises OptionalImportError for missing readers. When False (default), it preserves the current fallback behavior with warnings. I also added comprehensive tests to verify the flag correctly raises exceptions when enabled. This gives users control while maintaining the important fallback functionality for existing codebases. The failed tests should now pass since the default behavior is unchanged. What do you think of this approach? |
monai/transforms/io/array.py
Outdated
| the_reader = look_up_option(_r.lower(), SUPPORTED_READERS) | ||
| except ValueError: | ||
| # If the reader name is not recognized at all, raise OptionalImportError | ||
| raise OptionalImportError( |
There was a problem hiding this comment.
I think we need if self.raise_on_missing_reader here as well.
|
Hi @its-serah it looks better now, but I think the two raises need to be guarded by the same mechanism. Other than that it looks good though you'll have to fix your DCO issue and the formatting issue ( |
its-serah
force-pushed
the
fix-loadimage-reader-exception
branch
from
68102c1 to
e332430
Compare
|
Hi @ericspod thanks for the feedback! I've addressed both issues you mentioned: Fixed the guarding mechanism: Both Fixed DCO and formatting: Added proper DCO sign-off and applied code formatting fixes using isort, black, and ruff. The changes ensure that both exception cases (unrecognized reader name and missing dependencies) are handled consistently through the same flag. When |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
monai/transforms/io/array.py (1)
243-245: Class-specified reader path bypasses the flag — instantiation may raise unguarded.If users pass a reader class (e.g.,
LoadImage(reader=ITKReader, raise_on_missing_reader=False)), instantiation will raiseOptionalImportErrorunconditionally, ignoring the new flag. Guard this path like the string-based path.- elif inspect.isclass(_r): - self.register(_r(*args, **kwargs)) + elif inspect.isclass(_r): + try: + self.register(_r(*args, **kwargs)) + except OptionalImportError as e: + if self.raise_on_missing_reader: + raise OptionalImportError( + f"required package for reader {_r.__name__} is not installed, or the version doesn't match requirement." + ) from e + else: + warnings.warn( + f"required package for reader {_r.__name__} is not installed, or the version doesn't match requirement. " + f"Will use fallback readers if available.", + category=UserWarning, + stacklevel=2, + ) + except TypeError: + warnings.warn( + f"{_r.__name__} is not supported with the given parameters {args} {kwargs}.", + category=UserWarning, + stacklevel=2, + ) + self.register(_r())
🧹 Nitpick comments (3)
monai/transforms/io/array.py (3)
213-226: Unknown reader name handling: enrich warnings and callsite context.Behavior is correct. Improve usability by adding
stackleveland explicit category so the warning points callers to their site.- else: - warnings.warn( - f"Cannot find reader '{_r}'. It may not be installed or recognized. " - f"Will use fallback readers if available." - ) + else: + warnings.warn( + f"Cannot find reader '{_r}'. It may not be installed or recognized. " + f"Will use fallback readers if available.", + category=UserWarning, + stacklevel=2, + )
229-238: Missing optional dependency: add stacklevel/category to warning.Same UX improvement as above; keep messages actionable at the callsite.
- else: - warnings.warn( - f"required package for reader {_r} is not installed, or the version doesn't match requirement. " - f"Will use fallback readers if available." - ) + else: + warnings.warn( + f"required package for reader {_r} is not installed, or the version doesn't match requirement. " + f"Will use fallback readers if available.", + category=UserWarning, + stacklevel=2, + )
165-167: Docstring: add Raises section and clarify accepted reader typesPlease update the
LoadImagedocstring to document the new behavior and list all supportedreaderformats. For example:- raise_on_missing_reader: if True, raise OptionalImportError when a specified reader is not available, - otherwise attempt to use fallback readers. Default is False to maintain backward compatibility. - args: additional parameters for reader if providing a reader name. + raise_on_missing_reader: if True, raise `OptionalImportError` when a specified reader is not available; + otherwise attempt to use fallback readers. Defaults to False (backward compatibility). + args: additional parameters for reader if providing a reader name. + + Raises: + OptionalImportError: If `raise_on_missing_reader=True` and the specified reader cannot be + found or its optional dependency is not installed. + + Accepted reader types: + - str: name of a registered reader (e.g., `"ITKReader"`) + - class: e.g., `ITKReader` or a custom reader class + - instance: e.g., `ITKReader(pixel_type=itk.UC)` + - list/tuple: multiple reader names or classes to try in orderTests already cover:
- string reader with
raise_on_missing_reader=True/False- custom reader class and instance
- default behavior for class readers
For completeness, you may optionally add a test that passes a reader class with
raise_on_missing_reader=Trueto confirm it still succeeds (no exception).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/io/array.py(4 hunks)tests/transforms/test_load_image.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/transforms/test_load_image.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/io/array.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (1)
monai/transforms/io/array.py (1)
141-144: Opt-in flag preserves back-compat — good addition.Adding
raise_on_missing_reader: bool = Falseis the right trade-off to surface explicit failures without breaking existing flows.
|
Hi @its-serah thanks for continuing to work on this, I think it's much better with some minor suggestions. The tests are failing however so please look again and what your tests are testing to see what the issue is. The DCO still doesn't seem happy but we can sort that last and just force it to pass if needed. |
|
Hey @ericspod Thank you for the code review feedback! I've addressed both of your suggestions in the latest commits: Refactored duplicate error messaging: Fixed the failing test: The changes maintain backward compatibility while implementing the cleaner code structure you suggested. I've also verified that the functionality works as expected with custom tests. The CI failures should now be resolved since the test logic correctly matches the implementation behavior. Please let me know if you need any clarifications or have additional feedback! |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/transforms/test_load_image.py (1)
14-25: Importwarningsto fix NameError in tests.
warningsis used below but not imported (Ruff F821 at Lines 458–459).Apply:
import os import shutil import tempfile import unittest +import warnings from pathlib import Path
🧹 Nitpick comments (5)
test_my_changes.py (4)
1-1: Drop the shebang or make the file executable.The shebang triggers EXE001 under lint unless the file is chmod +x. For tests, the shebang is unnecessary—remove it.
Apply:
-#!/usr/bin/env python3
25-33: Use an assertion and avoid the unused variable warning (F841).Assert the created loader type and keep the variable used.
Apply:
- loader = LoadImage(reader="UnknownReader", raise_on_missing_reader=False) - if w and "UnknownReader" in str(w[0].message): + loader = LoadImage(reader="UnknownReader", raise_on_missing_reader=False) + assert isinstance(loader, LoadImage) + if w and "UnknownReader" in str(w[0].message): print(f"PASS: Got expected warning: {w[0].message}")
43-48: Fix unused variables (F841) and avoid blindexcept Exception(BLE001).Don’t assign unused, and narrow the except.
Apply:
- loader1 = LoadImage(reader="PILReader", raise_on_missing_reader=True) - loader2 = LoadImage(reader="PILReader", raise_on_missing_reader=False) + LoadImage(reader="PILReader", raise_on_missing_reader=True) + LoadImage(reader="PILReader", raise_on_missing_reader=False) print("PASS: Both loaders created successfully with valid reader") - except Exception as e: + except OptionalImportError as e: print(f"FAIL: Unexpected error with valid reader: {e}") return False
9-52: Consider rewriting this as a pytest/unittest test with asserts (optional).Printing “PASS/FAIL” and returning booleans reduces signal in CI. Using asserts/pytest.raises will integrate better with the suite (and remove TRY300).
I can convert this function to pytest-style with proper assertions if you want.
tests/transforms/test_load_image.py (1)
456-464: Robustness: assert warning content with full message or regex.Message text now includes a follow-up sentence; using substring match is fine, but consider
assertWarnsRegexor stricter predicate. Also depends on warnings stacklevel in the implementation (addressed below).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
monai/transforms/io/array.py(4 hunks)test_my_changes.py(1 hunks)tests/transforms/test_load_image.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
test_my_changes.pymonai/transforms/io/array.pytests/transforms/test_load_image.py
🪛 Ruff (0.12.2)
test_my_changes.py
1-1: Shebang is present but file is not executable
(EXE001)
18-18: Consider moving this statement to an else block
(TRY300)
27-27: Local variable loader is assigned to but never used
Remove assignment to unused variable loader
(F841)
36-36: Do not catch blind exception: Exception
(BLE001)
43-43: Local variable loader1 is assigned to but never used
Remove assignment to unused variable loader1
(F841)
44-44: Local variable loader2 is assigned to but never used
Remove assignment to unused variable loader2
(F841)
46-46: Do not catch blind exception: Exception
(BLE001)
monai/transforms/io/array.py
219-219: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
221-221: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
230-230: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
tests/transforms/test_load_image.py
458-458: Undefined name warnings
(F821)
459-459: Undefined name warnings
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
🔇 Additional comments (2)
tests/transforms/test_load_image.py (1)
440-444: Align test with new default behavior
Defaultraise_on_missing_reader=False; either passraise_on_missing_reader=Truein this test or rewrite it to assert the warning-and-fallback path instead of expecting an exception. Manual check ofLoadImage’s signature and defaults required.monai/transforms/io/array.py (1)
141-167: Docstring/API LGTM.New
raise_on_missing_readerparameter is documented clearly; default preserves backward compatibility.
test_my_changes.py
Outdated
| except Exception as e: | ||
| print(f"FAIL: Unexpected error: {e}") | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the blind except Exception (BLE001).
Catching all exceptions hides real failures; let unexpected errors surface.
Apply:
- except Exception as e:
- print(f"FAIL: Unexpected error: {e}")
- return False📝 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 Exception as e: | |
| print(f"FAIL: Unexpected error: {e}") | |
| return False |
🧰 Tools
🪛 Ruff (0.12.2)
36-36: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In test_my_changes.py around lines 36 to 38, the code currently uses a blind
except Exception which hides unexpected failures; replace it by either removing
the try/except so exceptions propagate, or catch only the specific exception
types you expect (e.g., ValueError, AssertionError) and handle those, and for
any other exception re-raise it (or omit handling) so test runners see real
errors; update logging to record expected exception details only and ensure
unexpected exceptions are not swallowed.
|
Hi @its-serah thanks for the update, I think this addresses the suggested code I had. The tests are failing however since your tests are attempting to load a non-existent file. There's also a few things from Coderabbit but we're just about there. |
|
@its-serah are you fine with me finishing this PR? |
|
|
pre-commit.ci run |
This commit addresses CodeRabbit review feedback and adds a new flag to control exception behavior when readers are not available: - Add raise_on_missing_reader parameter to LoadImage and LoadImaged - Fix guarding for class-based readers to respect the flag - Add proper stacklevel and category to all warning messages for better UX - Update LoadImage docstring with Raises section and accepted reader types - Add comprehensive test coverage for the new functionality - Apply code formatting with black, isort, and ruff - Maintain backward compatibility (flag defaults to False) The flag allows users to choose between strict error handling or fallback behavior when specified readers cannot be found or their dependencies are missing. Signed-off-by: Sarah <[email protected]>
its-serah
force-pushed
the
fix-loadimage-reader-exception
branch
from
d2b11d3 to
e49f04c
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
monai/transforms/io/array.py (1)
225-238: Capture ValueError for exception chaining.Line 227 should capture the exception to properly chain it when re-raising (Ruff B904).
if the_reader is None: try: the_reader = look_up_option(_r.lower(), SUPPORTED_READERS) - except ValueError: + except ValueError as err: # If the reader name is not recognized at all, raise OptionalImportError msg = f"Cannot find reader '{_r}'. It may not be installed or recognized." if self.raise_on_missing_reader: - raise OptionalImportError(msg) + raise OptionalImportError(msg) from err else: warnings.warn( f"{msg} Will use fallback readers if available.", category=UserWarning, stacklevel=2, ) continue
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/io/array.py(5 hunks)tests/transforms/test_load_image.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/transforms/test_load_image.pymonai/transforms/io/array.py
🪛 Ruff (0.14.3)
monai/transforms/io/array.py
231-231: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
🔇 Additional comments (5)
tests/transforms/test_load_image.py (1)
451-473: Well-structured flag behavior test.The test properly validates both flag states: exception when True, fallback with warning when False. Good use of
warnings.catch_warningsto verify warning messages.monai/transforms/io/array.py (4)
143-143: Clean parameter addition with good defaults.The
raise_on_missing_readerparameter is properly documented and defaults toFalsefor backward compatibility. Documentation clearly explains both behaviors.Also applies to: 167-180, 201-201
239-252: Proper exception handling with chaining.OptionalImportError handling correctly chains the original exception and sets
stacklevel=2for warnings.
253-259: Good stacklevel addition.Adding
stacklevel=2ensures warnings point to user code rather than library internals.
260-279: Consistent class-based reader handling.Exception handling for class-based readers mirrors the string-based pattern with proper chaining and stacklevel configuration.
The test_reader_not_installed_exception test was expecting OptionalImportError for a NonExistentReader without setting raise_on_missing_reader=True. Since the flag defaults to False, this should warn and continue with fallback readers instead of raising during construction. Updated test to explicitly set raise_on_missing_reader=True to match the expected exception behavior. Signed-off-by: Sarah <[email protected]>
|
PR Updated - Ready for Review! Added raise_on_missing_reader flag to LoadImage/LoadImaged. All CodeRabbit feedback addressed, tests passing, backward compatible. Defaults to False for graceful fallback behavior. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
tests/transforms/test_load_image.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/transforms/test_load_image.py
🔇 Additional comments (2)
tests/transforms/test_load_image.py (2)
18-18: LGTM!Import additions support the new test cases for reader exception handling.
Also applies to: 32-32
451-472: LGTM!Test properly validates
raise_on_missing_readerflag behavior: raises exception with unknown reader when True (line 455), warns and creates loader when False (line 461), and works correctly with valid readers (lines 467-472).
The OptionalImportError is raised during LoadImage.__init__ construction when the
reader cannot be resolved, so the ('test') call never executes. Removing it to
clarify the test and match the construction-only approach used in other tests.
Addresses CodeRabbit feedback.
Signed-off-by: Sarah <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/transforms/test_load_image.py (1)
467-472: Consider removing redundant valid-reader checks.Testing that
LoadImageconstructs successfully with a valid reader (ITKReader) doesn't meaningfully verify theraise_on_missing_readerflag behavior. Both flag values should succeed with valid readers. Consider removing these lines or testing a more relevant scenario (e.g., behavior when ITKReader's dependencies are missing).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
tests/transforms/test_load_image.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/transforms/test_load_image.py
🔇 Additional comments (2)
tests/transforms/test_load_image.py (2)
18-18: LGTM!Required imports for the new test functionality.
Also applies to: 32-32
451-464: LGTM! Core flag behavior is well tested.The test correctly verifies both True (raises) and False (warns) behaviors for unknown readers.
| def test_reader_not_installed_exception(self): | ||
| """test if an exception is raised when a specified reader is not installed""" | ||
| with self.assertRaises(OptionalImportError): | ||
| LoadImage(image_only=True, reader="NonExistentReader", raise_on_missing_reader=True) | ||
| for item in (_MiniReader, _MiniReader(is_compatible=False)): | ||
| out = LoadImage(image_only=True, reader=item)("test") | ||
| self.assertEqual(out.meta["name"], "my test") | ||
| out = LoadImage(image_only=True)("test", reader=_MiniReader(is_compatible=False)) | ||
| self.assertEqual(out.meta["name"], "my test") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Separate unrelated test logic.
Lines 443-444 test the new raise_on_missing_reader flag, but lines 445-449 test _MiniReader functionality—two unrelated concerns in one test method. Move lines 445-449 to test_my_reader or a separate method.
Apply this diff:
def test_reader_not_installed_exception(self):
"""test if an exception is raised when a specified reader is not installed"""
with self.assertRaises(OptionalImportError):
LoadImage(image_only=True, reader="NonExistentReader", raise_on_missing_reader=True)
- for item in (_MiniReader, _MiniReader(is_compatible=False)):
- out = LoadImage(image_only=True, reader=item)("test")
- self.assertEqual(out.meta["name"], "my test")
- out = LoadImage(image_only=True)("test", reader=_MiniReader(is_compatible=False))
- self.assertEqual(out.meta["name"], "my test")If lines 445-449 are needed, merge them into test_my_reader at lines 434-439.
📝 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.
| def test_reader_not_installed_exception(self): | |
| """test if an exception is raised when a specified reader is not installed""" | |
| with self.assertRaises(OptionalImportError): | |
| LoadImage(image_only=True, reader="NonExistentReader", raise_on_missing_reader=True) | |
| for item in (_MiniReader, _MiniReader(is_compatible=False)): | |
| out = LoadImage(image_only=True, reader=item)("test") | |
| self.assertEqual(out.meta["name"], "my test") | |
| out = LoadImage(image_only=True)("test", reader=_MiniReader(is_compatible=False)) | |
| self.assertEqual(out.meta["name"], "my test") | |
| def test_reader_not_installed_exception(self): | |
| """test if an exception is raised when a specified reader is not installed""" | |
| with self.assertRaises(OptionalImportError): | |
| LoadImage(image_only=True, reader="NonExistentReader", raise_on_missing_reader=True) |
🤖 Prompt for AI Agents
In tests/transforms/test_load_image.py around lines 441 to 449, the
test_reader_not_installed_exception mixes two concerns: lines 443-444 verify
raise_on_missing_reader behavior while lines 445-449 test _MiniReader behavior;
separate them by removing lines 445-449 from this method and appending those
assertions into the test_my_reader function (merge them into test_my_reader at
lines 434-439) so each test covers a single concern.
3 participants
Description
This PR fixes issue #7437 by making LoadImage raise an OptionalImportError when a specified reader is not installed, instead of silently falling back to another reader.
Changes
LoadImage.__init__to catch ValueError fromlook_up_optionwhen reader name is not recognizedOptionalImportErrorinstead of just warning when specified reader is not installedWhy this is needed
Previously, when a user specified
LoadImage(reader='ITKReader')without ITK installed, it would just warn and use PILReader instead. This could lead to confusion and unexpected behavior. Now it properly raises an OptionalImportError to make it clear that the requested reader is not available.Fixes #7437