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

Add missing Thrust includes #2310

Merged
merged 21 commits into from
Jun 29, 2022
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented May 25, 2022

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, version 1.16 of 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 makes it more obvious where cugraph depends on thrust/detail headers.

@BradReesWork BradReesWork added this to the 22.08 milestone May 25, 2022
@BradReesWork BradReesWork added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change DRAFT labels May 25, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

Merging #2310 (9d33fce) into branch-22.08 (71f143e) will increase coverage by 0.01%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.08    #2310      +/-   ##
================================================
+ Coverage         60.08%   60.10%   +0.01%     
================================================
  Files               102      102              
  Lines              5158     5158              
================================================
+ Hits               3099     3100       +1     
+ Misses             2059     2058       -1     
Impacted Files Coverage Δ
...ython/cugraph/cugraph/community/ktruss_subgraph.py 88.23% <0.00%> (+2.94%) ⬆️

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 71f143e...9d33fce. Read the comment docs.

@bdice
Copy link
Contributor Author

bdice commented Jun 16, 2022

From the CI output, it appears that Thrust 1.15.0 is being used.

I also built this branch locally with rapids-compose's build-cugraph-cpp command. I see:

-- CPM: adding package [email protected] (1.7.0)
-- CPM: adding package [email protected] (0ca860b824f5dc22cf8a41f09912e62e11f07d82)
-- CPM: cuco: adding package [email protected] (1.12.0)
-- Found Thrust: /home/bdice/rapids1/cugraph/cpp/build/cuda-11.7.0/thrust-includes/release/_deps/thrust-src/thrust/cmake/thrust-config.cmake (found version "1.17.0.0")
-- Found CUB: /home/bdice/rapids1/cugraph/cpp/build/cuda-11.7.0/thrust-includes/release/_deps/thrust-src/dependencies/cub/cub/cmake/cub-config.cmake (found suitable version "1.17.0.0", minimum required is "1.17.0.0")
-- CPM: adding package [email protected] (branch-22.08)
-- Found OpenMP_C: -fopenmp (found version "4.5")
-- Found OpenMP_CXX: -fopenmp (found version "4.5")
CMake Warning at build/cuda-11.7.0/thrust-includes/release/cmake/CPM_0.35.0.cmake:286 (message):
  CPM: raft: requires a newer version of Thrust (1.17.0) than currently
  included (1.12.0).

It looks like this commit of cuco is using CPM to include an older Thrust 1.12.0 version, which happens before my manual versions.json sets Thrust 1.17.0. This behavior is described in the rapids-cmake docs as "first to record, wins."

I pushed a commit 0ba6551 to force using 1.17.0 by including it before all other packages that also depend on Thrust (cuco, for instance). We'll see if that gets used by the CI this time. I was unable to test locally due to an error caused by rapids_find_package(cugraph-ops). I don't know how this package is supposed to be found...

@bdice
Copy link
Contributor Author

bdice commented Jun 17, 2022

This is blocked until raft is upgraded: rapidsai/raft#678

(compiling CMakeFiles/cugraph.dir/src/linear_assignment/hungarian.cu.o)
$PREFIX/include/raft/lap/lap.cuh(182): error: namespace "thrust" has no member "device"

@bdice
Copy link
Contributor Author

bdice commented Jun 17, 2022

rerun tests

@bdice
Copy link
Contributor Author

bdice commented Jun 17, 2022

Maybe this needs to wait until a new nightly package of libraft-headers is generated, and then re-run.

@bdice
Copy link
Contributor Author

bdice commented Jun 22, 2022

This is now blocked by rapidsai/cuhornet#57.

@bdice bdice marked this pull request as ready for review June 22, 2022 23:38
@bdice bdice requested review from a team as code owners June 22, 2022 23:38
@bdice
Copy link
Contributor Author

bdice commented Jun 22, 2022

Looks like the build completed for 3fd8e6a, but there's some issue related to finding a libcudf that has an older CUB. I'm not sure if we need to debug this and override things. I think we can probably move forward without debugging that issue, since the build didn't fail with Thrust inclusion problems and it's just an issue of my version override not matching the rest of RAPIDS. I reverted my CMake changes overriding the Thrust version in 9d33fce, and I think this should be ready to review/merge.

Error from the previous build:

CMake Error at /workspace/.conda-bld/.../include/libcudf/Thrust/thrust/cmake/thrust-config.cmake:218 (message):
The version of CUB found by CMake is not compatible with this release of
Thrust.  CUB is now included in the CUDA Toolkit, so you no longer need to
use your own checkout of CUB.  Pass IGNORE_CUB_VERSION_CHECK to
thrust_create_target to ignore.  (CUB 1.17.0.0, Thrust 1.15.0.0).

@BradReesWork
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d0863d4 into rapidsai:branch-22.08 Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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