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] use ruff, enforce flake8-bugbear and flake8-comprehensions checks #5871

Merged
merged 6 commits into from
May 16, 2023

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented May 7, 2023

Proposes using ruff (https://github.com/charliermarsh/ruff) in this project's lint task, to enforce the following checks:

  • flake8 (pyflakes + pycodestyle)
  • flake8-bugbear
  • flake8-comprehensions
  • pydocstyle

Proposes enabling the following flake8 plugins (via ruff):

This PR also fixes the following types of issues found by flake8-bugbear...

B006 Do not use mutable data structures for argument defaults.  They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
B007 Loop control variable 'k' not used within the loop body. If this is intended, start the name with an underscore.
B009 Do not call getattr with a constant attribute value, it is not any safer than normal property access.
B023 Function definition does not bind loop variable 'init_score_value'.
B904 Within an `except` clause, raise exceptions with `raise ... from err`

... and these found by flake8-comprehensions

C401 Unnecessary generator - rewrite as a set comprehension.
C405 Unnecessary list literal - rewrite as a set literal.
C408 Unnecessary dict call - rewrite as a literal.
C416 Unnecessary list comprehension - rewrite using list().
full logs (click me)
tests/python_package_test/test_engine.py:889:20: C408 Unnecessary dict call - rewrite as a literal.
tests/python_package_test/test_engine.py:1774:28: C401 Unnecessary generator - rewrite as a set comprehension.
tests/python_package_test/test_engine.py:2863:16: C405 Unnecessary list literal - rewrite as a set literal.
tests/python_package_test/test_engine.py:2870:16: C405 Unnecessary list literal - rewrite as a set literal.
tests/python_package_test/test_basic.py:327:13: B007 Loop control variable 'k' not used within the loop body. If this is intended, start the name with an underscore.
tests/python_package_test/test_basic.py:368:29: C416 Unnecessary list comprehension - rewrite using list().
tests/python_package_test/test_basic.py:410:9: B007 Loop control variable 'k' not used within the loop body. If this is intended, start the name with an underscore.
tests/python_package_test/test_basic.py:422:13: B007 Loop control variable 'k' not used within the loop body. If this is intended, start the name with an underscore.
tests/python_package_test/test_basic.py:448:13: B007 Loop control variable 'k' not used within the loop body. If this is intended, start the name with an underscore.
tests/python_package_test/test_basic.py:755:17: C408 Unnecessary dict call - rewrite as a literal.
python-package/lightgbm/basic.py:1000:17: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
python-package/lightgbm/basic.py:1019:17: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
python-package/lightgbm/basic.py:1872:17: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
tests/python_package_test/test_dask.py:1065:77: B023 Function definition does not bind loop variable 'init_score_value'.
tests/python_package_test/test_dask.py:1067:72: B023 Function definition does not bind loop variable 'init_score_value'.
tests/python_package_test/test_sklearn.py:316:14: C408 Unnecessary dict call - rewrite as a literal.
tests/python_package_test/test_sklearn.py:318:19: C408 Unnecessary dict call - rewrite as a literal.
tests/python_package_test/test_sklearn.py:322:18: C408 Unnecessary dict call - rewrite as a literal.
tests/python_package_test/test_sklearn.py:353:14: C408 Unnecessary dict call - rewrite as a literal.
tests/python_package_test/test_sklearn.py:355:18: C408 Unnecessary dict call - rewrite as a literal.
tests/python_package_test/test_sklearn.py:358:18: C408 Unnecessary dict call - rewrite as a literal.
tests/python_package_test/test_sklearn.py:1142:16: C405 Unnecessary list literal - rewrite as a set literal.
tests/distributed/_test_distributed.py:109:70: B006 Do not use mutable data structures for argument defaults.  They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
tests/distributed/_test_distributed.py:137:56: B006 Do not use mutable data structures for argument defaults.  They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
python-package/lightgbm/callback.py:304:25: C401 Unnecessary generator - rewrite as a set comprehension.
python-package/lightgbm/dask.py:790:28: C401 Unnecessary generator - rewrite as a set comprehension.
python-package/lightgbm/basic.py:1802:23: B009 Do not call getattr with a constant attribute value, it is not any safer than normal property access.
python-package/lightgbm/basic.py:1804:37: B009 Do not call getattr with a constant attribute value, it is not any safer than normal property access.
python-package/lightgbm/basic.py:1920:28: C416 Unnecessary list comprehension - rewrite using list().
python-package/lightgbm/basic.py:2478:64: C414 Unnecessary list call within sorted().
python-package/lightgbm/basic.py:2777:42: C416 Unnecessary list comprehension - rewrite using list().

Benefits of these changes

ruff...

  • ...implements flake8 and a ton of other checks (see the list under "Rules" at https://pypi.org/project/ruff/)
  • ...is RIDICULOUSLY fast relative to flake8

These two new flake8 plugins...

... reduce variability in code style across the project's Python code.

... for some cases, very very very slightly improve efficiency. For example, using a set literal ({}) is a bit more efficient than constructing a list, creating a set from it, and immediately throwing away that list.

# e.g. this...
set([x, y, z])

# is a bit slower and uses a bit more peak memory than
{x, y, z}

... push into automation something that previously required maintainer effort in pull requests... preventing the use of mutable data structures as function argument defaults.

Notes for Reviewers

I originally opened this PR just trying to add flake8-bugbear and flake8-comprehensions, but then switched it to adding ruff based on @jmoralez 's recommendation in #5871 (comment).

@jmoralez
Copy link
Collaborator

jmoralez commented May 9, 2023

Have you seen ruff? I remember one of the reasons to adopt it was that it's a single dependency that has many linters, for example I see these two in there. What do you think of using that instead?

@jameslamb
Copy link
Collaborator Author

Sure let's try it!

We can use this PR to test that out, I'll push some changes later.

@jameslamb jameslamb changed the title [ci] [python-package] enforce flake8-bugbear and flake8-comprehensions checks [ci] [python-package] use ruff, enforce flake8-bugbear and flake8-comprehensions checks May 11, 2023
@jameslamb
Copy link
Collaborator Author

@jmoralez I tried ruff tonight ... it's AWESOME. I'm usually very skeptical of new Python dev tools but wow... cannot say strongly enough how impressive this is.

Great suggestion 🤩

It's ridiculously fast and really easy to configure, following these docs:

I just pushed some commits that propose using ruff to run pydocstyle and flake8.

I chose NOT to have it run isort, even though it can, because it currently has some inconsistencies with isort (astral-sh/ruff#2104 and astral-sh/ruff#1718, for example) that result in errors on this branch. Maybe we could explore that in the future.

Copy link
Collaborator

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

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

LGTM. Just a suggestion but it's fine to leave it out of this.

python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
@jameslamb jameslamb merged commit d47006f into master May 16, 2023
@jameslamb jameslamb deleted the ci/flake8-plugins branch May 16, 2023 03:49
@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 15, 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.

2 participants