Make gpuCI and pre-commit style configurations consistent

[charlesbluca]

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.

[charlesbluca]

Some minor quirks around the behavior of isort:

[charlesbluca]

For some reason, the pre-commit hooks for isort automatically skip Cython files, while the CLI does not, so I had to specify to skip them here. Do we want package sorting for Cython files?

[charlesbluca]

Looks like we can get Cython/PYI resorting in pre-commit's isort hook with:

types: [text]
types_or: [python, cython, pyi]

So if we would like package resorting on Cython and/or module files it's now totally doable.

[mythrocks]

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.

The fact that this runs on java/src/main/native/ makes me a tad nervous. Not to imply that I disagree; I would prefer the formatting be applied too. It's just that this implies a fair bit of formatting changes to the existing JNI/native code.
cc @revans2, @jlowe.

[mythrocks]

I think this PR would need to include the results of running run-clang-format on cpp/libcudf_kafka and java/src/main/native. For the moment, we should include this as a separate commit, so as not to muddy the review.

Also, @charlesbluca, could you please confirm that as a result of this commit, the intention is for clang formatting to run on cpp/libcudf_kafka and java/src/main/native, even for PRs where [skip-ci] is specified?

I ask because java/src/main tests (even the native ones) are not currently run as part of CI for historical reasons. These PRs (e.g. #8209) are submitted with [skip-ci] so as not to use up CI resources in rerunning unrelated libcudf tests.

It would be good to have [skip-ci] not skip clang-format checks in libcudf_kafka and java. Otherwise, the errant code would get merged, and fail in other PRs that don't [skip-ci].

[charlesbluca]

For the moment, we should include this as a separate commit, so as not to muddy the review.

I agree - I was planning to run through the pre-commit hooks once all the config changes were ironed out.

the intention is for clang formatting to run on cpp/libcudf_kafka and java/src/main/native, even for PRs where [skip-ci] is specified?

No, the intent is so that when gpuCI is ran, it will run clang-format on cpp/libcudf_kafka and java/src/main/native, in addition to all the other (in your case, unnecessary) tests. What you're describing does sound like a good idea though, but I'm not really sure how gpuCI handles conditionals for testing (i.e., only run this set of tests if these files are impacted by a PR).

A relatively simple workaround for this which would address the situation you described and also make this PR a little simpler would be to make a GitHub Actions workflow running a pre-commit action (or using precommit.ci, which seems to be its successor). This would ensure that the style checks being ran when making commits and opening PRs are always the same, since they rely on identical configs; it would also create a workflow to check for formatting independent of if a PR is submitted with [skip ci].

That being said, I'm not sure of the potential downsides to moving aspects of our testing outside of gpuCI.

[ajschmidt8]

Approving ops-codeowner file changes

[charlesbluca]

To update the separate style CI issue, rapidsai/ops#1580 has been opened - @mythrocks, would you prefer that get resolved before merging this?

[harrism]

@charlesbluca what needs to happen to get this in? We are entering code freeze tomorrow.
@mythrocks can you answer the question above?

[charlesbluca]

I think this would be better retargetted to 21.08; the C++ formatting is generally sorted out, but I would like more of a conversation around if we want to do package sorting for Cython code. Other than that, we would need to run the updated hooks against the codebase and make a lot of formatting changes, which we probably don't want to do this close to release.

[mythrocks]

Sorry I missed this question. Please go ahead with this change.
I wanted to avoid cases where skip-ci PRs were checked in, and broke unrelated PRs for others. We can address that later, if that's more acceptable.

[charlesbluca]

Thanks @mythrocks! Now the only blocker on this is rapidsai/integration#286, which would allow us to resort packages in .pxd files.

[charlesbluca]

To be specific on the failure that happened in python/dask_cudf/dask_cudf/io/tests/test_json.py, it looks like isort=5.0.7 doesn't allow imports like:

import dask.dataframe as dd

instead preferring:

from dask import dataframe as dd

Whereas isort=5.6.4 generally allows either of these import styles. Do we have a preference for which style we want to enforce? There might be a setting which forces imports to be one or the other.

[galipremsagar]

Whereas isort=5.6.4 generally allows either of these import styles. Do we have a preference for which style we want to enforce? There might be a setting which forces imports to be one or the other.

I'd suggest to keep it simple as:

import dask.dataframe as dd

[vyasr]

fwiw import dask.distributed as dd is also faster than from dask import distributed as dd. Generally a negligible microoptimization, but for function-local imports that can matter.

[charlesbluca]

I couldn't find an option to force that specific import style so I just skipped isort for that specific import; once rapidsai/integration#286 is merged, I can remove that skip

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@3ed87f3). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 0436347 differs from pull request most recent head 367e1cd. Consider uploading reports for the commit 367e1cd to get more accurate results

@@               Coverage Diff               @@
##             branch-21.08    #8215   +/-   ##
===============================================
  Coverage                ?   10.67%           
===============================================
  Files                   ?      109           
  Lines                   ?    18669           
  Branches                ?        0           
===============================================
  Hits                    ?     1993           
  Misses                  ?    16676           
  Partials                ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ed87f3...367e1cd. Read the comment docs.

[galipremsagar]

Can you update the copyright year for all the files?

[rjzamora]

Dask-cudf changes seem fine - Just left a minor question

[rjzamora]

Why the override here?

[charlesbluca]

Now that gpuCI's isort==5.0.7 check is targetting this file, it want to change this line to

from dask import dataframe as dd

I couldn't find an option for isort to allow this type of import, but can confirm that isort==5.6.4 (which we are planning to bump gpuCI to after this PR is merged) allows either style of import, so this override can be removed once rapidsai/integration#286 is merged.

[jlowe]

Java approval

[charlesbluca]

@gpucibot merge

[charlesbluca]

After playing around with Cython resorting for a while, I noticed a quirk of isort means we probably aren't seeing nearly as much resorting as we would if our pyx/pxd files used exclusively cimport statements. I've opened up an issue describing this in PyCQA/isort#1787, so that hopefully we can make some modifications to our setup.cfg files to account for this.