Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ci] [python-package] enforce flake8 checks (fixes #5566) #5659

Merged
merged 4 commits into from
Jan 12, 2023

Conversation

jameslamb
Copy link
Collaborator

Fixes #5566.

Proposes enforcing flake8 checks on all Python code maintained in this repo, for the reasons described in #5566.

This also fixes the following warnings found by flake8

./tests/python_package_test/test_engine.py:2906:9: F841 local variable 'bst' is assigned to but never used
./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:1727:29: F841 local variable 'client' is assigned to but never used
./tests/python_package_test/test_dask.py:1806:29: F841 local variable 'client' is assigned to but never used
./tests/python_package_test/test_sklearn.py:447: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
./docs/conf.py:108:5: F401 'sklearn' imported but unused

Notes for Reviewers

In a followup PR, I might also propose also adding some flake8 plugins like flake8-bugbear. But didn't want to make this PR any larger.

@@ -445,9 +445,19 @@ def test_clone_and_property():
gbm.fit(X, y)

gbm_clone = clone(gbm)

# original estimator is unaffected
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

via this warning...

./tests/python_package_test/test_sklearn.py:447:5: F841 local variable 'gbm_clone' is assigned to but never used

...flake8 caught that this test wasn't really checking the correctness of sklearn.clone() applied to an LGBMRegressor. It was basically just checking that clone() ran without error.

I chose to fix the warning by also just making the test stricter.

See https://scikit-learn.org/stable/modules/generated/sklearn.base.clone.html#sklearn.base.clone for details on what this function does.

@jameslamb jameslamb changed the title WIP: [ci] [python-package] enforce flake8 checks (fixes #5566) [ci] [python-package] enforce flake8 checks (fixes #5566) Jan 3, 2023
@jameslamb jameslamb marked this pull request as ready for review January 3, 2023 15:15
@jameslamb jameslamb merged commit 02d212b into master Jan 12, 2023
@jameslamb jameslamb deleted the ci/enforce-flake8 branch January 12, 2023 21:59
@github-actions
Copy link

This pull request 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] [ci] Enforce flake8 on Python code
2 participants