Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move linting to ruff #6966

Merged
merged 4 commits into from
Nov 6, 2023
Merged

Move linting to ruff #6966

merged 4 commits into from
Nov 6, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Feb 23, 2023

This PR switches linting (flake8, isort, bandit, yesqa, pygrep-hooks) for Pillow to the ruff linter.

  • Running a full pre-commit run is much faster (45.15 seconds of CPU time to 8.62 seconds on my machine)
  • It's easier to enable more rules if need be since the configuration is all in one place
  • 👉 The only rule that may not be yet supported by ruff that had been previously enabled is rst-backticks from pygrep-hooks.
    • That being said, this PR still enables more lints than had been in use from pygrep-hooks since it enables the whole PGH family.
  • 👉 Instead of touching expressions like x, y, l, r, w, a, d, f = metrics[ix], I opted to adding specific noqa directives to quiesce Ruff's "ambiguous name" for l (which could be an 1 or an I 😁) No longer an issue via Clarify some local variable names #6971.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Running a full pre-commit run is much faster (45.15 seconds of CPU time to 8.62 seconds on my machine)

That's a good speedup! I get 5s -> 2s for pre-commit run --all-files, which is still welcome. On pre-commit.ci, it's 23.5s -> 12.3s.

Makefile Outdated
python3 -c "import isort" > /dev/null 2>&1 || python3 -m pip install isort
python3 -m black --target-version py37 .
python3 -m isort .
ruff --fix .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove Black?

Please also make sure Ruff is installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, oversight there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, should this actually be pre-commit run --all-files black ruff?

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@@ -18,10 +18,9 @@
import tempfile

from . import Image, ImageFile
from ._binary import i8
from ._binary import i8, o8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened here? Does Ruff not support isort's Black profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isort is a bit looser than Ruff with these; Ruff wants either everything or nothing to be grouped like this, and isort doesn't always care.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing, isort always sorts this like the original, with or without the Black profile.

Anyway, I don't mind too much, as long as it remains relatively stable.

On that topic, maybe we should wait until Ruff is at "v1" or "stable", which looks to be close:

astral-sh/ruff#1992

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruff has Black profile as the ONLY profile possible.

src/PIL/BdfFontFile.py Outdated Show resolved Hide resolved
src/PIL/Image.py Outdated Show resolved Hide resolved
src/PIL/ImageFile.py Outdated Show resolved Hide resolved
src/PIL/PcfFontFile.py Outdated Show resolved Hide resolved
src/PIL/TiffImagePlugin.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@akx akx marked this pull request as ready for review February 23, 2023 13:53
@akx akx requested a review from hugovk February 23, 2023 13:59
@hugovk hugovk added the Testing label Feb 23, 2023
@hugovk
Copy link
Member

hugovk commented Feb 23, 2023

The addition of pyproject.toml has changed the build process from setup.py-based to pyproject.toml-based, leading to failures on MinGW:

https://github.com/python-pillow/Pillow/actions/runs/4253283135/jobs/7404119827

@akx
Copy link
Contributor Author

akx commented Feb 24, 2023

The addition of pyproject.toml has changed the build process from setup.py-based to pyproject.toml-based, leading to failures on MinGW

Ah, should've known there are still uncivilized OSes like that... 😁

Switched to ruff.toml instead 👍

@akx
Copy link
Contributor Author

akx commented Feb 24, 2023

@hugovk CI is green now, and #6971 will fix up the local variable names.

Makefile Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Feb 25, 2023

Switched to ruff.toml instead 👍

Makes sense, we'll probably have to move to pyproject.toml at some point, but I agree it's out of scope for this. Please could you also add check-toml to .pre-commit-config.yaml?

@radarhere radarhere mentioned this pull request Feb 26, 2023
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@akx
Copy link
Contributor Author

akx commented Feb 26, 2023

@hugovk

Is there an issue at Ruff to limit Bandit by severity? If not, please could you open one?

I don't think there is - Ruff issues don't have the same sort of severity + confidence metrics Bandit's rules have.

If you want, I'm not against "belt and suspenders"ing here and keep Bandit also in the configuration...

@aclark4life
Copy link
Member

Yeah lots not switch, just add ruff … if possible/practical to do so.

@cclauss
Copy link
Contributor

cclauss commented Mar 17, 2023

If you do ruff --format=github then you get GitHub Annotations like:
image
Which some contributors might find quite helpful. The problem is that when you run in pre-commit, you are unsure if $CI is true or false and you probably do not want --format=github unless it is true.

I run a separate GitHub that is superfast (<10 seconds) to quicky add annotations to code changes.

ruff.toml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using the real Bandit directly in pre-commit because it supports severities, so let's remove entirely from Ruff.

.ruff.toml Outdated Show resolved Hide resolved
.ruff.toml Outdated Show resolved Hide resolved
.ruff.toml Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Mar 30, 2023

  • Running a full pre-commit run is much faster (45.15 seconds of CPU time to 8.62 seconds on my machine)

I'm curious why the original is so slow, what machine do you have? How do the actual elapsed times compare?

@cclauss
Copy link
Contributor

cclauss commented Mar 30, 2023

Running a full pre-commit run is much faster

Ruff linting the CPython codebase from scratch 0.29 sec.

@akx
Copy link
Contributor Author

akx commented Mar 30, 2023

  • Running a full pre-commit run is much faster (45.15 seconds of CPU time to 8.62 seconds on my machine)

I was measuring CPU time on purpose – wallclock time isn't affected that much, but the old tooling isn't able to utilize all cores on my machine (macOS 12.6.3, 2,4 GHz 8-Core Intel Core i9, etc.).

@hugovk
Copy link
Member

hugovk commented Mar 30, 2023

Ah right, still nice but less of a visible benefit. The old pre-commit speed isn't really a problem locally, and on CI it barely makes a difference compared to the full test matrix.

Ruff linting the CPython codebase from scratch 0.29 sec.

Impressive indeed, although CPython doesn't use Flake8 and this repo isn't CPython :)

@hugovk
Copy link
Member

hugovk commented Mar 30, 2023

  • 👉 The only rule that may not be yet supported by ruff that had been previously enabled is rst-backticks from pygrep-hooks.

    • That being said, this PR still enables more lints than had been in use from pygrep-hooks since it enables the whole PGH family.

https://beta.ruff.rs/docs/rules/#pygrep-hooks-pgh has four of the 10 hooks from https://github.com/pre-commit/pygrep-hooks:

  • PGH001 eval No builtin eval() allowed
  • PGH002 deprecated-log-warn warn is deprecated in favor of warning
  • PGH003 blanket-type-ignore Use specific rule codes when ignoring type issues
  • PGH004 blanket-noqa Use specific rule codes when using noqa

rst-backticks is useful, let's add it back via the original pygrep-hooks.

@akx
Copy link
Contributor Author

akx commented Oct 30, 2023

@hugovk Done! I also have another branch that would stack on top of this to switch from black to the Ruff formatter but that's a different concern so not muddling these waters with that. FWIW, the diff there is tiny, mostly just some manual changes where ruff-format doesn't do # fmt: comments in statements like Black does:

 .pre-commit-config.yaml    |  7 +------
 Makefile                   |  3 +--
 Tests/check_jpeg_leaks.py  |  7 ++-----
 Tests/helper.py            |  4 ++--
 Tests/test_box_blur.py     | 40 ++++++++++++++++++++--------------------
 Tests/test_file_jpeg.py    |  8 ++------
 Tests/test_font_leaks.py   |  5 ++++-
 Tests/test_image_access.py |  4 +---
 Tests/test_image_filter.py | 28 +++++++++++++---------------
 setup.py                   |  9 ++++-----
 10 files changed, 50 insertions(+), 65 deletions(-)

@hugovk
Copy link
Member

hugovk commented Oct 30, 2023

Thanks! Yeah, let's keep this for the linter for now.

How does this look with the UP031 fixes?

@akx
Copy link
Contributor Author

akx commented Oct 30, 2023

The UP031 fixes would involve rewriting the "something %dx%d" % some_tuple style expressions in another way case by case - I'd leave them for an imminent follow up PR?

@akx
Copy link
Contributor Author

akx commented Nov 1, 2023

@hugovk @radarhere Could this maybe be good to go now? 😁 I could follow up with a PR to remove the six UP031 noqas next...

Makefile Outdated
python3 -m black --target-version py38 .
python3 -m isort .
python3 -c "import ruff" > /dev/null 2>&1 || python3 -m pip install ruff
python3 -m ruff --preview --fix .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If stability is a concern, then wouldn't it be better to not use --preview? I have to imagine that any features that were in preview back when this PR was created are no longer in preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sans --preview:

warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(LOG was added via #7421.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go for stability and avoid --preview. Any idea when LOG will leave preview?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think there's a big huge stability issue here, since whoever ends up updating the Ruff version in this repo will probably look at what they're doing?

Also, the more happy users a preview rule has, the better its chances are to graduate, I think 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove --preview, I'd prefer to keep things stable and not spend time double checking potentially-unstable changes every month.

Copy link
Contributor Author

@akx akx Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed --preview, and also the "LOG" family of lints, because it would otherwise warn about it on every --previewless invocation (that's not --quietened):

(Pillow) ~/b/Pillow (ruff) $ ruff .
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
(Pillow) ~/b/Pillow (ruff) $ ruff --quiet .
(Pillow) ~/b/Pillow (ruff) $

@radarhere
Copy link
Member

I don't think I have strong opinions on the style changes seen here, so once @hugovk is ok with this, I'm happy for it to be merged.

@radarhere radarhere mentioned this pull request Nov 3, 2023
@akx akx force-pushed the ruff branch 4 times, most recently from c4c9f53 to ec98fb9 Compare November 3, 2023 22:09
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also delete .flake8 (recently added in #7484).

@akx
Copy link
Contributor Author

akx commented Nov 6, 2023

@hugovk Done, rebased, etc. :)

@hugovk
Copy link
Member

hugovk commented Nov 6, 2023

Thanks, a followup for UP031 would be good. And I'd prefer to keep Black rather than the Ruff formatter.

@hugovk hugovk merged commit 0660886 into python-pillow:main Nov 6, 2023
53 checks passed
@akx akx deleted the ruff branch November 6, 2023 16:20
@akx akx mentioned this pull request Nov 6, 2023
akx added a commit to akx/ruff that referenced this pull request Nov 6, 2023
zanieb pushed a commit to astral-sh/ruff that referenced this pull request Nov 6, 2023
## Summary

See python-pillow/Pillow#6966 :)

## Test Plan

Looked at the Markdown preview!
@hugovk hugovk mentioned this pull request Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants