-
Notifications
You must be signed in to change notification settings - Fork 115
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
Install llama.cpp for mac and nocontainer tests #792
Conversation
Reviewer's Guide by SourceryThis PR introduces modifications to the CI workflows and system tests to install llama.cpp on mac systems and in nocontainer environments. It updates the installation steps in the CI configuration to include llama.cpp via brew and removes test skips to allow llama.cpp related tests to run on darwin. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rhatdan - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a separate job for macOS-specific steps to improve clarity.
- It's great that you're enabling tests on macOS, but please remove the skip_if_darwin calls in a separate commit.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
cdc1381
to
294e21b
Compare
Signed-off-by: Daniel J Walsh <[email protected]>
make install-requirements | ||
git clone https://github.com/ggerganov/llama.cpp | ||
cd llama.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend doing something like:
container-images/scripts/build_llama_and_whisper.sh build
and introduce this "build" or similar argument to build_llama_and_whisper.sh .
Keeping everything on the same version of llama.cpp will help avoid flakey builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at that but the script was pretty focused on dnf install commands, which will not work on Ubuntu.
Lets get this in and then we ca work towards that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, as a follow-on:
It's pretty easy to avoid the dnf stuff in that script, if dnf doesn't exist, don't execute that part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary by Sourcery
Install
llama.cpp
for macOS and non-containerized environments to enable running tests in those environments.CI:
llama.cpp
using Homebrew on macOS and Linux runners.Tests: