Automate include grouping order in .clang-format #15063

harrism · 2024-02-15T01:40:56Z

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_guide

I 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.

To make a separate task much easier. We are undertaking a refactoring of RMM that will replace rmm::mr::device_memory_resource* with rmm::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.
To increase velocity. Everywhere in RAPIDS that we have automated code standard/style/formatting/other, the benefits to velocity have outweighed the costs. To paraphrase @bdice, $auto \nearrow \rightarrow \mu \searrow \rightarrow v \nearrow$
The previous PR Automatic include sorting with clang-format #12760 had nearly 50 categories of headers. There was no way this could be applied universally across RAPIDS repos. My proposal has 10 categories. I tried to reduce it further but realized that it wouldn't be much less configuration to maintain, so I stopped at 10.

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 in cpp/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 before cudf headers. I tried both orders, and putting cudf_test first generated significantly fewer changes in the PR, meaning that it's already the more common ordering (I suppose cudf_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.

IncludeBlocks: Regroup
IncludeCategories:
  - Regex:           '^"' # quoted includes
    Priority:        1
  - Regex:           '^<(benchmarks|tests)/' # benchmark includes
    Priority:        2
  - Regex:           '^<cudf_test/' # cuDF includes
    Priority:        3
  - Regex:           '^<cudf/' # cuDF includes
    Priority:        4
  - Regex:           '^<(nvtext|cudf_kafka)' # other libcudf includes
    Priority:        5
  - Regex:           '^<(cugraph|cuml|cuspatial|raft|kvikio)' # Other RAPIDS includes
    Priority:        6
  - Regex:           '^<rmm/' # RMM includes
    Priority:        7
  - Regex:           '^<(thrust|cub|cuda)/' # CCCL includes
    Priority:        8
  - Regex:           '^<(cooperative_groups|cuco|cuda.h|cuda_runtime|device_types|math_constants|nvtx3)' # CUDA includes
    Priority:        8
  - Regex:           '^<.*\..*' # other system includes (e.g. with a '.')
    Priority:        9
  - Regex:           '^<[^.]+' # STL includes (no '.')
    Priority:        10

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

wence- · 2024-02-15T09:38:22Z

As mentioned on the other PR, I think this is great, thanks for doing it! I just had one bikeshed colour policy question:

  - Regex:           '^<cudf/' # cuDF includes
    Priority:        4

Is it desirable to have a separation of detail API headers from public ones? (so '^<cudf/detail' with a separate priority from '^<cudf/') This would work because matching is done top down, so we would just put the detail regex before the public API regex in the list.

davidwendt · 2024-02-15T13:18:13Z

As mentioned on the other PR, I think this is great, thanks for doing it! I just had one bikeshed colour policy question:
  - Regex:           '^<cudf/' # cuDF includes
    Priority:        4
Is it desirable to have a separation of detail API headers from public ones? (so '^<cudf/detail' with a separate priority from '^<cudf/') This would work because matching is done top down, so we would just put the detail regex before the public API regex in the list.

I don't think we need this extra group.

harrism · 2024-02-16T06:50:27Z

Moving out of draft

davidwendt · 2024-02-16T13:30:41Z

cpp/src/io/parquet/reader_impl.cpp

@@ -15,6 +15,7 @@
 */

 #include "reader_impl.hpp"
+


I wonder what happened here.

From a comment @harrism had with @bdice, this might be a clang-format bug,

Yep: #14993 (comment)

davidwendt · 2024-02-16T13:34:11Z

cpp/src/join/mixed_join_size_kernels_semi.cu

+#include <join/join_common_utils.cuh>
+#include <join/join_common_utils.hpp>
+#include <join/mixed_join_common_utils.cuh>


This move seems wrong. Perhaps they are meant to be quoted?

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.

davidwendt · 2024-02-16T13:37:52Z

cpp/src/utilities/logger.cpp

@@ -14,10 +14,11 @@
 * limitations under the License.
 */

+#include "spdlog/sinks/stdout_sinks.h"


This should match with its companion at line 22.
https://github.com/rapidsai/cudf/pull/15063/files#diff-906fc8cdd3162b1f4778c3e85d9e72505ff171ccaaaab5c4a8f704f2cef7637eR22

Fixed, thx.

wence-

Up to the start of the test files.

wence- · 2024-02-16T16:30:43Z

cpp/include/cudf/fixed_point/temporary.hpp

-// To avoid https://github.com/NVIDIA/libcudacxx/issues/460
-// in libcudacxx with CTK 12.0/12.1
-#include <cuda_runtime.h>


No longer necessary because we have a CCCL with the fix?

That is correct but potentially better for a separate PR.

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)?

Nevermind. It was addressed here. I misread the diff.

@bdice I went over all of your feedback and made sure it was addressed (except the things where I said "please open an issue".)

wence- · 2024-02-16T16:38:32Z

cpp/src/io/json/legacy/reader_impl.cu

@@ -14,14 +14,9 @@
 * limitations under the License.
 */

+#include "hash/concurrent_unordered_map.cuh"


Deliberate change from <> to ""?

wence- · 2024-02-16T16:39:57Z

cpp/src/io/parquet/reader_impl.cpp

@@ -15,6 +15,7 @@
 */

 #include "reader_impl.hpp"
+


From a comment @harrism had with @bdice, this might be a clang-format bug,

wence- · 2024-02-16T16:40:26Z

cpp/src/io/parquet/reader_impl_helpers.cpp

@@ -16,7 +16,7 @@

 #include "reader_impl_helpers.hpp"

-#include <io/utilities/row_selection.hpp>
+#include "io/utilities/row_selection.hpp"


Deliberate change to ""?

wence- · 2024-02-16T16:44:47Z

cpp/src/io/utilities/datasource.cpp

 #include <fcntl.h>
+#include <io/utilities/config_utils.hpp>


question: should this be "", et seq. throughout?

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/.

… includes.

PointKernel · 2024-02-20T19:13:21Z

Other than headers from RAPIDS repos, I group all other headers that have a file extension in a single group, and all files that have no file extension in another group. Since the latter also matches includes some files from libcu++, I have an explicit category for <cuda/ includes to keep them separate from STL includes. A frequent effect of the single "." group is that cub and thrust headers get grouped without a blank line between them. I don't think this is a problem.

PR description to be updated. It seems all CCCL headers now belong to one header group.

harrism · 2024-02-20T20:45:27Z

PR description to be updated. It seems all CCCL headers now belong to one header group.

Updated, thanks.

harrism · 2024-02-21T19:41:59Z

/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

vyasr

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

@KyleFromNVIDIA

…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

harrism added 3 commits February 15, 2024 00:15

Add IncludeCategories to clang-format

551f36d

apply clang-format and copyright updates using new rules

bb56954

Changes based on PR reviews

f30728b

harrism added feature request New feature or request non-breaking Non-breaking change labels Feb 15, 2024

harrism self-assigned this Feb 15, 2024

github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. labels Feb 15, 2024

wence- mentioned this pull request Feb 15, 2024

Automate include grouping order in .clang-format #14993

Closed

3 tasks

harrism marked this pull request as ready for review February 16, 2024 06:50

harrism requested review from a team as code owners February 16, 2024 06:50

harrism requested review from wence-, mroeschke, bdice, divyegala and davidwendt February 16, 2024 06:50

davidwendt reviewed Feb 16, 2024

View reviewed changes

wence- reviewed Feb 16, 2024

View reviewed changes

harrism added 2 commits February 20, 2024 05:48

Merge branch 'branch-24.04' into fea/clang-format-include-grouping-2

c7c7cef

Change <> to "" for src includes. Add missing needed forward decl and…

6738774

… includes.

harrism requested review from davidwendt and wence- February 20, 2024 06:39

PointKernel mentioned this pull request Feb 20, 2024

Replace thrust::tuple with cuda::std::tuple #15089

Closed

Force include of JIT header to be last to fix failing JIT tests.

e168f3c

ttnghia approved these changes Feb 21, 2024

View reviewed changes

rapids-bot bot merged commit d053323 into rapidsai:branch-24.04 Feb 21, 2024
69 checks passed

vyasr reviewed Feb 23, 2024

View reviewed changes

bdice mentioned this pull request Mar 4, 2024

[FEA] Automate include grouping order in .clang-format #15024

Closed

harrism mentioned this pull request Mar 5, 2024

Update developer_guide.md with new guidance on quoted internal includes #15238

Merged

3 tasks

PointKernel mentioned this pull request Mar 19, 2024

Clean up pair utilities NVIDIA/cuCollections#447

Merged

PointKernel mentioned this pull request May 24, 2024

Automate include grouping order with clang format NVIDIA/cuCollections#491

Merged

vyasr mentioned this pull request Jun 26, 2024

Kernel copy for pinned memory #15934

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automate include grouping order in .clang-format #15063

Automate include grouping order in .clang-format #15063

harrism commented Feb 15, 2024 •

edited

Loading

wence- commented Feb 15, 2024

davidwendt commented Feb 15, 2024

harrism commented Feb 16, 2024

davidwendt Feb 16, 2024

wence- Feb 16, 2024

harrism Feb 20, 2024

davidwendt Feb 16, 2024

harrism Feb 20, 2024 •

edited

Loading

davidwendt Feb 16, 2024

harrism Feb 20, 2024

wence- left a comment

wence- Feb 16, 2024

bdice Feb 17, 2024 •

edited

Loading

bdice Feb 17, 2024

bdice Feb 17, 2024

harrism Feb 20, 2024 •

edited

Loading

wence- Feb 16, 2024

harrism Feb 20, 2024

wence- Feb 16, 2024

wence- Feb 16, 2024

harrism Feb 20, 2024

wence- Feb 16, 2024

harrism Feb 20, 2024 •

edited

Loading

PointKernel commented Feb 20, 2024

harrism commented Feb 20, 2024

harrism commented Feb 21, 2024

vyasr left a comment

Automate include grouping order in .clang-format #15063

Automate include grouping order in .clang-format #15063

Conversation

harrism commented Feb 15, 2024 • edited Loading

Description

Checklist

wence- commented Feb 15, 2024

davidwendt commented Feb 15, 2024

harrism commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wence- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

PointKernel commented Feb 20, 2024

harrism commented Feb 20, 2024

harrism commented Feb 21, 2024

vyasr left a comment

Choose a reason for hiding this comment

harrism commented Feb 15, 2024 •

edited

Loading

harrism Feb 20, 2024 •

edited

Loading

bdice Feb 17, 2024 •

edited

Loading

harrism Feb 20, 2024 •

edited

Loading

harrism Feb 20, 2024 •

edited

Loading