-
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
Attempt to use build_llama_and_whisper.sh #795
Conversation
Reviewer's Guide by SourceryThis pull request streamlines the CI workflow by replacing manual build steps with a dedicated build script and includes safeguards in the build script to ensure that dnf commands are executed only when available. Sequence diagram for streamlined CI build processsequenceDiagram
participant CI as CI Workflow
participant BS as Build Script
participant DNF as dnf (optional)
participant W as Whisper Build
participant L as Llama Build
CI->>BS: Execute build_llama_and_whisper.sh
BS->>BS: Configure common flags
alt dnf is available
BS->>DNF: command -v dnf (check availability)
BS->>DNF: dnf_install
else dnf is not available
BS->>BS: Skip dnf_install
end
BS->>W: clone_and_build_whisper_cpp
BS->>BS: Append '-DLLAMA_CURL=ON' flag
BS->>L: clone_and_build_llama_cpp
alt dnf is available
BS->>DNF: command -v dnf (check availability)
BS->>DNF: dnf -y clean all
else dnf is not available
BS->>BS: Skip dnf cleanup
end
BS->>BS: Remove caches and run ldconfig
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 adding a comment to explain why the llama.cpp build steps were moved into a separate script.
- It might be good to check for the existence of
dnf
before calling it, rather than relying oncommand -v dnf && dnf
.
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.
.github/workflows/ci.yml
Outdated
@@ -62,12 +62,7 @@ jobs: | |||
sudo apt-get update | |||
sudo apt-get install podman bats bash codespell python3-argcomplete pipx git cmake |
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.
libcurl4-openssl-dev package will likely fix the build failure
@@ -132,10 +132,9 @@ main() { | |||
set_install_prefix | |||
local common_flags | |||
configure_common_flags | |||
common_flags+=("-DGGML_CCACHE=OFF" "-DCMAKE_INSTALL_PREFIX=$install_prefix") | |||
common_flags+=("-DGGML_CCACHE=OFF" "-DCMAKE_INSTALL_PREFIX=$install_prefix -DLLAMA_CURL=OFF") |
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.
Ah you fixed it already, happy like this :)
A user might want to add this again (although RamaLama never uses this), we can just re-add if that happens
.github/workflows/ci.yml
Outdated
@@ -60,14 +60,9 @@ jobs: | |||
shell: bash | |||
run: | | |||
sudo apt-get update | |||
sudo apt-get install podman bats bash codespell python3-argcomplete pipx git cmake | |||
sudo apt-get install podman bats bash codespell python3-argcomplete pipx git cmake curl |
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.
What we actually want is this, curl is just the CLI tool:
libcurl4-openssl-dev
huggingface-cli failures |
Signed-off-by: Daniel J Walsh <[email protected]>
Summary by Sourcery
Use a script to build llama.cpp and whisper.cpp in the CI workflow.
Enhancements:
CI:
build_llama_and_whisper.sh
script instead of building them directly in the workflow.dnf_install
anddnf -y clean all
only ifdnf
is available.