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

Make native code portable and add GitHub workflow for building #949

Merged
merged 61 commits into from
Feb 5, 2024

Conversation

rickardp
Copy link
Contributor

@rickardp rickardp commented Jan 2, 2024

This PR fixes

  • Adds the CMake support based on the original contribution by @niclimcy ( + Apple build targets)
  • Adds GitHub workflow to build first native libs using a build matrix. Builds CUDA for Linux arm64/x64 + Windows x64, CPU for Windows, Linux and Mac. Python builds bdist_wheels for matrix of Python versions (packages native libs built once).
  • Adds all known source code patches for making the code build on Windows, Mac and Linux/arm64

It does not add #847, which should be considered

See RFC #1020 for discussion

@rickardp rickardp mentioned this pull request Jan 2, 2024
@niclimcy
Copy link

niclimcy commented Jan 3, 2024

I would split github workflow into one commit, cmakelist into another, and platform portability changes into one more commit

@rickardp
Copy link
Contributor Author

rickardp commented Jan 3, 2024

I would split github workflow into one commit, cmakelist into another, and platform portability changes into one more commit

I guess the source code changes could be broken out, but there would be no way of verifying them without a working build system.

Same goes for GitHub actions. Without actually being able to build for multiple arch's/OSs it would only fail. And conversely - without GH actions there is no way to verify the code working unless you happen to own all hardware.

@Titus-von-Koeller
Copy link
Collaborator

Hey both!

Thanks for your contribution. You're right CMake might definitely make sense to move to, especially given our current effort to support cross-platform. This needs a bit more thought and input from Tim, which we should get in the coming weeks. Unfortunately, Tim's availability is very limited right now. He mentioned concerns about migrating away from Make: I'll have to find out more and will get back to you!

.github/workflows/python-package.yml Outdated Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
rickardp and others added 5 commits January 25, 2024 10:09
Co-authored-by: Aarni Koskela <[email protected]>
Co-authored-by: Aarni Koskela <[email protected]>
Co-authored-by: Aarni Koskela <[email protected]>
Co-authored-by: Aarni Koskela <[email protected]>
Co-authored-by: Aarni Koskela <[email protected]>
.github/workflows/python-package.yml Show resolved Hide resolved
.github/workflows/python-package.yml Outdated Show resolved Hide resolved
rickardp and others added 2 commits January 25, 2024 15:28
Co-authored-by: Aarni Koskela <[email protected]>
Co-authored-by: Aarni Koskela <[email protected]>
@younesbelkada
Copy link
Collaborator

Amazing work @rickardp ! we'll review this asap with @Titus-von-Koeller !

Copy link

github-actions bot commented Feb 4, 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.

@akx
Copy link
Contributor

akx commented Feb 4, 2024

@rickardp You should be able to run pip install pre-commit and pre-commit run --all-files to autofix instead of saving things manually 😄

@rickardp
Copy link
Contributor Author

rickardp commented Feb 4, 2024

@rickardp You should be able to run pip install pre-commit and pre-commit run --all-files to autofix instead of saving things manually 😄

Got it! But now I wanted to confirm that saving the files in VS code made the linter happy. I created PR #1028 to make format on save use Ruff if this is the configured formatter for this project.

@akx
Copy link
Contributor

akx commented Feb 4, 2024

A cmake -DCOMPUTE_BACKEND=mps . compilation builds fine on my Macbook 🎉

Some additional patches in cuda_setup/ (which really isn't cuda_setup at this point at all) are required to find the dylib on Macs though, but that can be done in a follow-up 😄

@rickardp
Copy link
Contributor Author

rickardp commented Feb 4, 2024

A cmake -DCOMPUTE_BACKEND=mps . compilation builds fine on my Macbook 🎉

Some additional patches in cuda_setup/ (which really isn't cuda_setup at this point at all) are required to find the dylib on Macs though, but that can be done in a follow-up 😄

Yes, I purposefully left that part out, as well as the unit test fixes. Those are still left in the draft PR I created (#947) if anyone wants to have a look. I think my solution to those predates the later discussions and work by other contributors so I think this is more of historical value right now. Happy to collaborate on a follow up PR once the CUDA setup refactoring has been settled.

@akx
Copy link
Contributor

akx commented Feb 5, 2024

@Titus-von-Koeller @younesbelkada I think we should prioritize getting this merged to enable other work :)

@wkpark
Copy link
Contributor

wkpark commented Feb 5, 2024

This PR tried to address a lot of issues, and I've drawn/referenced heavily from it for my PRs, which I've adapted. so thank you very much!!
and I've tested already but this PR is not completed yet.

  • CPU module not working. (setup.py is not correctly fixed.)
  • MPS module not working yet.

so I've extracted some working stuff with minimal fixes to reduce possible conflicts into #1018, #1019

I suspect @rickardp had this in mind, too, he already has extracted some PRs.
I think it would be easier to leave this PR as a reference and review each extracted PR individually.

@Titus-von-Koeller
Copy link
Collaborator

Yes, I agree. I've been trying to merge it from my phone yesterday but it's not allowing me from this device somehow. I'm on vacation until next Monday. I'll figure sth out.

Thanks for the wonderful work everyone once again!

@Titus-von-Koeller
Copy link
Collaborator

Ok, well, seems I can merge through the mobile browser but not the app.

Please resolve merge conflicts and @younesbelkada and I will merge right away.

@akx
Copy link
Contributor

akx commented Feb 5, 2024

I'm on vacation until next Monday. I'll figure sth out.

Hey, no worries, enjoy your vacation! 🏖️ There's no hurry here.

@rickardp
Copy link
Contributor Author

rickardp commented Feb 5, 2024

This PR is now integrated with the latest main.

CPU module not working. (setup.py is not correctly fixed.) MPS module not working yet.

I commented this earlier. It is left in the "old reference" PR. I think it conflicts with the ongoing refactoring of cuda_setup which is why I think that needs to settle before attempting to integrate this. With this PR, macOS builds can run the CPU tests but does not use MPS.

I suspect @rickardp had this in mind, too, he already has extracted some PRs. I think it would be easier to leave this PR as a reference and review each extracted PR individually.

My idea was to break out some of the stuff to make it easier to merge this one. Note that I do think the remainder of this PR still needs to be merged (either through this or through a new one) as this PR still does this

  • Sets up the pip-based workflow with the xplat targets for macOS
  • Adds the cmake changes to support macOS
  • Builds CPU only for macOS

If so desired, I think the easiest way if we don't want the dead code of the .metal file in this PR, is to simply remove them from this PR. They don't do anything, but it is currently dead code until the MPS parts are added back

After this has been integrated, the following work can then commence

  • Make cuda_setup work on mac (can probably draw inspiration from my previous PR, but I more or less expect a complete rewrite)
  • Make unit tests not fail when no CUDA device present, i..e skip tests based on h/w device (maybe this was already fixed? I did this in my old PR, but we might want to do this differently post device abstraction)
  • Add back unit tests to GH workflow

And then the real fun begins :)

@Titus-von-Koeller Titus-von-Koeller merged commit 73d3e7b into bitsandbytes-foundation:main Feb 5, 2024
31 checks passed
wkpark added a commit to wkpark/bitsandbytes that referenced this pull request Feb 5, 2024
wkpark added a commit to wkpark/bitsandbytes that referenced this pull request Feb 5, 2024
@wkpark wkpark mentioned this pull request Feb 5, 2024
17 tasks
akx added a commit to akx/bitsandbytes that referenced this pull request Feb 5, 2024
@younesbelkada
Copy link
Collaborator

Thanks a lot @rickardp @akx @wkpark for your hardwork and for the entire refactor !
Our previous Dockerfiles uses make cuda11x to build bnb from source in order to run our tests, I am trying to adapt the docker files to reflect the correct way of building bnb from source here: huggingface/peft#1437 but with no success at the moment, could you help out there ? 🙏

@younesbelkada
Copy link
Collaborator

False alarm, should be all good now (see the attached PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants