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

Update clang-format to 16.0.1. #5361

Merged
merged 7 commits into from
Apr 25, 2023

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Apr 13, 2023

This PR updates the clang-format version used by pre-commit.

@dantegd dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 13, 2023
@bdice bdice changed the title Update clang-format Update clang-format to 16.0.1. Apr 13, 2023
@bdice bdice marked this pull request as ready for review April 14, 2023 19:14
@bdice bdice requested a review from a team as a code owner April 14, 2023 19:14
@bdice
Copy link
Contributor Author

bdice commented Apr 14, 2023

@dantegd I think this just needs reviews, then should be good to merge. (Merging sooner means less likelihood of conflicts.)

@bdice
Copy link
Contributor Author

bdice commented Apr 14, 2023

Actually, I take that back -- this isn't ready to merge. I am not sure if the clang-format script is working as intended on this repo. We saw minor changes for most other RAPIDS repos but the hook did not appear to make any changes in cuML.

cuML is the one RAPIDS repository that isn't using pre-commit in the same way as other RAPIDS projects, and still relies on a custom Python script for the hook rather than the semi-official mirrors-clang-format hook. Perhaps we need to revisit pre-commit support in cuML before this will work as intended? cc: @csadorf if you have insight here.

After all the other RAPIDS clang-format updates are merged, I will come back to this one and standardize it to match the rest of RAPIDS.

@csadorf
Copy link
Contributor

csadorf commented Apr 14, 2023

cuML is the one RAPIDS repository that isn't using pre-commit in the same way as other RAPIDS projects, and still relies on a custom Python script for the hook rather than the semi-official mirrors-clang-format hook. Perhaps we need to revisit pre-commit support in cuML before this will work as intended? cc: @csadorf if you have insight here.

I don't think there is a strong motivation for the custom hook at least that I know of. Did you try to just use the semi-official hook instead?

Also, we should probably coordinate this effort with #5235 , right?

@bdice Maybe revert this PR back to draft status until it is ready?

@bdice bdice marked this pull request as draft April 14, 2023 19:35
@bdice
Copy link
Contributor Author

bdice commented Apr 14, 2023

@csadorf This should be totally independent of #5235 because we use pre-commit. The clang-format binary that is installed by pre-commit (into its own virtual environment) is unrelated to the CI job using clang-tidy.

I reverted to a draft for now. I will attempt to migrate this repo to use the same pre-commit hook as the rest of RAPIDS, then re-open if that works.

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Apr 14, 2023
@bdice
Copy link
Contributor Author

bdice commented Apr 14, 2023

Using the same pre-commit hook as the rest of RAPIDS appears to have worked -- but it's possible that clang-format hasn't been applied for a while, so the changeset is large. I'll reopen for review.

@bdice bdice marked this pull request as ready for review April 14, 2023 19:42
@bdice bdice requested a review from a team as a code owner April 14, 2023 19:42
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This script is not used by pre-commit any more. May we remove this file in favor of pre-commit's own logic for running clang-format? It would only matter if developers still want to use this script locally instead of running pre-commit locally (which I recommend, since it matches the CI outputs).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's remove it, I don't think there's much of a case for using it instead of pre-commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I removed it. This is now ready for merge @dantegd.

@csadorf
Copy link
Contributor

csadorf commented Apr 14, 2023

@csadorf This should be totally independent of #5235 because we use pre-commit. The clang-format binary that is installed by pre-commit (into its own virtual environment) is unrelated to the CI job using clang-tidy.

@bdice I was more thinking in terms of matching versions.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Reviewed the changes to the pre-commit config and associated docs, as well as the header file under python/ – LGTM.

Thanks a lot!

@bdice
Copy link
Contributor Author

bdice commented Apr 14, 2023

@bdice I was more thinking in terms of matching versions.

Discussed offline. Because clang-format is managed through pre-commit and clang-tidy is managed through some other means (conda or system), matching versions is not necessary.

@jolorunyomi jolorunyomi merged commit 8a84f3c into rapidsai:branch-23.06 Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CUDA/C++ Cython / Python Cython or Python issue 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