Clean up Thrust includes. #4675

bdice · 2022-03-30T23:29:03Z

Description

This PR cleans up some #includes for Thrust. This is meant to help ease the transition to Thrust 1.17 when that is updated in rapids-cmake.

Context

I opened a PR rapidsai/cudf#10489 that updates cuDF to Thrust 1.16. Notably, Thrust reduced the number of internal header inclusions:

#1572 Removed several unnecessary header includes. Downstream projects may need to update their includes if they were relying on this behavior.

I spoke with @robertmaynard and he recommended making similar changes to clean up includes ("include what we use," in essence) to make sure we have compatibility with future versions of Thrust across all RAPIDS libraries.

This changeset also removes dependence on thrust/detail headers.

PR #10489 updated from Thrust 1.15 to Thrust 1.16. However, this appears to be causing conflicts with other repositories -- [cuSpatial](rapidsai/cuspatial#511 (comment)) and cuGraph have reported issues where their builds are finding Thrust 1.16 from libcudf instead of Thrust 1.15 which is [currently pinned by rapids-cmake](https://github.com/rapidsai/rapids-cmake/blob/06a657281cdd83781e49afcdbb39abc491eeab17/rapids-cmake/cpm/versions.json#L26). This PR is intended to unblock local builds and CI builds for other RAPIDS packages until we are able to identify the root cause (which may be due to CMake include path orderingsrapids-cmake). Last time Thrust was updated, [rapids-cmake was updated](rapidsai/rapids-cmake#138) one day before [libcudf was updated](#9912). That may explain why we didn't notice this problem with the 1.15 update. The plan I currently have in mind is: 1. Merge this PR to roll back libcudf to Thrust 1.15 (and revert the patch for Thrust 1.16 [10577](#10577)). This will hopefully unblock CI for cugraph and cuspatial. 2. Try to work out whatever issues with CMake / include paths may exist. 3. Prepare all rapids-cmake repos for Thrust 1.16 compatibility. I've [done this for RMM already](rapidsai/rmm#1011), and I am working on [PR 4675](rapidsai/cuml#4675) to cuML now. I am planning to make the same fixes for `#include`s in cuCollections, raft, cuSpatial, and cuGraph so they will be compatible with Thrust 1.16. 4. Try to upgrade libcudf to Thrust 1.16 again (and re-apply the updated patch). If (2) has been resolved, I hope we won't see any issues in other RAPIDS libraries 5. Upgrade rapids-cmake to Thrust 1.16. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Mark Harris (https://github.com/harrism) URL: #10586

PR rapidsai#10489 updated from Thrust 1.15 to Thrust 1.16. However, this appears to be causing conflicts with other repositories -- [cuSpatial](rapidsai/cuspatial#511 (comment)) and cuGraph have reported issues where their builds are finding Thrust 1.16 from libcudf instead of Thrust 1.15 which is [currently pinned by rapids-cmake](https://github.com/rapidsai/rapids-cmake/blob/06a657281cdd83781e49afcdbb39abc491eeab17/rapids-cmake/cpm/versions.json#L26). This PR is intended to unblock local builds and CI builds for other RAPIDS packages until we are able to identify the root cause (which may be due to CMake include path orderingsrapids-cmake). Last time Thrust was updated, [rapids-cmake was updated](rapidsai/rapids-cmake#138) one day before [libcudf was updated](rapidsai#9912). That may explain why we didn't notice this problem with the 1.15 update. The plan I currently have in mind is: 1. Merge this PR to roll back libcudf to Thrust 1.15 (and revert the patch for Thrust 1.16 [10577](rapidsai#10577)). This will hopefully unblock CI for cugraph and cuspatial. 2. Try to work out whatever issues with CMake / include paths may exist. 3. Prepare all rapids-cmake repos for Thrust 1.16 compatibility. I've [done this for RMM already](rapidsai/rmm#1011), and I am working on [PR 4675](rapidsai/cuml#4675) to cuML now. I am planning to make the same fixes for `#include`s in cuCollections, raft, cuSpatial, and cuGraph so they will be compatible with Thrust 1.16. 4. Try to upgrade libcudf to Thrust 1.16 again (and re-apply the updated patch). If (2) has been resolved, I hope we won't see any issues in other RAPIDS libraries 5. Upgrade rapids-cmake to Thrust 1.16. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Mark Harris (https://github.com/harrism) URL: rapidsai#10586

github-actions · 2022-04-30T01:33:23Z

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

bdice · 2022-05-26T03:26:56Z

CI is failing because libcumlprims=22.08 can't be found. This will probably be resolved with time. I'll come back to this PR after the rest of the RAPIDS repos.

…includes

bdice · 2022-06-17T00:59:29Z

Blocked by issue with xgboost version pinning: #4777.

…includes

bdice · 2022-06-28T14:16:17Z

I'm going to go ahead and mark this as ready for review. The CMake changes will be reverted once CI passes with Thrust 1.17. All the #include changes will remain.

cpp/src/fil/fil.cu

bdice · 2022-07-05T18:53:23Z

@rapidsai/cuml-cpp-codeowners This is ready for review. It's the last PR needed for rapidsai/rapids-cmake#199 and would be nice to get through as early as possible before 22.08 burndown.

wphicks

This PR brings me great joy :)

bdice · 2022-07-07T15:38:46Z

rerun tests

codecov-commenter · 2022-07-07T18:35:38Z

Codecov Report

Merging #4675 (62b85f4) into branch-22.08 (b26fe7e) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.08    #4675      +/-   ##
================================================
- Coverage         77.62%   77.52%   -0.10%     
================================================
  Files               180      180              
  Lines             11382    11382              
================================================
- Hits               8835     8824      -11     
- Misses             2547     2558      +11

Flag	Coverage Δ
dask	`45.51% <ø> (ø)`
non-dask	`67.15% <ø> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/cuml/neighbors/kernel_density.py	`80.26% <0.00%> (-7.24%)`	⬇️
python/cuml/thirdparty_adapters/adapters.py	`91.48% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b26fe7e...62b85f4. Read the comment docs.

bdice · 2022-07-07T19:07:17Z

@gpucibot merge

bdice · 2022-07-07T19:07:32Z

@wphicks Can you merge? I don’t have permission.

Also, CMake review should not be required since I reverted the CMake changes. I just can’t remove the review request.

cjnolet · 2022-07-07T19:09:48Z

@gpucibot merge

@robertmaynard

## Description This PR cleans up some `#include`s for Thrust. This is meant to help ease the transition to Thrust 1.17 when that is updated in rapids-cmake. ## Context I opened a PR rapidsai/cudf#10489 that updates cuDF to Thrust 1.16. Notably, Thrust reduced the number of internal header inclusions: > [rapidsai#1572](NVIDIA/thrust#1572) Removed several unnecessary header includes. Downstream projects may need to update their includes if they were relying on this behavior. I spoke with @robertmaynard and he recommended making similar changes to clean up includes ("include what we use," in essence) to make sure we have compatibility with future versions of Thrust across all RAPIDS libraries. This changeset also removes dependence on `thrust/detail` headers. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - William Hicks (https://github.com/wphicks) URL: rapidsai#4675

github-actions bot added the CUDA/C++ label Mar 30, 2022

dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 31, 2022

bdice mentioned this pull request Apr 4, 2022

Revert Thrust 1.16 to Thrust 1.15 rapidsai/cudf#10586

Merged

github-actions bot added the inactive-30d label Apr 30, 2022

robertmaynard mentioned this pull request May 12, 2022

[FEA] Update to Thrust 1.16 rapidsai/cudf#10841

Closed

bdice changed the base branch from branch-22.06 to branch-22.08 May 24, 2022 01:19

github-actions bot added the CMake label May 26, 2022

bdice added 4 commits May 25, 2022 21:59

Avoid thrust::detail call.

04510ef

Add missing thrust includes.

aa0a3d0

Clean up some include sections.

7d0da41

Test with Thrust 1.17.0-rc2.

b7390f0

bdice force-pushed the fix-thrust-includes branch from cf259c7 to b7390f0 Compare May 26, 2022 03:01

github-actions bot removed the inactive-30d label May 26, 2022

bdice mentioned this pull request May 26, 2022

Update thrust tag to 1.17.0. rapidsai/rapids-cmake#199

Closed

8 tasks

bdice added 3 commits June 13, 2022 12:59

Merge remote-tracking branch 'upstream/branch-22.08' into fix-thrust-…

d013251

…includes

Update versions.json to 1.17.0.

8f78af4

Merge remote-tracking branch 'upstream/branch-22.08' into fix-thrust-…

f404464

…includes

bdice added 2 commits June 24, 2022 17:43

Merge remote-tracking branch 'upstream/branch-22.08' into fix-thrust-…

307109c

…includes

Merge remote-tracking branch 'upstream/branch-22.08' into fix-thrust-…

e336700

…includes

bdice marked this pull request as ready for review June 28, 2022 14:15

bdice requested review from a team as code owners June 28, 2022 14:15

bdice commented Jun 29, 2022

View reviewed changes

cpp/src/fil/fil.cu Outdated Show resolved Hide resolved

bdice added 2 commits June 29, 2022 12:21

Fix is_power_of_2.

88656a5

Revert CMake changes.

62b85f4

github-actions bot removed the CMake label Jul 5, 2022

wphicks approved these changes Jul 5, 2022

View reviewed changes

rapids-bot bot merged commit 892558b into rapidsai:branch-22.08 Jul 7, 2022

bdice deleted the fix-thrust-includes branch July 8, 2022 03:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up Thrust includes. #4675

Clean up Thrust includes. #4675

bdice commented Mar 30, 2022 •

edited

Loading

github-actions bot commented Apr 30, 2022

bdice commented May 26, 2022

bdice commented Jun 17, 2022

bdice commented Jun 28, 2022

bdice commented Jul 5, 2022

wphicks left a comment

bdice commented Jul 7, 2022

codecov-commenter commented Jul 7, 2022

bdice commented Jul 7, 2022

bdice commented Jul 7, 2022 •

edited

Loading

cjnolet commented Jul 7, 2022

Clean up Thrust includes. #4675

Clean up Thrust includes. #4675

Conversation

bdice commented Mar 30, 2022 • edited Loading

Description

Context

github-actions bot commented Apr 30, 2022

bdice commented May 26, 2022

bdice commented Jun 17, 2022

bdice commented Jun 28, 2022

bdice commented Jul 5, 2022

wphicks left a comment

Choose a reason for hiding this comment

bdice commented Jul 7, 2022

codecov-commenter commented Jul 7, 2022

Codecov Report

bdice commented Jul 7, 2022

bdice commented Jul 7, 2022 • edited Loading

cjnolet commented Jul 7, 2022

bdice commented Mar 30, 2022 •

edited

Loading

bdice commented Jul 7, 2022 •

edited

Loading