Removing CUDA Runtime Dependency from plugin - #1359
Merged
Merged
Conversation
Contributor
|
For bisectability, doesn't your first commit (removing the CUDA linking) need to come last, after you have changed everything to the driver API? |
Contributor
Author
Ahh yes. I think you are right. I kept the order of development but I will change that. |
amitrad-aws
previously approved these changes
Aug 18, 2026
amitrad-aws
left a comment
amitrad-aws
left a comment
Contributor
There was a problem hiding this comment.
Agree with the order change, please do that
hershys-aws
force-pushed
the
fix-cuda-runtime-error
branch
from
August 18, 2026 23:53
2206a83 to
1b9cef6
Compare
bwbarrett
requested changes
Aug 18, 2026
bwbarrett
left a comment
bwbarrett
left a comment
Contributor
There was a problem hiding this comment.
A couple of your commit messages could use some changes. We use subsection prefixes but not the fix/whatever/ type prefixes. It's also not clear why we're making the change in some, and leaving future us some notes would be helpful.
| cudaFree(d_cq); | ||
| int ret; | ||
|
|
||
| if (!d_cq) return; |
Contributor
There was a problem hiding this comment.
this isn't a 1:1 rewrite; why the changes?
Contributor
Author
hershys-aws
force-pushed
the
fix-cuda-runtime-error
branch
2 times, most recently
from
August 19, 2026 01:37
f1e607c to
ada90e5
Compare
bwbarrett
reviewed
Aug 19, 2026
hershys-aws
force-pushed
the
fix-cuda-runtime-error
branch
from
August 19, 2026 05:17
ada90e5 to
03de6be
Compare
Contributor
|
Who maintains |
hershys-aws
force-pushed
the
fix-cuda-runtime-error
branch
5 times, most recently
from
August 19, 2026 18:10
7242739 to
9f416f2
Compare
amitrad-aws
reviewed
Aug 19, 2026
bwbarrett
approved these changes
Aug 19, 2026
bwbarrett
left a comment
bwbarrett
left a comment
Contributor
There was a problem hiding this comment.
nit: your commit message subjects should be declartive, so something like:
efa-gda: Translate cuda runtime function calls to cuda driver function calls
not the current
efa-gda: Translating cuda runtime function calls to cuda driver function calls
…n calls The efa-gda host code (libefa_cuda_dp) used the CUDA Runtime API, which forces the plugin to link the runtime and breaks --enable-cudart-dynamic. Translate those calls to the CUDA driver API, provided through wrappers in nccl_ofi_cuda.cpp, so a later commit can drop the runtime link. cuGetErrorName / cuGetErrorString are wrapped to provide a driver equivalent of cudaGetErrorString, which efa-gda uses and the driver API does not offer directly. Signed-off-by: Hershel Shah <hershys@amazon.com>
The gdrcopy path used the CUDA Runtime (cudaGetDevice/cudaSetDevice) to bind a device for its lazy signal-segment discovery, which keeps the runtime linked and breaks --enable-cudart-dynamic. The driver API has no cudaSetDevice equivalent, so rather than bind a device we now capture the application's current CUDA context at comm init (cuCtxGetCurrent) and make it current (cuCtxSetCurrent) on NCCL's context-less GIN progress thread before cuMemGetAddressRange. Also replaces the remaining runtime call, cudaMemset, with cuMemsetD8. Signed-off-by: Hershel Shah <hershys@amazon.com>
$(CUDA_RUNTIME_LIBS) was added to libinternal_plugin only because the efa-gda GDAKI host code (libefa_cuda_dp.la) used CUDA Runtime APIs. That code is now on the CUDA driver API, so the extra linkage is unnecessary; dropping it also avoids a hard libcudart dependency in the plugin under --enable-cudart-dynamic. Signed-off-by: Hershel Shah <hershys@amazon.com>
The gdrcopy unit test calls cudaSetDevice and cudaGetErrorString from the CUDA runtime directly. It previously resolved these transitively through libinternal_plugin, but the plugin no longer links the CUDA runtime so that --enable-cudart-dynamic keeps working. Link CUDA_RUNTIME_LIBS into the gdrcopy target so the test builds without pulling the runtime back into the plugin. CUDA_RUNTIME_LIBS is -lcudart for dynamic builds and empty for static builds, where cudart_static is already provided by CUDA_LIBS. Signed-off-by: Hershel Shah <hershys@amazon.com>
hershys-aws
force-pushed
the
fix-cuda-runtime-error
branch
from
August 19, 2026 20:00
0120b2e to
9b9b9f3
Compare
bwbarrett
approved these changes
Aug 19, 2026
amitrad-aws
approved these changes
Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Description of changes:
Removing CUDA runtime functions introduced by multi-segment host memory functionality as well as EFA DP Direct.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.