-
Notifications
You must be signed in to change notification settings - Fork 627
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
Cmake + workflows #908
Cmake + workflows #908
Conversation
|
||
matrix: | ||
os: [ubuntu-latest] | ||
python-version: ['3.10', '3.11'] |
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.
I would recommend building only the native library here using a build matrix, then a separate job to build the Python binary wheels.
Is this PR complete? I remember there being some Windows-specific stuff needed, also some things for Linux/Arm64 that required specific handling. Unfortunately the original PRs (#220 etc) cannot be viewed because of force pushes. I'll see if I can dig up a relevant diff |
Windows specific stuff extracted on PR #873 and #876 (and pytest stuff is not fixed/tested much since I have no sm_75 capable GPU.) |
I don't have a Windows machine at hand right now, but there are also specific compiler flags that MSVC needs. I think this is what PR #220 did. I spend a few minutes to rebase and create a draft PR in #947 just for reference, which should compile on all platforms. Maybe it's worth just comparing notes on those? @wkpark @TimDettmers |
I am not sure if current versions of bitsandbytes depend on pthread but attempt #220 imports pthread-win32 as pthread isn’t supported on windows either. For context this is the original commit: |
rebased to test the current action workflows. |
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 a lot @wkpark
I am very much in favor of this PR as it currently
1- tests bnb compilation on windowns AND ubuntu on each commit on main + on each PR to make sure future PRs will not break bnb compilation
2- makes bnb compilation easier through Cmake
3- Once we merge this PR #876 should be closed
I will let @Titus-von-Koeller give a final decision on this - I assume the classic make xxx
commands will still work - @wkpark once this gets merged, would you be happy to update the way we build bnb on our docker images to leverage CMake ? We do it exactly here: https://github.com/huggingface/peft/blob/189a9a666dc90f335b6a4db77f4b856b6a8bbdfd/docker/peft-gpu-bnb-source/Dockerfile#L60 and here: https://github.com/huggingface/peft/blob/189a9a666dc90f335b6a4db77f4b856b6a8bbdfd/docker/peft-gpu-bnb-latest/Dockerfile#L60
PR #220 by @niclimcy and PR #229 by @acpopescu are almost identical with PR #788 @Jamezo97
See also: |
Partially squashed, updated. |
fixed and partially squashed |
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. |
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.
@wkpark could you also update the documentation and the main reamde with instructions on how to build bnb with windows? |
For the docs you can start filling some content here: https://github.com/TimDettmers/bitsandbytes/blob/main/docs/source/installation.mdx |
* fix project name and add lib prefix for win32 (2024/01/31) * set LIBRARY_OUTPUT_DIRECTORY property Co-authored-by: Won-Kyu Park <[email protected]>
* build matrix for ubuntu + python 3.10, 3.11 + cuda 11.8 + 12.1 (windows is disabled for now) * add environment-bnb.yml for building * more fixes suggested by @akx (2024/01/30) * use python -m build --wheel suggested by @akx Co-authored-by: Aarni Koskela <[email protected]>
* add a comment suggested by @akx (2024/01/30) Co-authored-by: Aarni Koskela <[email protected]>
rebased and
|
@wkpark Thanks a lot ! let me know if you need any help for #908 (comment) - could you just share how to build bnb from source using a windows machine, i.e, the exact commands and I'll push on your branch |
this is just a note for windows user who wanted to build from source.
|
@wkpark check out https://moon-ci-docs.huggingface.co/docs/bitsandbytes/pr_908/en/installation now there is a nice table that showcases how to install bnb: |
target_compile_definitions(bitsandbytes PUBLIC BUILD_CUDA) | ||
target_link_libraries(bitsandbytes PUBLIC cudart cublas cusparse) | ||
if(NO_CUBLASLT) | ||
target_compile_definitions(bitsandbytes PUBLIC NO_CUBLASLT) | ||
else() | ||
target_link_libraries(bitsandbytes PUBLIC 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.
Should have used targets from https://cmake.org/cmake/help/latest/module/FindCUDAToolkit.html . This now assumes that the libraries are on the library search path
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.
All of this will be tweaked soon anyway, don't worry – see #949 for the follow-up.
CmakeList.txt + github workflows action
Please see
https://github.com/wkpark/bitsandbytes/actions/runs/7166014267https://github.com/wkpark/bitsandbytes/actions/runs/7167362163https://github.com/wkpark/bitsandbytes/actions/runs/7648577944