Remove direct UCX and NCCL dependencies #5038

vyasr · 2022-11-30T18:28:01Z

libcuml does not compile against UCX and NCCL, it relies on raft for those communicators. Moreover, because of raft's design cuML does not even need to link against those libraries. It should be sufficient to simply use raft-dask's Python API since that package is what will link to UCX and NCCL.

vyasr · 2022-11-30T18:29:03Z

CC @cjnolet

I've targeted 23.02 just in case there are any surprises that pop up, don't want to break us right before code freeze.

cjnolet · 2022-11-30T18:36:41Z

This looks good, but I think we will also need to remove the kmeans mg test from the build, which is trying to use std comms directly. It should instead be using the mpi comms for integration tests.

dantegd · 2022-12-08T11:25:30Z

Changes look good pending conflict resolution. Also @vyasr could you open an issue to fix the issue with the kmeans_mg test? It is not being built currently, but we should fix (or move) it later.

cjnolet · 2022-12-12T19:53:03Z

cpp/test/CMakeLists.txt

@@ -16,7 +16,7 @@

 function(ConfigureTest)

-  set(options OPTIONAL NCCL CUMLPRIMS MPI ML_INCLUDE)


The more I think about this, I believe we are still going to need to link against NCCL to use raft/comms/mpi_comms.hpp but only for the mg tests and nowhere else. The mpi_comms.hpp are essentially creating a NCCL cluster using MPI and then handing off to NCCL everywhere (think of it like replacing Dask w/ MPI).

Maybe we could use the raft::distributed COMPONENT for that? (or maybe not, since it also brings in UCX...).

Components are neat, added raft::distributed linkage and notes of added dependencies (MPI, NCCL) for C++ MG tests

Are cuml's MG tests using libraft for NCCL in the same way that raft-dask uses libraft for UCX? I'm not quite sure how this code works; does cuml's MG code not directly use NCCL, but the MG tests do?

Cuml's MG googletests should be using MPI, however they should still be tied to the comms interface so shouldn't need to know NCCL exists at all (this NCCL can be provided transitively through the raft::distributed component). Note also that wherher the underlying MPI configuration has been configured with UCX underneath is up to the user running the tests (so the raft MPI communicator will use ucx by proxy and not directly)

Cuml's mg pytests all use raft-dask.

Right the python tests are fine. I guess the difference is that with the pytests raft-dask is a Python package whose underlying extension modules are linked to NCCL, and then cuml Python only interacts with it at the Python layer, whereas the cuml C++ tests are either linking directly to libraft or building against raft's headers, and in either case the distributed components are not precompiled anyway so there's always NCCL linking required?

Since we are using PIMPL for the comms, the mg tests shouldnt need nccl during compile time. But yes, the mg tests binary will need to have raft::distributed on the link libraries (which should pass nccl through)

Yup OK cool. makes sense. Dante has already made the necessary change here by adding the distributed component in get_raft.cmake, so I think we're good here.

cjnolet · 2022-12-12T19:54:53Z

cpp/test/CMakeLists.txt

@@ -197,14 +196,15 @@ endif()

 if(BUILD_CUML_MG_TESTS)

-  ConfigureTest(PREFIX MG NAME KMEANS_TEST PATH mg/kmeans_test.cu OPTIONAL NCCL CUMLPRIMS ML_INCLUDE)
+  # This test needs to be rewritten to use the MPI comms, not the std comms.


I think it's fine to comment this out, but can you also create a github issue just so it doesn't get forgotten and reference the issue # here?

Added link to GH issue

ajschmidt8

Approving ops-codeowner file changes

vyasr · 2022-12-16T21:58:33Z

@cjnolet looks like there's still an open question re:NCCL linking here, once we resolve that this PR should be good to go.

codecov-commenter · 2022-12-17T05:21:02Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.02@38dea2f). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff               @@
##             branch-23.02    #5038   +/-   ##
===============================================
  Coverage                ?   79.21%           
===============================================
  Files                   ?      192           
  Lines                   ?    12377           
  Branches                ?        0           
===============================================
  Hits                    ?     9805           
  Misses                  ?     2572           
  Partials                ?        0

Flag	Coverage Δ
dask	`45.81% <0.00%> (?)`
non-dask	`69.23% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

cjnolet · 2022-12-17T05:35:06Z

@gpucibot merge

these probably should've been removed in rapidsai#5038

libcuml does not compile against UCX and NCCL, it relies on raft for those communicators. Moreover, because of raft's design cuML does not even need to link against those libraries. It should be sufficient to simply use `raft-dask`'s Python API since that package is what will link to UCX and NCCL. Authors: - Vyas Ramasubramani (https://github.com/vyasr) - Dante Gama Dessavre (https://github.com/dantegd) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) - AJ Schmidt (https://github.com/ajschmidt8) - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai#5038

vyasr added 2 commits November 30, 2022 10:22

Remove direct dependence on ucx.

de7c83b

Try removing nccl as well.

11c8721

vyasr added 2 - In Progress Currenty a work in progress Tech Debt Issues related to debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 30, 2022

vyasr requested a review from a team as a code owner November 30, 2022 18:28

vyasr self-assigned this Nov 30, 2022

github-actions bot added CMake CUDA/C++ labels Nov 30, 2022

Remove all NCCL references.

a2d829c

vyasr added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Dec 1, 2022

dantegd approved these changes Dec 8, 2022

View reviewed changes

dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Dec 8, 2022

dantegd added 2 commits December 12, 2022 13:38

Merge 23.02 into PR

b974490

UPD remove ucx/nccl from all recipes/envs

f4f9649

dantegd requested a review from a team as a code owner December 12, 2022 19:44

github-actions bot added the conda conda issue label Dec 12, 2022

FIX remove straggling references

858bdb4

cjnolet requested changes Dec 12, 2022

View reviewed changes

ajschmidt8 approved these changes Dec 12, 2022

View reviewed changes

UPD Updates from PR review

3d25dca

dantegd requested a review from a team as a code owner December 12, 2022 21:51

FIX typo

838b83d

Merge branch 'branch-23.02' into remove_direct_ucx

2838327

dantegd mentioned this pull request Dec 17, 2022

Update ucx to 0.30 in dev envs #5086

Closed

cjnolet approved these changes Dec 17, 2022

View reviewed changes

rapids-bot bot merged commit 1ea431d into rapidsai:branch-23.02 Dec 17, 2022

ajschmidt8 added a commit to csadorf/cuml that referenced this pull request Dec 17, 2022

rm extraneous nccl and ucx references

b98db89

these probably should've been removed in rapidsai#5038

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove direct UCX and NCCL dependencies #5038

Remove direct UCX and NCCL dependencies #5038

vyasr commented Nov 30, 2022

vyasr commented Nov 30, 2022 •

edited

Loading

cjnolet commented Nov 30, 2022

dantegd commented Dec 8, 2022

cjnolet Dec 12, 2022

dantegd Dec 12, 2022

vyasr Dec 13, 2022

cjnolet Dec 16, 2022

vyasr Dec 16, 2022

cjnolet Dec 16, 2022

vyasr Dec 17, 2022

cjnolet Dec 12, 2022

dantegd Dec 12, 2022

ajschmidt8 left a comment

vyasr commented Dec 16, 2022

codecov-commenter commented Dec 17, 2022

cjnolet commented Dec 17, 2022

		@@ -16,7 +16,7 @@

		function(ConfigureTest)

		set(options OPTIONAL NCCL CUMLPRIMS MPI ML_INCLUDE)

Remove direct UCX and NCCL dependencies #5038

Remove direct UCX and NCCL dependencies #5038

Conversation

vyasr commented Nov 30, 2022

vyasr commented Nov 30, 2022 • edited Loading

cjnolet commented Nov 30, 2022

dantegd commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajschmidt8 left a comment

Choose a reason for hiding this comment

vyasr commented Dec 16, 2022

codecov-commenter commented Dec 17, 2022

Codecov Report

cjnolet commented Dec 17, 2022

vyasr commented Nov 30, 2022 •

edited

Loading