Minor Fixes From Testing 1.6#8897
Conversation
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR updates the Dockerfile dependency installation to install/upgrade pip, wheel, and wheel-stub using --no-build-isolation and --no-cache-dir and applies the same flags when installing requirements-dev.txt. Separately, BundleAlgo.init now accepts template_path: PathLike | None = None (docstring notes it may be supplied later via load_state_dict) and BundleAlgo.state_dict now includes template_path in its returned serialization. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/apps/auto3dseg/bundle_gen.py (1)
75-82: ⚡ Quick winAdd a regression test for
BundleAlgo()with notemplate_path.This public API change needs coverage for default construction plus JSON restore, otherwise this serialization fix can regress silently.
As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 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 `@monai/apps/auto3dseg/bundle_gen.py` around lines 75 - 82, Add a regression unit test that constructs BundleAlgo() with no template_path, serializes its state to JSON (or via state_dict) and then restores a new instance using BundleAlgo().load_state_dict (or the corresponding JSON restore API), asserting the restored instance matches expected key attributes/behavior; specifically target BundleAlgo.__init__ (default construction) and BundleAlgo.load_state_dict/JSON restore to ensure default construction + serialization round-trip is covered and will fail if the fix regresses.
🤖 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 `@monai/apps/auto3dseg/bundle_gen.py`:
- Around line 75-82: state_dict() currently omits template_path so
load_state_dict() cannot restore it, causing export_to_disk() to fail; update
the serialization/deserialization to include template_path in the saved state
and restore it on load (or pass template_path into __init__ when
reconstructing). Specifically, add template_path to the dict returned by
state_dict(), and modify load_state_dict()/the deserialization path to read that
key and set self.template_path (or call __init__(template_path=...)) so
export_to_disk(), __init__, state_dict, load_state_dict, and export_to_disk
consistently handle template_path.
---
Nitpick comments:
In `@monai/apps/auto3dseg/bundle_gen.py`:
- Around line 75-82: Add a regression unit test that constructs BundleAlgo()
with no template_path, serializes its state to JSON (or via state_dict) and then
restores a new instance using BundleAlgo().load_state_dict (or the corresponding
JSON restore API), asserting the restored instance matches expected key
attributes/behavior; specifically target BundleAlgo.__init__ (default
construction) and BundleAlgo.load_state_dict/JSON restore to ensure default
construction + serialization round-trip is covered and will fail if the fix
regresses.
🪄 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: 529cafdf-db1a-4ac3-bda9-43df88b7feaa
📒 Files selected for processing (2)
Dockerfilemonai/apps/auto3dseg/bundle_gen.py
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
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/apps/auto3dseg/bundle_gen.py (2)
377-382:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
state_dict()note.Lines 377-379 still say
template_pathis excluded, but Line 382 now serializes it. The docstring now documents the opposite behavior. 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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/apps/auto3dseg/bundle_gen.py` around lines 377 - 382, The state_dict() docstring is out of sync: it claims template_path is excluded while the method actually returns "template_path". Update the state_dict() docstring to accurately describe that template_path is included in the returned dict (or, if you intended to exclude it, remove it from the returned dict); specifically edit the state_dict() docstring to list "template_path" under the returned keys and describe its meaning, or change the return payload in state_dict() to omit template_path so docstring and code match; refer to the state_dict() method and the template_path key for the exact change.
75-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
template_pathbefore export.Lines 75-86 now allow
BundleAlgo()with no template path, but Line 190 still builds a source path fromself.template_pathunconditionally. That turns a direct caller mistake into a latecopytree("None/...")failure instead of a clear contract error.Proposed fix
def export_to_disk(self, output_path: str, algo_name: str, **kwargs: Any) -> None: """ Fill the configuration templates, write the bundle (configs + scripts) to folder `output_path/algo_name`. @@ """ + if self.template_path is None: + raise ValueError("template_path must be set before export_to_disk().") if kwargs.pop("copy_dirs", True): self.output_path = os.path.join(output_path, algo_name) os.makedirs(self.output_path, exist_ok=True) if os.path.isdir(self.output_path): shutil.rmtree(self.output_path)Also applies to: 184-192
🤖 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 `@monai/apps/auto3dseg/bundle_gen.py` around lines 75 - 86, The constructor sets self.template_path but doesn't guard it before it's used later (e.g., when building a source path in the export routine such as export_bundle or the method that does copytree), so add an explicit check that template_path is not None and raise a clear ValueError if it is (either in __init__ or at the start of the method that constructs source_path); also add the same guard before the code block that uses self.template_path in the other spot referenced (the block around the copytree call), so callers get an immediate, descriptive error instead of a late copytree("None/...") failure.
🤖 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.
Outside diff comments:
In `@monai/apps/auto3dseg/bundle_gen.py`:
- Around line 377-382: The state_dict() docstring is out of sync: it claims
template_path is excluded while the method actually returns "template_path".
Update the state_dict() docstring to accurately describe that template_path is
included in the returned dict (or, if you intended to exclude it, remove it from
the returned dict); specifically edit the state_dict() docstring to list
"template_path" under the returned keys and describe its meaning, or change the
return payload in state_dict() to omit template_path so docstring and code
match; refer to the state_dict() method and the template_path key for the exact
change.
- Around line 75-86: The constructor sets self.template_path but doesn't guard
it before it's used later (e.g., when building a source path in the export
routine such as export_bundle or the method that does copytree), so add an
explicit check that template_path is not None and raise a clear ValueError if it
is (either in __init__ or at the start of the method that constructs
source_path); also add the same guard before the code block that uses
self.template_path in the other spot referenced (the block around the copytree
call), so callers get an immediate, descriptive error instead of a late
copytree("None/...") failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 109899f6-358c-4458-8741-1c7187547283
📒 Files selected for processing (1)
monai/apps/auto3dseg/bundle_gen.py
2 participants
Description
This fixes the install issue in the Docker image seen earlier with MetricsReloaded, and fixes a serialisation issue related to moving from pickle to json for Auto3DSeg.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.