[RFC] [ci] Enforce flake8 on Python code

[jameslamb]

Created based on this conversation with @jmoralez : #5559 (review).

---

Proposal

LightGBM should use flake8 in CI.

Motivation

flake8 is a static analyzer for Python that I've found very useful in catching efficiency and correctness issues like:

• unused imports
• unused variables
• unnecessary use of f-strings
• repeating dictionary keys (which silently ignores all but the last one)

And many more! ([full list of error codes])

flake8 by default has 4 types of checks:

• style (via pycodestyle)
• efficiency and correctness (via pyflakes)
• efficiency and correctness (via its own checks)
• code complexity (via mccabe)

It also has a rich ecosystem of optional plugins (see e.g. https://github.com/DmytroLitvinov/awesome-flake8-extensions).

LightGBM already enforces pycodestyle

LightGBM/.ci/test.sh

Line 80 in 0c0eb2a

pycodestyle --ignore=E501,W503 --exclude=./.nuget,./external_libs . || exit -1

So integrating this wouldn't change any of LightGBM's expectations for code style.

We've also had several instances over the last few years where flake8 was use to catch issues. Enforcing it in CI could have prevented those issues from being introduced at all:

• [python][tests] unused and missing imports #3023
• [ci] [python-package] remove unused import in tests #4233
• [python-package] remove unused imports #4850
• [python-package] remove unused imports #5559

flake8 can also be adopted gradually, via:

• ignoring specific lines with #noqa commments
• ignoring specific checks with --ignore command-line flag

Description

If this is accepted, the work would look like:

• address all flake8 warnings on master
• replace pycodestyle with flake8 in the lint CI task
• (maybe) put flake8 configuration for --ignore and --exclude in a file, instead of in CI scripts, for easier local development (docs for reference)

References

As of 0c0eb2a and the latest version of flake8 (v4.0.1), the following

flake8 \
    --ignore=E501,W503 \
    --exclude=./.nuget,./external_libs,./python-package/build/,./python-package/compile \
    .

Returns the following.

./tests/python_package_test/test_callback.py:6:1: F401 '.utils.pickle_obj' imported but unused
./tests/python_package_test/test_callback.py:6:1: F401 '.utils.unpickle_obj' imported but unused
./tests/python_package_test/test_dask.py:1720:29: F841 local variable 'client' is assigned to but never used
./tests/python_package_test/test_dask.py:1799:29: F841 local variable 'client' is assigned to but never used
./tests/python_package_test/test_sklearn.py:446:5: F841 local variable 'gbm_clone' is assigned to but never used
./python-package/setup.py:99:5: F841 local variable 'err' is assigned to but never used
./python-package/lightgbm/compat.py:39:5: F401 'matplotlib' imported but unused
./python-package/lightgbm/compat.py:46:5: F401 'graphviz' imported but unused
./python-package/lightgbm/dask.py:20:1: F401 '.basic._safe_call' imported but unused
./python-package/lightgbm/dask.py:24:1: F401 '.sklearn._LGBM_ScikitCustomEvalFunction' imported but unused
./python-package/lightgbm/sklearn.py:6:1: F401 'typing.Iterable' imported but unused
./docs/conf.py:108:5: F401 'sklearn' imported but unused

Request for Comment

@jmoralez @StrikerRUS will you please comment on this proposal when you have time?

I think LightGBM would benefit from adopting more static analyzers, and that flake8 is a powerful and easy one to add next.

[jmoralez]

I agree with this, as you pointed out it's been several times where we miss unused imports in reviews that would've been caught by it. Unused variables is also a good catch because sometimes they're just typos and are easy to miss in reviews if they manage to get past the tests.

[github-actions]

This issue has been automatically locked since there has not been any recent activity since it was closed.
To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues
including a reference to this.