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

Make cuco a private dependency and leverage rapids-cmake #2432

Merged
merged 10 commits into from
Jul 25, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jul 19, 2022

After #2398 cuco is not actually part of libcugraph's public API and should not be required by consumers. Since it is already not being installed, this PR removes it from the public link interface of libcugraph. This PR also uses rapids-cmake to fetch cuco, ensuring that libcugraph remains in sync with the rest of RAPIDS.

There are a handful of tests that still rely on libcugraph::detail APIs that use cuco. To minimize the leakage of these dependencies, they are explicitly linked to cuco (rather than the current approach where they receive the cuco headers transitively from the libcugraph target).

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Jul 20, 2022

It looks like there are a handful of tests and test utilities that make use of cugraph detail APIs that use cuco. Is that expected? This looks like dependency leakage due to use of detail APIs outside of the package. I am investigating what a minimal solution would be for removing that dependency, perhaps simply compiling the necessary components into a small test utilities component inside cugraph similar to what cudf does. However, an ideal solution (which requires more cugraph expertise than I have) is finding a way to not use internal APIs outside of the main package.

CC @rlratzel @robertmaynard

@vyasr vyasr mentioned this pull request Jul 20, 2022
@rlratzel
Copy link
Contributor

cc @ChuckHastings

@vyasr vyasr force-pushed the build/cuco_private branch from 4e08e7b to f08929b Compare July 21, 2022 21:15
@vyasr
Copy link
Contributor Author

vyasr commented Jul 21, 2022

From some offline discussions, we've decided that the current tests are useful in their current form. While they may be rewritten at a later date after some significant internal refactorings, for now we're just going to link them directly to cuco to enable the continued used of the detail APIs.

@vyasr vyasr marked this pull request as ready for review July 21, 2022 21:20
@vyasr vyasr requested a review from a team as a code owner July 21, 2022 21:20
@rlratzel rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 22, 2022
@rlratzel rlratzel added this to the 22.08 milestone Jul 22, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Jul 22, 2022

I was surprised to find how many of the multi-GPU tests have a cuco dependency, but I think I've got them all now.

@ChuckHastings
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 2f301ad into rapidsai:branch-22.08 Jul 25, 2022
@vyasr vyasr deleted the build/cuco_private branch July 25, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants