Move flake8 to ci_lint

[Mousius]

This fixes the scenario where you lint with ci_lint but it can still
fail in PR due to flake8 being injected only into the Mac build.

[Mousius]

This is going to need to be followed up by another patch to remove it from the MacOS workflow and enable the lint script again once Docker has been rebuilt @leandron @areusch

[comaniac]

Interesting. I didn't realize that Mac build uses flake8 in addition to pylint. The flake8 was introduced by #4688 but only in the Github action. I'm wondering if the flake8 rules being tested (i.e., E9,F63,F7) have the corresponding one in pylint, so that we only need to change the pylintrc?

Also cc @tqchen

[Mousius]

@comaniac, that's a good thought actually, we run only a reduced set of of flake8 options. Looking at the errors enabled:

• E9 looks like errors which would immediately error when tests start executing?
• F63 isn't covered much in pylint but it seems to cover some interesting edge cases in Python
• F7 is mostly covered by pylint and I'd suggest that mypy covers most of the missing checks in tandem with running the doctest's if we had them.

Flake8	Pylint	Description
E901		SyntaxError or IndentationError
E902		IOError
F631	W0199	assertion test is a tuple, which is always True
F632		use ==/!= to compare str, bytes, and int literals
F633		use of >> is invalid with print function
F634		if test is a tuple, which is always True
F701	E0103	a break statement outside of a while or for loop
F702	E0103	a continue statement outside of a while or for loop
F703	E0116	a continue statement in a finally block in a loop
F704	E0105	a yield or yield from statement outside of a function
F705	E0106	a return statement with arguments inside a generator
F706	E0104	a return statement outside of a function/method
F707	E0701 ?	an except: block as not the last exception handler
F721		syntax error in doctest
F722		syntax error in forward annotation
F723		syntax error in type comment

[comaniac]

Thanks for the investigation! Looks like E9 and E7 cover a set of checking, and they indeed have some mismatches with pylint. I'm not sure about the best practice either and would like to hear feedbacks from others.

@cclauss would you please advise as well?

[cclauss]

https://flake8.pycqa.org/en/latest/user/error-codes.html

On the flake8 test selection, this PR should not focus on "style violations" (the majority of flake8 error codes that psf/black can autocorrect). Instead, these tests should focus on runtime safety and correctness:

• E9 tests are about Python syntax errors usually raised because flake8 can not build an Abstract Syntax Tree (AST). Often these issues are a sign of unused code or code that has not been ported to Python 3. These would be compile-time errors in a compiled language but in a dynamic language like Python, they result in the script halting/crashing on the user.
• F63 tests are usually about the confusion between identity and equality in Python. Use ==/!= to compare str, bytes, and int literals is the classic case. These are areas where a == b is True but a is b is False (or vice versa). Python >= 3.8 will raise SyntaxWarnings on these instances.
• F7 tests logic errors and syntax errors in type hints
• F82 tests are almost always undefined names which are usually a sign of a typo, missing imports, or code that has not been ported to Python 3. These also would be compile-time errors in a compiled language but in Python, a NameError is raised which will halt/crash the script on the user.

[areusch]

thanks @Mousius i think this does make sense. i sort of think ci-lint should be the one source of lint, rather than the mac build, because dependencies may differ between the two installations. will leave the PR unsubmitted til tomorrow to allow the other reviewers to comment.

[junrushao]

My only concern is if there is guarantee that flake8 and pylint won't conflict. Happy to merge it in if it is not the case

[cclauss]

Conflict should not be a problem…
https://github.com/PyCQA/pylint
https://github.com/PyCQA/flake8
https://github.com/PyCQA/bandit
https://github.com/PyCQA/isort

This repo currently has 574 instances of flake8 F821 undefined name.

[junrushao]

Thanks for the PR @cclauss! Thanks @areusch @comaniac @electriclilies for the discussion!