Update cuvs to properly create a NCCL::NCCL target#720
Conversation
|
@robertmaynard Does this need to be revived for 25.06? @divyegala may care about this as he's doing some NCCL packaging work. |
bdice
added
improvement
Yes it does |
| if(BUILD_CAGRA_HNSWLIB) | ||
| ConfigureTest(NAME NEIGHBORS_HNSW_TEST PATH neighbors/hnsw.cu GPUS 1 PERCENT 100) | ||
| ConfigureTest( | ||
| NAME NEIGHBORS_HNSW_TEST PATH neighbors/hnsw.cu GPUS 1 PERCENT 100 ADDITIONAL_DEP NCCL::NCCL |
There was a problem hiding this comment.
Why was the link needed here? AFAIK this code path does not use NCCL.
There was a problem hiding this comment.
It pulls in nccl headers via helper headers it uses
There was a problem hiding this comment.
Do you have the trace? This seems like a leak to me. No problem if not, I can track it down - but shouldn't hold this PR on that, can fix as a follow up.
There was a problem hiding this comment.
re-testing locally and I can't reproduce the nccl dependency. So I am dropping this change
| ) | ||
| find_package(NCCL REQUIRED) | ||
| target_link_libraries(cuvs_objs PRIVATE NCCL::NCCL) | ||
| target_link_libraries(cuvs PUBLIC NCCL::NCCL) |
There was a problem hiding this comment.
I believe this should be a PRIVATE link. @viclafargue correct me if I am wrong but does any cuvs header include NCCL header or a RAFT header that includes NCCL header?
There was a problem hiding this comment.
Thanks for noticing this. The public cuVS MNMG API should only take in a raft::resources, everything related to NCCL is hidden inside the implementation. At least that is what we are aiming for with rapidsai/raft#2549 and #454.
There was a problem hiding this comment.
What is currently the status though? Are we exposing the NCCL header directly or indirectly to cuVS users from a header file?
There was a problem hiding this comment.
I believe that we do not have any direct or indirect reference to NCCL with the current version of the codebase.
There was a problem hiding this comment.
re-testing localled shows that I can have this as PRIVATE.
robertmaynard
force-pushed
the
properly_use_nccl_cmake_target
branch
from
7900cd8 to
611c9d2
Compare
|
@robertmaynard is this PR ready for a merge once CI passes? |
|
Yes it isOn Apr 16, 2025, at 2:23 PM, Divye Gala ***@***.***> wrote:
@robertmaynard is this PR ready for a merge once CI passes?—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
divyegala left a comment (rapidsai/cuvs#720)
@robertmaynard is this PR ready for a merge once CI passes?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
/merge |
5 participants
Previously cuvs was relying on the fact that nccl would be found when passed as
-lnccland be found on one of the implicit link directories.