Raise exception for uninstalled LoadImage reader and print writer pkg…#8915
Raise exception for uninstalled LoadImage reader and print writer pkg…#8915VedhSontha wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR improves error reporting when image I/O backends fail due to missing optional dependencies. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/data/test_image_rw.py (1)
142-153: ⚡ Quick winMake the missing-backend path deterministic.
Lines 144-153 only exercise the new
OptionalImportErrormessage when bothnibabelanditkare absent. In most environments at least one is installed, so the new diagnostic path can regress without any test coverage. Isolate this branch with a temporary registry or mocked writer constructors instead of ambient package state.As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/data/test_image_rw.py` around lines 142 - 153, The current test relies on ambient package presence; make the missing-backend path deterministic by mocking or isolating the registry so resolve_writer and OptionalImportError are exercised reliably: in the test for the "else" branch, patch optional_import (or monkeypatch the module that defines optional_import) to return (_, False) for both "nibabel" and "itk", or temporarily replace the writer registry looked up by resolve_writer with an empty/mock registry, then call resolve_writer(".nrrd") and resolve_writer("unknown") to assert OptionalImportError contains "Please install: itk or nibabel."; ensure you restore the original state after the assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/transforms/io/array.py`:
- Line 217: The warning call in LoadImage.__init__ currently uses
warnings.warn(f"{_r} is not supported with the given parameters {args}
{kwargs}.") which points the stack to MONAI internals; update this call to pass
stacklevel=2 so the emitted warning points to the user's call site (i.e., change
the warnings.warn call inside the LoadImage.__init__ method to include
stacklevel=2).
In `@tests/data/test_init_reader.py`:
- Around line 47-53: TestInitLoadImage.test_load_image currently calls
optional_import(pkg_map[r]) (defaulting allow_namespace_pkg=False) to set
has_pkg, but runtime code imports ITK (and NRRD) with optional_import(...,
allow_namespace_pkg=True), causing false negatives; update the test to call
optional_import(pkg_map[r], allow_namespace_pkg=True) when probing readers for
"itk" and "nrrd" (or set allow_namespace_pkg=True for those pkg_map entries) so
has_pkg matches the runtime import behavior used by LoadImaged and the
OptionalImportError expectation is correct.
---
Nitpick comments:
In `@tests/data/test_image_rw.py`:
- Around line 142-153: The current test relies on ambient package presence; make
the missing-backend path deterministic by mocking or isolating the registry so
resolve_writer and OptionalImportError are exercised reliably: in the test for
the "else" branch, patch optional_import (or monkeypatch the module that defines
optional_import) to return (_, False) for both "nibabel" and "itk", or
temporarily replace the writer registry looked up by resolve_writer with an
empty/mock registry, then call resolve_writer(".nrrd") and
resolve_writer("unknown") to assert OptionalImportError contains "Please
install: itk or nibabel."; ensure you restore the original state after the
assertions.
🪄 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: 056e8222-bb33-4ffe-899d-a967383763a4
📒 Files selected for processing (4)
monai/data/image_writer.pymonai/transforms/io/array.pytests/data/test_image_rw.pytests/data/test_init_reader.py
| ) | ||
| ) from e | ||
| except TypeError: # the reader doesn't have the corresponding args/kwargs | ||
| warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and show the relevant section around line 217
ls -la monai/transforms/io/array.py
python3 - <<'PY'
import itertools, pathlib
p = pathlib.Path("monai/transforms/io/array.py")
lines = p.read_text().splitlines()
start, end = 200, 240
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PY
# Find all warnings.warn occurrences in this file
rg -n "warnings\.warn\s*\(" monai/transforms/io/array.pyRepository: Project-MONAI/MONAI
Length of output: 2624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
p = pathlib.Path("monai/transforms/io/array.py")
lines = p.read_text().splitlines()
# show more context to identify surrounding def/class
for i in range(150, 230):
if i < 0 or i >= len(lines):
continue
print(f"{i+1:4d}: {lines[i]}")
PYRepository: Project-MONAI/MONAI
Length of output: 5311
Add stacklevel=2 to the warning in LoadImage.__init__.
monai/transforms/io/array.py line 217 uses warnings.warn(...) without stacklevel, so the warning location points at MONAI internals instead of the user call site.
Suggested change
- 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,
+ )🧰 Tools
🪛 Ruff (0.15.15)
[warning] 217-217: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@monai/transforms/io/array.py` at line 217, The warning call in
LoadImage.__init__ currently uses warnings.warn(f"{_r} is not supported with the
given parameters {args} {kwargs}.") which points the stack to MONAI internals;
update this call to pass stacklevel=2 so the emitted warning points to the
user's call site (i.e., change the warnings.warn call inside the
LoadImage.__init__ method to include stacklevel=2).
Source: Linters/SAST tools
| _, has_pkg = optional_import(pkg_map[r]) | ||
| if has_pkg: | ||
| inst = LoadImaged("image", reader=r) | ||
| self.assertIsInstance(inst, LoadImaged) | ||
| else: | ||
| with self.assertRaises(OptionalImportError): | ||
| LoadImaged("image", reader=r) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=py 'optional_import\("itk"' -C2Repository: Project-MONAI/MONAI
Length of output: 5327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact code under review
sed -n '1,140p' tests/data/test_init_reader.py | nl -ba | sed -n '35,90p'
# Find the pkg_map definition / ITKReader mapping and the exact optional_import call site
rg -n "pkg_map|ITKReader|LoadImaged|test_load_image|optional_import" tests/data/test_init_reader.py -n
# Inspect optional_import signature/docs in repo
optional_import_file="$(rg -n --type=py "def optional_import" -S . | head -n 1 | cut -d: -f1)"
echo "Found optional_import definition in: $optional_import_file"
sed -n '1,200p' "$optional_import_file" | nl -ba | sed -n '1,200p'Repository: Project-MONAI/MONAI
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show reviewed section with line numbers (no `nl` in this sandbox)
sed -n '1,120p' tests/data/test_init_reader.py | cat -n | sed -n '35,90p'
# Locate the exact test and the ITK probe line
rg -n "TestInitLoadImage|test_load_image|pkg_map|optional_import|ITKReader|LoadImaged" tests/data/test_init_reader.py
# Find optional_import implementation and show its signature/defaults
opt_file="$(rg -n --type=py "def optional_import" -S . | head -n 1 | cut -d: -f1)"
echo "optional_import definition: $opt_file"
sed -n '1,220p' "$opt_file" | cat -n | sed -n '1,220p'Repository: Project-MONAI/MONAI
Length of output: 13955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get line number of optional_import definition
opt_def_line="$(rg -n --type=py "^def optional_import\\b" monai/utils/module.py | head -n 1 | cut -d: -f1)"
echo "optional_import def line: $opt_def_line"
# Print a window around the function start (signature + docstring header)
start=$((opt_def_line-20))
end=$((opt_def_line+120))
sed -n "${start},${end}p" monai/utils/module.py | cat -n
# Also find any occurrences of allow_namespace_pkg inside the function/file
rg -n "allow_namespace_pkg" monai/utils/module.py | head -n 50Repository: Project-MONAI/MONAI
Length of output: 7091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where ITKReader is resolved/used during LoadImaged/LoadImage
rg -n "ITKReader" monai -S
rg -n "LoadImaged\\(|LoadImage\\(" monai/transforms -S
rg -n "reader=.*ITKReader|resolve_reader|resolve_reader\\(|image_reader" monai -S
# Inspect relevant reader resolution code (likely monai/data/image_reader.py and monai/data/__init__.py / transforms loader)
sed -n '1,220p' monai/data/image_reader.py | cat -n
# Also inspect image_writer if needed (for writer mapping consistency)
sed -n '1,140p' monai/data/image_writer.py | cat -nRepository: Project-MONAI/MONAI
Length of output: 21170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=py "^def require_pkg\\b" monai/utils/module.py
req_file="monai/utils/module.py"
req_line="$(rg -n --type=py "^def require_pkg\\b" "$req_file" | head -n 1 | cut -d: -f1)"
echo "require_pkg def line: $req_line"
start=$((req_line-20))
end=$((req_line+220))
sed -n "${start},${end}p" "$req_file" | cat -n
# See whether allow_namespace_pkg is referenced inside require_pkg
rg -n "allow_namespace_pkg" "$req_file" | head -n 50Repository: Project-MONAI/MONAI
Length of output: 10172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "has_itk|\\bitk\\b" monai/data/image_reader.pyRepository: Project-MONAI/MONAI
Length of output: 1442
Handle ITK as a namespace package in TestInitLoadImage.test_load_image
tests/data/test_init_reader.py:47 probes ITK with optional_import(pkg_map[r]) (defaults allow_namespace_pkg=False), but runtime monai/data/image_reader.py loads ITK with optional_import("itk", allow_namespace_pkg=True, so namespace-style ITK installs can make has_pkg false and cause a false OptionalImportError expectation.
- Same mismatch likely applies to
nrrd(runtime also usesallow_namespace_pkg=Truethere, but this test doesn’t special-case it).
Suggested change
- _, has_pkg = optional_import(pkg_map[r])
+ import_kwargs = {"allow_namespace_pkg": True} if pkg_map[r] == "itk" else {}
+ _, has_pkg = optional_import(pkg_map[r], **import_kwargs)📝 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.
| _, has_pkg = optional_import(pkg_map[r]) | |
| if has_pkg: | |
| inst = LoadImaged("image", reader=r) | |
| self.assertIsInstance(inst, LoadImaged) | |
| else: | |
| with self.assertRaises(OptionalImportError): | |
| LoadImaged("image", reader=r) | |
| import_kwargs = {"allow_namespace_pkg": True} if pkg_map[r] == "itk" else {} | |
| _, has_pkg = optional_import(pkg_map[r], **import_kwargs) | |
| if has_pkg: | |
| inst = LoadImaged("image", reader=r) | |
| self.assertIsInstance(inst, LoadImaged) | |
| else: | |
| with self.assertRaises(OptionalImportError): | |
| LoadImaged("image", reader=r) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/data/test_init_reader.py` around lines 47 - 53,
TestInitLoadImage.test_load_image currently calls optional_import(pkg_map[r])
(defaulting allow_namespace_pkg=False) to set has_pkg, but runtime code imports
ITK (and NRRD) with optional_import(..., allow_namespace_pkg=True), causing
false negatives; update the test to call optional_import(pkg_map[r],
allow_namespace_pkg=True) when probing readers for "itk" and "nrrd" (or set
allow_namespace_pkg=True for those pkg_map entries) so has_pkg matches the
runtime import behavior used by LoadImaged and the OptionalImportError
expectation is correct.
1 participant
… suggestion
Fixes # .
Description
A few sentences describing the changes proposed in this pull request.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.