-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Use pre-commit-uv in CI #17092
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
Use pre-commit-uv in CI #17092
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.
How much do you think this is likely to buy us? Testing locally, it still seems to take pre-commit a while to setup the many virtual environments it needs if the pre-commit cache is cold, even with pre-commit-uv. But if the pre-commit cache is hot (which it usually is in our CI), that doesn't cost us anything at all.
I'm also not sure I love the monkeypatching approach pre-commit-uv takes. I appreciate that there's not really any other way to get pre-commit to use uv internally, but it seems kinda fragile. If it bought us a lot, that would be one thing... but it seems like it might not?
I've locally installed my global
Looks like it, yeah. The pre-commit lint in this branch is approximately the same speed as in
I did try for more first-class support once upon a time... pre-commit/pre-commit#3131
Yeah, I guess it's not worth it here – feel free to close if you like. FWIW, I wrote a simple action (https://github.com/akx/pre-commit-uv-action/) to do this that I use for some of my projects :) Now that I am investigating pre-commit, though, running the same incantation as in the CI step locally (MacBook Pro, no slouch), with Looking at It could be worth it to limit EDIT: Oh... I guess it's the WASI-compiled shellcheck that's slow and heavy, but even more so on a GitHub Actions machine that's starved for cores. EDIT 2: See #17108 🔧 |
yup, I'm aware of the history 😄 thanks for your efforts there!
Nice. ISTM that it probably has much more of an impact locally than in CI, though. So I'm inclined not to go with this. But thanks anyway!! |
Summary
Follows up on #17052 to add in pre-commit-uv, which patches
pre-committo useuvfor venv creation as well.More dogfooding! More speed! 😄
Note
I suspect caching could be removed from the
pre-commitCI step with this...Test Plan
We're about to find out...