8627 perceptual loss errors out after hitting the maximum number of downloads #8652
Conversation
…um-number-of-downloads' of github.com:virginiafdez/MONAI into 8627-perceptual-loss-errors-out-after-hitting-the-maximum-number-of-downloads
WalkthroughThe PR changes perceptual loss model handling to load pretrained backbones from Project-MONAI/perceptual-models via torch.hub (replacing Google Drive). It adds HF_MONAI_MODELS as the allowed model set, extends PercetualNetworkType with new network names (radimagenet_resnet50, medicalnet_resnet10_23datasets, medicalnet_resnet50_23datasets, resnet50), and updates MedicalNet/RadImageNet similarity classes to accept and propagate a new cache_dir parameter. Validation and error messages were tightened (MedicalNet requires spatial_dims=3 and is_fake_3d=False; invalid model names raise ValueError listing allowed models), docstrings updated, and channel_wise handling emits warnings when inappropriate. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ 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 |
…-the-maximum-number-of-downloads
for more information, see https://pre-commit.ci
…um-number-of-downloads' of github.com:virginiafdez/MONAI into 8627-perceptual-loss-errors-out-after-hitting-the-maximum-number-of-downloads
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/losses/perceptual.py (1)
290-293: Indexing bug in channel-wise slicing.
l_idx : i + r_idxshould bel_idx : r_idx. Current code produces incorrect slice ranges fori > 0.for i in range(input.shape[1]): l_idx = i * feats_per_ch r_idx = (i + 1) * feats_per_ch - results[:, i, ...] = feats_diff[:, l_idx : i + r_idx, ...].sum(dim=1) + results[:, i, ...] = feats_diff[:, l_idx:r_idx, ...].sum(dim=1)
🧹 Nitpick comments (3)
monai/losses/perceptual.py (3)
105-106: Addstacklevel=2to warning.Ensures warning points to caller's code, not this internal line.
if not channel_wise: - warnings.warn("MedicalNet networks support channel-wise loss. Consider setting channel_wise=True.") + warnings.warn("MedicalNet networks support channel-wise loss. Consider setting channel_wise=True.", stacklevel=2)
219-225: Documentcache_dirparameter in docstring.The
cache_dirparameter (line 232) is missing from the Args section.channel_wise: if True, the loss is returned per channel. Otherwise the loss is averaged over the channels. Defaults to ``False``. + cache_dir: path to cache directory to save the pretrained network weights. """
324-328: Documentcache_dirparameter in docstring.The
cache_dirparameter is missing from the Args section.verbose: if false, mute messages from torch Hub load function. + cache_dir: path to cache directory to save the pretrained network weights. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/losses/perceptual.py(7 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/losses/perceptual.py
🧬 Code graph analysis (1)
monai/losses/perceptual.py (2)
monai/utils/enums.py (1)
StrEnum(68-90)monai/utils/module.py (1)
optional_import(315-445)
🪛 Ruff (0.14.7)
monai/losses/perceptual.py
102-104: Avoid specifying long messages outside the exception class
(TRY003)
106-106: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
235-235: Unused lambda argument: a
(ARG005)
235-235: Unused lambda argument: b
(ARG005)
235-235: Unused lambda argument: c
(ARG005)
237-239: Avoid specifying long messages outside the exception class
(TRY003)
333-335: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
🔇 Additional comments (1)
monai/losses/perceptual.py (1)
23-28: LGTM!Immutable tuple for allowed model names addresses the security concern from prior review.
| elif "radimagenet_" in network_type: | ||
| self.perceptual_function = RadImageNetPerceptualSimilarity(net=network_type, verbose=False) |
There was a problem hiding this comment.
Missing cache_dir propagation for RadImageNet.
RadImageNetPerceptualSimilarity accepts cache_dir (line 330) but it's not passed here.
elif "radimagenet_" in network_type:
- self.perceptual_function = RadImageNetPerceptualSimilarity(net=network_type, verbose=False)
+ self.perceptual_function = RadImageNetPerceptualSimilarity(net=network_type, verbose=False, cache_dir=cache_dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif "radimagenet_" in network_type: | |
| self.perceptual_function = RadImageNetPerceptualSimilarity(net=network_type, verbose=False) | |
| elif "radimagenet_" in network_type: | |
| self.perceptual_function = RadImageNetPerceptualSimilarity(net=network_type, verbose=False, cache_dir=cache_dir) |
🤖 Prompt for AI Agents
In monai/losses/perceptual.py around lines 133-134, the
RadImageNetPerceptualSimilarity is instantiated without forwarding cache_dir
(the class accepts a cache_dir parameter at its definition), so update the
instantiation to pass the module's cache_dir (e.g., cache_dir=self.cache_dir or
cache_dir=cache_dir) into RadImageNetPerceptualSimilarity along with net and
verbose; ensure the attribute or variable exists or default to None.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monai/losses/perceptual.py (2)
112-116: Incorrect error message.Message references "Adversarial Loss" but this is
PerceptualLoss.if network_type.lower() not in list(PercetualNetworkType): raise ValueError( - "Unrecognised criterion entered for Adversarial Loss. Must be one in: %s" + "Unrecognised criterion entered for Perceptual Loss. Must be one in: %s" % ", ".join(PercetualNetworkType) )
291-293: Off-by-one slicing bug.The slice
l_idx : i + r_idxshould bel_idx : r_idx. Thei +causes incorrect indices for channels after the first.l_idx = i * feats_per_ch r_idx = (i + 1) * feats_per_ch - results[:, i, ...] = feats_diff[:, l_idx : i + r_idx, ...].sum(dim=1) + results[:, i, ...] = feats_diff[:, l_idx:r_idx, ...].sum(dim=1)
♻️ Duplicate comments (1)
monai/losses/perceptual.py (1)
133-134: Missingcache_dirpropagation.
RadImageNetPerceptualSimilarityacceptscache_dirbut it's not passed here.elif "radimagenet_" in network_type: - self.perceptual_function = RadImageNetPerceptualSimilarity(net=network_type, verbose=False) + self.perceptual_function = RadImageNetPerceptualSimilarity(net=network_type, verbose=False, cache_dir=cache_dir)
🧹 Nitpick comments (4)
monai/losses/perceptual.py (4)
105-106: Addstacklevel=2to warning.Without explicit stacklevel, the warning will point to this line instead of the caller.
if not channel_wise: - warnings.warn("MedicalNet networks support channel-wise loss. Consider setting channel_wise=True.") + warnings.warn("MedicalNet networks support channel-wise loss. Consider setting channel_wise=True.", stacklevel=2)
120-123: Addstacklevel=2to warning.warnings.warn( - f"Setting cache_dir to {cache_dir}, this may change the default cache dir for all torch.hub calls." + f"Setting cache_dir to {cache_dir}, this may change the default cache dir for all torch.hub calls.", + stacklevel=2, )
219-225: Missingcache_dirin docstring.Per coding guidelines, all parameters should be documented.
Add to Args section:
cache_dir: path to cache directory to save the pretrained network weights.
324-328: Missingcache_dirin docstring.Add to Args section:
cache_dir: path to cache directory to save the pretrained network weights.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/losses/perceptual.py(7 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/losses/perceptual.py
🪛 Ruff (0.14.7)
monai/losses/perceptual.py
102-104: Avoid specifying long messages outside the exception class
(TRY003)
106-106: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
235-235: Unused lambda argument: a
(ARG005)
235-235: Unused lambda argument: b
(ARG005)
235-235: Unused lambda argument: c
(ARG005)
237-239: Avoid specifying long messages outside the exception class
(TRY003)
333-335: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: packaging
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
🔇 Additional comments (4)
monai/losses/perceptual.py (4)
23-28: LGTM!Valid model names centralized as a constant.
34-44: LGTM!New network types properly added to the enum.
235-244: LGTM!Model validation and hub loading properly configured with
trust_repo=Trueand explicit branch. The lambda disabling fork validation is an intentional workaround.
330-337: LGTM!Consistent with
MedicalNetPerceptualSimilarity: validates model names, uses:mainbranch, and includestrust_repo=True.
2 participants
Fixes #8627.
Moves the perceptual loss code to MONAI repository https://github.com/Project-MONAI/perceptual-models and the checkpoints to Huggingface.
Description
This PR changes and simplifies the torch.hub loading process and gets the models from Huggingface lirbary.
A few sentences describing the changes proposed in this pull request.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.