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

Switch formatting to ruff format #8291

Closed
wants to merge 5 commits into from
Closed

Conversation

akx
Copy link
Contributor

@akx akx commented Aug 6, 2024

This is a reheat (I gave it a couple minutes in the microwave) and rebase of #7531ruff format has matured enough in the intervening ~9 months that this could presumably be reconsidered :)

The new thing c.f. then is the normalization of escapes; see astral-sh/ruff#9280 and https://docs.astral.sh/ruff/formatter/black/#hex-codes-and-unicode-sequences for discussion. Most of the escape sequences in the repo were already lower-case.

@hugovk
Copy link
Member

hugovk commented Aug 6, 2024

Thanks, but I'm still happy with Black.

has matured enough

Hmm, it caused a syntax error on 3.9-3.11 in 4eb6f7d:

     try:
         assert epsilon >= ave_diff, (
-            (msg or "")
-            + f" average pixel value difference {ave_diff:.4f} > epsilon {epsilon:.4f}"
+            f"{msg or ""} average pixel value difference "
+            f"{ave_diff:.4f} > epsilon {epsilon:.4f}"
         )
  File "/home/runner/work/Pillow/Pillow/Tests/helper.py", line 127
    )
    ^
SyntaxError: f-string: expecting '}'

Caused by double quotes inside double quotes in an f-string. Worth reporting to Ruff?

@aclark4life
Copy link
Member

I'm definitely pro-ruff for the speed & other less-than-logical reasons so I suspect we'll probably end up there at some point, but it doesn't have to be right now. In any event, thank you for the PR!

@aclark4life
Copy link
Member

Worth reporting to Ruff?

If format or check replaced working code with broken code, I'd say yes.

@Yay295
Copy link
Contributor

Yay295 commented Aug 6, 2024

Having one fewer dependency would be nice. Though I prefer uppercase hex codes.

@hugovk
Copy link
Member

hugovk commented Aug 6, 2024

We already use Ruff for linting, which replaces lots of individual tools. Black is already pretty fast.

On my machine, running pre-commit run --all-files with main takes 4 seconds, and with this PR it also takes 4 seconds.

@akx
Copy link
Contributor Author

akx commented Aug 6, 2024

has matured enough

Hmm, it caused a syntax error on 3.9-3.11

That wasn't Ruff, that was yours truly (who can be quite immature at times). Nothing to report to Astral, and I might have actually had target-version = "py312" or something back then. TBH, that (revision of) that commit was from the last time I proposed this :)

On my machine, running pre-commit run --all-files with main takes 4 seconds, and with this PR it also takes 4 seconds.

Because you have a warm cache. Try env BLACK_CACHE_DIR=/dev/null RUFF_NO_CACHE=true (to simulate a CI environment); this was also discussed in #7531.

$ env BLACK_CACHE_DIR=/dev/null RUFF_NO_CACHE=true hyperfine --warmup 5 'git checkout main-ruff-check-compat; make lint-fix' 'git checkout ruff-format-2024; make lint-fix'
Benchmark 1: git checkout main-ruff-check-compat; make lint-fix
  Time (mean ± σ):      1.374 s ±  0.044 s    [User: 7.576 s, System: 0.430 s]
  Range (min … max):    1.331 s …  1.478 s    10 runs

Benchmark 2: git checkout ruff-format-2024; make lint-fix
  Time (mean ± σ):     116.2 ms ±   2.5 ms    [User: 247.5 ms, System: 82.3 ms]
  Range (min … max):   110.8 ms … 120.0 ms    25 runs

Summary
  git checkout ruff-format-2024; make lint-fix ran
   11.82 ± 0.45 times faster than git checkout main-ruff-check-compat; make lint-fix

(Before you ask, main-ruff-check-compat is main + 4672bd3 :) )

Also skipping having to install black (22 megabytes according to docker run -it python:3.12 sh -c 'df; pip install black; df') is a win in my books.

@hugovk
Copy link
Member

hugovk commented Aug 6, 2024

env BLACK_CACHE_DIR=/dev/null RUFF_NO_CACHE=true hyperfine --warmup 5 'git checkout main-ruff-check-compat; make lint-fix' 'git checkout ruff-format-2024; make lint-fix'
Benchmark 1: git checkout main-ruff-check-compat; make lint-fix
  Time (mean ± σ):     115.4 ms ±   5.7 ms    [User: 268.9 ms, System: 50.6 ms]
  Range (min … max):   109.3 ms … 134.7 ms    24 runs

Benchmark 2: git checkout ruff-format-2024; make lint-fix
  Time (mean ± σ):     118.6 ms ±   4.4 ms    [User: 269.0 ms, System: 54.4 ms]
  Range (min … max):   114.6 ms … 135.7 ms    24 runs

Summary
  git checkout main-ruff-check-compat; make lint-fix ran
    1.03 ± 0.06 times faster than git checkout ruff-format-2024; make lint-fix

@radarhere
Copy link
Member

radarhere commented Aug 12, 2024

The feedback here seems to be 'Thanks, but maybe later'.

@akx do you mind if we close this? It'll still be helpful as a reference if we make this change further down the road.

@akx
Copy link
Contributor Author

akx commented Aug 12, 2024

I don't mind (didn't mind the last time around 😄), but I'd be curious to see if anyone else gets benchmark numbers like Hugo's – I'm really surprised the performance is so similar there?

@aclark4life
Copy link
Member

but I'd be curious to see if anyone else gets benchmark numbers like Hugo's – I'm really surprised the performance is so similar there?

Same. Not really following the numbers but just curious. Clearly if there was some overwhelming performance win or elimination of complexity gained by switching we'd probably switch, otherwise "six of one half dozen of the other", but thank you again for the PR!

@radarhere
Copy link
Member

radarhere commented Aug 12, 2024

On my machine, I get

$ env BLACK_CACHE_DIR=/dev/null RUFF_NO_CACHE=true hyperfine --warmup 5 'git checkout main; make lint-fix' 'git checkout ruff-format-2024; make lint-fix'
Benchmark 1: git checkout main; make lint-fix
  Time (mean ± σ):      2.070 s ±  0.073 s    [User: 9.582 s, System: 0.498 s]
  Range (min … max):    1.934 s …  2.155 s    10 runs
 
Benchmark 2: git checkout ruff-format-2024; make lint-fix
  Time (mean ± σ):     252.6 ms ±  16.4 ms    [User: 513.1 ms, System: 99.9 ms]
  Range (min … max):   237.9 ms … 287.3 ms    10 runs
 
Summary
  git checkout ruff-format-2024; make lint-fix ran
    8.19 ± 0.61 times faster than git checkout main; make lint-fix

Comment on lines +118 to +119
python3 -m ruff check --fix .
python3 -m ruff format .
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python3 -m ruff check --fix .
python3 -m ruff format .
python3 -m ruff check --fix
python3 -m ruff format

See astral-sh/ruff#10217

@radarhere
Copy link
Member

I ran a test on GHA, and those results were more impressive - https://github.com/radarhere/Pillow/actions/runs/10394561096/job/28784619575#step:6:53

Benchmark 1: git checkout 8291_test; make lint-fix
  Time (mean ± σ):      5.855 s ±  0.047 s    [User: 19.248 s, System: 0.307 s]
  Range (min … max):    5.768 s …  5.931 s    10 runs
 
Benchmark 2: git checkout ruff-format-2024; make lint-fix
  Time (mean ± σ):     201.1 ms ±   1.9 ms    [User: 496.4 ms, System: [57.8 ms]
  Range (min … max):   197.9 ms … 203.8 ms    14 runs
 
Summary
  'git checkout ruff-format-2024; make lint-fix' ran
   29.11 ± 0.37 times faster than 'git checkout 8291_test; make lint-fix'

@hugovk
Copy link
Member

hugovk commented Aug 14, 2024

A better test on GHA will be comparing via pre-commit, but ~5s of CI time saved isn't so exciting when we have to wait around half an hour for the other jobs (or literally hours if wheels are involved).

It could be more useful shaving off minutes there rather than micro/seconds of CI here.

@radarhere radarhere closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants