Skip to content

minimal patch to fix Windows compilation issues #876

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.

Sign up for GitHub

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

Jump to bottom
Merged
younesbelkada merged 1 commit into from
Feb 1, 2024
Merged

minimal patch to fix Windows compilation issues #876

younesbelkada merged 1 commit into from
Feb 1, 2024

Conversation

@wkpark
Copy link
Contributor

@wkpark wkpark commented Nov 16, 2023 •
edited
Loading

manually cherry-picked from PR #788 and PR #229 and cleaned up by me

Original work done by @Jamezo97 and @acpopescu

@wkpark wkpark mentioned this pull request Nov 16, 2023
Merged
@wkpark
Copy link
Contributor Author

wkpark commented Nov 29, 2023 •
edited
Loading

This was referenced Dec 10, 2023
Open
Closed
@wkpark
Copy link
Contributor Author

wkpark commented Dec 11, 2023

cmake + github workflows + cuda 12.1, 11.8 + python 3.10, 3.11 + windows, ubuntu build matrix available.
https://github.com/wkpark/bitsandbytes/actions/runs/7165411186/job/19507307550

@wkpark wkpark mentioned this pull request Dec 17, 2023
Closed
@TimDettmers TimDettmers mentioned this pull request Jan 1, 2024
Closed
@TimDettmers
Copy link
Collaborator

TimDettmers commented Jan 2, 2024

This is awesome, thanks for your contribution! We will discuss this internally and get back to you. @Titus-von-Koeller @younesbelkada please remind me that we talk about this.

@TimDettmers TimDettmers added the Medium Priority (will be worked on after all high priority issues) label Jan 2, 2024
@wkpark wkpark mentioned this pull request Jan 2, 2024
Merged
rickardp
rickardp reviewed Jan 2, 2024
View reviewed changes
csrc/cpu_ops.cpp
{
long long valid_chunks = num_blocks - offset >= thread_wave_size ? thread_wave_size : num_blocks - offset;
#ifdef _WIN32
std::thread *threads = (std::thread *) malloc(sizeof(std::thread) * valid_chunks);
Copy link
Contributor

@rickardp rickardp Jan 2, 2024

Choose a reason for hiding this comment

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

Any reason not to use std::thread everywhere?

Copy link
Contributor Author

@wkpark wkpark Jan 3, 2024

Choose a reason for hiding this comment

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

Any reason not to use std::thread everywhere?

I just leaving this as the original patch creator intended.

@wkpark wkpark mentioned this pull request Jan 29, 2024
Closed
@Titus-von-Koeller
Copy link
Collaborator

Titus-von-Koeller commented Jan 29, 2024

@wkpark

Do I understand correctly that this is ready for review?

@wkpark
Copy link
Contributor Author

wkpark commented Jan 29, 2024 •
edited
Loading

@wkpark

Do I understand correctly that this is ready for review?

Yes. I think so.

minimal fixes have been cherry-picked and squashed. (several unrelated hunks were carefully removed/simplified manually)

see also PR #908,
https://github.com/wkpark/bitsandbytes/actions/runs/7700400727/job/20984178019

@Jamezo97 @wkpark
minimal fix to support Windows
fd319d5 
based on @Jamezo97 and @acpopescu work

manually cherry-picked from PR bitsandbytes-foundation#788 and PR bitsandbytes-foundation#229 and cleanup by wkpark

Signed-off-by: Won-Kyu Park <[email protected]>
younesbelkada
younesbelkada approved these changes Feb 1, 2024
View reviewed changes
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 !
I think we should merge this PR before #908 right? Could you re-enable windows build in the workflow for #908 after we merge this PR?

@younesbelkada younesbelkada merged commit 89876bb into bitsandbytes-foundation:main Feb 1, 2024
@wkpark
Copy link
Contributor Author

wkpark commented Feb 1, 2024

Thanks @wkpark ! I think we should merge this PR before #908 right? Could you re-enable windows build in the workflow for #908 after we merge this PR?

right and thank you.
now I can add windows build matrix!

@younesbelkada
Copy link
Collaborator

younesbelkada commented Feb 1, 2024

Perfect thanks @wkpark !

@younesbelkada
Copy link
Collaborator

younesbelkada commented Feb 1, 2024

Thanks for your great work @wkpark I we can confirm with @Titus-von-Koeller that this PR did not break anything with respect to bnb + HF (PEFT, transformers, etc.) Looking forward to your next contributions!

wkpark added a commit to wkpark/bitsandbytes that referenced this pull request Feb 1, 2024
@wkpark
add missing definitions accidentally removed by mistake in PR bitsand…
64e2058 
…bytes-foundation#876
wkpark added a commit to wkpark/bitsandbytes that referenced this pull request Feb 1, 2024
@wkpark
add missing definitions accidentally removed by mistake in PR bitsand…
d97700a 
…bytes-foundation#876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@rickardp rickardp rickardp left review comments

@younesbelkada younesbelkada younesbelkada approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Medium Priority (will be worked on after all high priority issues)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wkpark @TimDettmers @Titus-von-Koeller @younesbelkada @rickardp @Jamezo97