Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[FEA] Remove FAISS dependency, inherit other common dependencies from raft #1863
[FEA] Remove FAISS dependency, inherit other common dependencies from raft #1863
Changes from 20 commits
d9f5ec6
efe559d
332fed7
8e509de
d0077e0
b32f1cc
9ea0f75
e7e4160
ce9d0df
a9f3630
4155315
896df79
de281b4
4d579f8
b3c7e61
5d0c4db
fa1e96c
92d794c
7e635ff
91d217f
583024c
6eeead5
fb155ab
1d71d15
efa2d26
0ceb8d0
c3c7422
13022c1
0e47956
dbc90e7
b012c62
23c146f
68afa4d
242c7bb
639fd12
64721b2
822c747
9d5c700
f62fb52
15e89b9
15010b6
aa68e0d
6e85d8f
e0d20f4
028b893
90281a6
02e755c
e204658
2f93d49
04f6728
bad10fa
f5710c3
0d7c110
940c0e3
9729a3d
ac8fcfa
8b0ef92
a167883
1865b40
d5b5c5d
b732c18
e69f402
1c6b000
877faad
6fa9197
fddef0e
b264da1
db9e359
300b0e4
879223c
23de039
c65d2ff
a8a13f5
bb815aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
raft also uses nccl. Is there a reason for including the nccl dependency here as well? Seems like if we're going to remove dependencies we inherit from raft we should not include nccl here.
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.
Yeah I agree, just wasn't successful at making raft's
get_nccl.cmake
export itsNCCL::NCCL
target. Could use some help from @robertmaynard here.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.
It looks RAFT only optional uses nccl. Specifically only the
comms
components of RAFT require NCCL.What RAFT should do is have finer grained targets ( raft_comm ) which allow you to bring in the NCCL dependency.
I expect that CUML would want to keep NCCL as an optional dependency of RAFT since they don't require it when building for single gpu.
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.
@ChuckHastings, isnt cugraph is still using the RAFT comms?
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.
Yes, cugraph uses the RAFT comms implementation.
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.
Ah, I misunderstood the original statement- you were saying cugraph should be getting nccl transitively through raft.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.