Conversation
- Removes hardcoded dtype exclusion list from pad_nd() - Enables torch.nn.functional.pad for bool and integer dtypes - Try-except fallback to numpy still handles unsupported combinations - Fixes Project-MONAI#7842
📝 WalkthroughWalkthroughTwo files modified. In Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monai/handlers/tensorboard_handlers.py (2)
41-49:⚠️ Potential issue | 🟡 MinorMissing
Raisessection in the class docstring.The
__init__now raisesRuntimeErrorwhen tensorboard isn't installed, but the docstring doesn't document it.📝 Proposed addition
Args: summary_writer: user can specify TensorBoard or TensorBoardX SummaryWriter, default to create a new TensorBoard writer. log_dir: if using default SummaryWriter, write logs to this directory, default is `./runs`. + Raises: + RuntimeError: When ``summary_writer`` is ``None`` and the ``tensorboard`` package is not installed. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/handlers/tensorboard_handlers.py` around lines 41 - 49, Add a "Raises" section to the class docstring (above the __init__) documenting that __init__ may raise RuntimeError when TensorBoard/TensorBoardX is not installed; mention the condition (failure to import or create the SummaryWriter) and the exception type RuntimeError so callers know to handle it, and keep the existing parameter descriptions for summary_writer and log_dir intact.
34-34:⚠️ Potential issue | 🔴 CriticalThe
SummaryWriter is Noneguard is dead code.
optional_importreturns a_LazyRaisestub on import failure, neverNone. The boolean flag is discarded at line 34, so the condition at line 53 always evaluates to False and theRuntimeErroris never raised.Preserve the availability flag and check it instead:
Fix
- SummaryWriter, _ = optional_import("torch.utils.tensorboard", name="SummaryWriter") + SummaryWriter, _sw_available = optional_import("torch.utils.tensorboard", name="SummaryWriter")- if SummaryWriter is None: + if not _sw_available:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/handlers/tensorboard_handlers.py` at line 34, The guard for SummaryWriter is dead because optional_import's availability flag was discarded; change the assignment to capture the availability boolean (e.g., SummaryWriter, has_tb = optional_import("torch.utils.tensorboard", name="SummaryWriter")) and then use that flag in the existing runtime check (replace the current SummaryWriter is None check with if not has_tb: raise RuntimeError(...)) so the RuntimeError is raised when tensorboard is unavailable; reference SummaryWriter and optional_import to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@monai/handlers/tensorboard_handlers.py`:
- Around line 41-49: Add a "Raises" section to the class docstring (above the
__init__) documenting that __init__ may raise RuntimeError when
TensorBoard/TensorBoardX is not installed; mention the condition (failure to
import or create the SummaryWriter) and the exception type RuntimeError so
callers know to handle it, and keep the existing parameter descriptions for
summary_writer and log_dir intact.
- Line 34: The guard for SummaryWriter is dead because optional_import's
availability flag was discarded; change the assignment to capture the
availability boolean (e.g., SummaryWriter, has_tb =
optional_import("torch.utils.tensorboard", name="SummaryWriter")) and then use
that flag in the existing runtime check (replace the current SummaryWriter is
None check with if not has_tb: raise RuntimeError(...)) so the RuntimeError is
raised when tensorboard is unavailable; reference SummaryWriter and
optional_import to locate the change.
- Checks if tensorboard is installed before attempting to create SummaryWriter - Provides installation instructions when tensorboard is missing - Improves user experience for debugging missing dependencies - Addresses issue Project-MONAI#7980 Signed-off-by: skdas20 <skdas5405@gmail.com>
skdas20
force-pushed
the
improve/writer-error-messages
branch
from
8fdad6d to
58e774d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/handlers/tensorboard_handlers.py (1)
40-57:⚠️ Potential issue | 🟡 MinorAdd a
Raises:section to the docstring.The new
RuntimeErroris undocumented. Per coding guidelines, all raised exceptions must appear in a Google-styleRaises:section.📝 Proposed docstring update
""" Base class for the handlers to write data into TensorBoard. Args: summary_writer: user can specify TensorBoard or TensorBoardX SummaryWriter, default to create a new TensorBoard writer. log_dir: if using default SummaryWriter, write logs to this directory, default is `./runs`. + Raises: + RuntimeError: When ``summary_writer`` is ``None`` and tensorboard is not installed. """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/handlers/tensorboard_handlers.py` around lines 40 - 57, Update the TensorBoardHandler class docstring to include a Google-style "Raises:" section documenting the RuntimeError thrown in __init__; specifically state that RuntimeError is raised if tensorboard is not installed (i.e., SummaryWriter is None) and include the condition and brief message text (e.g., instructing to install tensorboard via pip) so readers of TensorBoardHandler and its __init__ know the error and when it occurs.
🤖 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/handlers/tensorboard_handlers.py`:
- Around line 53-57: Add a unit test that ensures the new RuntimeError path in
tensorboard_handlers.py is covered: patch or monkeypatch the module-level flag
_tb_available to False and then import/instantiate TensorBoardHandler (or call
the code path that references SummaryWriter) and assert it raises a RuntimeError
with the exact message "TensorBoardHandler requires tensorboard to be installed.
Please install it with: pip install tensorboard"; reference the module symbol
names _tb_available, TensorBoardHandler and SummaryWriter when locating the code
to patch and the call to trigger the exception.
- Around line 53-57: The guard checking "if SummaryWriter is None" is
unreachable because optional_import returns a (module, flag) pair; update the
import call that produces SummaryWriter to capture both the symbol and the
availability flag (e.g., SummaryWriter, _tb_available = optional_import(...)),
then change the runtime check to "if not _tb_available: raise RuntimeError(...)"
in the TensorBoardHandler initializer; also update the TensorBoardHandler
docstring to include a "Raises: RuntimeError" entry describing this error and
add a unit test that simulates tensorboard missing to assert the RuntimeError is
raised.
---
Outside diff comments:
In `@monai/handlers/tensorboard_handlers.py`:
- Around line 40-57: Update the TensorBoardHandler class docstring to include a
Google-style "Raises:" section documenting the RuntimeError thrown in __init__;
specifically state that RuntimeError is raised if tensorboard is not installed
(i.e., SummaryWriter is None) and include the condition and brief message text
(e.g., instructing to install tensorboard via pip) so readers of
TensorBoardHandler and its __init__ know the error and when it occurs.
| if SummaryWriter is None: | ||
| raise RuntimeError( | ||
| "TensorBoardHandler requires tensorboard to be installed. " | ||
| "Please install it with: pip install tensorboard" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
No test covers the new RuntimeError path.
The guard is the sole new behavior in this PR; without a test, a regression (or the current dead-code bug) could go unnoticed. Add a unit test that mocks _tb_available as False and asserts the RuntimeError with the expected message is raised.
As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 54-57: 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/handlers/tensorboard_handlers.py` around lines 53 - 57, Add a unit test
that ensures the new RuntimeError path in tensorboard_handlers.py is covered:
patch or monkeypatch the module-level flag _tb_available to False and then
import/instantiate TensorBoardHandler (or call the code path that references
SummaryWriter) and assert it raises a RuntimeError with the exact message
"TensorBoardHandler requires tensorboard to be installed. Please install it
with: pip install tensorboard"; reference the module symbol names _tb_available,
TensorBoardHandler and SummaryWriter when locating the code to patch and the
call to trigger the exception.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the optional_import function
fd optional_import --type pyRepository: Project-MONAI/MONAI
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Look for the optional_import definition
rg -n "def optional_import" --type py -A 20Repository: Project-MONAI/MONAI
Length of output: 1539
🏁 Script executed:
#!/bin/bash
# Examine the full context of optional_import return behavior
find . -name "*.py" -path "*utils*" | grep -E "(module|optional)" | head -20Repository: Project-MONAI/MONAI
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Read the specific file mentioned in the review
cat -n monai/handlers/tensorboard_handlers.py | head -100Repository: Project-MONAI/MONAI
Length of output: 4740
🏁 Script executed:
#!/bin/bash
# Read the full optional_import implementation
sed -n '315,400p' monai/utils/module.pyRepository: Project-MONAI/MONAI
Length of output: 4194
🏁 Script executed:
#!/bin/bash
# Check for _LazyRaise class definition
rg -n "_LazyRaise" monai/utils/module.py -A 10 -B 2 | head -50Repository: Project-MONAI/MONAI
Length of output: 997
🏁 Script executed:
#!/bin/bash
# Check TensorBoardHandler docstring for Raises section
sed -n '40,63p' monai/handlers/tensorboard_handlers.pyRepository: Project-MONAI/MONAI
Length of output: 1005
🏁 Script executed:
#!/bin/bash
# Check test file for TensorBoardHandler
fd test_tensorboard -type fRepository: Project-MONAI/MONAI
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Search for tests of TensorBoardHandler
rg -n "TensorBoardHandler" tests/ --type f -l | head -5Repository: Project-MONAI/MONAI
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check if there are tests that verify the RuntimeError is raised
rg -n "RuntimeError" monai/handlers/tensorboard_handlers.py tests/Repository: Project-MONAI/MONAI
Length of output: 6031
The SummaryWriter is None guard is dead code and will never execute.
optional_import returns a _LazyRaise() instance on failure, not None. Since the boolean flag indicating success/failure is discarded on line 34, there's no way to check import availability—the guard always evaluates to False, and the intended RuntimeError path is unreachable. The original OptionalImportError will surface instead.
Fix: Capture the availability flag and check it:
Proposed fix
- SummaryWriter, _ = optional_import("torch.utils.tensorboard", name="SummaryWriter")
+ SummaryWriter, _tb_available = optional_import("torch.utils.tensorboard", name="SummaryWriter") if summary_writer is None:
- if SummaryWriter is None:
+ if not _tb_available:
raise RuntimeError(
"TensorBoardHandler requires tensorboard to be installed. "
"Please install it with: pip install tensorboard"
)Also add Raises: section to TensorBoardHandler docstring documenting the RuntimeError, and add a test verifying this error path.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 54-57: 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/handlers/tensorboard_handlers.py` around lines 53 - 57, The guard
checking "if SummaryWriter is None" is unreachable because optional_import
returns a (module, flag) pair; update the import call that produces
SummaryWriter to capture both the symbol and the availability flag (e.g.,
SummaryWriter, _tb_available = optional_import(...)), then change the runtime
check to "if not _tb_available: raise RuntimeError(...)" in the
TensorBoardHandler initializer; also update the TensorBoardHandler docstring to
include a "Raises: RuntimeError" entry describing this error and add a unit test
that simulates tensorboard missing to assert the RuntimeError is raised.
1 participant
Added helpful error checking for missing tensorboard package
Provides installation instructions when package not found