-
Notifications
You must be signed in to change notification settings - Fork 639
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
Dynamic cuda wrapper #1065
Dynamic cuda wrapper #1065
Conversation
* fix cmake version: default cmake version of ubuntu20.04 is too low
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
if(NOT USE_CUDA_WRAPPER) | ||
target_link_libraries(bitsandbytes PUBLIC CUDA::cublas CUDA::cusparse) | ||
endif() | ||
if(NO_CUBLASLT) | ||
target_compile_definitions(bitsandbytes PUBLIC NO_CUBLASLT) | ||
else() | ||
target_link_libraries(bitsandbytes PUBLIC CUDA::cublasLt) | ||
if(NOT USE_CUDA_WRAPPER) | ||
target_link_libraries(bitsandbytes PUBLIC CUDA::cublasLt) | ||
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.
CMake has CMAKE_CUDA_RUNTIME_LIBRARY
to control this; maybe would be better here.
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.
https://cmake.org/cmake/help/latest/variable/CMAKE_CUDA_RUNTIME_LIBRARY.html
docuement says CMAKE_CUDA_RUNTIME_LIBRARY
only control cudart
(Currently, with or without this PR, cudart_static
added by default)
I'm a little worried that the API or ABI could have changed between 11 and 12, causing very weird runtime errors if we just use symbols from v12 and call them like they were v11, or vice versa. 🤔 |
Hey @wkpark @akx @matthewdouglas, I'm hoping to merge this in the coming two weeks. One thing I feel we need to discuss is how we can test if this works as intended without introducing any issues: Do you have any thoughts on that? I'm willing to do the work, but if you already have a few concrete things in mind, that would definitely help speed this up. Thanks @wkpark on your initiative on this, really appreciated! Another thing needed would be to resolve the conflicts. |
I'm not sure how I feel about this one yet. I will think about it a little more but my initial thought is that it seems to be quite hacky feeling. Maybe this is a common thing in C++ world, but I'm not used to seeing it. Edit: This would definitely conflict with what I was thinking with #1126 too. |
I also get a hacky/too-complex feeling here, but I'm not sure what the better option would be. |
Thanks for your valuable inputs ❤️ Really appreciated. |
9b72679
to
7800734
Compare
Thank you so much for this thoughtful PR. We were thinking about binary size over time and this is an important issue together with support for many different CUDA minor versions. Your work on this is truly appreciated. Overall, this would be very nice to lead to a smaller binary size and support all kind of different cuda minor versions we are currently not supporting with a direct pip install. Your approach is clever and well-implemented. However, currently we are very limited in providing support and guidance for debugging installation issues that come from users that might have problems with their CUDA setup. In particular, in the past, we had a more dynamic installation and it cause problems if a user had multiple CUDA installations. As such, we do not see us in the position at the moment to be able to support or switch to a dynamic CUDA wrapper since it would add additional maintenance burden. This means, we will only support a few CUDA options at this point. We will evaluate the way we ship binaries and support binaries every now and then and it might be that we will come back to your PR if we decide to go with dynamic CUDA wrapping. Your contribution has certainly given us valuable insights to consider for the future. We genuinely appreciate the time and effort you've put into this PR, and we hope you'll continue to be an active part of our open-source community. |
This is a proof-of-concept, quick and dirty, CUDA version independent, dymamic CUDA library wrapper fix.
cuda_setup/main.py
compatible with older libraries. (older libraries mean libs with cuda version tags)cublas*
,cusparse*
,cublasLt*
functions are wrapped usingdlsym()/GetProcessAddress()
cudart_static
lib.ubuntu-20.04 + cuda11.8 environment.
windows 10 + cuda11.8 environment (mingw64 terminal)