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.

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

llama.cpp 2950 (New formula) #172186

Closed
wants to merge 9 commits into from
Closed

llama.cpp 2950 (New formula) #172186

wants to merge 9 commits into from

Conversation

Vaibhavs10
Copy link
Contributor

Adds a formula for compiling llama.cpp from a github release. We expose both the CLI as well as the server through this Formula. The Formula is compliant with the strict brew audit.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@Vaibhavs10
llama.cpp 2950 (New formula)
d08feb7 
Adds a formula for compiling llama.cpp from a github release. We expose both the CLI as well as the server through this Formula. The Formula is compliant with the strict brew audit.
@github-actions github-actions bot added the new formula PR adds a new formula to Homebrew/homebrew-core label May 20, 2024
@github-actions GitHub Actions
Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label May 20, 2024
@chenrui333
Copy link
Member

relates to #169894

@Vaibhavs10 Vaibhavs10 mentioned this pull request May 20, 2024
Closed
6 tasks
@Vaibhavs10
Copy link
Contributor Author

Ah! that's my bad! Happy to close this in favor of the other (although I haven't seen much activity on the PR)

What's the best way forward in this case?

SMillerDev
SMillerDev reviewed May 21, 2024
View reviewed changes
Formula/l/llama.cpp.rb Outdated Show resolved Hide resolved
julien-c
julien-c reviewed May 21, 2024
View reviewed changes
Formula/l/llama.cpp.rb Outdated Show resolved Hide resolved
@Vaibhavs10
Copy link
Contributor Author

Updated the PR with the following things:

  1. Added autobump for llama.cpp project (throttling it at 10 checks - since llama.cpp is a pretty active project)
  2. Updated the test to be more comprehensive - we now download a model and then check if it was loaded and subsequently offloaded too.

The test ran perfectly for me offline, looking into why it fails on the CI.

@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request autobump labels May 22, 2024
@Vaibhavs10
llama.cpp 2960 (New Formula) Fix failing test.
dd9ad47 
llama.cpp 2960 (New Formula) Fix test.

Adds a more comprehensive test which loads a model and then checks whether the generation happened or not in the first place.

llama.cpp 2960 (New Formula) Add llama.cpp to autombump

Adding the update to autobump file, as I forgot to add it earlier.

llama.cpp 2960 (New Formula) Add curl dependency

llama.cpp 2960 (New Formula) Fix the order of statements.

llama.cpp 2960 (New Formula) Fix the failing test.

llama.cpp 2960 (New Formula) Fix the failing test. x2

llama.cpp 2960 (New Formula) Fix the failing test. x3

llama.cpp 2960 (New Formula) Fix style issues.

llama.cpp 2960 (New Formula) Fix order.

llama.cpp 2960 (New Formula) All test pass locally.

All style and the actual test cases pass locally now

llama.cpp 2960 (New Formula) Restrict the macos version.

All style and the actual test cases pass locally now

llama.cpp 2960 (New Formula) Fix Xcode issues.

llama.cpp 2960 (New Formula) Fix style issues.
@Vaibhavs10 Vaibhavs10 force-pushed the add-llama-cpp-formula branch from 35ca770 to dd9ad47 Compare May 24, 2024 16:03
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label May 24, 2024
@Vaibhavs10
Copy link
Contributor Author

Hi @SMillerDev, All tests pass now. I'm sorry it took ages. I eventually found out that the Runners on GitHub CI don't work as well with Metal.

I think the PR is good to merge if you don't have any other questions! 🚀

@Vaibhavs10 Vaibhavs10 requested a review from SMillerDev May 25, 2024 09:37
SMillerDev
SMillerDev reviewed May 25, 2024
View reviewed changes
Formula/l/llama.cpp.rb Outdated Show resolved Hide resolved
Formula/l/llama.cpp.rb Outdated Show resolved Hide resolved
@Vaibhavs10 Vaibhavs10 requested a review from SMillerDev May 25, 2024 12:59
@Vaibhavs10
Copy link
Contributor Author

Brilliant! Thanks for the suggestions. I've applied them.
I'm looking forward to getting this merged! 🤗

SMillerDev
SMillerDev reviewed May 25, 2024
View reviewed changes
Formula/l/llama.cpp.rb Outdated Show resolved Hide resolved
Formula/l/llama.cpp.rb Outdated Show resolved Hide resolved
@Vaibhavs10 Vaibhavs10 requested a review from SMillerDev May 26, 2024 12:06
@Vaibhavs10
Copy link
Contributor Author

Thanks for the suggestions again! 🤗 - I just reasked for a review

carlocab
carlocab reviewed May 27, 2024
View reviewed changes
Formula/l/llama.cpp.rb
Comment on lines +10 to +12
depends_on xcode: ["15.0", :build]
depends_on arch: :arm64
depends_on macos: :ventura
Copy link
Member

Choose a reason for hiding this comment

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

Where are these requirements documented?

Formula/l/llama.cpp.rb
depends_on xcode: ["15.0", :build]
depends_on arch: :arm64
depends_on macos: :ventura
depends_on :macos
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
depends_on :macos

This doesn't seem correct.

@carlocab carlocab mentioned this pull request May 27, 2024
Merged
6 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 26, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlocab carlocab carlocab left review comments

@julien-c julien-c julien-c left review comments

@SMillerDev SMillerDev Awaiting requested review from SMillerDev

Assignees
No one assigned
Labels
autobump automerge-skip `brew pr-automerge` will skip this pull request new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Vaibhavs10 @chenrui333 @julien-c @SMillerDev @carlocab