pytorch
Add flake8 pre commit hook script
PyTorch's pre commit hooks, scripts that are called are in tools
Really I selfishly just want the flake8 ones so I don't have to remember to run it against my changes each time. We could also get the clang tidy info while we're in there
@samdow @zou3519 @Chillee are you guys all using pre-commit ? It may make sense to add this hook and such that we could avoid PRs with flake8 issue fixes :) What do you think ?
@vfdev-5 I am because I set it up for PyTorch development (this assumed that most people would be coming from PyTorch development so would already have it set up but we could also add some instructions to set it up in case someone isn't).
It would be really great to add this! I just haven't had the bandwidth to try out a copy/paste
I don't use pre-commit but I'm down to use it, I get annoyed at having to manually format code
@samdow if I understand correctly https://github.com/pytorch/pytorch/blob/master/tools/git-pre-commit is for C/C++ and related code ? I mean it wont fix python flake8 issues, right ? The guide "pre-commit-tidylinting-hook" is saying about flake8 errors :
Fix the code so that no errors are reported when you re-run the above check again, and then commit the fix.
EDIT:
We can maybe add the following to local .git/hooks/pre-commit:
cat .git/hooks/pre-commit
#!/bin/bash
set -e
echo "Run flake8 ..."
flake8 .
So, on commit it will run flake8 and just report errors
@vfdev-5 Ahh good eye! I'm seeing that they actually recently removed the flake8 integration about a month ago. This is the issue that talks about it. It looks like we can add it as a hook using this framework.
If this seems like too much, I'm happy with your solution. It might run flake over some files that you don't intend to check in, but that seems like a small price if it makes it significantly easier
@samdow yes, in my previous message I was talking about this pre-commit tool: https://github.com/pytorch/functorch/issues/512#issuecomment-1108752994 It's very common tool used in oss: it uses predefined versions and can handle different code formatting tools. Here is example in torch vision: https://github.com/pytorch/vision/blob/main/.pre-commit-config.yaml
@vfdev-5 I see! Sorry I misunderstood. I think setting that up is fine. Either using the script like above like described in the issue seems like it should be fine (but not certain about that)
To your other point that I didn't answer earlier,
I mean it wont fix python flake8 issues, right ?
I think this is fine for now. At least personally I appreciate knowing on commit so that I can fix it before I push it. There might be a way to automate the fix in the future, but I think having the signal for now is good (and if PyTorch isn't doing it, it feels like it might be non-trivial?)
I agree, let's do in a simple way.
There might be a way to automate the fix in the future, but I think having the signal for now is good (and if PyTorch isn't doing it, it feels like it might be non-trivial?)
Yes, to fix any flake8 error is not always possible. Majority of them about code formatting can be fixed with autopep8 or black. Black brings also code structure formatting. autopep8 can be configured to fix certain warnings/error that flake8 reports (but this tool wont be able to fix any flake8 error). One can run autopep8 in pre-commit hook. Another option is to put it into a GHA and commit a fix if a job checking flake8 fails on main branch... We can keep that in mind anyway.