[generate] Add proper MTP support#46229
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
vasqu
left a comment
There was a problem hiding this comment.
Some initial comments from my side
My main concern is that the whole design is inference centric and I'm not sure if we build technical debt when we want to add this to training in the future. Would love some discussion around this to make sure we are aligned.
Other than that, maybe some concerns about the usage of a cache (seems to be mandatory) and whether hybrid models exist with MTP (e.g. SWA and full attn) as they are not reflected here
| class MTPCandidateGenerator(AssistedCandidateGenerator): | ||
| requires_model_outputs: bool = True | ||
| # We always need to pass the hidden states from the main model | ||
| model_kwargs_overrides: dict[str, Any] = {"output_hidden_states": True} |
There was a problem hiding this comment.
Is this the same as in gemma4 assistant? (See the note I added there :D)
If we only need the last hidden state, it would maybe make sense to fix the output recorder to only really extract the last one for example or simplify in some other way.
There was a problem hiding this comment.
We indeed only need the last one here, but for simplicity it integrates with the usual output_hidden_states which exists for all models. We could definitely tweak the output recorder to allow output_last_hidden_states, but a bit out of scope of this PR I'd say
There was a problem hiding this comment.
Yes for sure, sorry was meant as a general note not specific to this PR 🫡
| self.device = next(x.device for x in main_model.base_model.layers[-1].parameters()) # type: ignore | ||
| self.mtp_model = MtpLayerStack.from_pretrained(main_model, device_map={"": self.device}) |
There was a problem hiding this comment.
Any reason we really need this heuristic or do you think a .to(device) call when generating the candidate would be to expensive?
Edit: seen we call to device either way. Don't really see any reason why we cant do mtp_model.device instead then with auto or pass the original kwargs from the original model here as well? E.g. The attention implementation should also be toggleable
There was a problem hiding this comment.
IMO, it makes a lot of sense to put it on last gpu in general, as it's an additional layer of some sort that would usually be put on last device as well if loaded from the beginning - we can change though if you think otherwise
There was a problem hiding this comment.
Ah no I think that's also a fair point. Haven't considered it that way
| if len(cache) != main_model.config.get_text_config().num_hidden_layers + self.num_mtp_layers: | ||
| cache.layers.extend([DynamicLayer() for _ in range(self.num_mtp_layers)]) | ||
| else: | ||
| raise ValueError("No cache yet") |
There was a problem hiding this comment.
What speaks against no cache? Should be possible as well, no?
There was a problem hiding this comment.
It would, but the _assisted_decoding decoding method forces a DynamicCache for now, no cache is not an option yet (due to rollback ops etc)
There was a problem hiding this comment.
Argh, ok can weadd this comment at least so it's clear?
|
|
||
| self.post_init() | ||
|
|
||
| def forward( |
There was a problem hiding this comment.
maybe a generate vs forward split would be nicer. I'm still thinking of training tbh and how that flow into this wholse design
There was a problem hiding this comment.
yeah I think drafting training is important as well!
There was a problem hiding this comment.
The fact is that it's kind of the same: the usual forward is a generation loop, in the sense that later layers need both the last hidden states from previous layer AND the new drafted token -> so they cannot be separated from one another in general
ArthurZucker
left a comment
There was a problem hiding this comment.
Nice! its elegant! I'd be down to see how it scales with different architectures + training as @vasqu said!
For from pretrained would want to re-use or let it behave normally rather than not go through the general path!
|
|
||
| self.post_init() | ||
|
|
||
| def forward( |
There was a problem hiding this comment.
yeah I think drafting training is important as well!
| # Decode one token | ||
| next_token_logits = logits[:, -1, :].to(device=input_ids.device) | ||
| if logits_processor is not None and full_input_ids is not None: | ||
| next_token_scores = logits_processor(full_input_ids, next_token_logits.to(torch.float32)) | ||
| if do_sample: | ||
| probs = nn.functional.softmax(next_token_scores, dim=-1, dtype=torch.float32) | ||
| next_mtp_token = torch.multinomial(probs, num_samples=1) | ||
| else: | ||
| next_mtp_token = torch.argmax(next_token_scores, dim=-1, keepdim=True) | ||
| drafted_tokens.append(next_mtp_token) |
There was a problem hiding this comment.
something we ought to reuse if possible from normal flow
| WeightRenaming("mlp.shared_expert.", "mlp.shared_experts."), | ||
| ] | ||
|
|
||
| mapping["MtpLayerStack"] = [ |
There was a problem hiding this comment.
this should be model familiy dependant non?
There was a problem hiding this comment.
Unless we create a MTP layer for each model, we cannot really do it - or we need to alter a bit the logic of getting the conversion mapping for mtp
There was a problem hiding this comment.
we can leave this for the case where the mtp layer is different!
…gface#46229 Wire up Multi-Token Prediction (MTP) training for Qwen3.5 by reusing the inference-side MtpLayer from huggingface#46229 instead of the inline MTP definition from huggingface#45638, and add the matching Trainer hook that combines the auxiliary loss. Model side (Qwen3.5): * CausalLMOutputWithPast gains an mtp_loss field carrying the unweighted MTP loss (averaged over MTP layers and non-ignored tokens). * Qwen3_5TextConfig exposes num_nextn_predict_layers (canonical MTP layer count, aligned with huggingface#46229) and output_mtp_loss (training-only switch). * Qwen3_5ForCausalLM dynamically instantiates MtpLayer copies that mirror the main decoder layer architecture (full / linear attention mix, RMSNorm variant) and recomputes the per-layer loss with properly sliced inputs, position embeddings, and labels. Trainer side: * TrainingArguments gains mtp_loss_coef (default 0.0, BC-safe). * Trainer.compute_loss adds the unweighted output.mtp_loss * mtp_loss_coef to the main loss at the very end, leaving the model forward signature clean and the combination policy centralized in the trainer. Supersedes huggingface#45638's inline-MTP approach in favor of a single shared MtpLayer implementation that the inference path already uses. Refs: huggingface#46229, huggingface#45638
|
@Cyrilvallez — following your A bit of context: I started prototyping this back in #45638 with an inline
|
|
Hey @ArthurZucker @vasqu thanks for the reviews! I believe I answered all comments, let me know if something is unclear. |
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=46229&sha=862db4 |
|
Awesome work overall!! I think it would be nice to provide the basic training path for MTP then. Iiuc, it's just the "missing" labels in the forward. While they may be similar, I want to avoid that we encounter something unexpected. I don't think packed sequences are a big priority for now tbh so let's keep it simple for sure. For the specific new patterns (e.g. hybrid linear attention). I think it makes sense to have the general implementation ("normal" attention) which we override to allow more specific patterns (hybrid linear attention). I would expect all hybrids to follow a similar pattern there so that we won't need the model specifics(?) and only variations of MTP if that makes sense |
renames mtp_num_hidden_layers to num_mtp_layers (matches huggingface#46229) and removes mtp_loss_weight / output_mtp_loss. forward still returns mtp_loss separately so the trainer can apply its own weighting.
renames Qwen3_5MTPLayer/Qwen3_5MTP to Qwen3_5MtpLayer/Qwen3_5MtpLayerStack and adds the unexpected-key filter used by the generic MtpLayerStack. once huggingface#46229 lands these classes can be replaced by direct imports.
mirrors the entries huggingface#46229 adds for MtpLayerStack, so ckpts trained with this PR can be loaded back by the generic loader once huggingface#46229 lands.
|
[For maintainers] Suggested jobs to run (before merge) run-slow: deepseek_v3, glm4_moe, glm4v_moe, mimo_v2_flash, solar_open, youtu |
| "assistant_confidence_threshold": 0.4, | ||
| "assistant_lookbehind": 10, | ||
| "target_lookbehind": 10, | ||
| "use_mtp": False, |
There was a problem hiding this comment.
lets not add anything new here, as it was created for BC. The default value can be just None imo
CI recapDashboard: View test results in Grafana |
ArthurZucker
left a comment
There was a problem hiding this comment.
great PR, its just missing some tests. TY for iterating
| # just drafted from the main model. We need to slice to get only what the main model just processed, shifted by 1 to the | ||
| # right to take the new token as well. Say the main model just had token positions [2, 3] as input, the tensors | ||
| # contains the data for position [0, 1, 2, 3, 4], i.e. full inputs + new drafted token from last position 3 that was processed. | ||
| # We want to slice to get data for positions [3, 4] for the 1st mtp layer, i.e. same as main model, shifted by 1 to the right. |
| WeightRenaming("mlp.shared_expert.", "mlp.shared_experts."), | ||
| ] | ||
|
|
||
| mapping["MtpLayerStack"] = [ |
There was a problem hiding this comment.
we can leave this for the case where the mtp layer is different!
What does this PR do?
As per the title! Finally add MTP decoding to Transformers!
For models supporting it and having proper trained weights, it can be used as simply as:
It will automatically scan the repo corresponding to the model calling
generate, and load the weights corresponding to the MTP layers.Note that it integrates nicely into the existing
_assisted_decoding, thus avoiding to add yet another decoding method intogenerate.Benchmark and expected perfs
On the previous example, I benchmarked the speed (excluding loading time of the MTP weights of course) and token matching, and obtained an acceptance rate of 73.68% for the drafted tokens from the (unique) MTP layer, resulting in a decoding speedup ratio of about 1.4, or 40% faster decoding compared to not using MTP.
This is of course a single test on a small toy example, so proper benchmarking may be needed, but it gives a good idea of what we can expect in general I believe.
Limitations
Note that this is unfortunately NOT supported for the recent Deepseek v4, as they changed the naming/conventions of the MTP layers (they mostly split the
eh_projlayer ine_projandh_proj, and use quantization on them). It should be relatively straightforward to fix it and make the weights match with simpleConversionOps, either on the general conversion mapping or on a model attribute such as_mtp_conversionsor something.