Replace pyupgrade with builtin Ruff's UP rule
#8606
Conversation
pre-commit-ci
bot
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request modernizes Python syntax and tooling configuration across the MONAI codebase. Primary changes include: updating type annotations from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (16)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🪛 Ruff (0.14.2)monai/apps/nnunet/nnunet_bundle.py44-44: Unused function argument: (ARG001) monai/losses/dice.py497-497: 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)
🔇 Additional comments (9)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR modernizes Python syntax across five files by replacing outdated string formatting methods (.format()) with f-strings and updating lru_cache(maxsize=None) to the modern cache decorator. The changes align with Ruff's UP (pyupgrade) rule, consolidating linting tools to reduce tooling complexity while maintaining code quality. All modifications are syntactically equivalent transformations that adopt Python 3.6+ (f-strings) and Python 3.9+ (cache) best practices. The affected files span profiling utilities, TensorBoard/stats handlers, metrics utilities, and deepgrow datasets—all converting legacy string formatting to the recommended f-string syntax without altering any logic or output.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| tests/profile_subclass/profiling.py | 5/5 | Replaced .format() with f-string in a single print statement for profiling output |
| monai/handlers/tensorboard_handlers.py | 5/5 | Converted two warning messages from .format() to f-strings in TensorBoardStatsHandler |
| monai/handlers/stats_handler.py | 5/5 | Modernized two warning messages by replacing .format() with f-strings |
| monai/metrics/utils.py | 5/5 | Replaced lru_cache(maxsize=None) with cache decorator and updated import |
| monai/apps/deepgrow/dataset.py | 5/5 | Converted two logging.info statements from .format() to f-strings |
Confidence score: 5/5
- This PR is safe to merge with minimal risk—all changes are non-breaking syntax modernizations with identical functional behavior.
- Score reflects straightforward string formatting upgrades and standard library improvements that are well-established Python best practices with no logic changes or potential runtime issues.
- No files require special attention—all transformations are syntactically equivalent and maintain identical output.
5 files reviewed, 2 comments
Signed-off-by: jirka <jirka.borovec@seznam.cz>
for more information, see https://pre-commit.ci Signed-off-by: jirka <jirka.borovec@seznam.cz>
Signed-off-by: jirka <jirka.borovec@seznam.cz>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. Since the previous reviews, two critical issues were introduced: a fatal syntax error in monai/losses/dice.py (missing closing quote) that will break imports, and semantically incorrect type hints in monai/losses/sure_loss.py (parameters typed as | None but using non-None sentinel defaults). Additionally, a logical inconsistency exists in monai/losses/perceptual.py where an error message says "Adversarial Loss" instead of "Perceptual Loss" (pre-existing but now more visible). The remaining changes continue the PR's goal of replacing pyupgrade with Ruff's UP rules by modernizing string formatting (.format() → f-strings) and type annotations (Union[X, Y] → X | Y, Optional[X] → X | None, lru_cache(maxsize=None) → cache). These syntactic updates are safe and maintain backward compatibility via from __future__ import annotations already present throughout the codebase.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| monai/losses/dice.py | 1/5 | BREAKING: Missing closing quote in f-string causes syntax error on line 497 |
| monai/losses/sure_loss.py | 2/5 | Incorrect type hints: eps and complex_input typed as | None but use non-None sentinel defaults |
| monai/losses/perceptual.py | 4/5 | String formatting updated but error message incorrectly says "Adversarial Loss" instead of "Perceptual Loss" |
| monai/handlers/tensorboard_handlers.py | 4/5 | Format string conversion loses space separator on line 260 (: becomes :) |
| monai/losses/ds_loss.py | 5/5 | Modernizes Union to pipe syntax; safe with postponed annotations |
| monai/losses/spatial_mask.py | 5/5 | Replaces Optional with PEP 604 syntax; functionally equivalent |
| monai/losses/focal_loss.py | 5/5 | Converts Optional to pipe syntax across four type annotations |
| monai/losses/adversarial_loss.py | 5/5 | Updates %-formatting to .format() in error message |
| monai/transforms/utility/array.py | 5/5 | Removes Union import and uses pipe syntax in two method signatures |
| monai/metrics/utils.py | 5/5 | Replaces lru_cache(maxsize=None) with Python 3.9+ cache decorator |
| monai/handlers/stats_handler.py | 5/5 | Converts two .format() calls to f-strings in warning messages |
| monai/apps/deepgrow/dataset.py | 5/5 | Updates logging statements to use f-strings instead of .format() |
| tests/profile_subclass/profiling.py | 5/5 | Replaces .format() with f-string in print statement |
Confidence score: 0/5
- This PR introduces a syntax error that will prevent the module from importing and break production code immediately.
- Score is 0/5 due to the fatal syntax error in
monai/losses/dice.py(missing quote) and semantically incorrect type hints inmonai/losses/sure_loss.pythat misrepresent the parameter contracts. Additional concerns include a copy-paste error inmonai/losses/perceptual.pyand a formatting inconsistency inmonai/handlers/tensorboard_handlers.py. - Critical attention required for
monai/losses/dice.py(line 497),monai/losses/sure_loss.py(lines 46-48, 153),monai/losses/perceptual.py(line 98), andmonai/handlers/tensorboard_handlers.py(line 260).
Additional Comments (1)
-
monai/apps/nnunet/nnunet_bundle.py, line 508-509 (link)style: Type annotations now use PEP 604 syntax but are redundant here since the string literals already enforce types. The
: strcan be removed from both lines
14 files reviewed, 8 comments
| raise ValueError( | ||
| "Unrecognised criterion entered for Adversarial Loss. Must be one in: %s" | ||
| % ", ".join(PercetualNetworkType) | ||
| "Unrecognised criterion entered for Adversarial Loss. Must be one in: {}".format(", ".join(PercetualNetworkType)) |
There was a problem hiding this comment.
syntax: Error message says "Adversarial Loss" but this is the PerceptualLoss class. Copy-paste error from adversarial_loss.py?
| perturb_noise: Optional[torch.Tensor] = None, | ||
| complex_input: Optional[bool] = False, | ||
| y_ref: torch.Tensor | None = None, | ||
| eps: float | None = -1.0, |
There was a problem hiding this comment.
logic: eps defaults to -1.0 which is a sentinel value, so None as the type hint is misleading. Consider using float = -1.0 instead of float | None = -1.0. Is -1.0 the only sentinel value, or can callers explicitly pass None to trigger special behavior?
| y_ref: torch.Tensor | None = None, | ||
| eps: float | None = -1.0, | ||
| perturb_noise: torch.Tensor | None = None, | ||
| complex_input: bool | None = False, |
There was a problem hiding this comment.
logic: complex_input defaults to False (a bool), so the type hint should be bool = False not bool | None = False. The parameter is never checked for None in the function body
| y_ref: Optional[torch.Tensor] = None, | ||
| complex_input: Optional[bool] = False, | ||
| y_ref: torch.Tensor | None = None, | ||
| complex_input: bool | None = False, |
There was a problem hiding this comment.
logic: same issue: complex_input defaults to False but is typed as bool | None. Should be bool = False
| raise ValueError( | ||
| "Unrecognised criterion entered for Adversarial Loss. Must be one in: %s" | ||
| % ", ".join(AdversarialCriterions) | ||
| "Unrecognised criterion entered for Adversarial Loss. Must be one in: {}".format(", ".join(AdversarialCriterions)) |
There was a problem hiding this comment.
style: Using .format() is less idiomatic than f-strings in modern Python. Consider f"Unrecognised criterion entered for Adversarial Loss. Must be one in: {', '.join(AdversarialCriterions)}"
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR consolidates Python linting by replacing the standalone pyupgrade pre-commit hook with Ruff's built-in UP (pyupgrade) rules. The changes modernize Python syntax across the codebase: type hints are converted from typing.Union[A, B] and typing.Optional[T] to PEP 604 pipe-union syntax (A | B, T | None), .format() and % string formatting are replaced with f-strings, and functools.lru_cache(maxsize=None) is replaced with functools.cache. The .pre-commit-config.yaml removes the pyupgrade hook entirely and moves its exclusion patterns to the ruff hook, while pyproject.toml adds the UP rule to Ruff's lint selection. This architectural change reduces external dependencies, improves linting consistency, and simplifies CI/CD by unifying multiple syntax-modernization checks into a single tool.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
.pre-commit-config.yaml |
5/5 | Removes standalone pyupgrade hook and consolidates exclusions under ruff |
pyproject.toml |
5/5 | Adds Ruff's UP rule to lint selection, enabling built-in pyupgrade checks |
monai/losses/dice.py |
0/5 | String formatting update introduces syntax error (unclosed quote in error message) |
monai/losses/perceptual.py |
0/5 | String formatting update plus critical indexing bug (off-by-one in feature slice) |
tests/profile_subclass/profiling.py |
1/5 | F-string split across lines will truncate output (missing concatenation) |
monai/losses/sure_loss.py |
2/5 | Type hint conversions introduce incorrect None annotations for sentinel-value parameters |
monai/metrics/utils.py |
3/5 | Replaces lru_cache(maxsize=None) with cache, but function takes mutable default device=None |
monai/handlers/stats_handler.py |
3/5 | F-string replacements missing spacing in warning messages |
monai/handlers/tensorboard_handlers.py |
4/5 | F-string replacements missing spacing in warning messages |
monai/losses/focal_loss.py |
4/5 | Type hints modernized to `T |
monai/apps/deepgrow/dataset.py |
4.5/5 | Logging statements converted to multi-line f-strings correctly |
monai/losses/adversarial_loss.py |
5/5 | String formatting updated from % to .format() correctly |
monai/losses/ds_loss.py |
5/5 | Type hints modernized to PEP 604 union syntax correctly |
monai/losses/spatial_mask.py |
5/5 | Type hints modernized to `T |
monai/transforms/utility/array.py |
5/5 | Type hints modernized from Union to pipe-union syntax correctly |
monai/apps/nnunet/nnunet_bundle.py |
5/5 | Type hints modernized across all function signatures correctly |
Confidence score: 0/5
- This PR is NOT safe to merge due to critical syntax errors and logic bugs that will break production code immediately.
- Score reflects three blocking issues: unclosed string quote in
dice.py(syntax error), off-by-one indexing bug inperceptual.py(incorrect computation), and broken output formatting inprofiling.py(missing string concatenation). Additionally, incorrect type hints insure_loss.pywill mislead type checkers and several f-string replacements introduce minor spacing issues in warning messages. - Pay close attention to
monai/losses/dice.py(syntax error on line497),monai/losses/perceptual.py(indexing bug on line 260 and copy-paste error on line 98),tests/profile_subclass/profiling.py(split f-string on lines 66-67), andmonai/losses/sure_loss.py(incorrect type hints forepsandcomplex_input).
Additional Comments (1)
-
monai/losses/perceptual.py, line 260 (link)logic: off-by-one slicing error: should be
l_idx:r_idxnotl_idx:i+r_idx. This will cause incorrect channel-wise feature indexing.
16 files reviewed, 5 comments
| f"Type {t.__name__} time (microseconds):" | ||
| f" min: {10**6 * b_min}, avg: {(10**6) * b_avg}, median: {(10**6) * b_med}, and std {(10**6) * b_std}." |
There was a problem hiding this comment.
syntax: f-string split across lines without proper string concatenation or parentheses. Python will treat these as two separate expressions and only print the first line. Lines 66-67 need either parentheses around both strings or explicit concatenation.
| f"Type {t.__name__} time (microseconds):" | |
| f" min: {10**6 * b_min}, avg: {(10**6) * b_avg}, median: {(10**6) * b_med}, and std {(10**6) * b_std}." | |
| f"Type {t.__name__} time (microseconds):" | |
| f" min: {10**6 * b_min}, avg: {(10**6) * b_avg}, median: {(10**6) * b_med}, and std {(10**6) * b_std}." |
|
|
||
|
|
||
| @lru_cache(maxsize=None) | ||
| @cache |
There was a problem hiding this comment.
logic: The decorator @cache does not accept any arguments, so the parameter device=None is not being cached as part of the key. This means calls with different devices will return the same cached tensor on the wrong device, causing errors. Either use @lru_cache(maxsize=None) with a hashable device wrapper, or accept that the cache is device-agnostic and convert the result after retrieval. Should the cache be keyed by device, or is it acceptable to convert the cached tensor to the requested device after retrieval?
| " make sure `output_transform(engine.state.output)` returns" | ||
| " a scalar or dictionary of key and scalar pairs to avoid this warning." | ||
| " {}:{}".format(name, type(value)) | ||
| f" {name}:{type(value)}" |
There was a problem hiding this comment.
syntax: Missing space after colon in f-string. Should be f" {name}: {type(value)}"
| " make sure `output_transform(engine.state.output)` returns" | ||
| " a scalar or a dictionary of key and scalar pairs to avoid this warning." | ||
| " {}".format(type(loss)) | ||
| f" {type(loss)}" |
There was a problem hiding this comment.
syntax: Missing space after colon in f-string. Should be f" {type(loss)}"
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The developer has corrected a critical syntax error in monai/losses/dice.py that was identified in the previous review. The error was a missing closing quote in an error message string within the GeneralizedWassersteinDiceLoss class constructor. This fix addresses a SyntaxError that would have prevented the entire monai.losses.dice module from being imported. The correction changes the malformed string 'GDL to the properly quoted 'GDL', ensuring the error message displays correctly when invalid weighting_mode values are provided. This change is part of the broader PR goal to migrate linting from pyupgrade to Ruff's UP rule, which caught this pre-existing syntax error during the linting process.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| monai/losses/dice.py | 5/5 | Fixed missing closing quote in error message string on line 497 |
Confidence score: 5/5
- This change is a straightforward syntax error fix with zero risk of unintended side effects
- The fix correctly addresses the exact issue identified in the previous review by properly closing the quoted string
- No files require special attention as the single-line fix is trivial and clearly correct
1 file reviewed, no comments
1 participant
simplify the lintong and reduce the number of used tools to increase consistency without any formatting loss
Description
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.