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

Cmake + workflows #908

Merged
merged 7 commits into from
Feb 1, 2024
Merged

Conversation

wkpark
Copy link
Contributor

@wkpark wkpark commented Dec 11, 2023

CmakeList.txt + github workflows action

  • CMake by @Jamezo97
  • github workflows actions - support ubuntu and windows (windows disabled for now)
  • cu11.8, cu12.1 + py310, py311 build matrix

Please see https:/wkpark/bitsandbytes/actions/runs/7166014267
https:/wkpark/bitsandbytes/actions/runs/7167362163
https:/wkpark/bitsandbytes/actions/runs/7648577944

@TimDettmers
Copy link
Collaborator

Thank you! We are planning to switch to make, and this is a great PR!

I do not have an overview of how this is different or could benefit from #220 and #229. Could you please have a look and discuss the differences and if we could benefit from some of the old code. Thank you!

@TimDettmers TimDettmers added high priority (first issues that will be worked on) medium priority (will be worked on after all high priority issues) labels Jan 2, 2024

matrix:
os: [ubuntu-latest]
python-version: ['3.10', '3.11']
Copy link
Contributor

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.

@rickardp
Copy link
Contributor

rickardp commented Jan 2, 2024

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

@wkpark
Copy link
Contributor Author

wkpark commented Jan 2, 2024

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
See also https:/wkpark/bitsandbytes/actions (several times compiled, partial squashed rebased blah blah..)

(and pytest stuff is not fixed/tested much since I have no sm_75 capable GPU.)

@rickardp
Copy link
Contributor

rickardp commented Jan 2, 2024

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 See also https:/wkpark/bitsandbytes/actions (several times compiled, partial squashed rebased blah blah..)

(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 extracted the relevant portability fixes in #949. I don't mind which one to proceed with, but I just want to preserve the work of the previous closed PRs as I believe not all of the patches are part of the currently open set of PRs.
Additionally, this implements my proposal to split the building of the native libs into a different job matrix than the Python distribution building.

@niclimcy
Copy link

niclimcy commented Jan 3, 2024

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:
niclimcy@4c1d308

@wkpark
Copy link
Contributor Author

wkpark commented Jan 24, 2024

@wkpark
Copy link
Contributor Author

wkpark commented Jan 29, 2024

rebased to test the current action workflows.
https:/wkpark/bitsandbytes/actions/runs/7700400727/job/20984178019 (with minimal windows fix #876)
ubuntu, windows / cuda 11.8, 12.0 / python 3.10, 3.11 matrix (total 8 builds are available)

Copy link
Collaborator

@younesbelkada younesbelkada left a 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:/huggingface/peft/blob/189a9a666dc90f335b6a4db77f4b856b6a8bbdfd/docker/peft-gpu-bnb-source/Dockerfile#L60 and here: https:/huggingface/peft/blob/189a9a666dc90f335b6a4db77f4b856b6a8bbdfd/docker/peft-gpu-bnb-latest/Dockerfile#L60

.github/workflows/cmake.yml Outdated Show resolved Hide resolved
.github/workflows/cmake.yml Outdated Show resolved Hide resolved
.github/workflows/cmake.yml Outdated Show resolved Hide resolved
.github/workflows/cmake.yml Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@wkpark
Copy link
Contributor Author

wkpark commented Jan 30, 2024

@TimDettmers says:
Thank you! We are planning to switch to make, and this is a great PR!

I do not have an overview of how this is different or could benefit from #220 and #229. Could you please have a look and discuss the differences and if we could benefit from some of the old code. Thank you!

PR #220 by @niclimcy and PR #229 by @acpopescu are almost identical with PR #788 @Jamezo97
(Some PRs have already been closed.)
so I've cleaned up some unrelated stuff, cherry picked/partial squashed and extracted into minimal subsets of several PRs.

  • pthread vs std::thread -> make pthread optional. : win32 fix PR
  • do not rename pythonInterface.cpp : add cmake option
  • AVX related : win32 fix PR
  • cmake -> extract PR
  • lib detection -> extract PR
  • documents and trivial fixes -> drop

See also:

@wkpark
Copy link
Contributor Author

wkpark commented Jan 30, 2024

Partially squashed, updated.

@akx akx mentioned this pull request Jan 30, 2024
@wkpark
Copy link
Contributor Author

wkpark commented Jan 31, 2024

need to check cmake config for ubuntu. (*.so are not included)

  • need to add LIBRARY_OUTPUT_DIRECTORY property
  • set_target_properties(bitsandbytes PROPERTIES PREFIX "lib") for win32

fixed and partially squashed

Copy link

github-actions bot commented Feb 1, 2024

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.

Copy link
Collaborator

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks @wkpark for your great work ! as #876 being merged could you re-enable windows build in the workflow + rebase with main?

.github/workflows/cmake.yml Outdated Show resolved Hide resolved
@younesbelkada
Copy link
Collaborator

@wkpark could you also update the documentation and the main reamde with instructions on how to build bnb with windows?

@younesbelkada
Copy link
Collaborator

For the docs you can start filling some content here: https:/TimDettmers/bitsandbytes/blob/main/docs/source/installation.mdx

wkpark and others added 5 commits February 1, 2024 13:48
 * 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]>
@wkpark
Copy link
Contributor Author

wkpark commented Feb 1, 2024

rebased and

@younesbelkada
Copy link
Collaborator

@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

@wkpark
Copy link
Contributor Author

wkpark commented Feb 1, 2024

For the docs you can start filling some content here: https:/TimDettmers/bitsandbytes/blob/main/docs/source/installation.mdx

this is just a note for windows user who wanted to build from source.

  1. git clone
  2. cmake -B build -DBUILD_CUDA=ON -S .
  3. cmake --build build --config Release
  4. python -m build --wheel (build python wheel using build module.) then you can get dist/*.whl

@younesbelkada
Copy link
Collaborator

@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:

Screenshot 2024-02-01 at 16 16 56 Screenshot 2024-02-01 at 16 17 17

@younesbelkada
Copy link
Collaborator

Thanks again for all your awesome contributions ! @rickardp @wkpark @akx @Jamezo97 !

@younesbelkada younesbelkada merged commit 3a630c5 into bitsandbytes-foundation:main Feb 1, 2024
10 checks passed
Comment on lines +106 to +112
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()

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

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium priority (will be worked on after all high priority issues)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants