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

Patch cccl and cuco so that all CUDA kernels they provide have internal linkage #14735

Conversation

robertmaynard
Copy link
Contributor

Description

Patch both CCCL and CUCO to have only internal linkage.

For cuco I am working on upstreaming these changes ( NVIDIA/cuCollections#422 ). Once that is accepted and we have validated that moving cuco is stable ( e.g. changes around cuco::experimental::static_set ) we can drop this patch set.

For cccl the long term fix is to move to CCCL 2.3+, but due to issues ( NVIDIA/cccl#1249, maybe others ) that isn't viable currently. If these patches work for cudf CI, I believe we can move them to rapids-cmake and have the rapids_cpm_cccl package automatically provide them.
Since the CCCL changes mean C++ and CUDA sources have non compatible ABI's, we need to specify THRUST_DISABLE_ABI_NAMESPACE. Likewise we need to specify THRUST_IGNORE_ABI_NAMESPACE_ERROR so that we don't change our ABI ( since it depends on thrust types ).

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@robertmaynard robertmaynard added bug Something isn't working Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change labels Jan 10, 2024
@robertmaynard robertmaynard requested a review from a team as a code owner January 10, 2024 19:03
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Jan 10, 2024
@robertmaynard robertmaynard force-pushed the bug/mark_cccl_cuco_kernels_as_hidden branch from 439ffdc to dd165b0 Compare January 11, 2024 17:18
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

This seems fine to me. @robertmaynard Can you expand a bit more on these questions before I approve:

  1. How this was tested/validated with Spark-RAPIDS?

Since the CCCL changes mean C++ and CUDA sources have non compatible ABI's, we need to specify THRUST_DISABLE_ABI_NAMESPACE.

To clarify, "the CCCL changes" are from the patch in this PR, and they are the same namespace changes as in 2.3.0? (In other words, this patch is a partial backport of 2.3.0?)

If these patches work for cudf CI, I believe we can move them to rapids-cmake and have the rapids_cpm_cccl package automatically provide them.

This PR's CI is passing. Will we be applying these same CCCL/cuCollections patches to the rapids-cmake CCCL/cuCollections? Do you want to open that PR and test it on other RAPIDS repos (e.g. RAFT, cuML, cuGraph) before merging this one?

@robertmaynard
Copy link
Contributor Author

robertmaynard commented Jan 16, 2024

@bdice

  1. This was manually tested with a known failure case in a spark-rapids test. @ttnghia can elaborate if needed
  2. Yes this is a backport of Fix Thrust/CUB Linkage Issues NVIDIA/cccl#443
    3a. Yes we can backport the cucollections changes to rapids-cmake. We can't move forward the cuCollections SHA1 without code changes so I expect we can wait till 24.04 for that
    3b. I am happy to move the CCCL changes over to rapids-cmake and test with cuml/cugrap/etc. Since burndown starts in 2 days we should have time to get these changes merged into rapids-cmake

Edit:
rapidsai/rapids-cmake#523

@robertmaynard
Copy link
Contributor Author

Closing as rapidsai/rapids-cmake#523 has been merged

@robertmaynard robertmaynard deleted the bug/mark_cccl_cuco_kernels_as_hidden branch January 18, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants