Sync nnUNet robustness updates to cchmc app from seg_metrics_op - #581
Open
chezhia wants to merge 4 commits into
Open
Sync nnUNet robustness updates to cchmc app from seg_metrics_op#581chezhia wants to merge 4 commits into
chezhia wants to merge 4 commits into
Conversation
Contributor
|
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:
WalkthroughThe update adds secure ChangesnnU-Net conversion and inference flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as convert_nnunet_ckpts.py
participant Bundle as nnunet_bundle.py
participant Operator as NNUnetSegOperator
participant Postprocess as PostProcessNNUnet
participant DICOM as DICOM SEG
CLI->>Bundle: convert selected checkpoint type
Bundle-->>Operator: provide resolved predictors and model configurations
Operator->>Postprocess: apply postprocessing recipe when present
Postprocess-->>Operator: return processed segmentation
Operator->>DICOM: emit filtered segmentation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
chezhia
force-pushed
the
nnunet-example-fix
branch
from
April 30, 2026 17:27
11d8808 to
fc1154c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.py (1)
307-317: ⚡ Quick winConsider using
cascade_model_dictdirectly instead of indexing[-1].The code uses
best_model_dict["selected_model_or_models"][-1]to get trainer and plans for3d_lowres, butcascade_model_dictwas already captured when3d_cascade_fullreswas found. Using it directly would be more explicit and less fragile if model ordering changes.♻️ Suggested fix
lowres_nnunet_config = dict(base_nnunet_config) lowres_nnunet_config["nnunet_configuration"] = "3d_lowres" - lowres_nnunet_config["nnunet_trainer"] = best_model_dict["selected_model_or_models"][-1][ - "trainer" - ] # Using the same trainer as the cascade model - lowres_nnunet_config["nnunet_plans"] = best_model_dict["selected_model_or_models"][-1][ - "plans_identifier" - ] # Using the same plans id as the cascade model + lowres_nnunet_config["nnunet_trainer"] = cascade_model_dict["trainer"] + lowres_nnunet_config["nnunet_plans"] = cascade_model_dict["plans_identifier"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.py` around lines 307 - 317, The code retrieves trainer and plans for the 3d_lowres config by indexing best_model_dict["selected_model_or_models"][-1], which is fragile; instead use the previously captured cascade_model_dict directly (the one identified when "3d_cascade_fullres" was found) to populate lowres_nnunet_config["nnunet_trainer"] and ["nnunet_plans"] before looping folds and calling convert_nnunet_to_monai_bundle with bundle_root_folder, fold, and checkpoint_type so the lowres config explicitly uses cascade_model_dict values.examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py (1)
264-268: 💤 Low valueConsider adding
strict=Truetozip()for defensive clarity.While
np.unique(..., return_counts=True)guarantees equal-length arrays, addingstrict=Truemakes the invariant explicit and catches unexpected edge cases during refactoring.♻️ Suggested fix
- unique_values, counts = np.unique(array.astype(np.int64), return_counts=True) - summary = {int(value): int(count) for value, count in zip(unique_values, counts)} + unique_values, counts = np.unique(array.astype(np.int64), return_counts=True) + summary = {int(value): int(count) for value, count in zip(unique_values, counts, strict=True)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py` around lines 264 - 268, The zip between unique_values and counts in _log_label_summary should be made explicit/defensive by passing strict=True to zip to ensure lengths match; update the comprehension in _log_label_summary (the dict comprehension using zip(unique_values, counts)) to use zip(unique_values, counts, strict=True) so any future mismatch raises immediately and aids debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.py`:
- Around line 307-317: The code retrieves trainer and plans for the 3d_lowres
config by indexing best_model_dict["selected_model_or_models"][-1], which is
fragile; instead use the previously captured cascade_model_dict directly (the
one identified when "3d_cascade_fullres" was found) to populate
lowres_nnunet_config["nnunet_trainer"] and ["nnunet_plans"] before looping folds
and calling convert_nnunet_to_monai_bundle with bundle_root_folder, fold, and
checkpoint_type so the lowres config explicitly uses cascade_model_dict values.
In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py`:
- Around line 264-268: The zip between unique_values and counts in
_log_label_summary should be made explicit/defensive by passing strict=True to
zip to ensure lengths match; update the comprehension in _log_label_summary (the
dict comprehension using zip(unique_values, counts)) to use zip(unique_values,
counts, strict=True) so any future mismatch raises immediately and aids
debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be94bb33-599f-4b05-9928-60b447a387af
📒 Files selected for processing (3)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/convert_nnunet_ckpts.pyexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.pyexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.py`:
- Around line 307-314: The lowres configuration is pulling trainer/plans from
best_model_dict instead of the detected cascade model, which can select the
wrong artifacts; update the assignments that set
lowres_nnunet_config["nnunet_trainer"] and lowres_nnunet_config["nnunet_plans"]
to use cascade_model_dict (e.g.,
cascade_model_dict["selected_model_or_models"][-1]["trainer"] and
cascade_model_dict["selected_model_or_models"][-1]["plans_identifier"]) so the
lowres conversion uses the cascade model's trainer and plans metadata rather
than best_model_dict.
- Line 47: Replace hardcoded checkpoint filename literals with the existing
DEFAULT_MODEL_FILENAMES constant: locate where "final_model.pt",
"best_model.pt", and "model.pt" are used in save path construction (e.g., in the
module's checkpoint/save routine around the save logic, such as the
save_checkpoint/save_model functions referenced in this file) and substitute
them with values from DEFAULT_MODEL_FILENAMES (either by destructuring it into
FINAL_MODEL, BEST_MODEL, MODEL at top of the file or by using
DEFAULT_MODEL_FILENAMES[0/1/2]) so all filename usage reuses the single named
constant and removes the duplicated string literals.
In `@monai/deploy/graphs/__init__.py`:
- Around line 8-11: The current broad except hides real import errors; change
the import error handling around "from holoscan.graphs import *" to only swallow
ModuleNotFoundError for the holoscan.graphs module and re-raise any other
exceptions—i.e., replace the blanket "except Exception" with an "except
ModuleNotFoundError as e" that sets __all__ = [] only when e.name indicates the
missing holoscan.graphs module (otherwise re-raise), keeping the import
statement and the __all__ symbol intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d9d2f69-930d-4b50-a2b9-37a3395dd8ae
📒 Files selected for processing (4)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/convert_nnunet_ckpts.pyexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.pyexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.pymonai/deploy/graphs/__init__.py
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py (1)
91-95:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSeparate auxiliary cascade stages from final ensemble members.
_get_model_list_from_plans()now pulls every config directory from the bundle, andprediction_keysis built from that same list. For cascade bundles,3d_lowresis only a prerequisite for3d_cascade_fullres, but this wiring also feeds its saved probabilities intoEnsembleProbabilitiesToSegmentation, so the final mask depends on an auxiliary stage that should never be averaged. It also makes the cascade execution order depend onplans.jsonordering instead of explicit selection metadata.A safer split is: keep one ordered list for models that must run (
3d_lowres+ final models), and a separate list for models that participate in the final ensemble (3d_lowresexcluded).Also applies to: 137-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py` around lines 91 - 95, The current code builds self.model_list and self.prediction_keys from the same list returned by _get_model_list_from_plans(), which causes auxiliary cascade stages like "3d_lowres" to be included in the final ensemble; instead, create two lists: one ordered_run_list (e.g., self.run_model_list) that contains all models required to run in the correct order (including "3d_lowres" as a prerequisite) and a separate ensemble_model_list (e.g., self.ensemble_model_list) that excludes auxiliary-only stages such as "3d_lowres" and is used to build self.prediction_keys; update the assignments where self.model_list and prediction_keys are created (the block initializing self.model_list, model_name, save_probabilities, save_files, prediction_keys) and the equivalent code around lines 137-163 to use run_model_list for execution/order and ensemble_model_list to construct prediction_keys (and ensure EnsembleProbabilitiesToSegmentation uses prediction_keys from ensemble_model_list).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.py`:
- Around line 921-925: The guard that raises if checkpoint_name is falsy
prevents callers from passing checkpoint_name=None so _resolve_checkpoint_name
can auto-detect; remove or change that check so None is accepted and passed
through to _resolve_checkpoint_name (i.e., stop raising ValueError in the
constructor/wrapper and let self.checkpoint_name =
_resolve_checkpoint_name(model_folder, checkpoint_name) handle auto-detection),
referencing the checkpoint_name parameter and the _resolve_checkpoint_name(...)
call in the wrapper/constructor (e.g., ModelnnUNetWrapper or the surrounding
init) to locate where to change.
---
Outside diff comments:
In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py`:
- Around line 91-95: The current code builds self.model_list and
self.prediction_keys from the same list returned by
_get_model_list_from_plans(), which causes auxiliary cascade stages like
"3d_lowres" to be included in the final ensemble; instead, create two lists: one
ordered_run_list (e.g., self.run_model_list) that contains all models required
to run in the correct order (including "3d_lowres" as a prerequisite) and a
separate ensemble_model_list (e.g., self.ensemble_model_list) that excludes
auxiliary-only stages such as "3d_lowres" and is used to build
self.prediction_keys; update the assignments where self.model_list and
prediction_keys are created (the block initializing self.model_list, model_name,
save_probabilities, save_files, prediction_keys) and the equivalent code around
lines 137-163 to use run_model_list for execution/order and ensemble_model_list
to construct prediction_keys (and ensure EnsembleProbabilitiesToSegmentation
uses prediction_keys from ensemble_model_list).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33a1bd81-a092-46c8-9354-79dd371b0a55
📒 Files selected for processing (3)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.pyexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.pymonai/deploy/graphs/__init__.py
chezhia
force-pushed
the
nnunet-example-fix
branch
from
August 12, 2026 18:34
426531d to
8f4bd05
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py (1)
240-247: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConvert
depth_pixel_spacingbefore the reader derivesspacing.
InMemImageReader._get_meta_dictconstructsspacingfrom row, column, and depth spacing. This conversion normalizes only row and column spacing. A pydicom value indepth_pixel_spacingremains unconverted, so anisotropic series can retain the incompatible metadata type.Proposed fix
- known_conversions = {"SeriesInstanceUID": str, "row_pixel_spacing": float, "col_pixel_spacing": float} + known_conversions = { + "SeriesInstanceUID": str, + "row_pixel_spacing": float, + "col_pixel_spacing": float, + "depth_pixel_spacing": float, + }🤖 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 `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py` around lines 240 - 247, Update the known_conversions mapping in the metadata conversion loop to include depth_pixel_spacing with float conversion, alongside row_pixel_spacing and col_pixel_spacing, so InMemImageReader._get_meta_dict derives spacing from consistently normalized values.
🤖 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 `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/convert_nnunet_ckpts.py`:
- Around line 88-96: Update the command-line argument table in README.md to
document --checkpoint_type, including its default value of final, supported
values final, best, and both, and the corresponding output files produced by
each mode.
---
Outside diff comments:
In `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py`:
- Around line 240-247: Update the known_conversions mapping in the metadata
conversion loop to include depth_pixel_spacing with float conversion, alongside
row_pixel_spacing and col_pixel_spacing, so InMemImageReader._get_meta_dict
derives spacing from consistently normalized values.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c8d40f7-5cf6-4fda-890c-fe4b3b1d49eb
📒 Files selected for processing (3)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/README.mdexamples/apps/cchmc_nnunet_fifteen_ckpt_app/convert_nnunet_ckpts.pyexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py
Signed-off-by: chezhia <chezhipower@gmail.com>
chezhia
force-pushed
the
nnunet-example-fix
branch
from
August 12, 2026 19:10
74ec674 to
388a834
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/README.md`:
- Line 41: Update the checkpoint output example in the README to match the
documented default `--checkpoint_type final` behavior: show `final_model.pt`
rather than `best_model.pt` in the output tree, while preserving the
descriptions for the `best` and `both` modes.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 12c1dbcc-fb21-4256-9493-48fdd15e17dc
📒 Files selected for processing (3)
examples/apps/cchmc_nnunet_fifteen_ckpt_app/README.mdexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.pyexamples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_seg_operator.py
💤 Files with no reviewable changes (1)
- examples/apps/cchmc_nnunet_fifteen_ckpt_app/my_app/nnunet_bundle.py
| | `--dataset_name_or_id` | Name or ID of the nnUNet dataset to convert | Yes | N/A | | ||
| | `--MAP_root` | Output directory for the converted MONAI bundle | No | Current directory | | ||
| | `--nnUNet_results` | Path to nnUNet results directory with trained models | Yes | Uses environment variable if set | | ||
| | `--checkpoint_type` | Checkpoints to convert: `final` produces `final_model.pt`, `best` produces `best_model.pt`, and `both` produces both files | No | `final` | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the output-structure example.
Line 41 states that the default mode creates final_model.pt. The output tree at line 68 shows only best_model.pt. This gives users an incorrect expected artifact for the default command.
Proposed fix
│ ├── nnunet_checkpoint.pth
│ └── fold_X/ # Each fold's model weights
- │ └── best_model.pt
+ │ ├── final_model.pt # Default checkpoint mode
+ │ └── best_model.pt # Present for `best` or `both`🤖 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 `@examples/apps/cchmc_nnunet_fifteen_ckpt_app/README.md` at line 41, Update the
checkpoint output example in the README to match the documented default
`--checkpoint_type final` behavior: show `final_model.pt` rather than
`best_model.pt` in the output tree, while preserving the descriptions for the
`best` and `both` modes.
Contributor
Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was an bug with handling anisotropic spacing in the affine matrix, this patch fixes that and a few other patches for safety.
Summary by CodeRabbit
New Features
Bug Fixes
MAP_rootwithin the working directory.Documentation
MAP_rootpath requirements and checkpoint conversion options.