Add missing Thrust includes

[bdice]

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

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]

rerun tests

[cjnolet]

rerun tests

[bdice]

I'm going to go ahead and open this for review, because of the long CI delays due to sccache issues. See #717.

The CI logs indicate 5a3f8f3 is building with Thrust 1.17.0. If CI passes on 5a3f8f3, I will revert all CMake changes so that only the updates to Thrust #includes remain. Then we can merge once the PR is approved and CI passes again.

[robertmaynard]

CMake override looks correct. Approving even thought we will need to remove it at some point

[cjnolet]

rerun tests

[cjnolet]

rerun tests

[bdice]

@robertmaynard It looks like the pyraft package is failing to build and not finding the Thrust 1.17.0 version override that I specified. Do you have any insight on this?

Log file:

11:13:01 CMake Error at /opt/conda/envs/rapids/include/rapids/thrust/thrust/cmake/thrust-config-version.cmake:10 (file):
11:13:01   file failed to open for reading (No such file or directory):
11:13:01 
11:13:01     /workspace/.conda-bld/work/python/raft/_THRUST_VERSION_INCLUDE_DIR-NOTFOUND/thrust/version.h
...
11:13:02 -- Found Thrust: $PREFIX/include/rapids/thrust/thrust/cmake/thrust-config.cmake (found version "1.15.0.0") 
11:13:02 -- Found Thrust: $PREFIX/lib/cmake/thrust/thrust-config.cmake (found version "1.15.0.0") 
...
11:13:02 -- Configuring incomplete, errors occurred!

I pinged @vyasr since I thought it might be scikit-build related at first. He said,

You can see that the thrust it's finding is just in the rapids env. [...] I don't see thrust bundled in any of our raft conda packages. Normally I'd say that is the issue because otherwise there's no way for pyraft to know what thrust to get (it's not a direct dependency, only a transitive one). But I know that for thrust specifically we do things differently because of the need to synchronize across RAPIDS, so I'm not sure what the "correct" solution is to make this testing work (if there is one).

It looks like the C++ side built fine, so maybe we can ignore this problem and just revert the CMake changes / merge as-is. Then hope that the rapids-cmake change to 1.17.0 doesn't break anything.

[cjnolet]

@bdice I can't think of any reason why pyraft would need Thrust. In fact, I'd prefer for it not be a requirement for building it.

[bdice]

Okay. If it's not a dependency, then this is probably an issue with the build system that may be out of scope for this PR (I'd like to get it merged so that the cugraph PR is unblocked). For now, I'm going to revert the manual overrides of rapids-cmake versions and see if CI passes with only the #include <thrust/...> changes.

[vyasr]

Yes, I am also a little confused by that and am going to dig a little deeper. Just to be sure, the libraft builds that happen in CI actually compile the libraries, correct? We're not building pyraft against a header-only libraft? Assuming that is the case, I am not sure where the dependency on Thrust is coming from here. I'll try to dig a little further in a bit.

[vyasr]

@cjnolet what is the purpose of the RAFT_ENABLE_thrust_DEPENDENCY option? I suspect that the issue is https://github.com/rapidsai/raft/blob/branch-22.08/cpp/CMakeLists.txt#L175, which is causing pyraft linking to raft to also search for Thrust. I don't really see the purpose here, but I know there are a lot of competing requirements making the raft build configuration extra complex so perhaps there is a use case where this is necessary? If so, perhaps the conda builds should be setting that option to OFF?

Definitely think addressing that is out of scope for this PR.

[cjnolet]

what is the purpose of the RAFT_ENABLE_thrust_DEPENDENCY option?

@vyasr, since RAFT is header only and contains a significant amount of non-thrust code (such as the handle and communicator, as two examples), that option allows users to build without RAFT forcing the Thrust dependency. Absolutely, I think that should be used to build both pyraft and pylibraft. However, it is still needed for the libraft conda packages.

[robertmaynard]

It looks like the pyraft package is failing to build and not finding the Thrust 1.17.0 version override that I specified. Do you have any insight on this?

My guess is that by having both librmm and libraft-headers in the conda env you are getting the version of thrust that exists in librmm which is the older 1.15. For some reason the resolution of this conflict causes a mismatch which causes find_package(Thrust) to fail.

We would want to add an explicit override and call to rapids_cpm_thrust in pylibraft before the call to find_package(raft) to ignore the version of thrust that is being brought in by librmm.

[vyasr]

@vyasr, since RAFT is header only and contains a significant amount of non-thrust code (such as the handle and communicator, as two examples), that option allows users to build without RAFT forcing the Thrust dependency. Absolutely, I think that should be used to build both pyraft and pylibraft. However, it is still needed for the libraft conda packages.

Right, but the issue is that if we build libraft with the thrust dependency on then when pyraft goes to find_package(raft) it has to search for thrust because libraft told it to. If we build the C++ libs as part of the Python build (i.e. if we do python setup.py build_ext -- -DFIND_RAFT_CPP=OFF) then we can configure the libraft build to not include the thrust dependency. I think that is OK because the components of libraft that are exposed in pyraft are the parts that don't need thrust. But if we use FIND_RAFT_CPP=ON and we have a prebuilt libraft, then the cmake files exported by that libraft will tell pyraft to search for thrust. In other words, I'm not sure that this request:

@bdice I can't think of any reason why pyraft would need Thrust. In fact, I'd prefer for it not be a requirement for building it.

is possible. If libraft is header-only, then unless it bundles the thrust files there's no way to avoid the dependency when building pyraft. And I don't think we can bundle because we need libraft (and all other RAPIDS libraries) to use the thrust bundled in librmm to ensure that we use identical thrust across RAPIDS (at least until we switch to using the custom namespaces for thrust).

My guess is that by having both librmm and libraft-headers in the conda env you are getting the version of thrust that exists in librmm which is the older 1.15. For some reason the resolution of this conflict causes a mismatch which causes find_package(Thrust) to fail.

I was suspicious about that as well, but I wasn't sure because the thrust library that's being found is here:

$PREFIX/include/rapids/thrust/thrust/cmake/thrust-config.cmake

Does the thrust from librmm get installed directly into the conda environment like this? I thought it would have been installed into some lower namespace contained within rmm directories, but I guess now that I'm thinking that through it doesn't make a whole lot of sense. At minimum the cmake files need to be discoverable in the standard locations I suppose.

[robertmaynard]

Does the thrust from librmm get installed directly into the conda environment like this?

Yes that is the expected location of the 'real' thrust-config.cmake for version 1.15. There exists a $PREFIX/lib/cmake/thrust/thrust-config.cmake that will forward to that file.

I'm not sure that this request..is possible.

I agree

And I don't think we can bundle because we need libraft (and all other RAPIDS libraries) to use the thrust bundled in librmm to ensure that we use identical thrust across RAPIDS (at least until we switch to using the custom namespaces for thrust).

We most definetly need to ensure a consistent thrust version across all of RAPIDS

[vyasr]

We most definetly need to ensure a consistent thrust version across all of RAPIDS

Agreed, I just meant that we could ensure a consistent version while still having separate copies of thrust in each lib (rather than pointing them all at the one from librmm) once we switched to using thrust custom namespaces.

[cjnolet]

@gpucibot merge