enable kleidiAI in llama.cpp asahi build - #2765
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe build script for llama.cpp is updated to support KleiDIAI on Asahi. A new ChangesKleidiAI Asahi Integration
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant main as main()
participant clone_and_build_kleidiai as clone_and_build_kleidiai()
participant git as git
participant cmake as CMake
participant configure_common_flags as configure_common_flags()
main->>main: check containerfile == asahi
main->>clone_and_build_kleidiai: invoke build step
clone_and_build_kleidiai->>git: clone KleiDIAI at KLEIDIAI_PULL_REF
clone_and_build_kleidiai->>cmake: configure, build, install
main->>configure_common_flags: set CMake flags for llama.cpp
configure_common_flags->>configure_common_flags: enable GGML_VULKAN, GGML_KLEIDIAI, FLAME BLAS
main->>main: build llama.cpp
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@container-images/scripts/build_llama.sh`:
- Around line 314-315: The build currently defaults kleidiai_commit to "main"
via kleidiai_commit="${KLEIDIAI_PULL_REF:-main}", which makes builds
non-deterministic; change the logic so kleidiai_commit defaults to a pinned,
immutable ref (e.g., a tested tag or commit SHA stored in a constant like
KLEIDIAI_DEFAULT_REF) and only uses KLEIDIAI_PULL_REF when explicitly provided;
update the assignment and any documentation so
git_clone_specific_commit("${KLEIDIAI_REPO:-...}", "$kleidiai_commit") pulls the
pinned reference by default while still allowing opt-in override via
KLEIDIAI_PULL_REF.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8550cbaf-b14d-4f2e-8954-5cdbc9a69fb8
📒 Files selected for processing (1)
container-images/scripts/build_llama.sh
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds KleidiAI and BLIS support to the Asahi build target in container-images/scripts/build_llama.sh. It introduces a new function to clone and build KleidiAI and updates the build flags for Asahi. Reviewers identified that the vulkan and asahi targets should be decoupled to avoid dependency issues on Vulkan builds. Additionally, feedback highlighted the need to specify installation paths and prefixes for KleidiAI to ensure it is correctly discovered by llama.cpp and included in the final container image.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
container-images/scripts/build_llama.sh (1)
272-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd BLIS runtime dependency for Asahi when enabling BLAS
Asahi builds llama.cpp with BLAS/BLIS (
-DGGML_BLAS=ONand-DBLAS_INCLUDE_DIRS=/usr/include/blis) and installsblis-devel, but the Asahi runtime deps omitblis(they only add Vulkan packages), which can break runtime loading of BLAS/BLIS shared libraries.Suggested patch
diff --git a/container-images/scripts/build_llama.sh b/container-images/scripts/build_llama.sh @@ elif [ "$containerfile" = "asahi" ]; then dnf copr enable -y `@asahi/fedora-remix-branding` dnf install -y asahi-repos - runtime_pkgs+=(vulkan-loader vulkan-tools mesa-vulkan-drivers) + runtime_pkgs+=(vulkan-loader vulkan-tools mesa-vulkan-drivers blis)Also,
KLEIDIAI_PULL_REFdefaults tomain, reducing reproducibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@container-images/scripts/build_llama.sh` around lines 272 - 274, The asahi case that sets common_flags for building llama.cpp enables BLAS/BLIS but does not include the BLIS runtime package, so add the runtime dependency "blis" (or the distro package name used elsewhere) to the Asahi runtime deps list where the asahi build variant is configured so BLIS shared libraries are present at runtime; also remove the hard default of KLEIDIAI_PULL_REF="main" (or change it to an empty/explicitly required value) so builds are reproducible by forcing callers to provide a concrete commit/ref instead of defaulting to main. Ensure you update the asahi runtime deps block and the variable default for KLEIDIAI_PULL_REF accordingly, referencing the asahi case and the KLEIDIAI_PULL_REF variable in the script.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@container-images/scripts/build_llama.sh`:
- Around line 272-274: The asahi case that sets common_flags for building
llama.cpp enables BLAS/BLIS but does not include the BLIS runtime package, so
add the runtime dependency "blis" (or the distro package name used elsewhere) to
the Asahi runtime deps list where the asahi build variant is configured so BLIS
shared libraries are present at runtime; also remove the hard default of
KLEIDIAI_PULL_REF="main" (or change it to an empty/explicitly required value) so
builds are reproducible by forcing callers to provide a concrete commit/ref
instead of defaulting to main. Ensure you update the asahi runtime deps block
and the variable default for KLEIDIAI_PULL_REF accordingly, referencing the
asahi case and the KLEIDIAI_PULL_REF variable in the script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7f6fc39-9235-4f50-9114-f0a2c2d852ce
📒 Files selected for processing (1)
container-images/scripts/build_llama.sh
|
@Biasio Thanks, |
@rhatdan FYI I have some questions about this in #2764 (comment) before we should consider merging it |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ramalama/shortnames.py (1)
123-140: ⚡ Quick winSort entry blocks, not bare mapping lines.
Only the mapping lines move here; comments and blank lines after
[shortnames]stay in their original slots. If someone adds a per-entry comment, a rewrite will attach that comment to the wrong shortname. Sorting(leading comments + mapping)blocks avoids that drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ramalama/shortnames.py` around lines 123 - 140, Current logic sorts only bare mapping lines (mappings) which detaches their preceding comments/blank-lines; instead group each mapping with its leading comment/blank-line block and sort those blocks. Scan entries to build blocks where each block is (leading_comments_and_blanks + mapping_line) using is_mapping to detect mapping lines, then compute keys for sorting using the mapping line inside each block with entry_sort_key and entry_name, perform the two-pass stable sorts on the blocks, and finally rebuild result by replacing the original blocks in entries with the sorted blocks before writing the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ramalama/shortnames.py`:
- Around line 93-99: The sorter currently only recognizes mappings that start
with a double-quote which mismatches Shortnames.__init__'s accepted syntax;
update is_mapping and parse_key (and the duplicate functions later in the file)
to detect any non-comment line containing '=' as a mapping and to extract the
key as the trimmed text left of the '=' with optional surrounding quotes
removed. Concretely: change is_mapping to return True for lines that (after
strip) are non-empty, not starting with '#' and contain '='; change parse_key to
take the portion before the first '=' then strip whitespace and optional
surrounding single or double quotes and return that string so sort_file()
accepts the same mapping syntax as the runtime loader.
---
Nitpick comments:
In `@ramalama/shortnames.py`:
- Around line 123-140: Current logic sorts only bare mapping lines (mappings)
which detaches their preceding comments/blank-lines; instead group each mapping
with its leading comment/blank-line block and sort those blocks. Scan entries to
build blocks where each block is (leading_comments_and_blanks + mapping_line)
using is_mapping to detect mapping lines, then compute keys for sorting using
the mapping line inside each block with entry_sort_key and entry_name, perform
the two-pass stable sorts on the blocks, and finally rebuild result by replacing
the original blocks in entries with the sorted blocks before writing the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 886d4692-5d67-45bd-9607-afd4c52adb41
📒 Files selected for processing (2)
container-images/scripts/build_llama.shramalama/shortnames.py
🚧 Files skipped from review as they are similar to previous changes (1)
- container-images/scripts/build_llama.sh
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ramalama/shortnames.py (1)
123-140: ⚡ Quick winSort entry blocks, not bare mapping lines.
Only the mapping lines move here; comments and blank lines after
[shortnames]stay in their original slots. If someone adds a per-entry comment, a rewrite will attach that comment to the wrong shortname. Sorting(leading comments + mapping)blocks avoids that drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ramalama/shortnames.py` around lines 123 - 140, Current logic sorts only bare mapping lines (mappings) which detaches their preceding comments/blank-lines; instead group each mapping with its leading comment/blank-line block and sort those blocks. Scan entries to build blocks where each block is (leading_comments_and_blanks + mapping_line) using is_mapping to detect mapping lines, then compute keys for sorting using the mapping line inside each block with entry_sort_key and entry_name, perform the two-pass stable sorts on the blocks, and finally rebuild result by replacing the original blocks in entries with the sorted blocks before writing the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ramalama/shortnames.py`:
- Around line 93-99: The sorter currently only recognizes mappings that start
with a double-quote which mismatches Shortnames.__init__'s accepted syntax;
update is_mapping and parse_key (and the duplicate functions later in the file)
to detect any non-comment line containing '=' as a mapping and to extract the
key as the trimmed text left of the '=' with optional surrounding quotes
removed. Concretely: change is_mapping to return True for lines that (after
strip) are non-empty, not starting with '#' and contain '='; change parse_key to
take the portion before the first '=' then strip whitespace and optional
surrounding single or double quotes and return that string so sort_file()
accepts the same mapping syntax as the runtime loader.
---
Nitpick comments:
In `@ramalama/shortnames.py`:
- Around line 123-140: Current logic sorts only bare mapping lines (mappings)
which detaches their preceding comments/blank-lines; instead group each mapping
with its leading comment/blank-line block and sort those blocks. Scan entries to
build blocks where each block is (leading_comments_and_blanks + mapping_line)
using is_mapping to detect mapping lines, then compute keys for sorting using
the mapping line inside each block with entry_sort_key and entry_name, perform
the two-pass stable sorts on the blocks, and finally rebuild result by replacing
the original blocks in entries with the sorted blocks before writing the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 886d4692-5d67-45bd-9607-afd4c52adb41
📒 Files selected for processing (2)
container-images/scripts/build_llama.shramalama/shortnames.py
🚧 Files skipped from review as they are similar to previous changes (1)
- container-images/scripts/build_llama.sh
🛑 Comments failed to post (1)
ramalama/shortnames.py (1)
93-99:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
sort_file()accept the same mapping syntax as the runtime loader.
Shortnames.__init__()on Lines 60-64 accepts anykey = valueentry and only strips optional quotes, butis_mapping()/parse_key()only recognize lines that begin with". Valid unquoted entries will be ignored by--checkand never moved by the formatter, so the sorter and loader disagree on what a valid shortname entry is.Proposed fix
- def is_mapping(line: str) -> bool: - return line.strip().startswith('"') + def is_mapping(line: str) -> bool: + stripped = line.strip() + return bool(stripped) and not stripped.startswith(("#", ";")) and "=" in stripped def parse_key(line: str) -> str: - s = line.strip() - return s[1 : s.index('"', 1)] + key, _sep, _value = line.partition("=") + return key.strip().strip("'\"")Also applies to: 123-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ramalama/shortnames.py` around lines 93 - 99, The sorter currently only recognizes mappings that start with a double-quote which mismatches Shortnames.__init__'s accepted syntax; update is_mapping and parse_key (and the duplicate functions later in the file) to detect any non-comment line containing '=' as a mapping and to extract the key as the trimmed text left of the '=' with optional surrounding quotes removed. Concretely: change is_mapping to return True for lines that (after strip) are non-empty, not starting with '#' and contain '='; change parse_key to take the portion before the first '=' then strip whitespace and optional surrounding single or double quotes and return that string so sort_file() accepts the same mapping syntax as the runtime loader.
1512ab3 to
ea7c11a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ramalama/cli.py (1)
824-836: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't swallow
KeyErrorfrompush().New()only raisesKeyErrorfor unknown shortnames or unsupported transports, but this handler also catchesKeyErrorfromtarget_model.push(), which can come from unrelated transport failures. Split the fallback so only the lookup path is covered, or re-raise push-timeKeyErrors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ramalama/cli.py` around lines 824 - 836, The current exception handling around New() and target_model.push() is too broad and can incorrectly treat KeyError from push() as a lookup failure. Update the logic in the cli push flow so that only the New(target, args) resolution path falls back on KeyError/NotImplementedError, and ensure any KeyError raised by target_model.push() is re-raised instead of triggering the OCI fallback. Use the existing symbols New, target_model.push, MODEL_TYPES, and TransportFactory to split the lookup failure handling from push-time failures cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ramalama/cli.py`:
- Around line 824-836: The current exception handling around New() and
target_model.push() is too broad and can incorrectly treat KeyError from push()
as a lookup failure. Update the logic in the cli push flow so that only the
New(target, args) resolution path falls back on KeyError/NotImplementedError,
and ensure any KeyError raised by target_model.push() is re-raised instead of
triggering the OCI fallback. Use the existing symbols New, target_model.push,
MODEL_TYPES, and TransportFactory to split the lookup failure handling from
push-time failures cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cdeb2b00-fea5-4e28-9e35-86bc88ad381e
📒 Files selected for processing (4)
container-images/scripts/build_llama.shramalama/cli.pyramalama/transports/huggingface.pytest/unit/test_huggingface.py
🚧 Files skipped from review as they are similar to previous changes (1)
- container-images/scripts/build_llama.sh
|
DCO is failing on 574a6c0 |
|
Please squash all of the commits into a single PR. |
ea7c11a to
b1b1d80
Compare
|
Sorry, I am not so used to git versioning... Trying my best. Now it should be all correct |
b1b1d80 to
1b1aee4
Compare
olliewalsh
changed the title
GitHub Actions
Inactive
|
|
||
| setup_build_env | ||
|
|
||
| if [[ "$containerfile" == "asahi" ]]; then |
There was a problem hiding this comment.
Wonder if it would make sense to use KleidiAI on other arm64 platforms? E.g the 16gb rpi5?
There was a problem hiding this comment.
It should since armv8.2-A implements SME and KleidiAI benefits from SME support.
I don't own one, so I cannot test it myself.
Another interesting platform might be Android...
Regarding Apple Silicon, SME was first introduced with the M4 chip, mine is an M1 so the performance gap is small than it might be on a newer model.
There was a problem hiding this comment.
I can test on rpi5 and M4
…ahi targets, issue containers#2764 Signed-off-by: Biasio <91698445+Biasio@users.noreply.github.com> Signed-off-by: Biasio <alessandro.biasioli02@gmail.com>
666608e to
64d1b41
Compare
|
LGTM |
3 participants
Fixes: #2764