Granite-Switch Architecture#25107
Conversation
|
Hi @barvhaim, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
I already added before "AI usage disclosure" |
gabe-l-hart
left a comment
There was a problem hiding this comment.
A bunch of thoughts that loosely fall into the following buckets:
- Should we explore implementing the adapters themselves using the existing adapter mechanism so it's more extensible in the future?
- Some restructuring in the conversion code to hopefully simplify a little and extend flexibility
@CISC I'm particularly curious your thoughts on reusing existing adapter mechanisms vs this implementation that reimplements adapters in just the right places for this model so that it doesn't need to change anything about the existing adapter code paths.
|
|
||
| @ModelBase.register("GraniteSwitchForCausalLM") | ||
| class GraniteSwitchModel(GraniteMoeModel): | ||
| """Dense, all-attention Granite model with N embedded LoRA adapters selected |
There was a problem hiding this comment.
I know the current model is built on the Dense 4.1 bases. I believe the intent is to also release switch models for other architectures going forward (including MoE). I haven't made it through the review yet, but I think we'll want to try to make sure the switch functionality is loosely coupled to the core model so it can easily extend to other core model architectures.
There was a problem hiding this comment.
with removing the hack, expert_count now follows num_local_experts from the config
(still need approval on shared code modification)
| def set_gguf_parameters(self): | ||
| super().set_gguf_parameters() | ||
|
|
||
| # the model is dense; force the dense invariant in case the llama parent |
There was a problem hiding this comment.
The GraniteMoeModel inherits directly from GraniteModel and only overrides set_gguf_parameters to optionally add the sahred_intermedaite_size (GraniteMoeShared). I don't think these explicit overrides are needed and they make the architecture less flexible to future core model architectures. In fact, I think the best course of action may be to inherit from GraniteHybridModel which itself acts as a child of both GraniteModel and GraniteMoeModel depending on the presence of SSM layers and experts. Some of the other logic may get more complex, but it's worth trying.
There was a problem hiding this comment.
Dropped the hardcoded add_expert_count(0), it's config-driven now. Kept one dense guard on expert_used_count: the model's config.json carries num_experts_per_tok: 2 because the upstream HF config (GraniteSwitchConfig extends transformers.GraniteMoeHybridConfig) sets it, so without the guard a dense model emits expert_used_count=2 with expert_count=0 and fails the loader assert.
There was a problem hiding this comment.
On reparenting to GraniteHybridModel: I tried it, seems like it's the wrong base here. Its __init__ auto-downgrades the arch when there are no SSM layers, and since the config has num_experts_per_tok: 2 it rewrites graniteswitch → granitemoe. It'd also pull in Mamba2 machinery (hparam_prefixes, SSM params, mamba set_vocab) that a dense all-attention model doesn't use. GraniteMoeModel stays the cleaner parent, the flexibility comes from the config-driven expert count, not the base class.
| auto * inp_attn = build_attn_inp_kv(); | ||
|
|
||
| // router attention: a single causal head whose K/V live in the KV cache at | ||
| // layer R, recovering the per-token adapter index in-graph. Only dim 0 carries |
There was a problem hiding this comment.
Ah, I see: you keep the KV cache for this layer at the end, not the beginning, so we don't have to shift everything else.
| ggml_tensor * Kcur = ggml_cont(ctx0, ggml_view_2d(ctx0, qkv, n_embd_kv, qkv->ne[1], qkv->nb[1], n_embd_q*ggml_element_size(qkv))); | ||
| ggml_tensor * Vcur = ggml_cont(ctx0, ggml_view_2d(ctx0, qkv, n_embd_kv, qkv->ne[1], qkv->nb[1], (n_embd_q + n_embd_kv)*ggml_element_size(qkv))); | ||
|
|
||
| Qcur = ggml_add(ctx0, Qcur, build_switched_lora_delta(sl.a_q, sl.b_q, cur, adapter_ids)); |
There was a problem hiding this comment.
Ok, I think this is the core of the in-model adapter implementation point. If we explore the route of using the existing llama_lora_adapter infra, this would get deferred into the build_attn below automatically I think.
We have avoided this path a couple of times in the past already, but I think it's long overdue to try and tackle this, I think it would be worth refactoring adapters to handle this, unfortunately the timing is bad for me as I am on forced vacation. :) I'm sure @ngxson has some thoughts on this as well. |
Zero problem (I hope everything is ok!). @ngxson if you're eager, I'd love to hear your thoughts, but I'm also game to take a pass at this if you're swamped. I had started it already, but hadn't gotten much further since @barvhaim was able to get things working end-to-end. |
Oh yes, given the circumstances fantastic in fact. :) Long story short I'm between jobs, all will be revealed by August... |
111d333 to
9249531
Compare
ngxson
left a comment
There was a problem hiding this comment.
Given recent models like deepseekv4 / minimax also have complicated cgraph and kv logic, I think it could be fine to add this one. We can accept it as long as the impl is mostly confined inside granite-switch.cpp
Things that I'm less comfortable about the PR:
- Excessive AI-generated comments. Most comments are just noise and doesn't add much to my understanding of the code
- Still a lot of hacks outside of
granite-switch.cpp
gabe-l-hart
left a comment
There was a problem hiding this comment.
A few more thoughts/comments, especially around simplifying the added tensor names with suffixes.
| def set_gguf_parameters(self): | ||
| super().set_gguf_parameters() | ||
|
|
||
| # dense model: override any expert counts the llama parent emitted |
There was a problem hiding this comment.
I'm still not sure we want to explicitly override like this. The upstream parents should handle this correctly for dense or MoE target models, so this just unnecessarily limits to dense-only.
758fbbb to
aa441eb
Compare
+1 |
gabe-l-hart
left a comment
There was a problem hiding this comment.
One tiny NIT, but otherwise this looks like it's in good shape to me. @ngxson I think it's ready for CI and your next review when you have time.
| // slots in dim 2 (slot 0 is zero-filled so base tokens add an exact-zero delta). | ||
| // A: {n_embd_in, max_lora_rank, N} B: {max_lora_rank, n_embd_out, N} | ||
| // 7 injection sites (q, k, v, o, gate, up, down), each with an A and a B = 14 tensors. | ||
| struct llama_layer_switch_lora { |
There was a problem hiding this comment.
Given @ngxson 's input, I think we're going to avoid the "overhaul how adapters work" piece for now.
bae32b6 to
0a7e411
Compare
290d8d4 to
97700cf
Compare
1cb71b5 to
f57f905
Compare
skipped the arch in |
NOTE: This shadows the work done for Granite Speech ggml-org#25107 Branch: GraniteSWAForCausalLM AI-usage: full (Bob) Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>
|
@ggerganov With @CISC out of pocket is there someone else that can take a look at the core library changes on this PR?
|
|
Quick note on this, running |
Overview
Adaptation of Granite-Switch model (https://huggingface.co/ibm-granite/granite-switch-4.1-3b-preview) for llama.cpp,
Today Granite-Switch supports HF and vLLM backends.
Granite-Switch OSS project: https://github.com/generative-computing/granite-switch
Converted model with this code: https://huggingface.co/barha/granite-switch-4.1-3b-preview-GGUF/blob/main/granite-switch-4.1-3b-preview-f16.gguf
The model is dense, not MoE. This value is only needed because
ggml_mul_mat_idusesn_expert_usedas the number of selected matrix IDs per token. For Granite Switch this is always one selected adapter slot per token.Requirements
P.S, maintainers, I'm looking forward for your comments on how to progress with getting this supported arch, so pretty much opened that PR for getting feedbacks. thanks!