-
Notifications
You must be signed in to change notification settings - Fork 309
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
bump NCCL floor to 2.18.1.1, include nccl.h where it's needed #4661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cugraph also ought to be able to rely on raft for its NCCL handling rather than doing anything on its own AFAICT. It does use some raft APIs that accept NCCL types directly as arguments, but I still think that's OK.
I'm not convinced that It has some direct uses, like these:
As well as others in tests and examples: cugraph/cpp/tests/mtmg/threaded_test.cu Line 41 in 7e058e2
Those look like direct-enough uses that |
I agree, those are direct uses. In that case, though, the files should be including nccl.h rather than relying on it being transitively included by raft's std_comms.h. |
Agreed! Pushed 7fca13f adding that. Maybe in the future we can explore using |
/merge |
Description
Contributes to rapidsai/build-planning#102
Some RAPIDS libraries are using
ncclCommSplit()
, which was introduced innccl==2.18.1.1
. This is part of a series of PRs across RAPIDS updating libraries' pins tonccl>=2.18.1.1
to ensure they get a new-enough version that supports that.