Support GLM DSA IndexShare layers - #897
Conversation
📝 WalkthroughWalkthroughA new patch adds support for GLM DSA "IndexShare" layers where some transformer layers lack indexer tensors. The loader marks ChangesGLM DSA IndexShare Layer Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1064cf8 to
0870fbf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
third_party/llama.cpp/patches/0110-Support-GLM-DSA-IndexShare-layers.patch (1)
60-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRenaming the shared
top_knode every layer may obscure graph inspection.
cb(top_k, "top_k_shared", il)re-labels the same tensor node produced in an earlier layer, so each indexer-less layer overwrites the node's name. Debug/inspection tooling that relies on stable tensor names will only ever see the lastil. Consider skipping the rename on the shared path (the originalcb(top_k, "top_k", il)name from the seeding layer is sufficient), or only attaching a view.🤖 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/0110-Support-GLM-DSA-IndexShare-layers.patch` around lines 60 - 63, The callback invocation `cb(top_k, "top_k_shared", il)` is being executed for every layer in the DSA IndexShare path, which repeatedly overwrites the name of the shared top_k tensor node. This prevents debugging and inspection tools from having a stable tensor name across layers. Remove the callback invocation on the shared path, or modify it to preserve the original tensor name from the seeding layer instead of relabeling it for each subsequent layer that reuses the shared tensor.
🤖 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.
Nitpick comments:
In `@third_party/llama.cpp/patches/0110-Support-GLM-DSA-IndexShare-layers.patch`:
- Around line 60-63: The callback invocation `cb(top_k, "top_k_shared", il)` is
being executed for every layer in the DSA IndexShare path, which repeatedly
overwrites the name of the shared top_k tensor node. This prevents debugging and
inspection tools from having a stable tensor name across layers. Remove the
callback invocation on the shared path, or modify it to preserve the original
tensor name from the seeding layer instead of relabeling it for each subsequent
layer that reuses the shared tensor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 651f3924-e6ad-4f7e-990d-db3bd6271a4c
📒 Files selected for processing (1)
third_party/llama.cpp/patches/0110-Support-GLM-DSA-IndexShare-layers.patch
Summary
Validation
Summary by CodeRabbit
Bug Fixes
Performance