Torchvision pretrain fix#8563
Conversation
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
monai/networks/blocks/fcn.py (1)
126-128: Weights API switch looks good; expose weights to callers for flexibility.Using
ResNet50_Weights.IMAGENET1K_V1preserves legacy behavior. Consider exposing aweightsarg (defaulting to V1 whenpretrained=True) so callers can opt into newer defaults (e.g., V2) or custom weights without patching code.Example diff:
- def __init__( - self, out_channels: int = 1, upsample_mode: str = "bilinear", pretrained: bool = True, progress: bool = True - ): + def __init__( + self, + out_channels: int = 1, + upsample_mode: str = "bilinear", + pretrained: bool = True, + progress: bool = True, + weights=None, + ): @@ - resnet = models.resnet50( - progress=progress, weights=models.ResNet50_Weights.IMAGENET1K_V1 if pretrained else None - ) + selected_weights = models.ResNet50_Weights.IMAGENET1K_V1 if (pretrained and weights is None) else weights + resnet = models.resnet50(progress=progress, weights=selected_weights)monai/networks/nets/milmodel.py (2)
19-22: Import path change is fine; consider aligning tests.Tests still import
optional_importfrommonai.utils.module. Not a blocker, but consider unifying onmonai.utilsfor consistency.
96-106: Add torchvision version-compat fallback for string backbones.Calling
torch_model(weights="DEFAULT")will fail on older torchvision. A tinyTypeErrorfallback keeps backward compatibility without reintroducing deprecation warnings on newer versions.- net = torch_model(weights="DEFAULT" if pretrained else None) + try: + net = torch_model(weights="DEFAULT" if pretrained else None) + except TypeError: + net = torch_model(pretrained=pretrained)Also applies to: 101-102
tests/networks/nets/test_milmodel.py (1)
53-61: Guard import-time backbone construction to avoid failures when torchvision isn't present.Building
backbone = models.densenet121()at import time will error if torchvision is missing. Wrap with a guard.-# custom backbone -backbone = models.densenet121() -backbone_nfeatures = backbone.classifier.in_features -backbone.classifier = torch.nn.Identity() -TEST_CASE_MILMODEL.append( - [ - {"num_classes": 5, "backbone": backbone, "backbone_num_features": backbone_nfeatures, "pretrained": False}, - (2, 2, 3, 512, 512), - (2, 5), - ] -) +# custom backbone +if models is not None: + backbone = models.densenet121() + backbone_nfeatures = backbone.classifier.in_features + backbone.classifier = torch.nn.Identity() + TEST_CASE_MILMODEL.append( + [ + {"num_classes": 5, "backbone": backbone, "backbone_num_features": backbone_nfeatures, "pretrained": False}, + (2, 2, 3, 512, 512), + (2, 5), + ] + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (5)
monai/networks/blocks/fcn.py(1 hunks)monai/networks/nets/milmodel.py(3 hunks)monai/networks/nets/torchvision_fc.py(1 hunks)tests/networks/nets/test_densenet.py(1 hunks)tests/networks/nets/test_milmodel.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/networks/blocks/fcn.pymonai/networks/nets/torchvision_fc.pytests/networks/nets/test_densenet.pytests/networks/nets/test_milmodel.pymonai/networks/nets/milmodel.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: packaging
🔇 Additional comments (4)
tests/networks/nets/test_densenet.py (1)
91-103: Harden test: wrap torchvision download in skip context and use tolerance-based compare. Avoid spurious CI failures if weights download flakes and prefer numerical closeness over exact equality.@parameterized.expand([TEST_PRETRAINED_2D_CASE_3]) @skipUnless(has_torchvision, "Requires `torchvision` package.") def test_pretrain_consistency(self, model, input_param, input_shape): example = torch.randn(input_shape).to(device) - with skip_if_downloading_fails(): - net = model(**input_param).to(device) - with eval_mode(net): - result = net.features.forward(example) - torchvision_net = torchvision.models.densenet121(weights="DEFAULT").to(device) - with eval_mode(torchvision_net): - expected_result = torchvision_net.features.forward(example) - self.assertTrue(torch.all(result == expected_result)) + with skip_if_downloading_fails(): + net = model(**input_param).to(device) + with eval_mode(net): + result = net.features.forward(example) + tv_net = torchvision.models.densenet121(weights="DEFAULT").to(device) + with eval_mode(tv_net): + expected = tv_net.features.forward(example) + # allow tiny numerical diffs across backends + torch.testing.assert_close(result, expected, rtol=1e-5, atol=1e-6)Torchvision supports
weights="DEFAULT"strings (>=0.13.0) (docs.pytorch.org, pytorch.org).monai/networks/nets/torchvision_fc.py (1)
115-121: Make pretrained/weights interplay explicit with compatibility fallback
- Document that
pretrained=True, weights=Nonemaps toweights="DEFAULT".- Add a
try/except AttributeErroraroundgetattr(models, model_name)to raiseValueErrorfor unknown models.- Wrap the weights‐based constructor in
try/except TypeErrorto fall back onpretrained=for older torchvision.Unable to verify torchvision’s API in this environment—please run the Python snippet in your local setup to confirm whether
weights=is supported and adjust the fallback accordingly.monai/networks/nets/milmodel.py (1)
75-79: Good: explicit ResNet50 V1 weights to preserve legacy behavior.No issues here.
tests/networks/nets/test_milmodel.py (1)
47-50: Nice coverage: exercise both pretrained states.LGTM.
|
/build |
|
|
Hi @ericspod, do you remember the reason why we fixed the isort version? Can we try loosen the range of package versions in this PR? |
No I don't remember I'm afraid, I'm going to try loosening it now. |
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
requirements-dev.txt (1)
21-21: Optional: cap to a minor while we validate, or consolidate on Ruff for import sorting.
- Conservative cap until CI passes broadly:
-isort>=5.1, !=6.0.0 +isort>=5.1, !=6.0.0, <6.1
- Or drop isort and rely on ruff’s isort rules to reduce tool overlap (follow-up PR).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
requirements-dev.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: build-docs
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
🔇 Additional comments (2)
requirements-dev.txt (2)
21-21: LGTM: loosens isort to admit 6.0.1 and resolves the resolver conflict.
This matches the CI feedback that complained about<6.0vs==6.0.1.
21-21: Verify no other isort pins/configs still conflict with 6.x.requirements-dev.txt:21 —
isort>=5.1, !=6.0.0(this blocks 6.x).
No other isort pins or pre-commit hook found in the repo.
setup.cfg contains a[isort]section (profile = black,known_first_party = monai) — review for v6 breaking changes.
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
|
If I remember, the new version of isort does weird things to sort imports that are a little complicated which old versions don't agree with. |
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)
monai/transforms/io/array.py (1)
544-553: Guard meta_data and add robust key fallback to avoid runtime errors.If img isn’t a MetaTensor, meta_data is undefined, causing UnboundLocalError. Also, be resilient to either enum being used.
Apply:
if isinstance(img, MetaTensor): meta_data = img.meta + else: + raise TypeError("WriteFileMapping expects a MetaTensor with metadata.") - if MetaKeys.SAVED_TO not in meta_data: + if MetaKeys.SAVED_TO not in meta_data: raise KeyError( "Missing 'saved_to' key in metadata. Check SaveImage argument 'savepath_in_metadict' is True." ) - input_path = meta_data[ImageMetaKey.FILENAME_OR_OBJ] + input_path = meta_data.get(ImageMetaKey.FILENAME_OR_OBJ) or meta_data.get(MetaKeys.FILENAME_OR_OBJ) + if input_path is None: + raise KeyError("Missing 'filename_or_obj' key in metadata.") output_path = meta_data[MetaKeys.SAVED_TO]
🧹 Nitpick comments (1)
monai/transforms/io/array.py (1)
389-393: Tighten type hint for padding_mode.Default is an enum value; reflect that in the annotation.
Apply:
- padding_mode: str = GridSamplePadMode.BORDER, + padding_mode: GridSamplePadMode | str = GridSamplePadMode.BORDER,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/transforms/io/array.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/io/array.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.12)
🔇 Additional comments (1)
monai/transforms/io/array.py (1)
296-298: Do not mirror FILENAME_OR_OBJ into MetaKeys.No references to MetaKeys.FILENAME_OR_OBJ were found; code consistently uses ImageMetaKey / Key.FILENAME_OR_OBJ (e.g. monai/transforms/io/array.py:297, monai/transforms/spatial/array.py:326, monai/data/csv_saver.py:95).
Likely an incorrect or invalid review comment.
|
/build |
Seems there are many conflicts in the PyTorch 25.0x base image. |
|
/build |
Fixes Project-MONAI#8552. ### Description This updates how pretrained model weights are loaded through Torchvision. This may not preserve historical results if the weights being loaded are now different since the "DEFAULT" weights may not be the weights loaded when using the `pretrained=True` argument. I tried to preserved behaviour as indicated in the Torchvision source code where possible. ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [ ] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Fixes Project-MONAI#8552. ### Description This updates how pretrained model weights are loaded through Torchvision. This may not preserve historical results if the weights being loaded are now different since the "DEFAULT" weights may not be the weights loaded when using the `pretrained=True` argument. I tried to preserved behaviour as indicated in the Torchvision source code where possible. ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [ ] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2 participants
Fixes #8552.
Description
This updates how pretrained model weights are loaded through Torchvision. This may not preserve historical results if the weights being loaded are now different since the "DEFAULT" weights may not be the weights loaded when using the
pretrained=Trueargument. I tried to preserved behaviour as indicated in the Torchvision source code where possible.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.