{{ message }}
Fix nnUNet runner store_true flag command construction (#8941)#8944
Open
aymuos15 wants to merge 5 commits into
Open
Fix nnUNet runner store_true flag command construction (#8941)#8944aymuos15 wants to merge 5 commits into
aymuos15 wants to merge 5 commits into
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>
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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>
Contributor
Author
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
1 participant
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.
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.