Avoid stale embedded Metal shaders in llama builds - #1027
Conversation
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
📝 WalkthroughWalkthroughThe build script updates sccache handling for Darwin Metal builds and explicitly clears CMake compiler launcher variables when sccache is unavailable or disabled. ChangesSccache compiler launcher configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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 `@scripts/build-llama.sh`:
- Around line 228-233: When sccache is unavailable, update the CMAKE_ARGS
fallback block to explicitly add -DGGML_CCACHE=OFF alongside the cleared
compiler launcher settings. Keep the existing no-caching behavior and mirror the
corresponding setting in the else branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae6dca9c-5379-44f6-b2cb-79b65981d186
📒 Files selected for processing (1)
scripts/build-llama.sh
| CMAKE_ARGS+=( | ||
| -DCMAKE_C_COMPILER_LAUNCHER= | ||
| -DCMAKE_CXX_COMPILER_LAUNCHER= | ||
| -DCMAKE_CUDA_COMPILER_LAUNCHER= | ||
| -DCMAKE_HIP_COMPILER_LAUNCHER= | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Explicitly disable GGML_CCACHE to prevent unintended fallback to ccache.
The log message on line 227 states that the build will run "without compiler caching." However, because llama.cpp defaults GGML_CCACHE to ON, it will automatically search for ccache and overwrite the explicitly cleared compiler launchers if ccache happens to be installed on the system.
To ensure caching is truly disabled when sccache is not found, pass -DGGML_CCACHE=OFF, mirroring the else branch below.
💻 Proposed fix
CMAKE_ARGS+=(
+ -DGGML_CCACHE=OFF
-DCMAKE_C_COMPILER_LAUNCHER=
-DCMAKE_CXX_COMPILER_LAUNCHER=
-DCMAKE_CUDA_COMPILER_LAUNCHER=
-DCMAKE_HIP_COMPILER_LAUNCHER=
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CMAKE_ARGS+=( | |
| -DCMAKE_C_COMPILER_LAUNCHER= | |
| -DCMAKE_CXX_COMPILER_LAUNCHER= | |
| -DCMAKE_CUDA_COMPILER_LAUNCHER= | |
| -DCMAKE_HIP_COMPILER_LAUNCHER= | |
| ) | |
| CMAKE_ARGS+=( | |
| -DGGML_CCACHE=OFF | |
| -DCMAKE_C_COMPILER_LAUNCHER= | |
| -DCMAKE_CXX_COMPILER_LAUNCHER= | |
| -DCMAKE_CUDA_COMPILER_LAUNCHER= | |
| -DCMAKE_HIP_COMPILER_LAUNCHER= | |
| ) |
🤖 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 `@scripts/build-llama.sh` around lines 228 - 233, When sccache is unavailable,
update the CMAKE_ARGS fallback block to explicitly add -DGGML_CCACHE=OFF
alongside the cleared compiler launcher settings. Keep the existing no-caching
behavior and mirror the corresponding setting in the else branch.
|
Holding this one - I don't think it does what it claims, so let's not merge as-is. The The likely reason it appeared to fix stale shaders: changing If we do want a real hardening, the minimal correct version keeps C+CXX cached and just disables ggml's fallback: (Note Also minor: the 'sccache not found' branch clears the launchers but leaves Separately - the failing Windows CPU check here is not caused by this PR; it's the pre-existing |
Yeah, this checks out. Since the shader gets compiled as ASM, changing the C launcher won’t really affect that step. Let’s keep the C/C++ launcher flags, (could add -DGGML_CCACHE=OFF to disable the fallback), and dig into the stale shader issue separately. We could start with |
Summary
sccachefor the C compiler launcher on Darwin Metal llama.cpp buildsWhy
The embedded Metal shader library is included through an assembly
.incbinobject.sccachedoes not include that embedded payload in the assembly cache key, which can leave a llama.cpp build using stale Metal shader contents after shader source changes.Validation
bash -n scripts/build-llama.shjust buildSummary by CodeRabbit