Skip to content

minimal patch to fix Windows compilation issues#876

Merged
younesbelkada merged 1 commit into
bitsandbytes-foundation:mainfrom
wkpark:minimal-win-fix
Feb 1, 2024
Merged

minimal patch to fix Windows compilation issues#876
younesbelkada merged 1 commit into
bitsandbytes-foundation:mainfrom
wkpark:minimal-win-fix

Conversation

@wkpark

@wkpark wkpark commented Nov 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

wkpark commented Nov 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This was referenced Dec 10, 2023
Open
Closed
@wkpark

wkpark commented Dec 11, 2023

Copy link
Copy Markdown
Contributor Author

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

TimDettmers commented Jan 2, 2024

Copy link
Copy Markdown
Collaborator

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
Comment thread 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);

@rickardp rickardp Jan 2, 2024

Copy link
Copy Markdown
Contributor

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?

@wkpark wkpark Jan 3, 2024

Copy link
Copy Markdown
Contributor Author

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

Titus-von-Koeller commented Jan 29, 2024

Copy link
Copy Markdown
Collaborator

@wkpark

Do I understand correctly that this is ready for review?

@wkpark

wkpark commented Jan 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 <wkpark@gmail.com>
younesbelkada
younesbelkada approved these changes Feb 1, 2024
View reviewed changes

@younesbelkada younesbelkada left a comment

Copy link
Copy Markdown
Collaborator

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

wkpark commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

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

younesbelkada commented Feb 1, 2024

Copy link
Copy Markdown
Collaborator

Perfect thanks @wkpark !

@younesbelkada

younesbelkada commented Feb 1, 2024

Copy link
Copy Markdown
Collaborator

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