-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Build] Fix cuda link target of cumem_allocator in CPU env #12863
[Build] Fix cuda link target of cumem_allocator in CPU env #12863
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
38bf907
to
3b09f0c
Compare
Signed-off-by: YuhongGuo <yuhong.gyh@antgroup.com>
@tlrmchlsmth do you some comments on this code change? |
@@ -192,7 +192,7 @@ set_gencode_flags_for_srcs( | |||
if(VLLM_GPU_LANG STREQUAL "CUDA") | |||
message(STATUS "Enabling cumem allocator extension.") | |||
# link against cuda driver library | |||
list(APPEND CUMEM_LIBS cuda) | |||
list(APPEND CUMEM_LIBS CUDA::cuda_driver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me -- @youkaichao WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can we just delete this line completely? We have this link line in define_gpu_extension_target
already:
Lines 437 to 443 in 2ae8890
# Don't use `TORCH_LIBRARIES` for CUDA since it pulls in a bunch of | |
# dependencies that are not necessary and may not be installed. | |
if (GPU_LANGUAGE STREQUAL "CUDA") | |
target_link_libraries(${GPU_MOD_NAME} PRIVATE CUDA::cudart CUDA::cuda_driver) | |
else() | |
target_link_libraries(${GPU_MOD_NAME} PRIVATE ${TORCH_LIBRARIES}) | |
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my analysis, only when GPU_LANGUAGE STREQUAL "CUDA"
the CUDA::cudart
lib is linked in define_gpu_extension_target
. However, the GPU_LANGUAGE
for cumem_allocator is CXX
. I think that is why extra libcuda is added to CUMEM_LIBS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Verified that this doesn't break my existing build process. I also feel comfortable with the change as this is the official way to do it -see https://cmake.org/cmake/help/latest/module/FindCUDAToolkit.html.
Going to see if this avoids the ld issues that popped up when I tried #12424
@tlrmchlsmth Thanks. I also tested whether we could remove this line of code. The building process was fine but there will be undefined symbol as follows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix!
…ect#12863) Signed-off-by: YuhongGuo <yuhong.gyh@antgroup.com> Co-authored-by: Tyler Michael Smith <tyler@neuralmagic.com> Signed-off-by: SzymonOzog <szymon.ozog@aleph-alpha.com>
…ect#12863) Signed-off-by: YuhongGuo <yuhong.gyh@antgroup.com> Co-authored-by: Tyler Michael Smith <tyler@neuralmagic.com>
…ect#12863) Signed-off-by: YuhongGuo <yuhong.gyh@antgroup.com> Co-authored-by: Tyler Michael Smith <tyler@neuralmagic.com>
…ect#12863) Signed-off-by: YuhongGuo <yuhong.gyh@antgroup.com> Co-authored-by: Tyler Michael Smith <tyler@neuralmagic.com>
FIX #12862
It looks like that the build system could not find
-lcuda
correctly in the CPU container.list(APPEND CUMEM_LIBS cuda)
is too simple for a CPU build env,ld
cannot find the location. It is better to useCUDA::cuda_driver
which is a full path.