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] Use FetchContent to get jitify and libcudacxx #5398

Merged
merged 6 commits into from
Jun 9, 2020

Conversation

harrism
Copy link
Member

@harrism harrism commented Jun 5, 2020

Following on #5315, for consistency this PR uses FetchContent to get the source of jitify and libcudacxx rather than using git submodules.

Also updates to use the latest jitify branch (cudf_0.15) to fix CUDA 11 testing issues.

Closes #5369.

@harrism harrism added 2 - In Progress Currently a work in progress CMake CMake build issue labels Jun 5, 2020
@harrism harrism requested a review from a team as a code owner June 5, 2020 07:50
@harrism harrism self-assigned this Jun 5, 2020
@harrism harrism changed the title [WIP] Use FetchContent to get jitify and libcudacxx instead of submodules. [WIP] Use FetchContent to get jitify and libcudacxx Jun 5, 2020
@kkraus14
Copy link
Collaborator

kkraus14 commented Jun 5, 2020

Just need to update these lines to fix the Cython errors: https://github.com/rapidsai/cudf/blob/branch-0.15/python/cudf/setup.py#L47-L49

Can remove the thrust / cub lines and then update the path to the libcudacxx/include.

"${CMAKE_CUDA_TOOLKIT_INCLUDE_DIRECTORIES}"
"${CMAKE_BINARY_DIR}/include"
"${CMAKE_SOURCE_DIR}/include"
"${CMAKE_SOURCE_DIR}"
"${CMAKE_SOURCE_DIR}/src"
"${CMAKE_SOURCE_DIR}/thirdparty/dlpack/include"
"${CMAKE_SOURCE_DIR}/thirdparty/jitify"
"${CMAKE_SOURCE_DIR}/thirdparty/libcudacxx/include"
"${LIBCUDACXX_INCLUDE_DIR}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

May want to move this above the CUDA toolkit includes as well just in case it ships in the toolkit in the future.

@harrism
Copy link
Member Author

harrism commented Jun 8, 2020

Just need to update these lines to fix the Cython errors: https://github.com/rapidsai/cudf/blob/branch-0.15/python/cudf/setup.py#L47-L49

Can remove the thrust / cub lines and then update the path to the libcudacxx/include.

Thanks. We can't remove the thrust/cub lines or Cython will build using the Toolkit version of thrust/cub while everything else uses the FetchContent version. I will update the libcudacxx line.

@kkraus14
Copy link
Collaborator

kkraus14 commented Jun 9, 2020

Thanks. We can't remove the thrust/cub lines or Cython will build using the Toolkit version of thrust/cub while everything else uses the FetchContent version. I will update the libcudacxx line.

There's no thrust / Cub types exposed in libcudf APIs that are plumbed into Cython so the headers are unused currently.

@harrism
Copy link
Member Author

harrism commented Jun 9, 2020

@kkraus14 I was able to remove those but I had to add the following:

"../../cpp/build/_deps/libcudacxx-src/include",
"../../cpp/build/release/_deps/libcudacxx-src/include",

This is really dependent on where the user decides to put the libcudf build directory. Is there a way to set the build directory generically?

EDIT: going with CUDF_ROOT env variable and falling back to cpp/build if it is not set.

@harrism harrism added the 3 - Ready for Review Ready for review by team label Jun 9, 2020
@harrism harrism changed the title [WIP] Use FetchContent to get jitify and libcudacxx [REVIEW] Use FetchContent to get jitify and libcudacxx Jun 9, 2020
@harrism harrism requested review from kkraus14 and trxcllnt June 9, 2020 04:12
cpp/CMakeLists.txt Show resolved Hide resolved
@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 2 - In Progress Currently a work in progress 3 - Ready for Review Ready for review by team labels Jun 9, 2020
@harrism harrism requested a review from a team as a code owner June 9, 2020 06:10
@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #5398 into branch-0.15 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.15    #5398   +/-   ##
============================================
  Coverage        88.63%   88.63%           
============================================
  Files               57       57           
  Lines            10769    10769           
============================================
  Hits              9545     9545           
  Misses            1224     1224           

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 36211b9...e2c16ff. Read the comment docs.

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 CMake build issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Add support for CUDA 11
2 participants