replace pre-commit with prek#402
Conversation
|
How much faster is it (before vs. after runtime comparison)? |
there are multiple things to measure here. local runs without cached venvs (first run):
local runs with cached venvs (any but the first run):
I'm not sure how much faster/ slower my laptop is than the github actions machine, but it probably wont make a massive difference for cicd, as we are using cached runs. however, prek supports running hooks in parallel, which i've added conservatively. this does tend to give a small speedup. |
|
hmm prek on CI seems to take 160s... see here |
lets take the logs you are referring to apart: prek took about 106 sec comparing this to a pre-commit run, we can see that, as i mentioned before, when the hooks are installed, they both run at very similar speeds. conclusion: i personally would recommend switching to prek.
|
|
Thanks for the detailed breakdown! I'm close to be convinced. just:
What do you mean by "and then remember the cicd and immediately push another commit to fix pre-commit"? Also, we would need to have a (very) short "migration guide" (I guess in this PR description is fine). And the arguments you list above should also go into the main PR description. And the main readme needs to be adjusted. |
this is probably just me being silly, but i have a tendency to forget running pre-commit before committing and pushing on about a quarter of my commits or so. whenever that happens, cicd fails because some hook found some left over whitespace somewhere, and i need to run pre-commit locally, commit and push again and then the cicd passes. if you do the prek install -f, git will run prek whenever you're trying to commit, and git will only commit if prek passes. this can be annoying, for sure, but it also makes sure that all commits come with a stamp of approval and all cicd runs pass (at least the pre-commit half) |
ah ok. so the same as installing the pre-commit git hook |
I had adjusted the main readme yesterday. please let me know whether that would do it, or what you think is lacking/ wrong. |
ah, sorry, you are right. it is fine like that |
There was a problem hiding this comment.
Just some minor remarks below regarding the PR description.
General: Please check the diff view on GitHub and ensure that all changes are documented in the PR description. And use real sentences in the PR description please (don't make it harder to read than necessary).
When this is merged, please cc all relevant devs of this project (I guess Leo and Amir are sufficient) at the end of the PR description so that they are aware of this change. And also mention it in our next DFKI-internal meeting to be double-sure.
I've edited the pr description. is it proper sentence enough or would you like me to properify it (and how)? |
My primary concern was/is that I think sentences should start with a capital letter. |
Ah, interesting point. I can do that for you 👍 |
|
Any other comments, or shall we merge? |
Looks good. Thanks for the adjustments! :-) |
2 participants
pre-commitis a slow relict of the past.[prek](https://github.com/j178/prek)is a drop in replacement that is much faster at setting up the hooks on the first run, and it supports nice features beyond pre-commits capabilities.HOW-TO: migrating from pre-commit to prek
Whenever you would've run a command like
uv run pre-commit run -a, just replacepre-commitwithprek.Before each commit, i would reccommend running
uv run prek run -ato make sure that the cicd will be passing.If you want to run prek on each commit to make sure you are not pushing something that fails the cicd, install prek as git hook:
uv run prek install -fThat way git will run prek on each commit, and prevent commits that fail prek.
If you want to add a new hook to prek, take into consideration when it should run and set the priority appropriately.
Hooks run in the order of their priority, starting at 0. hooks with the same priority run simultaneously.
@amisfar @leonhardhennig
main result:
prekneeds no dependencies.preks initial setup is about four times as fast.details:
pre-commitpackage and addedprekto our environment. This reduces the amount of dependecies in ouruv.lock. (prekhas no dependencies)pre-commithooks. This allows them to run simultaneously. For more clarity in the priority system, i have reordered some of the hooks. I did not change which hooks we are running, I just changed the order. Sadly that makes the git diff look quite confusing.prekgit hook is helpful. (But so was the one of pre-commit. no change here. I just wasn't aware of it)preksupports separate workspaces and other features for larger monorepos which we might want to look into in the future.prekis a drop in replacement. Adjustments were made to improve the performance further, not because compatability would have required them.pre-commitin our main readme has been adapted toprek.make precommitcommand has been adapted to callprekinstead.prek.