-
Notifications
You must be signed in to change notification settings - Fork 913
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
Automate include grouping order in .clang-format #15063
Automate include grouping order in .clang-format #15063
Conversation
As mentioned on the other PR, I think this is great, thanks for doing it! I just had one bikeshed colour policy question:
Is it desirable to have a separation of detail API headers from public ones? (so |
I don't think we need this extra group. |
Moving out of draft |
@@ -15,6 +15,7 @@ | |||
*/ | |||
|
|||
#include "reader_impl.hpp" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what happened here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep: #14993 (comment)
#include <join/join_common_utils.cuh> | ||
#include <join/join_common_utils.hpp> | ||
#include <join/mixed_join_common_utils.cuh> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This move seems wrong. Perhaps they are meant to be quoted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Changed all #include <SRCDIR/...>
to #include "SRCDIR/..."
where SRCDIR is any directory under cudf/src
. From now on all includes of cudf headers that are not in cudf/include
should be quoted. And the nice thing is when they are not, they end up down at the bottom of the includes, making it easier to spot.
We could consider quoting all includes of cuDF headers.
cpp/src/utilities/logger.cpp
Outdated
@@ -14,10 +14,11 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
#include "spdlog/sinks/stdout_sinks.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should match with its companion at line 22.
https://github.com/rapidsai/cudf/pull/15063/files#diff-906fc8cdd3162b1f4778c3e85d9e72505ff171ccaaaab5c4a8f704f2cef7637eR22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to the start of the test files.
// To avoid https://github.com/NVIDIA/libcudacxx/issues/460 | ||
// in libcudacxx with CTK 12.0/12.1 | ||
#include <cuda_runtime.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer necessary because we have a CCCL with the fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct but potentially better for a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted this on the previous PR that was closed, I think. Can we double check that my previous comments were addressed (or deferred for a later issue/PR)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. It was addressed here. I misread the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bdice I went over all of your feedback and made sure it was addressed (except the things where I said "please open an issue".)
@@ -14,14 +14,9 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
#include "hash/concurrent_unordered_map.cuh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deliberate change from <>
to ""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
@@ -15,6 +15,7 @@ | |||
*/ | |||
|
|||
#include "reader_impl.hpp" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -16,7 +16,7 @@ | |||
|
|||
#include "reader_impl_helpers.hpp" | |||
|
|||
#include <io/utilities/row_selection.hpp> | |||
#include "io/utilities/row_selection.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deliberate change to ""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
cpp/src/io/utilities/datasource.cpp
Outdated
#include <fcntl.h> | ||
#include <io/utilities/config_utils.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: should this be ""
, et seq. throughout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Now done for all io, hash, join, and many other directories under cudf/src/
. Basically I searched for regexp #include <(NAME/.*)>
and replaced with the same in quotes, where NAME is any folder name in cudf/src/
.
PR description to be updated. It seems all CCCL headers now belong to one header group. |
Updated, thanks. |
/merge |
This uses the IncludeCategories settings in .clang-format to attempt to enforce our documented #include order in librmm. For discussion, see rapidsai/cudf#15063. This PR uses a subset of the header grouping categories used in that PR. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Michael Schellenberger Costa (https://github.com/miscco) - Lawrence Mitchell (https://github.com/wence-) - Rong Ou (https://github.com/rongou) URL: #1463
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get a chance to review this earlier due to being out, but I agree with the approach and the results look good from a spot check of some of these files. Thanks for the ping, and thanks for doing this @harrism!
…5787) This uses the `IncludeCategories` settings in` .clang-format` to automate include ordering and grouping and to make include ordering more consistent with the rest of RAPIDS. For discussion, see rapidsai/cudf#15063. This PR uses a similar set of header grouping categories used in that PR, adapted for cuML. One purpose of this is to make it easier to automate injection of a header change with an upcoming RMM refactoring (and in the future). The header reordering in this PR uncovered multiple places where headers were not included where they are used. Most commonly this was a missing `#include <raft/core/handle.hpp>` Closes #5779 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: #5787
…4205) This uses the `IncludeCategories` settings in` .clang-format` to automate include ordering and grouping and to make include ordering more consistent with the rest of RAPIDS. For discussion, see rapidsai/cudf#15063. This PR uses a similar set of header grouping categories used in that PR, adapted for cuGraph. One purpose of this is to make it easier to automate injection of a header change with an upcoming RMM refactoring (and in the future). Note that this PR also updates all of cugraph to use quotes ("") when includeing local *source* headers. That is, whenever something is included from `cugraph/src/*`, it should be included with quotes rather than angle brackets. This makes it much easier to order includes from "nearest to farthest", and matches the approach now taken in other RAPIDS repos. Without switching to quotes, keeping these includes at the top requires special casing each subdirectory of `src` in the clang-format group regular expressions. Includes from the `include` directory stay as angle brackets, because they always use `#include <cugraph/...>`. cuGraph tests include a LOT of internal source headers. I think we should generally avoid this, but it depends on the testing philosophy. If one believes that tests should only cover the external interfaces of the library, then there is no need to include internal headers. But if we want to test internal functionality, then the tests need to be more "internal" to the library. The header reordering in this PR also uncovered some places where headers were not included where they are used, which I have fixed. Closes #4185 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Seunghwa Kang (https://github.com/seunghwak) - Naim (https://github.com/naimnv) - Chuck Hastings (https://github.com/ChuckHastings) URL: #4205
…2202) This uses the IncludeCategories settings in .clang-format to automate include ordering and grouping and to make include ordering more consistent with the rest of RAPIDS. For discussion, see rapidsai/cudf#15063. This PR uses a similar set of header grouping categories used in that PR, adapted for RAFT. The header reordering in this PR uncovered one place where `#include <cutlass/layout/pitch_linear.h>` was missing from `raft/cpp/include/raft/distance/detail/./pairwise_distance_epilogue_elementwise.h`, so I also added that include. One purpose of this is to make it easier to automate injection of a header change with an upcoming RMM refactoring (and in the future). Closes #2193 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2202
…1349) This uses the IncludeCategories settings in .clang-format to attempt to enforce our documented #include order in libcuspatial. For discussion, see rapidsai/cudf#15063. This PR uses a subset of the header grouping categories used in that PR. Note that this also updates the pre-commit configuration to check and correct file copyright years. This comes from rapidsai/cudf#14917. @KyleFromNVIDIA can you confirm whether or not it's OK to do this? It seems to be working fine. Closes #1345 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Michael Wang (https://github.com/isVoid) URL: #1349
…udes (#15238) Follow up to #15063 to add new guidance for quoting includes of internal headers from `src` paths. Also covers clang-format include grouping. Also fixes a single include that was added with `<>` recently that should be `""`. #15063 updated all includes to match the guidance in this PR (changing a lot of `<>` to `""` for includes from `src/...`. Authors: - Mark Harris (https://github.com/harrism) Approvers: - David Wendt (https://github.com/davidwendt) - Vukasin Milovanovic (https://github.com/vuule) URL: #15238
Replaces #14993
Closes #15103
Description
This uses the
IncludeCategories
settings in .clang-format to attempt to enforce our documented#include
order in libcudf. See https://docs.rapids.ai/api/libcudf/stable/developer_guideI realize that there was a previous attempt at this by @bdice that met with some resistance. Reading it, I wouldn't say it was vetoed; rather, reviewers requested something much simpler. I have a few reasons to attempt this again.
rmm::mr::device_memory_resource*
withrmm::device_async_resource-ref
everywhere in RAPIDS (not just cuDF). This requires adding an include to MANY files. Getting the location of the include right everywhere is very difficult without automatic grouping of headers. I started out writing a bash script to do this before realizing clang-format has the necessary feature. And I realized that my script would never properly handle files like this.Note that one of the ways that having few categories can work while still maintaining clear groups is that this PR updates many files to use quotes ("") instead of angle brackets (<>) for local cuDF headers that do not live in
cudf/cpp/include
. With our "near to far" include ordering policy, these are arguably the nearest files, and using quotes allows us to have our first category simply check for quotes. These files will be grouped and sorted without blank lines, but in practice this does not lose clarity because typically headers from more than two directories are not included from the same file. The downside of this change is I don't yet know how to automatically enforce it. I hope that when developers accidentally use <> for internal includes that don't start with (e.g.) "cudf", they will be grouped one of the lowest priority categories, and perhaps this will induce them to switch to "" to get the headers listed at the top. The rule is simple: if it's in libcudf but not incpp/include/cudf
, then use quotes. For everything else, use angle brackets.Other than headers from RAPIDS repos, we have a group for all CCCL/CUDA headers, a group for all other headers that have a file extension, and a final group for all files that have no file extension (e.g. STL).
Below I'm listing the (fairly simple, in my opinion) .clang-format settings for this PR. Note that categories 2-5 will require tweaking for different RAPIDS repos.
Some may ask why I ordered
cudf_test
headers beforecudf
headers. I tried both orders, and puttingcudf_test
first generated significantly fewer changes in the PR, meaning that it's already the more common ordering (I supposecudf_test
is closer to the files that include it, since they are libcudf tests).I've opened a similar PR for RMM with only 5 groups. rapidsai/rmm#1463
CC @davidwendt @vyasr @wence- @GregoryKimball for feedback
@isVoid contributed to this PR via pair programming.
Checklist