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

[REVIEW] cmake: handle thrust via target #579

Merged
merged 6 commits into from
Sep 28, 2020

Conversation

germasch
Copy link
Contributor

This is just a relatively straightforward switch from adding the thrust include dir manually to using a target. This also
changes CPM to prefer an external thrust if it can be found.

I figured I'd use this to pull in @allisonvacanti to ask whether I'm misusing something by using the Thrust::Thrust target directly, rather than calling thrust_create_target(). Since all I'm looking for is to get include path added, and RMM is clearly not designed for anything but CUDA, I'm hoping this is acceptable.

There are more problems down the road, when it comes to installing RMM while it depends on either an internal or an external thrust. But let's leave that for next.

@germasch germasch requested a review from a team as a code owner September 25, 2020 03:01
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@kkraus14
Copy link
Contributor

ok to test

@kkraus14 kkraus14 added 2 - In Progress Currently a work in progress CMake labels Sep 25, 2020
CMakeLists.txt Outdated Show resolved Hide resolved
That's its official name.
@alliepiper
Copy link

It would be best to use the thrust_create_target function to assemble a complete interface to Thrust. Thrust::Thrust is an incomplete interface specification -- it just sets the Thrust include path, and will not handle any dependencies (CUB is required on CUDA). thrust_create_target will also add some CMake checks that prevent multiple incompatible Thrust targets from being linked together.

Is there a reason that you want to avoid constructing a complete thrust interface target?

@germasch
Copy link
Contributor Author

Is there a reason that you want to avoid constructing a complete thrust interface target?

Well, I guess it was the misguided idea of avoiding the trouble of having to pick a specific target, as in some sense, I'd prefer to let the user of RMM pick the specific configuration of Thrust they'd like to use (RMM itself doesn't really care that much, at least not for the host, though it definitely wants to use CUDA). But I think my plan leads to more problems than it solves ;(

In any case, I opened NVIDIA/thrust#1295 which seems like the better place to discuss this.

Trying to just use the internal Thrust::Thrust target bears trouble
when trying to create a cmake config down the road, so this switches to
the recommended way, that is, using `thrust_create_target()`, and naming it
`rmm::Thrust` means one shouldn't run into naming conflicts even if
RMM is used inside of another project.

`FROM_OPTIONS` adds cmaeke cache variables `THRUST_HOST_SYSTEM` and
`THRUST_DEVICE_SYSTEM` that allow overriding the default config if anyway
needs to.
@germasch
Copy link
Contributor Author

I agree with @allisonvacanti that using is thrust_create_target() is the way to go, so I switched to using that.

My main concern was that if I make my own thrust target, and the downstream user of rmm also makes their own target, we could end up with two, possibly incompatibly configured thrust's. While this can't be prevented 100%, the the thrust cmake config does recognize when this happens and throws an error, so it's not going to cause subtle problems.

@germasch germasch changed the title [WIP] cmake: handle thrust via target [REVIEW] cmake: handle thrust via target Sep 27, 2020
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully we can move cuDF in this direction too...

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 2 - In Progress Currently a work in progress labels Sep 28, 2020
@kkraus14 kkraus14 merged commit 9325e15 into rapidsai:branch-0.16 Sep 28, 2020
@germasch germasch deleted the pr/cmake-thrust branch September 28, 2020 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge CMake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants