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

add rapids-dependency-file-generator pre-commmit hook #682

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

jameslamb
Copy link
Member

Proposes adding a pre-commit hook to run rapids-dependency-file-generator.

With that change, local pre-commit and CI will raise an error if changes have been made in dependencies.yaml that would affect any other files checked into the repo, like the conda envs in https://github.com/rapidsai/cucim/tree/branch-24.02/conda/environments.

How I tested this

Changed a cmake version constraint in dependencies.yaml.

Ran pre-commit run --all-files.

Observed the expected failure.

image

Reverted that change, ran pre-commit run --all-files again, saw everything pass.

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jan 24, 2024
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.

@jameslamb Also FYI: rapidsai/shared-workflows#46 (comment)

There's a list of a few repos needing this update.

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.

Also we can disable the dependency check from the checks job now that it's covered by style checks.

Like this in .github/workflows/pr.yaml:

  checks:
    secrets: inherit
    uses: rapidsai/shared-action-workflows/.github/workflows/[email protected]
    with:
      enable_check_generated_files: false

Example: https://github.com/rapidsai/cudf/pull/12819/files

@jakirkham
Copy link
Member

Regarding adding enable_check_generated_files. Would we actually just make that false by default (or drop that flag altogether) once pre-commit support for rdfg is added throughout RAPIDS?

@bdice
Copy link
Contributor

bdice commented Jan 26, 2024

Regarding adding enable_check_generated_files. Would we actually just make that false by default (or drop that flag altogether) once pre-commit support for rdfg is added throughout RAPIDS?

Yes. After we add pre-commit hooks we can change the default to false and later remove that option.

@jameslamb jameslamb requested a review from a team as a code owner January 26, 2024 15:06
@jameslamb
Copy link
Member Author

Also FYI: rapidsai/shared-workflows#46 (comment)

There's a list of a few repos needing this update.

Thanks! Hadn't seen that before! I can make those changes in those other repos right now, shouldn't take that long.

Also we can disable the dependency check from the checks job now that it's covered by style checks.

done in 4cf17d3

@jameslamb
Copy link
Member Author

done in 4cf17d3

On second thought... I just reverted that.

@bdice instead of checking in configuration like this in all the repos:

with:
      enable_check_generated_files: false

I think it'd be preferable to just add those pre-commit hooks to the remaining repos, and then remove the rapids-dependency-file-generator functionality entirely from the configuration of checks in shared-workflows.

That'd avoid the need to come back and do another round of PRs removing the with: enable_check_generated_files from configs here. Re-reading it, I think that's what @jakirkham was implying in #682 (comment) too.

What do you think?

@bdice
Copy link
Contributor

bdice commented Jan 26, 2024

That'd avoid the need to come back and do another round of PRs

That is smarter. Do that!

@jakirkham
Copy link
Member

Yep thinking the same thing. We can apply those cycles to some other goal

@jameslamb
Copy link
Member Author

I can make those changes in those other repos right now

This didn't happen. I did search for which ones need it and summarized that at rapidsai/shared-workflows#46 (comment).

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 9d6fc91 into rapidsai:branch-24.04 Jan 30, 2024
39 checks passed
@jakirkham
Copy link
Member

Thanks all! 🙏

@jameslamb jameslamb deleted the ci/pre-commit branch January 31, 2024 15:11
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.

5 participants