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

fix pre-commit issues, add pre-commit CI job for PRs #39

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jameslamb
Copy link
Member

Working on #38, I noticed that this repo has a pre-commit config but that the code isn't actually passing those checks.

This PR proposes:

  • updating all the hooks (pre-commit autoupdate)
  • adding the RAPIDS copyright hook
  • limiting ruff to a specific subset of checks
    • (instead of ALL, which is very noisy and sometimes has conflicts)
  • fixing all the issues reported by pre-commit
  • adding a CI job that runs on PRs to check this

Notes for Reviewers

Any cosmetic changes were made automatically by pre-commit run --all-files.

Why add a CI job?

This repo now contains some non-trivial code (not just GitHub Actions configs). For example:

I think the small cost of a CI job on every PR is worth it in exchange for improved release confidence in changes to that code.

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jan 13, 2025
@jameslamb jameslamb changed the title WIP: fix pre-commit issues, add pre-commit CI job for PRs fix pre-commit issues, add pre-commit CI job for PRs Jan 13, 2025
@jameslamb jameslamb marked this pull request as ready for review January 13, 2025 20:52
@jameslamb
Copy link
Member Author

@raydouglass if you agree with the spirit of this PR, could you also make the pr workflow's checks job required for merging PRs?

Copy link

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

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

Approved with one point of discussion for other repos.

- uses: actions/checkout@v4
with:
fetch-depth: 1
- uses: pre-commit/[email protected]

Choose a reason for hiding this comment

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

Don't most of our repos just run pre-commit directly in their ci/check_style.sh script? I'm wondering if we can somehow integrate this into shared-workflows and reduce duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, many of the repos end up having a check_style.sh that does this:

  • set up a conda environment (which almost always just has pre-commit in it)
  • run pre-commit run --all-files

For those cases, just using the pre-commit action like this would be faster and simpler, I think. pre-commit already manages creating a venv with the dependencies, and that action comes with some across-multiple-runs caching built in.

HOWEVER... some repos do other stuff in check_style.sh.

For example, a couple download cmake-format files like this:

https://github.com/rapidsai/cudf/blob/bbf4f7824c23c0c482f52bafdf1ece1213da2f65/ci/check_style.sh#L21-L24

The pattern of running a ci/check_style.sh is there to enable those per-repo types of customizations, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if we can somehow integrate this into shared-workflows and reduce duplication.

Also worth noting... for this particular PR, I wouldn't want to introduce a shared-workflows dependency here. That'd create a circular dependency between shared-workflows and this repo, and I'm not sure what the implications of that would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants