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

[FEA] Make style CI and pre-commit hooks consistent #8193

Closed
charlesbluca opened this issue May 10, 2021 · 3 comments · Fixed by #8215
Closed

[FEA] Make style CI and pre-commit hooks consistent #8193

charlesbluca opened this issue May 10, 2021 · 3 comments · Fixed by #8215
Labels
feature request New feature or request

Comments

@charlesbluca
Copy link
Member

Is your feature request related to a problem? Please describe.
Running pre-commit run --all-files against the codebase results in various test failures, while the style CI reports these same tests as passing.

Some of these are related to the pre-commit hooks checking files that are typically ignored (such as ci/utils/nbtestlog2junitxml.py), while others seem to be related to the fact that the config used when running the hooks is different from that used in the style CI (namely isort, black, and clang-format).

Describe the solution you'd like
Either:

  • Changing ci/checks/style.sh to enforce the additional rules set by the pre-commit hooks
  • Changing the various configuration files used by pre-commit so that the checks pass on the codebase as is

I imagine the latter option would be preferable as it would avoid having to run the pre-commit hooks against the codebase in a large PR.

Describe alternatives you've considered
Not doing anything; this is purely a QOL thing and can be entirely ignored since the pre-commit hooks don't seem to outright conflict with the style CI.

@charlesbluca charlesbluca added Needs Triage Need team to review and classify feature request New feature or request labels May 10, 2021
@kkraus14 kkraus14 added gpuCI and removed Needs Triage Need team to review and classify labels May 10, 2021
@vyasr
Copy link
Contributor

vyasr commented May 10, 2021

The second issue you pointed out (inconsistency between the selectors used for black, isort and clang-format) is definitely just an inconsistency that can be quickly fixed. The first issue (pre-commit checking files that are typically ignored) is likely due to the fact that pre-commit's include/exclude logic doesn't always play nice with the include/exclude logic embedded in config files. I've seen many issues related to this before when trying to resolve this problem (here are two examples) and since the issues are spread between pre-commit and the different linters I think our best bet will be to follow the community's preferred solution, which is basically to duplicate the include/exclude logic between both .pre-commit-config.yml and the various linter-specific config files (e.g. .flake8, setup.cfg, pyproject.toml, etc).

@charlesbluca
Copy link
Member Author

charlesbluca commented May 11, 2021

After looking into this for a bit, I think most of the changes here should be relatively simple:

  • For clang-format, it looks its respective gpuCI script cpp/scripts/run-clang-format.py skips over some additional C++ directories that never got added to the DEFAULT_DIRS (cpp/libcudf_kafka, java/src/main/native); we could either skip those in the pre-commit hook or add them to the gpuCI script and reformat them, with my preference being the latter.
  • For flake8, it looks like the only issue here is that a Python script used for CI is getting picked up and failing the tests; this is already skipped by gpuCI and can be skipped in the pre-commit hook with a files key with matching regex; my preference is to do this, as the failures mostly seem due to handling of the regex strings anyways.
  • For black it's essentially the same thing as above, except it also picks up the docs config; the same solution should work.

A more complicated hook is isort; I definitely think we should change something about the gpuCI check here, as it looks like it only actually checks the first level of nested Python files for sorting (so python/cudf/setup.py would get resorted, but not python/cudf/cudf/datasets.py). This could be resolved by doing something like

isort --check-only --skip-glob *.pyx python

But even then, it looks like it doesn't pick up the same configuration that the pre-commit hook does; not sure if there's a way to explicitly set the config file here.

@charlesbluca
Copy link
Member Author

Also do we know if the separate configs for isort present in cudf, cudf-kafka, dask-cudf, and custreamz are all being picked up and applied properly? From the documentation on config files, it sounds like it would just pick the first config file it encounters and use that across the entire codebase.

rapids-bot bot pushed a commit that referenced this issue Jul 15, 2021
Closes #8193

Makes changes to the pre-commit config, gpuCI style script, and `cpp/scripts/run-clang-format.py` so that the styling of the pre-commit hooks should roughly match the style checks done by gpuCI. In particular:

- pre-commit's black and flake8 hooks now only target `python/` to match with gpuCI
- gpuCI's `cpp/scripts/run-clang-format.py` now runs `clang-format` on `cpp/libcudf_kafka/` and `java/src/main/native/` to match with pre-commit (also removed `cpp/include/cudf` and `cpp/include/nvtext` as they were redundant here)
- isort has been split up by project (cudf, dask-cudf, etc.) and now runs on each project using its respective configuration; this was done because currently the isort check on gpuCI was only checking Python files one level deep in each project

It would probably be good to run the updated hooks on the codebase either in this PR or a linked one so that this changes doesn't break tests.

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Mike Wilson (https://github.com/hyperbolic2346)
  - MithunR (https://github.com/mythrocks)
  - Richard (Rick) Zamora (https://github.com/rjzamora)
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Nghia Truong (https://github.com/ttnghia)
  - Jason Lowe (https://github.com/jlowe)

URL: #8215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants