fix: print install hint when no suitable ImageWriter found#8761
fix: print install hint when no suitable ImageWriter found#8761yashnaiduu wants to merge 2 commits intoProject-MONAI:devfrom
Conversation
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
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 — user ko guess nahi karna padega |
There was a problem hiding this comment.
Non-English comment should be replaced.
Comment is in Hindi. Use English for international OSS project maintainability.
Proposed fix
- # install hints — user ko guess nahi karna padega
+ # install hints for common formats📝 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.
| # install hints — user ko guess nahi karna padega | |
| # install hints for common formats |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/data/image_writer.py` at 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.
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
| 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).
| 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.
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.