Speedup pre-commit run by excluding python files from trailing hooks #11777
Conversation
trailing-whitespace and end-of-file-fixer are handled by both black and ruff check --fix This reduces the amount of files these hooks have to run on, and even skips the hooks entirely when running protobuf generation scripts or as a pre-commit hook on changed files.
| @@ -3,7 +3,11 @@ repos: | |||
| rev: v4.5.0 # must match requirements-tests.txt | |||
| hooks: | |||
| - id: trailing-whitespace | |||
| # speedup hook by leaving python files to Black & Ruff | |||
| exclude: .*\.pyi?$ | |||
There was a problem hiding this comment.
Black does not necessarily fix trailing whitespace in string literals
There was a problem hiding this comment.
That would only be multiline string literals right?
Do we even have those in stubs? Given we don't have docstrings (outside maybe generated protobuf files)
After checking, yes we do, in deprecated multiline messages, but it seems think Ruff does catch those, although it's considered an "unsafe-fix" (which is true and why black doesn't touch them):
That being said, if it was removed anyway by another pre-commit hook, I can just add W291 to our unsafe fixes hook.
There was a problem hiding this comment.
On that note, @AlexWaygood any reason not to use https://docs.astral.sh/ruff/settings/#lint_extend-safe-fixes instead of a separate pre-commit hook?
There was a problem hiding this comment.
no reason other than that I forgot about that option, I guess 😄
|
Do you have a rough estimation/numbers for how much time these hooks take as a % of the total time it takes for |
I'm currently heading out, but I can try to whip something up later to have actual numbers rather than just relying on "what seems to make sense" |
…mmit-exclude-python-from-trailing-checks
|
Did you get any numbers? My intuition would be that these hooks are really fast and the extra complexity in our configuration is not worth the minor performance saving. |
…mmit-exclude-python-from-trailing-checks
|
Note that in all my tests, I've ignored the first run to let the cache populate correctly, and avoid issues where a first run fixes EOL from switching back & forth between branches on Windows vs WSL.
The difference is indeed really small. But I come to realize why I had the impression the first hook was slow: Under WSL pre-commit is just extremely slow to get going, and it made it look like the first hook was really slow. The pre-commit command at the end of the proto scripts nearly 2m to run. --all with all hooks takes over 6m with black taking the vast majority of that time. "after tensorflow" command:
(the variance between runs, even after ignoring the first one, is also pretty bad) Anyway, I'll close with
and consider finishing porting the proto scripts to Windows git bash (or as Python script) so I don't have to run them under WSL. After looking it up, it's apparently a known thing that in WSL2 specifically, performance for access across OS (ie files under |
trailing-whitespaceandend-of-file-fixerare handled by bothblackandruff check --fixThis reduces the amount of files these hooks have to run on (by a lot considering typeshed is mostly sutbs), and even skips the hooks entirely when running protobuf generation scripts or as a pre-commit hook on changed files.