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

Add support for RMA over UCP #166

Merged
merged 12 commits into from
Feb 23, 2024

Conversation

mdemoret-nv
Copy link
Contributor

@mdemoret-nv mdemoret-nv commented Jan 16, 2024

This PR adds support for RMA over UCP protocols, adding interfaces for the following functions: ucp_get_nbx, ucp_put_nbx, ucp_ep_flush_nbx and ucp_worker_flush_nbx. The implementation follows the same pattern use for all ucxx::Request operations.

Blocked waiting for #190 .

Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

In a first pass, things look like they're going in the right direction, I've left a couple of comments. It would be great if you could also add some tests.

@@ -77,6 +77,7 @@ rapids_cmake_support_conda_env(conda_env MODIFY_PREFIX_PATH)
# * compiler options ------------------------------------------------------------------------------
rapids_find_package(
ucx REQUIRED
GLOBAL_TARGETS ucx::ucp ucx::ucs ucx::uct
Copy link
Member

Choose a reason for hiding this comment

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

I think such changes shouldn't be necessary, you mentioned offline they were required to build from inside another project. We'll have our CMake experts look at what would need to change here when the PR is ready for review.

Comment on lines 79 to 83
bool send,
void* buffer,
size_t length,
uint64_t remote_addr,
ucp_rkey_h rkey,
Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline, the changes were made on top of an older branch before #121 was merged (on December 12), which reorganizes how data is passed to constructors and thus prevents adding several different arguments to them as is done here, we now instead have “data classes” that include all operation-specific arguments and makes the API cleaner and less error-prone (i.e., with the user accidentally swapping arguments), as you can see in the newest constructors. We will want to have this updated to the new style.

@pentschev pentschev changed the base branch from branch-0.36 to branch-0.37 February 9, 2024 20:47
@pentschev pentschev added feature request New feature or request non-breaking Introduces a non-breaking change DO NOT MERGE Hold off on merging; see PR for details labels Feb 9, 2024
@pentschev pentschev changed the title Add support for functions ucp_get_nbx and ucp_put_nbx Add support for RMA over UCP Feb 12, 2024
cpp/include/ucxx/request_flush.h Outdated Show resolved Hide resolved
cpp/src/request_mem.cpp Show resolved Hide resolved
cpp/src/request_mem.cpp Show resolved Hide resolved
cpp/src/request_mem.cpp Show resolved Hide resolved
@mdemoret-nv mdemoret-nv marked this pull request as ready for review February 15, 2024 21:50
@mdemoret-nv mdemoret-nv requested review from a team as code owners February 15, 2024 21:50
rapids-bot bot pushed a commit that referenced this pull request Feb 20, 2024
Add `ucxx::MemoryHandle` and `ucxx::RemoteKey` C++ classes, wrapping UCP memory handles (`ucp_mem_h`) and remote keys (`ucp_rkey_h`), respectively.

These new classes are required to provide basic support for RMA (Remote Memory Access) operations being implemented in #166 .

Authors:
  - Peter Andreas Entschev (https://github.com/pentschev)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)
  - Bradley Dice (https://github.com/bdice)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #190
@pentschev
Copy link
Member

Thanks @mdemoret-nv for working on this!

@pentschev
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit fc9f4f3 into rapidsai:branch-0.37 Feb 23, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Hold off on merging; see PR for details feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants