-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Meta: Run check-peps.py in pre-commit #4071
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
This reverts commit f3276ef.
Updated branch to remove regexes from pre-commit. Installing the venv (PR) is a bit slower. Here's clearing the pre-commit cache then installing the hooks (but not running the checks): ❯ hyperfine \
--prepare "pre-commit clean; git checkout main" "pre-commit install-hooks # main" \
--prepare "pre-commit clean; git checkout lint-all-os" "pre-commit install-hooks # PR"
Benchmark 1: pre-commit install-hooks # main
Time (mean ± σ): 19.100 s ± 0.357 s [User: 10.244 s, System: 3.620 s]
Range (min … max): 18.661 s … 19.648 s 10 runs
Benchmark 2: pre-commit install-hooks # PR
Time (mean ± σ): 21.741 s ± 1.071 s [User: 11.817 s, System: 4.185 s]
Range (min … max): 20.493 s … 24.397 s 10 runs
Summary
pre-commit install-hooks # main ran
1.14 ± 0.06 times faster than pre-commit install-hooks # PR But the regexes are slow to run. Here's clearing the cache, installing hooks and running the checks. It's slightly slower with the PR, but not by much: ❯ hyperfine \
--prepare "pre-commit clean; git checkout main" "pre-commit run --all-files # main" \
--prepare "pre-commit clean; git checkout lint-all-os" "pre-commit run --all-files # PR"
Benchmark 1: pre-commit run --all-files # main
Time (mean ± σ): 23.344 s ± 1.101 s [User: 19.321 s, System: 4.555 s]
Range (min … max): 22.267 s … 25.755 s 10 runs
Benchmark 2: pre-commit run --all-files # PR
Time (mean ± σ): 23.743 s ± 2.272 s [User: 18.638 s, System: 4.660 s]
Range (min … max): 21.625 s … 28.937 s 10 runs
Summary
pre-commit run --all-files # main ran
1.02 ± 0.11 times faster than pre-commit run --all-files # PR However, most of the time, we'll have a cache. Here's running the checks with a cache: ❯ hyperfine --warmup 1 \
--prepare "git checkout main" "pre-commit run --all-files # main" \
--prepare "git checkout lint-all-os" "pre-commit run --all-files # PR"
Benchmark 1: pre-commit run --all-files # main
Time (mean ± σ): 5.865 s ± 0.556 s [User: 11.285 s, System: 1.496 s]
Range (min … max): 5.309 s … 7.075 s 10 runs
Benchmark 2: pre-commit run --all-files # PR
Time (mean ± σ): 2.911 s ± 0.150 s [User: 8.989 s, System: 1.003 s]
Range (min … max): 2.774 s … 3.274 s 10 runs
Summary
pre-commit run --all-files # PR ran
2.01 ± 0.22 times faster than pre-commit run --all-files # main Twice as fast with the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the trouble that it causes I'm half minded to suggest that we don't add check-peps.py
to pre-commit. I fully agree with removing the regular expressions.
The reservation I have that isn't just with pre-commit
's configuration is that if we recommend that PEP authors install the pre-commit
hooks locally (which I think we do), it could be quite an annoying experience to write a PEP with small, frequent commits. The script is for quality control on PEP submission rather than during the drafting process (e.g. the headings checks are important for us as editors, but authors rightly focus more on the content).
That being said, the regexes are currently in pre-commit
and we regularly have new PEPs that fail linting, so I'm not sure how frequently used pre-commit
is in practice by said authors.
Anyway, I have tested the PR at present on Windows and can confirm it does work (slowly!).
A
In contributing docs, you could recommend |
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
I don't think it's much trouble?
Small frequent commits should now be quicker -- it's 2x faster to run the script than the regexes (on macOS). Some people really don't like pre-commit as a Git hook, and that's perfectly fine if they just run it on the CI. But at least I find it valuable to run it locally. Anyway, whether we recommend pre-commit locally is orthogonal to this PR.
Indeed, and I think this is fine. CI is the safety net.
Good to hear! 🎉
Can you quantify? On macOS, when there's no cache, the cost of installing the venv and running the script is about the same as the savings of ditching the regexes. And when there is a cache, the PR is twice as fast running the script instead of regexes. |
Creating a virtual environment is unfortunately far, far slower on Windows than it is on Unix, so this may be related to that. ( |
Yep, it's not any slower than creating a normal venv, but that is itself slow. Single digit seconds, though. |
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
For #4063.
This will allow us to remove the monster regexes in pre-commit (and also
check-peps
fromlint.yml
).We had commented it out when
check-peps.py
was first added in #3275 in August 2023. I seem to remember we tried to enable it during the October 2023 sprint, but couldn't find an invocation that worked on Windows and macOS/Linux.Let's try again.
This passes locally for me on macOS:
Or
check-peps
directly:This PR temporarily runs pre-commit all operating systems on the CI, and it passes, but perhaps GitHub Actions has extra aliases.
Please could someone on Windows try this?
📚 Documentation preview 📚: https://pep-previews--4071.org.readthedocs.build/