Export Thrust/CUB version used at build time #31

leofang · 2022-05-25T18:00:50Z

Close #30.

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

conda-forge-linter · 2022-05-25T18:00:54Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

leofang · 2022-05-25T18:00:58Z

@conda-forge-admin, please rerender

leofang · 2022-05-25T18:02:08Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2022.05.25.14.40.58

recipe/meta.yaml

leofang · 2022-05-25T19:15:33Z

I am confused. From the CI log it seems RMM always bundles a copy of Thrust/CUB headers via CPM. Can we disable this behavior?

leofang · 2022-05-25T19:25:46Z

Looks like we should do cmake -DCPM_LOCAL_PACKAGES_ONLY ....

recipe/build.sh

leofang · 2022-05-25T19:49:44Z

lol we didn't have thrust/cub 1.15 created...

kkraus14 · 2022-05-25T20:09:05Z

lol we didn't have thrust/cub 1.15 created...

Does 1.16 not work? That is created.

leofang · 2022-05-25T20:12:44Z

No I think it's pinned somewhere in the RAPIDS stack (rapids-cmake, maybe? @robertmaynard)

robertmaynard · 2022-05-25T21:06:55Z

lol we didn't have thrust/cub 1.15 created...

Does 1.16 not work? That is created.

1.16 changes some side-effect of header includes that can cause user failures. I am tracking progress of deploying corrections to all RAPIDS projects at: rapidsai/cudf#10841

So currently we are pinning at 1.15 since all of our libraries haven't been verified with 1.16

leofang · 2022-05-25T22:01:20Z

@conda-forge-admin, please restart ci

recipe/build.sh

leofang · 2022-05-26T02:37:43Z

The CPM trick seems to be working now. The benefit of it is Thrust headers are no longer bundled in librmm, which feels more natural now.

TODO:

CMake insists on using spdlog 1.8.5 as opposed to 1.10.0 from the global pinning. Need to figure out why.
CUB is not used by librmm, but when librmm is complied (in downstream), it's needed indirectly through Thrust. I think the current treatment is still needed, but I'd like to raise it for vis.

leofang · 2022-05-26T03:13:34Z

I think this is ready!

...
2022-05-26T14:33:34.3077589Z -- NO RAPIDS_VERSION for CUDAToolkit
2022-05-26T14:33:34.3084695Z -- NO RAPIDS_VERSION for CUDAToolkit
2022-05-26T14:33:34.3556740Z -- CPM: using local package spdlog@
2022-05-26T14:33:34.3566080Z -- NO RAPIDS_VERSION for spdlog
2022-05-26T14:33:34.3625485Z -- CPM: using local package Thrust@
2022-05-26T14:33:34.3894144Z -- Configuring done
2022-05-26T14:33:34.3939483Z -- Generating done
...

recipe/build.sh

recipe/meta.yaml

Co-authored-by: Robert Maynard <[email protected]>

…hem at runtime

kkraus14 · 2022-05-26T15:28:30Z

recipe/build.sh

+      -DCPM_Thrust_SOURCE=${PREFIX} \
+      -DCPM_spdlog_SOURCE=${PREFIX} \


We really shouldn't need these. It looks like RMM doesn't have the conda environment handling snippet that other RAPIDS libraries do (https://github.com/rapidsai/cudf/blob/branch-22.08/cpp/CMakeLists.txt#L122) which adds the conda env to the cmake prefix path to have it searched in find_package calls automatically.

I also think that you're pointing to these packages as if they're source directories that should be added via an add_subdirectory type of call, but they should be installed packages found via find_package. I think we could use Thrust_ROOT and spdlog_ROOT instead here.

How would setting -D<package>_ROOT=... prevent CPM from fetching a copy? I don't understand.

because CPM first calls find_package

Suggested change

-DCPM_Thrust_SOURCE=${PREFIX} \

-DCPM_spdlog_SOURCE=${PREFIX} \

-DThrust_ROOT=${PREFIX} \

-Dspdlog_ROOT=${PREFIX} \

@kkraus14 @robertmaynard Unfortunately it doesn't work...

@robertmaynard any insights here?

2022-05-26T17:22:44.7681580Z +CMAKE_PREFIX_PATH=$PREFIX:$BUILD_PREFIX/x86_64-conda-linux-gnu/sysroot/usr

This should be setting the search paths appropriately, but I think it's supposed to be semicolon separated as opposed to colon separated?

Looks like this may need to be fixed? https://github.com/conda-forge/ctng-compiler-activation-feedstock/blob/7ac7228866a63b3cd0f9db7ab9b7e362bf58650d/recipe/activate-gcc.sh#L92-L99

That is correct, You should use : for the the env variable CMAKE_PREFIX_PATH

I thought Conda's CMake would be able to find any cmake configs that come with the installed packages

It should. I will reproduce locally, and report back

Interesting, so a bad CMAKE_PREFIX_PATH would make the search fail...

~~Anyway, I sent a fix: conda-forge/ctng-compiler-activation-feedstock#76~~

Co-authored-by: Keith Kraus <[email protected]>

robertmaynard · 2022-05-26T20:21:02Z

Thinking about this more, we shouldn't do this.

The primary reason that rmm / RAPIDS packages thrust the way we do is to avoid having thrust placed in conda's env/include directory.

When CMake brings in thrust via find_package(Thrust) all the includes become system includes. As system includes are selected after user includes, nvcc implicit user include of the self packaged Thrust means that the conda installed Thrust will be ignored. ( See rapidsai/rapids-cmake#95 for more talk about this )

In short conda needs to:

Place the Thrust headers in a custom subfolder of <env>/include like RAPIDS does
Not bring header only packages like Thrust / Cub into conda OR document that they are unusable by CMake based projects

Note:
While https://gitlab.kitware.com/cmake/cmake/-/merge_requests/6627 mitigates this issue, it would need to be applied to all CMake projects in conda to ensure that none of them bring in <env>/include as a system path.

kkraus14 · 2022-05-26T20:29:17Z

nvcc implicit user include of the self packaged Thrust

Is this an nvcc thing or a CMake thing? Implicitly adding a user include seems very wrong...

robertmaynard · 2022-05-26T20:35:17Z

nvcc implicit user include of the self packaged Thrust

Is this an nvcc thing or a CMake thing? Implicitly adding a user include seems very wrong...

A nvcc thing.

leofang · 2022-05-26T20:44:57Z

Ouch thanks for the detailed explanation @robertmaynard, it's a big bummer... Do I understand you correctly regarding the search order issue that we should

abandon this librmm PR and stay with the status quo, and
find a way in rmm-feedstock to tell setup.py to look for librmm-bundled Thrust

?

robertmaynard · 2022-05-27T15:12:49Z

Ouch thanks for the detailed explanation @robertmaynard, it's a big bummer... Do I understand you correctly regarding the search order issue that we should

abandon this librmm PR and stay with the status quo, and

find a way in rmm-feedstock to tell setup.py to look for librmm-bundled Thrust

?

I think the best longer term option would be to update the thrust and cub packages in conda to install the headers using a 'stutter' approach. So instead of the headers being in <env>/include/thrust/ they would be in <env>/include/thrust/thrust/ and <env>/include/cub/cub/. This kind of layout is supported in Thrust and CUB starting in 1.16.

As an additional step we can patch thrust so that it sets the new IMPORTED_NO_SYSTEM target property which would allow any CMake based conda package to use the conda provided Thrust and CUB packages without issue.

In the shorter term I think we need to do the above and have setup.py to look for librmm-bundled Thrust

leofang · 2022-05-27T15:36:28Z

Thanks, @robertmaynard, one last question: The stutter approach would only work for CMake builds right? For packages like CuPy that do not use CMake to find Thrust/CUB and instead simply relying on -I this approach would break them?

robertmaynard · 2022-05-27T17:50:05Z

Thanks, @robertmaynard, one last question: The stutter approach would only work for CMake builds right? For packages like CuPy that do not use CMake to find Thrust/CUB and instead simply relying on -I this approach would break them?

Correct :(

leofang · 2022-05-27T18:37:14Z

@robertmaynard Would it be better then to modify the shutter approach such that <env>/include/thrust/thrust/ is a symlink to <env>/include/thrust/? Would CMake be happy with symlinks?

leofang · 2022-06-07T04:27:50Z

Let me close this PR and summary my finding learned from this PR and another (conda-forge/rmm-feedstock#11) in the original issue.

robertmaynard · 2022-07-15T17:54:55Z

To close the loop on this. I have proposed a fix to this in CMake itself: https://gitlab.kitware.com/cmake/cmake/-/issues/23731

rebuild to export thrust/cub

eb80087

leofang requested review from jakirkham, kkraus14 and rlratzel as code owners May 25, 2022 18:00

use Travis for aarch64

c86271f

MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.19.0, and co…

dac3727

…nda-forge-pinning 2022.05.25.14.40.58

kkraus14 reviewed May 25, 2022

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

pin major.minor

dc6d175

leofang changed the title ~~Pin Thrust/CUB exactly~~ Export Thrust/CUB version used at build time May 25, 2022

kkraus14 approved these changes May 25, 2022

View reviewed changes

disable CPM

6ca2b7d

leofang commented May 25, 2022

View reviewed changes

recipe/build.sh Outdated Show resolved Hide resolved

leofang added 2 commits May 25, 2022 15:32

try this fix

a883415

pin at thrust/cub 1.15

85cc23c

leofang mentioned this pull request May 25, 2022

Thrust v1.15 conda-forge/thrust-feedstock#17

Merged

5 tasks

leofang mentioned this pull request May 25, 2022

CUB v1.15 conda-forge/cub-feedstock#22

Merged

5 tasks

conda-forge-linter closed this May 25, 2022

conda-forge-linter reopened this May 25, 2022

try this

01028bf

kkraus14 reviewed May 25, 2022

View reviewed changes

recipe/build.sh Outdated Show resolved Hide resolved

leofang added 2 commits May 25, 2022 21:33

try this

938f923

retry: force using local copy

a47f803

Try using conda-forge's spdlog

facdefd

robertmaynard reviewed May 26, 2022

View reviewed changes

recipe/build.sh Outdated Show resolved Hide resolved

kkraus14 requested changes May 26, 2022

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

leofang and others added 2 commits May 26, 2022 10:28

fix style

0836c97

Co-authored-by: Robert Maynard <[email protected]>

we already pinned the versions, no need to force downstreams to see t…

a0ab5ee

…hem at runtime

kkraus14 requested changes May 26, 2022

View reviewed changes

set root instead

53569c6

Co-authored-by: Keith Kraus <[email protected]>

leofang added a commit to conda-forge-linter/rmm-feedstock that referenced this pull request Jun 1, 2022

pin thrust/cub at 1.15 (conda-forge/librmm-feedstock#31)

14a863d

leofang mentioned this pull request Jun 3, 2022

rmm v22.04.01 conda-forge/rmm-feedstock#11

Merged

3 tasks

leofang closed this Jun 7, 2022

leofang deleted the run_export branch June 7, 2022 04:28

leofang mentioned this pull request Jun 7, 2022

Thrust/CUB should be run-exported #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export Thrust/CUB version used at build time #31

Export Thrust/CUB version used at build time #31

leofang commented May 25, 2022 •

edited

Loading

conda-forge-linter commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

kkraus14 commented May 25, 2022

leofang commented May 25, 2022

robertmaynard commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 26, 2022 •

edited

Loading

leofang commented May 26, 2022 •

edited

Loading

kkraus14 May 26, 2022

leofang May 26, 2022

robertmaynard May 26, 2022

leofang May 26, 2022

leofang May 26, 2022

kkraus14 May 26, 2022

kkraus14 May 26, 2022

kkraus14 May 26, 2022

robertmaynard May 26, 2022

leofang May 26, 2022 •

edited

Loading

robertmaynard commented May 26, 2022

kkraus14 commented May 26, 2022

robertmaynard commented May 26, 2022

leofang commented May 26, 2022

robertmaynard commented May 27, 2022

leofang commented May 27, 2022

robertmaynard commented May 27, 2022

leofang commented May 27, 2022 •

edited

Loading

leofang commented Jun 7, 2022 •

edited

Loading

robertmaynard commented Jul 15, 2022

		-DCPM_Thrust_SOURCE=${PREFIX} \
		-DCPM_spdlog_SOURCE=${PREFIX} \

Export Thrust/CUB version used at build time #31

Export Thrust/CUB version used at build time #31

Conversation

leofang commented May 25, 2022 • edited Loading

conda-forge-linter commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 25, 2022

kkraus14 commented May 25, 2022

leofang commented May 25, 2022

robertmaynard commented May 25, 2022

leofang commented May 25, 2022

leofang commented May 26, 2022 • edited Loading

leofang commented May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofang May 26, 2022 • edited Loading

Choose a reason for hiding this comment

robertmaynard commented May 26, 2022

kkraus14 commented May 26, 2022

robertmaynard commented May 26, 2022

leofang commented May 26, 2022

robertmaynard commented May 27, 2022

leofang commented May 27, 2022

robertmaynard commented May 27, 2022

leofang commented May 27, 2022 • edited Loading

leofang commented Jun 7, 2022 • edited Loading

robertmaynard commented Jul 15, 2022

leofang commented May 25, 2022 •

edited

Loading

leofang commented May 26, 2022 •

edited

Loading

leofang commented May 26, 2022 •

edited

Loading

leofang May 26, 2022 •

edited

Loading

leofang commented May 27, 2022 •

edited

Loading

leofang commented Jun 7, 2022 •

edited

Loading