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

Run linters using pre-commit #965

Merged
merged 11 commits into from
Nov 10, 2022
Merged

Conversation

benfred
Copy link
Member

@benfred benfred commented Oct 28, 2022

This change adds a pre-commit hook to run on each commit, that will automatically run code linters and not allow you
to commit changes if the linters fail. This is similar to how cudf uses pre-commit, as described in rapidsai/cudf#9309 . The pre-commit checks will also be run in CI as part of the check style script.

This change also adds several new linters that weren't being run in CI before:

  • pydocstyle
  • mypy
  • black
  • isort
  • cmake-format
  • cmake-lint

Of these new linters - mypy caught an issue where the test_comms.py would always be skipped (python/raft-dask/raft_dask/test/test_comms.py:23: error: Module "raft_dask" has no attribute "Comms" ), and Pydocstyle caught some minor formatting inconsistencies in the python docstrings. black/isort ensure consistent code formatting for the python raft code

This change adds pre-commit hook to run on each commit, that will automatically
run code linters and not allow you commit changes if the linters fail. This
is similar to how cudf uses pre-commit, as described in rapidsai/cudf#9309 .
The pre-commit checks will also be run in CI as part of the check style script.

This change also adds several new linters that weren't been run in CI before:
* pydocstyle
* mypy
* black
* isort
* cmake-format
* cmake-lint

Of these new linters - mypy caught an issue where the test_comms.py would always be
skipped (`python/raft-dask/raft_dask/test/test_comms.py:23: error: Module "raft_dask"
has no attribute "Comms"` ), and Pydocstyle caught some minor formatting
inconsistencies in the python docstrings. black/isort ensure consistent code
formatting for the python raft code
@benfred benfred requested review from a team as code owners October 28, 2022 23:33
@benfred benfred added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function and removed cpp CMake gpuCI python labels Oct 28, 2022
Add pylibraft to known first party imports for isort
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

I think the workflow that results from these hooks is going to be quite efficient and I'm really looking forward to these changes. I did a pass through the changes and I'm going to check out your branch and play with the hooks locally as well.

cpp/cmake/config.json Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
ci/checks/style.sh Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Nov 7, 2022

I'm a fan of this change and am happy to provide any extra info you need on how this works in cudf or help address any specific issues anyone is concerned about!

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

I checked this out locally and played with a bunch of the linters. Really digging the cmake formatting.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

🔥 🎉 Thank you very much for this! I am planning to do similar changes for rmm and other libraries. I checked closely and this PR contains a lot of the minor fixes that I've adopted in cuDF for various issues with configurations.

@@ -0,0 +1,98 @@
# Copyright (c) 2022, NVIDIA CORPORATION.

repos:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll mention it here because I haven't fixed it yet in cuDF and this is a new file: the indentation of this file is ...aggressive. I'd prefer to see something more like 2 or 4 spaces instead of 8 spaces in some places and 2 or 4 in others. Maybe there's a standard YAML formatter that can be run once (no need to enforce the format with tooling, just get it into a normalized state).

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually add https://github.com/jumanjihouse/pre-commit-hook-yamlfmt to the pre-commit hooks for this reason.

ci/checks/style.sh Show resolved Hide resolved
@cjnolet
Copy link
Member

cjnolet commented Nov 10, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 560c66b into rapidsai:branch-22.12 Nov 10, 2022
@benfred benfred deleted the pre-commit branch November 10, 2022 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
Development

Successfully merging this pull request may close these issues.

7 participants