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 ucx linkage explicit and add a new CMake target for it #1032

Merged
merged 9 commits into from
Nov 18, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Nov 18, 2022

This PR removes the dlopen logic for libucp in ucp_helper.hpp in favor of calling the relevant APIs directly. It also adds a new CMake component raft::distributed that can be used by dependent libraries to indicate the dependency on parts of raft that require UCX.

While it does not change any public APIs, I have marked this PR as breaking since it does mean that any C++ code linking to UCX must now ensure that UCX is available at link time. It is no longer sufficient to make the library available at runtime.

Resolves #1031.

@vyasr vyasr added 3 - Ready for Review improvement Improvement / enhancement to an existing function breaking Breaking change cpp labels Nov 18, 2022
@vyasr vyasr self-assigned this Nov 18, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Nov 18, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Nov 18, 2022

Thanks for fixing this, @vyasr. Now we just need to add ucx as an explicit link dependency of cuml and cugraph

@vyasr vyasr marked this pull request as ready for review November 18, 2022 15:57
@vyasr vyasr requested review from a team as code owners November 18, 2022 15:57
@vyasr
Copy link
Contributor Author

vyasr commented Nov 18, 2022

Thanks for fixing this, @vyasr. Now we just need to add ucx as an explicit link dependency of cuml and cugraph

Done.

@vyasr vyasr mentioned this pull request Nov 18, 2022
@vyasr vyasr changed the title Make ucx linkage explicit Make ucx linkage explicit and add a new CMake target for it Nov 18, 2022
@cjnolet
Copy link
Member

cjnolet commented Nov 18, 2022

@robertmaynard this adds a new optional component for distributed (which currently adds ucx but will add NCCL in the future). Any chance we can get your blessing on these changes?

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @vyasr!

@cjnolet
Copy link
Member

cjnolet commented Nov 18, 2022

@gpucibot merge

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented Nov 18, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c743916 into rapidsai:branch-22.12 Nov 18, 2022
@vyasr vyasr deleted the make_ucx_explicit branch November 18, 2022 23:31
rapids-bot bot pushed a commit that referenced this pull request Dec 9, 2022
This is a follow-up to #1032 to include NCCL in raft::distributed so that all communications libraries are now cleanly encapsulated in a single interface dependency, allowing us to remove it from the raft-dask Python build and simplifying the process for other consumers.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #1077
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review breaking Breaking change CMake cpp improvement Improvement / enhancement to an existing function python
Projects
Development

Successfully merging this pull request may close these issues.

[FEA] Remove dlopen of ucx from std_comms
2 participants