Setup pre-commit.ci

[jakirkham]

To make it easier to use pre-commit, it would be great to have https://pre-commit.ci configured for all RAPIDS projects. We can also make this a requirement before running other CI

Additionally we could leverage pre-commit.ci to help us fix issues with its bot. Though would recommend that disable autofixing. Otherwise the bot pushes changes into PRs fairly aggressively. It would be better to have users request fixes from the bot. This can be done in .pre-commit-config.yaml like so

ci:
  autofix_prs: false

We may also want to add an exception in our own CI configuration to allow commits from the pre-commit bot when considering whether to run CI

[vyasr]

There are a number of reasons why we haven't been able to use it in the past and have opted for running pre-commit ourselves. I can link you to threads, please feel free to let us know if any of those reasons have materially changed. I'd love to use the service if possible.

[jakirkham]

Links would be helpful

Or simply a comment here with a list of outstanding issues. Perhaps we can use that as a way to close them out one-by-one so as to unblock this issue

[bdice]

We had some offline discussions about this. CCCL and cuCollections are using pre-commit.ci but there are some limitations.

• We can't run our doxygen checks through pre-commit.ci because it requires a package version that is hard to install through any means supported on pre-commit.ci (but easy via apt or conda -- and the pre-commit "conda" hooks are not viable for several reasons, including a hard requirement on having conda and being willing to put up with very slow initial pre-commit runs while conda packages install)
• Getting the bot to push to our PRs is apparently possible (CCCL does it) but it introduces an untrusted commit, so we have to comment /ok to test for all subsequent commits. This is nonviable for autofixes, only manually triggered fixes (by a comment like pre-commit.ci autofix)

Nonetheless, we are in agreement that this is worth trying. I will open a PR for RMM and request for ops to install the application in the rapidsai org so we can try it out.

[bdice]

Things seem to be working well in RMM aside from verify-alpha-spec (see rapidsai/pre-commit-hooks#58) so I would like to propose enabling this in more repos starting with cuDF.

cc: @davidwendt @vyasr

[jakirkham]

This was done for cuDF in PR: rapidsai/cudf#17795

[mroeschke]

Regarding the roll out.

1. When enabling pre-commit.ci, I think some (most?) repos have a ci/check_style.sh that also runs pre-commit which will now be redundant with pre-commit.ci
2. Before, the pre-commit checks were required to pass before kicking off other jobs. Now (at least shortly after Enable pre-commit.ci cudf#17795), other jobs can still run even if pre-commit.ci fails. Is that a desired change? There is an incremental risk of our style checks now dependent on an external service.

[vyasr]

The plan for the immediate future is to have both our style checks run and pre-commit CI at the same time. The primary purpose of the latter is to support autofixing so that we don't need to always make local change, although of course this introduces unverified commits which is annoying. The main beneficial case is to not have to do local work when CI indicates a style issue. It remains to be seen how many people will actually leverage this feature. I think between rmm and cudf we should get enough data in a few months to make a call. @bdice have we seen anyone actually use the autofix in rmm yet? I don't think so, but I could have missed something. I guess what I'm really asking is, what is our criteria going to be for determining whether pre-commit.ci is worth it given that we are going to have to maintain a separate style check CI run anyway for the foreseeable future?

[bdice]

I expect this to be useful in a few cases:

• working with external collaborators
  • external folks can know whether style checks pass without any /ok to test
  • using the autofixer is fine because we already require /ok to test
• final fixes for a PR with style issues
  • using the autofixer is fine and then you can /ok to test and /merge immediately after
  • handy for avoiding the need to go back to a terminal -- just the web interface is enough to make things merge-ready

I have used the autofix in RMM, but mostly as a test.

For me, the "worth it?" criteria is trivially satisfied. It gives us small but meaningful benefits at nearly zero ongoing cost.

[bdice]

Also, pre-commit.ci is faster than our style check jobs. You can get feedback in seconds rather than waiting 2-3 minutes.

[mroeschke]

Yup, I'm sold on having pre-commit.ci run across RAPIDS.

My leftover curiosity, mainly for the roll out, is if pre-commit.ci failing will block other CI jobs from running like current style checks do. But I guess since they will run together for the immediate future and should be equivalent checks, the old style checks failing will still serve to block other CI jobs from running.

[bdice]

Exactly. pre-commit.ci should be non-blocking (not required to merge). That way we won't be blocked by odd behavior or anything outside of our control. Because pre-commit.ci is a subset of the full style checks (it doesn't support what we need for some checks), we plan to keep running the existing self-hosted style check jobs.