fix(models): declare and honor logits_to_keep on Mistral3 VLM forward - #3789
Open
kabirvashisht4-glitch wants to merge 1 commit into
Conversation
Mistral3ForConditionalGeneration.forward absorbed logits_to_keep into **kwargs and never read it. _supports_logits_to_keep inspects the forward signature, so the recipe silently replaced a configured FusedLinearCrossEntropy with a fresh MaskedCrossEntropy -- dropping that loss's settings and materialising the [tokens, 131072] logits tensor. Declare the kwarg and honor it: when set, return the hidden states the fused loss needs, which applies the lm_head itself. The default path is unchanged and still returns logits. Same defect and fix as NVIDIA-NeMo#3510 for MiniMax-M3. Its own text backbone Mistral4ForCausalLM and its sibling Mistral3FP8VLMForConditionalGeneration already declared it. Signed-off-by: kabirvashisht4-glitch <kabirvashisht4@gmail.com>
kabirvashisht4-glitch
force-pushed
the
kabirvashisht4-glitch/fix/mistral3-logits-to-keep
branch
from
September 2, 2026 13:54
c1ece23 to
f99558b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do ?
Makes
Mistral3ForConditionalGenerationdeclare and honourlogits_to_keep, soa configured fused loss is no longer silently replaced.
forwardabsorbed the kwarg into**kwargsand never read it._supports_logits_to_keepinspects the forward signature(
components/utils/model_utils.pyL76-77), so the check returnedFalseandthe recipe swapped the configured loss for a fresh
MaskedCrossEntropy(
recipes/llm/train_ft.pyL159,recipes/vlm/finetune.pyL554,_transformers/infrastructure.pyL640). The class is registered(
_transformers/registry.pyL194-197), so this is user-reachable.Two consequences, both from #3510:
Mistral4Config.vocab_sizedefaults to 131072, so the head output is
[tokens, 131072].loss_fn's settings are dropped with it — the replacementis a fresh
MaskedCrossEntropy(), sofp32_upcast: falseis lost.Same defect and same fix as #3510 (MiniMax-M3). Evidence it was an oversight:
its own text backbone
Mistral4ForCausalLMdeclares it (same file, L417), andits sibling
Mistral3FP8VLMForConditionalGenerationdeclares it(
mistral3_vlm/model.pyL206).Contract. The recipe calls
model(logits_to_keep=1, **batch)and thenrequires
"hidden_states" in out;FusedLinearCrossEntropy.forwardtakeshidden_statesand applies the lm_head itself, fused with the loss. So thefused path returns the hidden states rather than sliced logits — matching what
#3510 did for M3. The default path (
logits_to_keep=0) is untouched and stillreturns the logits tensor.
Changelog
Mistral3ForConditionalGeneration.forwarddeclareslogits_to_keep: Union[int, torch.Tensor] = 0and returns{"hidden_states": ...}when it is set; the default path still returnslogits, unchanged.
is_thdlocal (it was computed twice fromkwargs, once before and once aftersqueeze_input_for_thdmutates it) andapplied the same batch-dim restoration on the hidden-states path.
tests/unit_tests/models/mistral4/test_mistral4_logits_to_keep.pycovering the recipe's
_supports_logits_to_keepprobe, the fused pathreturning hidden states without materialising logits, and the unchanged
default path.
Before your PR is "Ready for review"
Pre checks:
On the test file location. These live in a new file rather than in
test_mistral4_model.pybecause that module carries a module-levelpytest.mark.skipif(not torch.cuda.is_available()), so the contract would notbe checked on the CPU tier. The new file is CPU-runnable, mirroring
tests/unit_tests/models/minimax_m3_vl/test_minimax_m3_logits_to_keep.py.Verification (CPU, no GPU or checkpoint needed):
pytest tests/unit_tests/models/mistral4/test_mistral4_logits_to_keep.py→ 3 passed.test_recipe_detects_logits_to_keep_supportand
test_logits_to_keep_returns_hidden_states_and_skips_lm_headfail;test_without_logits_to_keep_still_returns_logitspasses either way by design,guarding the path this must not change.
pytest tests/unit_tests/models/mistral4/ tests/unit_tests/models/mistral3/ tests/unit_tests/models/mistral3_vlm/→ 147 passed, 70 skipped.tests/unit_tests/models/ tests/unit_tests/_transformers/run: 84 failuresbefore and after this change (gemma4 / glm5_next / retrieval), all pre-existing
on
mainin my environment and unrelated — the only delta is the 2 tests abovegoing green.
diffusion_gemmaandgemma4_unifiedwere excluded locally: theyfail to import
transformers.models.diffusion_gemmaonmaintoo.ruff format/ruff checkclean.Additional Information