Fix: handle precomputed crops and safe no-grad cleanup in auto3dseg#8803
Fix: handle precomputed crops and safe no-grad cleanup in auto3dseg#8803bluehyena wants to merge 3 commits intoProject-MONAI:devfrom
Conversation
Signed-off-by: Jun Hyeok Lee <bluehyena123@naver.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
🧹 Nitpick comments (2)
monai/auto3dseg/analyzer.py (2)
236-252: Misplaced docstring.The docstring block is placed after the validation code. Move it to immediately follow the
def __call__(self, data):line for proper function documentation.Proposed fix
The docstring (lines 236-252) should be moved to right after line 219 (
def __call__(self, data):), before the validation code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/auto3dseg/analyzer.py` around lines 236 - 252, The function-level docstring for __call__ is misplaced after validation; move the entire docstring block currently referencing ImageStatsKeys, SampleOperations, and report_format to immediately follow the def __call__(self, data): signature so it becomes the function's official docstring (before any validation or logic), preserving the existing text and formatting.
271-273: Considerstrict=Truefor zip.Adding
strict=Truecatches length mismatches early if SHAPE and SPACING ever diverge unexpectedly.- report[ImageStatsKeys.SIZEMM] = [ - a * b for a, b in zip(report[ImageStatsKeys.SHAPE][0], report[ImageStatsKeys.SPACING]) - ] + report[ImageStatsKeys.SIZEMM] = [ + a * b for a, b in zip(report[ImageStatsKeys.SHAPE][0], report[ImageStatsKeys.SPACING], strict=True) + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/auto3dseg/analyzer.py` around lines 271 - 273, The current computation of ImageStatsKeys.SIZEMM uses zip(report[ImageStatsKeys.SHAPE][0], report[ImageStatsKeys.SPACING]) which can silently truncate if lengths differ; update the comprehension in analyzer.py to use zip(..., strict=True) so mismatched lengths raise immediately (ensure environment uses Python 3.10+); reference the report dict keys ImageStatsKeys.SHAPE, ImageStatsKeys.SPACING and the target ImageStatsKeys.SIZEMM when making this change.
🤖 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/auto3dseg/analyzer.py`:
- Around line 236-252: The function-level docstring for __call__ is misplaced
after validation; move the entire docstring block currently referencing
ImageStatsKeys, SampleOperations, and report_format to immediately follow the
def __call__(self, data): signature so it becomes the function's official
docstring (before any validation or logic), preserving the existing text and
formatting.
- Around line 271-273: The current computation of ImageStatsKeys.SIZEMM uses
zip(report[ImageStatsKeys.SHAPE][0], report[ImageStatsKeys.SPACING]) which can
silently truncate if lengths differ; update the comprehension in analyzer.py to
use zip(..., strict=True) so mismatched lengths raise immediately (ensure
environment uses Python 3.10+); reference the report dict keys
ImageStatsKeys.SHAPE, ImageStatsKeys.SPACING and the target
ImageStatsKeys.SIZEMM when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb6ef620-5777-4a34-a6c5-5372ddb90b86
📒 Files selected for processing (2)
monai/auto3dseg/analyzer.pytests/apps/test_auto3dseg.py
|
I noticed a couple of technical differences worth flagging between this approach and the fixes in #8809 and #8810. On nda_croppeds handling: The ternary here has no validation on the pre-computed value , if a caller passes a wrong type or a list with the wrong number of entries, the error surfaces downstream as a confusing shape mismatch rather than a clear message. #8809 adds explicit type and length validation with a descriptive ValueError. On grad state: torch.no_grad() always restores grad to enabled after the block, regardless of what state the caller was in before. If the caller had grad disabled before calling the analyzer, this silently re-enables it , which is a subtle semantic change. The try/finally pattern in #8810 uses torch.is_grad_enabled() to capture and restore the exact pre-call state, so both enabled→enabled and disabled→disabled are handled correctly. |
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @bluehyena thanks for looking into this. I have minor comments to make, we should also look at @williams145 's PRs though I commented in there that using no_grad should work correctly and so I'm waiting to hear back from them.
Signed-off-by: Jun Hyeok Lee <bluehyena123@naver.com>
bluehyena
force-pushed
the
fix-auto3dseg-analyzer-bugs
branch
from
d9a2bc4 to
c921e78
Compare
|
Hi @ericspod thanks for the review! I moved the I also checked the current PyTorch behavior and local tests to make sure this works as intended: In addition, I incorporated the |
Signed-off-by: Jun Hyeok Lee <bluehyena123@naver.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/auto3dseg/analyzer.py (1)
221-242: AddArgs:fordatain the method docstring.The docstring has
Returns/Raises, but noArgssection for the input payload.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/auto3dseg/analyzer.py` around lines 221 - 242, Add a Google-style "Args:" section to the method docstring that documents the input parameter `data` (the payload) used by this callable (the method that references self.image_key and self.report_format); state that `data` must be a dict containing the image under the key `self.image_key` (value must be numpy.ndarray, torch.Tensor, or MetaTensor), optionally `nda_croppeds` as a list/tuple with one entry per image channel when precomputed, and any other expected keys used by the method (e.g., entries that produce ImageStatsKeys.INTENSITY); include types and brief descriptions and mention that missing/invalid types will raise KeyError/TypeError/ValueError as already listed in Raises.
🤖 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/auto3dseg/analyzer.py`:
- Around line 261-267: The current check only verifies nda_croppeds is a
sequence of the right length; extend validation to iterate over each element of
nda_croppeds (the variable in analyzer.py) and ensure every entry is non-None
and is array-like (e.g., has a shape/ndim attribute or is an instance of
expected array types used in this module) before proceeding to use them with
ndas; if any entry is invalid raise a clear ValueError referencing the offending
index and expected array-like type so later code paths (lines that consume
nda_croppeds) don't fail with unclear errors.
---
Nitpick comments:
In `@monai/auto3dseg/analyzer.py`:
- Around line 221-242: Add a Google-style "Args:" section to the method
docstring that documents the input parameter `data` (the payload) used by this
callable (the method that references self.image_key and self.report_format);
state that `data` must be a dict containing the image under the key
`self.image_key` (value must be numpy.ndarray, torch.Tensor, or MetaTensor),
optionally `nda_croppeds` as a list/tuple with one entry per image channel when
precomputed, and any other expected keys used by the method (e.g., entries that
produce ImageStatsKeys.INTENSITY); include types and brief descriptions and
mention that missing/invalid types will raise KeyError/TypeError/ValueError as
already listed in Raises.
🪄 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: 3194f7d4-05c1-4613-9f56-eba2a987cfec
📒 Files selected for processing (1)
monai/auto3dseg/analyzer.py
|
I want to correct something from my earlier comment. I said that torch.no_grad() unconditionally re-enables gradients on exit, that was wrong. I looked at the implementation and exit calls torch.set_grad_enabled(self.prev), so if gradients were already off going in, they stay off, it doesn't force them back on. I apologise for the inaccuracy. I can see the nda_croppeds validation has since been added. That addresses the concern I raised earlier, good stuff. |
There was a problem hiding this comment.
Thanks @bluehyena for this update, please look at the Coderabbit comment but I think @williams145 may want to address that instead, also please look at the DCO issue.
Thanks for contributing! I think we'll merge this one but then have a look at your PRs if there's anything further you want to address. |
bluehyena
force-pushed
the
fix-auto3dseg-analyzer-bugs
branch
from
a303ae6 to
020e930
Compare
|
Thanks very much for the review. I have rebased the branch with sign-offs and force-pushed the updated history, so DCO is green now. Also, regarding the earlier CodeRabbit follow-up: @williams145, are you planning to handle that as part of the related follow-up work, or would you prefer that I address it here? |
bluehyena
force-pushed
the
fix-auto3dseg-analyzer-bugs
branch
from
020e930 to
1284529
Compare
I see there are some CI issues now, I should be able to just rerun jobs to fix these. |
3 participants
Fixes #8802
Also addresses #5889
Description
This pull request fixes two issues in
monai.auto3dseg.analyzer.ImageStats.__call__()now correctly handles the optional precomputednda_croppedsinput path instead of raisingUnboundLocalError.ImageStats,FgImageStats, andLabelStatsnow use an exception-safe no-grad context so analyzer failures do not leak the global PyTorch grad-enabled state, which is related to data analyzer set_grad_enabled #5889.I also added tests covering both behaviors.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.