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

CI: Remove specification of manual stage for check_style.sh script. #1283

Conversation

csadorf
Copy link
Contributor

@csadorf csadorf commented Feb 17, 2023

Do not explicitly specify to run the "manual" stage when running pre-commits as part of the ci/check_style.sh script.

    
Do not explicitly specify to run the "manual" stage when running pre-commits as part of the ci/check_style.sh script.
@csadorf csadorf requested a review from a team as a code owner February 17, 2023 14:42
@github-actions github-actions bot added the ci label Feb 17, 2023
@ajschmidt8 ajschmidt8 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 17, 2023
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

@csadorf, I've got no objections to this change, but can you elaborate on why it's being made?

@csadorf
Copy link
Contributor Author

csadorf commented Feb 17, 2023

@csadorf, I've got no objections to this change, but can you elaborate on why it's being made?

It is simply not necessary. @vyasr can you elaborate?

@vyasr
Copy link
Contributor

vyasr commented Feb 17, 2023

It's a historical artifact. We were originally manually running all linters in CI. At some point, we discussed the possibility of centralizing around pre-commit usage to manage all style checks in CI. When I implemented that in rapidsai/cudf#9412, we were still using a special script to run local installations of clang-format i.e. we assumed that the user would have clang-format installed in their local environment. To avoid that breaking in cases where it was not installed, I configured the clang-format hook so that it would only run in certain pre-commit stages so that whether or not clang-format would be included in a pre-commit run was controllable by specifying --hook-stage=.... Since clang-format wheels became available, cudf switched to using them to run fully pre-commit-managed hooks. However, the --hook-stage=manual wasn't removed from the CI scripts at the time. As other RAPIDS repos followed cudf's pattern and copied the CI files, the same command invocation spread to everywhere. At this point it can be removed from every repo since all of our local hooks are either managing dependencies via additional_dependencies or are simply shell scripts with no dependencies at all.

@csadorf it's probably worth doing a quick check to verify that none of the repos where you've removed this have a stages: ... key present in their .pre-commit-config.yaml files. I am not aware of any, but I may be forgetting one somewhere.

@cjnolet
Copy link
Member

cjnolet commented Feb 18, 2023

/merge

@rapids-bot rapids-bot bot merged commit 02cfacf into rapidsai:branch-23.04 Feb 18, 2023
@csadorf
Copy link
Contributor Author

csadorf commented Feb 20, 2023

@csadorf it's probably worth doing a quick check to verify that none of the repos where you've removed this have a stages: ... key present in their .pre-commit-config.yaml files. I am not aware of any, but I may be forgetting one somewhere.

@vyasr Checked, none of them do.

@csadorf csadorf deleted the ci-remove-manual-stage-spec-for-style-check-pre-commit-runs branch February 20, 2023 07:49
@vyasr
Copy link
Contributor

vyasr commented Feb 22, 2023

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants