-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix LoadImage to raise OptionalImportError when specified reader is not available #8522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
a741d0b
3db882b
dae4264
e332430
46c84c8
a0f2484
1f642ff
5d1a48c
cbfeb28
51212cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -138,6 +138,7 @@ def __init__( | |||||||||||||||||||||||||||||||||||||
prune_meta_pattern: str | None = None, | ||||||||||||||||||||||||||||||||||||||
prune_meta_sep: str = ".", | ||||||||||||||||||||||||||||||||||||||
expanduser: bool = True, | ||||||||||||||||||||||||||||||||||||||
raise_on_missing_reader: bool = False, | ||||||||||||||||||||||||||||||||||||||
*args, | ||||||||||||||||||||||||||||||||||||||
**kwargs, | ||||||||||||||||||||||||||||||||||||||
) -> None: | ||||||||||||||||||||||||||||||||||||||
|
@@ -161,6 +162,8 @@ def __init__( | |||||||||||||||||||||||||||||||||||||
in the metadata (nested dictionary). default is ".", see also :py:class:`monai.transforms.DeleteItemsd`. | ||||||||||||||||||||||||||||||||||||||
e.g. ``prune_meta_pattern=".*_code$", prune_meta_sep=" "`` removes meta keys that ends with ``"_code"``. | ||||||||||||||||||||||||||||||||||||||
expanduser: if True cast filename to Path and call .expanduser on it, otherwise keep filename as is. | ||||||||||||||||||||||||||||||||||||||
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. | ||||||||||||||||||||||||||||||||||||||
kwargs: additional parameters for reader if providing a reader name. | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
@@ -183,6 +186,7 @@ def __init__( | |||||||||||||||||||||||||||||||||||||
self.pattern = prune_meta_pattern | ||||||||||||||||||||||||||||||||||||||
self.sep = prune_meta_sep | ||||||||||||||||||||||||||||||||||||||
self.expanduser = expanduser | ||||||||||||||||||||||||||||||||||||||
self.raise_on_missing_reader = raise_on_missing_reader | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
self.readers: list[ImageReader] = [] | ||||||||||||||||||||||||||||||||||||||
for r in SUPPORTED_READERS: # set predefined readers as default | ||||||||||||||||||||||||||||||||||||||
|
@@ -206,13 +210,24 @@ def __init__( | |||||||||||||||||||||||||||||||||||||
if not has_built_in: | ||||||||||||||||||||||||||||||||||||||
the_reader = locate(f"{_r}") # search dotted path | ||||||||||||||||||||||||||||||||||||||
if the_reader is None: | ||||||||||||||||||||||||||||||||||||||
the_reader = look_up_option(_r.lower(), SUPPORTED_READERS) | ||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||
the_reader = look_up_option(_r.lower(), SUPPORTED_READERS) | ||||||||||||||||||||||||||||||||||||||
except ValueError: | ||||||||||||||||||||||||||||||||||||||
# 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) | ||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||
warnings.warn(f"{msg} Will use fallback readers if available.") | ||||||||||||||||||||||||||||||||||||||
continue | ||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||
self.register(the_reader(*args, **kwargs)) | ||||||||||||||||||||||||||||||||||||||
except OptionalImportError: | ||||||||||||||||||||||||||||||||||||||
warnings.warn( | ||||||||||||||||||||||||||||||||||||||
f"required package for reader {_r} is not installed, or the version doesn't match requirement." | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
except OptionalImportError as e: | ||||||||||||||||||||||||||||||||||||||
msg = f"Required package for reader {_r} is not installed, or the version doesn't match requirement." | ||||||||||||||||||||||||||||||||||||||
if self.raise_on_missing_reader: | ||||||||||||||||||||||||||||||||||||||
raise OptionalImportError(msg) from e | ||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||
warnings.warn(f"{msg} Will use fallback readers if available.") | ||||||||||||||||||||||||||||||||||||||
except TypeError: # the reader doesn't have the corresponding args/kwargs | ||||||||||||||||||||||||||||||||||||||
warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.") | ||||||||||||||||||||||||||||||||||||||
self.register(the_reader()) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+225
to
233
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Also set Keep the raised error chained; just add Apply: except OptionalImportError as e:
msg = f"Required package for reader {_r} is not installed, or the version doesn't match requirement."
if self.raise_on_missing_reader:
raise OptionalImportError(msg) from e
else:
- warnings.warn(f"{msg} Will use fallback readers if available.")
+ warnings.warn(f"{msg} Will use fallback readers if available.", stacklevel=2)
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()) 📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.12.2)230-230: No explicit Set (B028) 232-232: No explicit Set (B028) 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,55 @@ | ||||||||
#!/usr/bin/env python3 | ||||||||
|
||||||||
"""Test to verify the changes to LoadImage raise_on_missing_reader flag work correctly.""" | ||||||||
|
||||||||
import warnings | ||||||||
from monai.transforms import LoadImage | ||||||||
from monai.utils import OptionalImportError | ||||||||
|
||||||||
def test_raise_on_missing_reader(): | ||||||||
"""Test the raise_on_missing_reader flag behavior.""" | ||||||||
print("Testing LoadImage raise_on_missing_reader flag...") | ||||||||
|
||||||||
# Test 1: Unknown reader with flag enabled - should raise OptionalImportError | ||||||||
print("Test 1: Unknown reader with raise_on_missing_reader=True") | ||||||||
try: | ||||||||
LoadImage(reader="UnknownReader", raise_on_missing_reader=True) | ||||||||
print("FAIL: Expected OptionalImportError but didn't get one") | ||||||||
return False | ||||||||
except OptionalImportError as e: | ||||||||
print(f"PASS: Got expected OptionalImportError: {e}") | ||||||||
|
||||||||
# Test 2: Unknown reader with flag disabled - should warn but not raise | ||||||||
print("\nTest 2: Unknown reader with raise_on_missing_reader=False") | ||||||||
try: | ||||||||
with warnings.catch_warnings(record=True) as w: | ||||||||
warnings.simplefilter("always") | ||||||||
loader = LoadImage(reader="UnknownReader", raise_on_missing_reader=False) | ||||||||
if w and "UnknownReader" in str(w[0].message): | ||||||||
print(f"PASS: Got expected warning: {w[0].message}") | ||||||||
else: | ||||||||
print("WARNING: Warning message may have been different than expected") | ||||||||
print("PASS: LoadImage instance created successfully with fallback behavior") | ||||||||
except OptionalImportError as e: | ||||||||
print(f"FAIL: Unexpected OptionalImportError with flag disabled: {e}") | ||||||||
return False | ||||||||
except Exception as e: | ||||||||
print(f"FAIL: Unexpected error: {e}") | ||||||||
return False | ||||||||
Comment on lines
+36
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Remove the blind 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
Suggested change
🧰 Tools🪛 Ruff (0.12.2)36-36: Do not catch blind exception: (BLE001) 🤖 Prompt for AI Agents
|
||||||||
|
||||||||
# Test 3: Valid reader - should work regardless of flag | ||||||||
print("\nTest 3: Valid reader (PILReader) with both flag settings") | ||||||||
try: | ||||||||
loader1 = LoadImage(reader="PILReader", raise_on_missing_reader=True) | ||||||||
loader2 = LoadImage(reader="PILReader", raise_on_missing_reader=False) | ||||||||
print("PASS: Both loaders created successfully with valid reader") | ||||||||
except Exception as e: | ||||||||
print(f"FAIL: Unexpected error with valid reader: {e}") | ||||||||
return False | ||||||||
|
||||||||
print("\nAll tests passed!") | ||||||||
return True | ||||||||
|
||||||||
if __name__ == "__main__": | ||||||||
success = test_raise_on_missing_reader() | ||||||||
exit(0 if success else 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Chain the original cause and set
stacklevel
on warnings.Use
raise ... from err
(B904) and addstacklevel=2
so warnings point at user code (B028).Apply:
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.12.2)
219-219: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
221-221: No explicit
stacklevel
keyword argument foundSet
stacklevel=2
(B028)
🤖 Prompt for AI Agents