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
|
Warning Rate limit exceeded@Borda has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
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. WalkthroughThe diff applies widespread non-functional modernizations: many type hints changed from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
![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 <[email protected]>
for more information, see https://pre-commit.ci Signed-off-by: jirka <[email protected]>
Signed-off-by: jirka <[email protected]>
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
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
|
|
||
|
|
||
| @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?
There was a problem hiding this comment.
I think this is a valid complaint that should be looked at.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Jirka Borovec <[email protected]>
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
| ^monai/networks/| # avoid typing rewrites | ||
| ^monai/apps/detection/utils/anchor_utils.py| # avoid typing rewrites | ||
| ^tests/test_compute_panoptic_quality.py # avoid typing rewrites | ||
| ^monai/networks/| # todo: avoid typing rewrites |
There was a problem hiding this comment.
These do not appear to be correct changes for these comments.
There was a problem hiding this comment.
It does since Ruff now also runs pyupgrade
There was a problem hiding this comment.
My point was these aren't comments about a TODO item, they explained what the line was for so they shouldn't have "todo" added (and if it did it should be "TODO" specifically anyway).
|
Hi @Borda thanks for the contribution. There's a lot of changes related to typing and f-strings rather than the PyUpgrade change, these are fine but it obscures what is being done here. What is the purpose of changing the |
Good call, but f-string is to be upgraded, and if original pyupgrade mised it, it is rather its issue than Ruff's doing its job, right?
removing
Yes, will do |
Signed-off-by: Jirka Borovec <[email protected]>
Signed-off-by: Jirka Borovec <[email protected]>
Signed-off-by: Jirka Borovec <[email protected]>
Signed-off-by: Jirka Borovec <[email protected]>
Signed-off-by: Jirka Borovec <[email protected]>
2 participants
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.