fix: print install hint when no suitable ImageWriter found#8761
fix: print install hint when no suitable ImageWriter found#8761yashnaiduu wants to merge 5 commits intoProject-MONAI:devfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a new public mapping Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/data/image_writer.py (1)
120-132: Consider moving_INSTALL_HINTSto module level.Defining the dict inside the conditional recreates it on each error. Moving it to module scope improves clarity and avoids repeated allocation (albeit minor since it's only on the error path).
Proposed refactor
At module level (e.g., after line 64):
# Install hints for common image formats _INSTALL_HINTS: dict[str, str] = { "nii": "nibabel", "nii.gz": "nibabel", "mha": "itk", "mhd": "itk", "nrrd": "itk", "png": "pillow", "jpg": "pillow", "jpeg": "pillow", "tif": "pillow", "tiff": "pillow", "bmp": "pillow", }Then in the function:
if not avail_writers and error_if_not_found: - # install hints — user ko guess nahi karna padega - _INSTALL_HINTS: dict = { - "nii": "nibabel", - "nii.gz": "nibabel", - "mha": "itk", - "mhd": "itk", - "nrrd": "itk", - "png": "pillow", - "jpg": "pillow", - "jpeg": "pillow", - "tif": "pillow", - "tiff": "pillow", - "bmp": "pillow", - } hint = _INSTALL_HINTS.get(fmt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/data/image_writer.py` around lines 120 - 132, The _INSTALL_HINTS dict is currently created inside the error-handling branch causing repeated recreation; move the _INSTALL_HINTS mapping to module scope (as a constant) so it is allocated once and can be reused, update its type annotation to dict[str, str] (or Mapping[str, str]) and remove the inline definition from the conditional, then import/reference the module-level _INSTALL_HINTS from the code that previously built it (e.g., the image-writing function/exception-handling block) so the error message assembly uses the shared constant.
🤖 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/data/image_writer.py`:
- Line 119: Replace the Hindi inline comment "# install hints — user ko guess
nahi karna padega" with an English equivalent for clarity and maintainability
(for example: "# installation hints — so the user doesn't have to guess").
Locate this comment in monai/data/image_writer.py (search for the exact Hindi
text) and update it to the English phrasing while preserving surrounding code
and punctuation.
---
Nitpick comments:
In `@monai/data/image_writer.py`:
- Around line 120-132: The _INSTALL_HINTS dict is currently created inside the
error-handling branch causing repeated recreation; move the _INSTALL_HINTS
mapping to module scope (as a constant) so it is allocated once and can be
reused, update its type annotation to dict[str, str] (or Mapping[str, str]) and
remove the inline definition from the conditional, then import/reference the
module-level _INSTALL_HINTS from the code that previously built it (e.g., the
image-writing function/exception-handling block) so the error message assembly
uses the shared constant.
ℹ️ Review info
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)
monai/data/image_writer.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/data/image_writer.py (1)
120-133: Move install-hint mapping to module scope.
_INSTALL_HINTSis recreated on every miss. Making it a module-level constant improves maintainability and avoids repeated allocation.Proposed refactor
+INSTALL_HINTS: Mapping[str, str] = { + "nii": "nibabel", + "nii.gz": "nibabel", + "mha": "itk", + "mhd": "itk", + "nrrd": "itk", + "png": "pillow", + "jpg": "pillow", + "jpeg": "pillow", + "tif": "pillow", + "tiff": "pillow", + "bmp": "pillow", +} + def resolve_writer(ext_name, error_if_not_found=True) -> Sequence: @@ if not avail_writers and error_if_not_found: - _INSTALL_HINTS: dict = { - "nii": "nibabel", - "nii.gz": "nibabel", - "mha": "itk", - "mhd": "itk", - "nrrd": "itk", - "png": "pillow", - "jpg": "pillow", - "jpeg": "pillow", - "tif": "pillow", - "tiff": "pillow", - "bmp": "pillow", - } - hint = _INSTALL_HINTS.get(fmt) + hint = INSTALL_HINTS.get(fmt) extra = f" Try installing the required package: pip install {hint}" if hint else "" raise OptionalImportError(f"No ImageWriter backend found for '{fmt}'.{extra}")As per coding guidelines, "Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/data/image_writer.py` around lines 120 - 133, The _INSTALL_HINTS dict is being recreated each time the code path runs; move it to module scope as a constant (e.g., INSTALL_HINTS) and change the lookup in the function to use INSTALL_HINTS.get(fmt) (keep the variable name hint and the fmt parameter as-is) so the mapping is allocated once and the function simply performs a lookup, improving maintainability and avoiding repeated allocations.
🤖 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/data/image_writer.py`:
- Around line 120-133: The _INSTALL_HINTS dict is being recreated each time the
code path runs; move it to module scope as a constant (e.g., INSTALL_HINTS) and
change the lookup in the function to use INSTALL_HINTS.get(fmt) (keep the
variable name hint and the fmt parameter as-is) so the mapping is allocated once
and the function simply performs a lookup, improving maintainability and
avoiding repeated allocations.
ℹ️ Review info
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)
monai/data/image_writer.py
When resolve_writer() fails to find a backend for a given file extension, the error was generic. Added a format-to-package lookup that appends an install hint (e.g. pip install nibabel) for common formats. Fixes Project-MONAI#7980 Signed-off-by: yashnaiduu <yashnaiduu@users.noreply.github.com>
yashnaiduu
force-pushed
the
fix/helpful-writer-missing-error-msg
branch
from
77273f4 to
b809c77
Compare
Signed-off-by: yashnaiduu <yashnaiduu@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/data/image_writer.py (1)
119-132: Avoid extension list drift betweeninit()andresolve_writer().The extension-to-format knowledge is duplicated in two places. Please centralize it in one module-level mapping/constant and reuse it in both paths.
Proposed refactor
+EXT_TO_INSTALL_HINT: dict[str, str] = { + "nii": "nibabel", + "nii.gz": "nibabel", + "mha": "itk", + "mhd": "itk", + "nrrd": "itk", + "png": "pillow", + "jpg": "pillow", + "jpeg": "pillow", + "tif": "pillow", + "tiff": "pillow", + "bmp": "pillow", +} ... - install_hints: dict = { - "nii": "nibabel", - "nii.gz": "nibabel", - "mha": "itk", - "mhd": "itk", - "nrrd": "itk", - "png": "pillow", - "jpg": "pillow", - "jpeg": "pillow", - "tif": "pillow", - "tiff": "pillow", - "bmp": "pillow", - } - hint = install_hints.get(fmt) + hint = EXT_TO_INSTALL_HINT.get(fmt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/data/image_writer.py` around lines 119 - 132, The extension-to-format/package mapping is duplicated between init() and resolve_writer(); extract the dict into a single module-level constant (e.g. EXTENSION_INSTALL_HINTS or EXTENSION_MAP) and have both init() and resolve_writer() reference that constant instead of defining their own mapping; update any variable names used in init() and resolve_writer() to use the shared symbol and remove the local duplicate dict so both paths always use the same source of truth.
🤖 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/data/image_writer.py`:
- Around line 119-132: The extension-to-format/package mapping is duplicated
between init() and resolve_writer(); extract the dict into a single module-level
constant (e.g. EXTENSION_INSTALL_HINTS or EXTENSION_MAP) and have both init()
and resolve_writer() reference that constant instead of defining their own
mapping; update any variable names used in init() and resolve_writer() to use
the shared symbol and remove the local duplicate dict so both paths always use
the same source of truth.
ℹ️ Review info
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)
monai/data/image_writer.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/data/image_writer.py (1)
119-131: Moveinstall_hintsto a module-level constant.Rebuilding this dict on every not-found path is avoidable. Defining it once near
EXT_WILDCARDwill improve maintainability and keepresolve_writerfocused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/data/image_writer.py` around lines 119 - 131, Move the local install_hints dict out of resolve_writer and declare it as a module-level constant (e.g., INSTALL_HINTS) near EXT_WILDCARD so it's constructed once at import time; then update resolve_writer to reference that constant instead of rebuilding the dict, keeping function logic unchanged and only replacing the local variable name with INSTALL_HINTS.
🤖 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/data/image_writer.py`:
- Around line 119-134: Add unit tests for the new install-hint behavior in
monai.data.image_writer: exercise the mapping stored in install_hints by calling
the ImageWriter error path (the code that raises OptionalImportError) with
formats "nii.gz", "png", "nrrd" and an unknown extension (e.g., "abc"); for each
of the first three assert the raised OptionalImportError message contains the
expected package string ("nibabel", "pillow", "itk" respectively) and for the
unknown extension assert the message does not contain a "pip install" hint;
ensure tests reference the image_writer module and OptionalImportError so they
fail if the mapping or message changes.
---
Nitpick comments:
In `@monai/data/image_writer.py`:
- Around line 119-131: Move the local install_hints dict out of resolve_writer
and declare it as a module-level constant (e.g., INSTALL_HINTS) near
EXT_WILDCARD so it's constructed once at import time; then update resolve_writer
to reference that constant instead of rebuilding the dict, keeping function
logic unchanged and only replacing the local variable name with INSTALL_HINTS.
ℹ️ Review info
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)
monai/data/image_writer.py
monai/data/image_writer.py
Outdated
| install_hints: dict = { | ||
| "nii": "nibabel", | ||
| "nii.gz": "nibabel", | ||
| "mha": "itk", | ||
| "mhd": "itk", | ||
| "nrrd": "itk", | ||
| "png": "pillow", | ||
| "jpg": "pillow", | ||
| "jpeg": "pillow", | ||
| "tif": "pillow", | ||
| "tiff": "pillow", | ||
| "bmp": "pillow", | ||
| } | ||
| hint = install_hints.get(fmt) | ||
| extra = f" Try installing the required package: pip install {hint}" if hint else "" | ||
| raise OptionalImportError(f"No ImageWriter backend found for '{fmt}'.{extra}") |
There was a problem hiding this comment.
Add regression tests for the new install-hint behavior.
Line 119 through Line 134 changes user-facing error output and format mapping, but no tests are included here. Please add cases for nii.gz -> nibabel, png -> pillow, nrrd -> itk, and unknown extensions (no hint).
As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 134-134: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/data/image_writer.py` around lines 119 - 134, Add unit tests for the
new install-hint behavior in monai.data.image_writer: exercise the mapping
stored in install_hints by calling the ImageWriter error path (the code that
raises OptionalImportError) with formats "nii.gz", "png", "nrrd" and an unknown
extension (e.g., "abc"); for each of the first three assert the raised
OptionalImportError message contains the expected package string ("nibabel",
"pillow", "itk" respectively) and for the unknown extension assert the message
does not contain a "pip install" hint; ensure tests reference the image_writer
module and OptionalImportError so they fail if the mapping or message changes.
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @yashnaiduu thanks for this update. I think it's fine as is with my suggested comment, but we should add at least one test to ensure the behaviour works. You would need only a test to check that the text in the exception message shows up to suggest a package when not installed, and doesn't when the file extension isn't known. You may need to mock the dictionary to emulate a package being missing (which you can do if it's a global variable).
monai/data/image_writer.py
Outdated
| avail_writers.append(_writer) | ||
| if not avail_writers and error_if_not_found: | ||
| raise OptionalImportError(f"No ImageWriter backend found for {fmt}.") | ||
| install_hints: dict = { |
There was a problem hiding this comment.
I would put this dictionary into a global variable at the top of this file. A name like FILETYPE_HINT or something else sticking to the all-caps convention would work. This keeps the dictionary easily found and modifiable at runtime.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
monai/data/image_writer.py (2)
134-136: Document the newOptionalImportErrorpath.
resolve_writer()now has format-specific error text, but its docstring still does not describe the raised exception. Please add aRaises:section for this branch.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/data/image_writer.py` around lines 134 - 136, Update the resolve_writer() docstring to include a "Raises:" section that documents OptionalImportError being raised when no ImageWriter backend is available for the requested fmt; mention that the exception message includes format-specific guidance (uses FILETYPE_HINT to suggest a pip install) so callers know to expect and handle OptionalImportError when a backend is missing.
66-79: Clarify whetherFILETYPE_HINTis public.This new all-caps module constant is omitted from
__all__, so its visibility is unclear. Either export it there or rename it_FILETYPE_HINTto keep the API surface explicit.As per coding guidelines, "Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/data/image_writer.py` around lines 66 - 79, The module-level constant FILETYPE_HINT is defined in all-caps but not exported via __all__, making its intended public visibility ambiguous; either add "FILETYPE_HINT" to the module's __all__ list to explicitly export it, or rename it to _FILETYPE_HINT to mark it private and update any internal references (e.g., usages in ImageWriter backends) accordingly so imports and tests remain correct; choose one approach and apply it consistently across the module (update __all__ or rename symbol and references).
🤖 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/data/image_writer.py`:
- Around line 134-136: Update the resolve_writer() docstring to include a
"Raises:" section that documents OptionalImportError being raised when no
ImageWriter backend is available for the requested fmt; mention that the
exception message includes format-specific guidance (uses FILETYPE_HINT to
suggest a pip install) so callers know to expect and handle OptionalImportError
when a backend is missing.
- Around line 66-79: The module-level constant FILETYPE_HINT is defined in
all-caps but not exported via __all__, making its intended public visibility
ambiguous; either add "FILETYPE_HINT" to the module's __all__ list to explicitly
export it, or rename it to _FILETYPE_HINT to mark it private and update any
internal references (e.g., usages in ImageWriter backends) accordingly so
imports and tests remain correct; choose one approach and apply it consistently
across the module (update __all__ or rename symbol and references).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e71e42b3-e955-447c-afd5-6d98678816ca
📒 Files selected for processing (1)
monai/data/image_writer.py
|
Thanks for the review @ericspod! I've moved the dictionary to a module-level constant |
yashnaiduu
force-pushed
the
fix/helpful-writer-missing-error-msg
branch
from
3a656a8 to
3f2a217
Compare
…l hints to module-level FILETYPE_HINT constant Signed-off-by: YASH NAIDU <152394598+yashnaiduu@users.noreply.github.com> Signed-off-by: yashnaiduu <yashnaidu@example.com>
… FILETYPE_HINT and resolve_writer error message Added tests for file type hints and error handling in image writer. Signed-off-by: YASH NAIDU <152394598+yashnaiduu@users.noreply.github.com> Signed-off-by: yashnaiduu <yashnaidu@example.com>
Signed-off-by: yashnaiduu <yashnaidu@example.com>
yashnaiduu
force-pushed
the
fix/helpful-writer-missing-error-msg
branch
from
3f2a217 to
5fc5be5
Compare
2 participants
Fixes #7980
What changed
When resolve_writer() fails to find a backend for a given file extension,
the error was generic and unhelpful:
Users had no idea which package was missing.
Fix
Added a format-to-package lookup in resolve_writer() that appends
an install hint for common formats:
.nii,.nii.gzpip install nibabel.mha,.mhd,.nrrdpip install itk.jpg,.tif,.bmppip install pillowAfter this fix
No behaviour change for users who already have the packages installed.