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 #539

Merged
merged 7 commits into from
Jun 17, 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 with the transition to Thrust 1.17 when that is updated in rapids-cmake (rapidsai/rapids-cmake#199).

Context

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 am making similar changes across all RAPIDS libraries to clean up includes ("include what we use," in essence) to make sure we have compatibility with future versions of Thrust.

@bdice bdice changed the base branch from branch-22.06 to branch-22.08 May 25, 2022 04:24
@github-actions github-actions bot added cmake Related to CMake code or build configuration conda Related to conda and conda configuration libcuspatial Relates to the cuSpatial C++ library labels May 25, 2022
@harrism
Copy link
Member

harrism commented May 25, 2022

Oops, still a draft, I jumped the gun.

@github-actions github-actions bot removed the conda Related to conda and conda configuration label May 26, 2022
@harrism harrism added this to PR-WIP in cuSpatial v22.08 Release via automation May 26, 2022
@harrism
Copy link
Member

harrism commented Jun 13, 2022

Seems to be a problem with CUDA Python?

04:41:04   File "cuda/_cuda/ccuda.pyx", line 3674, in cuda._cuda.ccuda._cuInit
04:41:04 RuntimeError: Function "cuInit" not found

cuSpatial v22.08 Release automation moved this from PR-WIP to PR-Reviewer approved Jun 16, 2022
@harrism harrism added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 16, 2022
@bdice
Copy link
Contributor Author

bdice commented Jun 16, 2022

Commit 1efbffd passes CI, but I wasn't able to locate a "Found Thrust" in the CI logs to verify that the version is correct. I think I just don't know enough about the build system for cuspatial -- should we expect to see that message?

I saw one occurrence here but it seems to be from libcudf:

-- Found Thrust: $PREFIX/include/libcudf/Thrust/thrust/cmake/thrust-config.cmake (found version "1.15.0.0") 

I will open the PR draft once we confirm that cuspatial will behave correctly with Thrust 1.17 and revert the versions.json / CMake alterations (which are just for testing).

@bdice bdice marked this pull request as ready for review June 17, 2022 02:50
@bdice bdice requested a review from a team as a code owner June 17, 2022 02:50
@bdice
Copy link
Contributor Author

bdice commented Jun 17, 2022

CI passed when I forced inclusion of Thrust 1.17.0 with commit 258fbbd. I am reasonably confident that this PR is good to go and will continue working when rapids-cmake is updated to Thrust 1.17.0. I have reverted the changes to CMake in 928f4ea and am marking this ready for review.

@vyasr
Copy link
Contributor

vyasr commented Jun 17, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 61e386b into rapidsai:branch-22.08 Jun 17, 2022
cuSpatial v22.08 Release automation moved this from PR-Reviewer approved to Done Jun 17, 2022
isVoid pushed a commit to isVoid/cuspatial that referenced this pull request Jun 23, 2022
## Description

This PR cleans up some `#include`s for Thrust. This is meant to help with the transition to Thrust 1.17 when that is updated in rapids-cmake (rapidsai/rapids-cmake#199).

## Context

Version 1.16 of Thrust reduced the number of internal header inclusions:
> [#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 am making similar changes across all RAPIDS libraries to clean up includes ("include what we use," in essence) to make sure we have compatibility with future versions of Thrust.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: rapidsai#539
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Related to CMake code or build configuration improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants