Add GLM DSA llama foundation patches - #1034
Conversation
|
Caution Review failedFailed to post review comments. We encountered an issue with GitHub. Use ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout. (18)
🧰 Additional context used📓 Path-based instructions (4)**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/skippy-ffi/src/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
{proto/**,crates/mesh-llm-host-runtime/src/{protocol,mesh,gossip.rs},crates/skippy-ffi/src/**/*.rs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
📝 WalkthroughWalkthroughThe PR adds GLM-DSA runtime configuration, staged top-k activation transport, an IndexShare graph, fused KV_B and DSA KV-cache support, expanded backend coverage, graph contract tests, reporting scripts, and a Skippy ABI patch bump. ChangesGLM-DSA runtime integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch (1)
1032-1118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value"Known-broken" route-pack path is shipped, not just deferred.
The function's own comment states: "Route packing only changes graph reorder grouping; it does not itself emit the fused route kernel. Keep the known-broken pack path behind an explicit unsafe debug switch until route fusion is represented by a real backend op or the encoder can reliably consume the packed subgraph." It's correctly gated behind three chained env-var checks that all default to disabled, so runtime risk is negligible, but shipping self-documented "known-broken" logic (even dead-by-default) is a maintainability smell better tracked via an issue/TODO with a target milestone rather than left as permanent debug scaffolding.
🤖 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 `@third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch` around lines 1032 - 1118, Remove the shipped known-broken route-pack implementation anchored at ggml_metal_topk_moe_route_pack_len, including its unsafe debug gating and candidate logging, and replace it with a TODO or issue reference that records the required target milestone. Leave route packing disabled until a real backend op or reliable packed-subgraph encoder support exists.
🤖 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
`@third_party/llama.cpp/patches/0020-skippy-expose-GLM-DSA-staged-runtime-controls.patch`:
- Around line 1999-2004: Remove the newly added duplicate status-guarded
skippy_mtp_sync_target_tokens call, leaving the pre-existing synchronization
block unchanged so target-token sync runs only once per decode.
In
`@third_party/llama.cpp/patches/0021-tests-cover-native-GLM-DSA-execution-paths.patch`:
- Around line 784-788: Remove the undefined ffn_q3_merged_shexp check from this
separate Python heredoc, or replace it with a locally available signal such as
combined_q3_us or q3. Ensure the routed whole-graph sanity probe no longer
raises NameError when generating decision_lines.
- Around line 3369-3377: Update the tensor initialization override in
initialize_tensors to avoid direct memset on t->data, since it may reference
non-CPU backend memory. Allocate or reuse a host buffer filled with 0xa5 for
glm_decode_block_byte_ceiling_weights and zero-filled for other tensors, then
write each buffer through ggml_backend_tensor_set in both branches.
---
Nitpick comments:
In
`@third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch`:
- Around line 1032-1118: Remove the shipped known-broken route-pack
implementation anchored at ggml_metal_topk_moe_route_pack_len, including its
unsafe debug gating and candidate logging, and replace it with a TODO or issue
reference that records the required target milestone. Leave route packing
disabled until a real backend op or reliable packed-subgraph encoder support
exists.
🪄 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: a80d059f-9b84-41ad-8bc7-c6313f5c7cc9
📒 Files selected for processing (10)
crates/mesh-llm-system/src/autoupdate.rscrates/skippy-ffi/src/lib.rscrates/skippy-runtime/src/config.rsthird_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patchthird_party/llama.cpp/patches/0020-skippy-expose-GLM-DSA-staged-runtime-controls.patchthird_party/llama.cpp/patches/0021-tests-cover-native-GLM-DSA-execution-paths.patchthird_party/llama.cpp/patches/0022-Support-GLM-DSA-fused-KV_B-tensors.patchthird_party/llama.cpp/patches/0023-Run-GLM-DSA-through-IndexShare-graph.patchthird_party/llama.cpp/patches/0024-Use-DSA-KV-cache-for-GLM-DSA.patchthird_party/llama.cpp/patches/0025-Bump-Skippy-ABI-for-GLM-DSA-runtime-config.patch
ndizazzo
left a comment
ndizazzo
left a comment
There was a problem hiding this comment.
@i386 curious - what happens when llamacpp adds something upstream WRT GLM5.2? Do we slowly strip out and adapt any of our support work to integrate it?
It seems inevitable that support will come through the base inference provider at some point, so I'm not sure what the protocol for us should be for stuff we've bolted on
Yeah we slowly strip it out |
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)
third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch (1)
32-36: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winRPC_PROTO_PATCH_VERSION not bumped despite
GGML_OP_COUNTchange.The static assert's own message says to update
RPC_PROTO_PATCH_VERSIONwheneverGGML_OP_COUNTchanges, andGGML_OP_COUNTmoved from 101 to 107 here (6 new ops), but the diff only updates the assert's expected value —RPC_PROTO_PATCH_VERSIONis left untouched. This defeats the version check's purpose: RPC clients/servers built against the old op set will silently interoperate with servers that support the new DSA/MoE ops instead of being flagged incompatible.🐛 Proposed fix
-#define RPC_PROTO_PATCH_VERSION 3 +#define RPC_PROTO_PATCH_VERSION 4🤖 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 `@third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch` around lines 32 - 36, Update RPC_PROTO_PATCH_VERSION alongside the GGML_OP_COUNT change in the version guard, incrementing it from 3 so RPC clients and servers using the old operation set are rejected as incompatible.
🧹 Nitpick comments (1)
third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch (1)
137-144: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRepeated
getenv()calls on the hot dispatch path.Helpers such as
ggml_backend_sched_glm_route_diag_enabled()(and the many analogousggml_metal_glm_dsa_*_enabled()/*_requested()helpers throughout this file) callgetenv()unconditionally on every graph split / node encode, i.e. once per token for hot ops. Since these values don't change at runtime, caching them once (e.g., static initialization) would avoid repeated syscall overhead on the decode hot path. Given the PR description flags this whole GLM-DSA lane as scaffolding to be pared down as upstream support lands, this is a minor/deferrable item rather than a blocker.🤖 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 `@third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch` around lines 137 - 144, Cache environment-variable results in ggml_backend_sched_glm_route_diag_enabled() and the analogous ggml_metal_glm_dsa_*_enabled()/ *_requested() helpers using one-time static initialization, so getenv() is not called on each dispatch. Preserve the existing variable precedence and enabled-value semantics.
🤖 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
`@third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch`:
- Around line 32-36: Update RPC_PROTO_PATCH_VERSION alongside the GGML_OP_COUNT
change in the version guard, incrementing it from 3 so RPC clients and servers
using the old operation set are rejected as incompatible.
---
Nitpick comments:
In
`@third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patch`:
- Around line 137-144: Cache environment-variable results in
ggml_backend_sched_glm_route_diag_enabled() and the analogous
ggml_metal_glm_dsa_*_enabled()/ *_requested() helpers using one-time static
initialization, so getenv() is not called on each dispatch. Preserve the
existing variable precedence and enabled-value semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2678def4-2ed9-4a4c-af6f-0abe177aa1b7
📒 Files selected for processing (4)
third_party/llama.cpp/patches/0019-ggml-add-GLM-DSA-sparse-execution-primitives.patchthird_party/llama.cpp/patches/0020-skippy-expose-GLM-DSA-staged-runtime-controls.patchthird_party/llama.cpp/patches/0021-tests-cover-native-GLM-DSA-execution-paths.patchthird_party/llama.cpp/patches/0026-Fix-GLM-DSA-Metal-get_rows-placement.patch
🚧 Files skipped from review as they are similar to previous changes (2)
- third_party/llama.cpp/patches/0020-skippy-expose-GLM-DSA-staged-runtime-controls.patch
- third_party/llama.cpp/patches/0021-tests-cover-native-GLM-DSA-execution-paths.patch
e7fc73e to
6af4462
Compare
2 participants
Summary
Why
This is the first clean split from the GLM-5.2 research branch. It keeps the native GLM-DSA llama.cpp foundation separate from quant tooling, package validation, correctness reporting, MoE micro-optimizations, and speculative/MTP experiments.
Validation
Summary by CodeRabbit
New Features
Performance
Bug Fixes
get_rowskernel placement.