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

minimal macos support #1018

Closed
wants to merge 1 commit into from

Conversation

wkpark
Copy link
Contributor

@wkpark wkpark commented Feb 3, 2024

This is a PR to support macos based on @rickardp's work (PR #949 )
for those who want to use bitsandbytes under OSX with minimal support.

  • original work done by @niclimcy and @rickardp
  • macos cpu module support
  • load libbitsandbytes_cpu.dylib correctly.

Copy link

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

@wkpark
Copy link
Contributor Author

wkpark commented Feb 5, 2024

fixed conflicts, rebased.

@Titus-von-Koeller
Copy link
Collaborator

@akx Would you be able to review, pls?

@akx
Copy link
Contributor

akx commented Feb 5, 2024

I think we should focus on merging #949 instead of this – the same work is done there. Merging #1019 (which also did work already done in #949) made #949 require conflict resolution 😅

@wkpark
Copy link
Contributor Author

wkpark commented Feb 5, 2024

now, only left dylib fix.

@rickardp
Copy link
Contributor

rickardp commented Feb 5, 2024

now, only left dylib fix.

Ah, I thought #996 solved it but it was the wrong main.py. Yes, this is needed

@Titus-von-Koeller
Copy link
Collaborator

Titus-von-Koeller commented Feb 5, 2024

Ok, I merged 949 now. Is this one obsolete then and can be closed?

Why were these split out then? To make them atomic and easier to review for us?

I like this approach a lot generally, but then it would be good to have a statement about it in the description and if you have some insight (even best guess) about dependencies between PRs, please make them explicit.

The more actionable something is, the better. Also, reviewing and approving each other's PRs is also very helpful as an extra pair of eyes and other set of expertise gives more certainty in moving forward.

Generally, @akx PRs for example were structured very well and the descriptions he gives were also very helpful: Context, atomic problem, solution. This makes it easy to review. If it's urgent to move forward and blocking it's also important to know.

Anyways, again, we're super happy about this surge in community contributions and will be very focused in supporting and enabling you.

One thing that would really help is to all work together to keep all info up to date in the respective RFC, so we have an overview of status, decisions and points that need further discussion. I ll then integrate the info from your comments into the top-most post to keep the overview up to date.

Also, keeping track of PRs and intended merge order there might make sense.

Please let us know how you think we should best move forward and we'll figure it out.

I'll be back on Monday. Feel free to contact @younesbelkada if it's more urgent than that.

@Titus-von-Koeller
Copy link
Collaborator

Sry, closing was by accident.

@wkpark wkpark mentioned this pull request Feb 5, 2024
17 tasks
@wkpark
Copy link
Contributor Author

wkpark commented Feb 5, 2024

@Titus-von-Koeller // As I personally consider the integrity of the main branch is the most important.
I did not expect PR #949 to be merged so quickly as it has several unresolved issues that have not been properly addressed. I have already commented on that PR (#949 (comment))

@wkpark wrote:
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.

And the recent series of merges were unexpected after months of delays. I wonder if you're putting too much faith in the reviewer 😋, and I do not give hasty reviews without thoroughly checking it myself.

Anyway, Thank you for your attention, even during your vacation!

@wkpark wkpark mentioned this pull request Feb 5, 2024
2 tasks
@wkpark wkpark closed this Feb 5, 2024
@wkpark
Copy link
Contributor Author

wkpark commented Feb 5, 2024

Please see #1038

@wkpark wkpark deleted the macos-minimal branch February 19, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants