debug: do not merge pr-3511 - #3545
Open
chtruong814 wants to merge 5 commits into
Open
Conversation
… are kept
MiniMaxM3SparseForConditionalGeneration.forward absorbed logits_to_keep into
**kwargs, so _supports_logits_to_keep (which inspects the signature) returned
False and the recipe silently replaced any memory-efficient loss with a fresh
MaskedCrossEntropy():
if not _supports_logits_to_keep(model) and not isinstance(self.loss_fn, MaskedCrossEntropy):
logger.warning("logits_to_keep not found in model.forward. Using MaskedCrossEntropy instead.")
self.loss_fn = MaskedCrossEntropy()
Two consequences: the [tokens, vocab_size] logits tensor is materialised (200064
vocab, so tens of GB at long context), and the configured loss_fn's own settings
-- including fp32_upcast: false -- are dropped with it, upcasting that tensor to
fp32. Callers already pass logits_to_keep=1 unconditionally, so the parameter
was accepted and then ignored.
The signature check is deliberately stricter than _supports_seq_lens (which does
accept **kwargs): absorbing the kwarg does not imply the model returns hidden
states, so an explicit parameter is the right contract. Every other custom model
in components/models declares it -- minimax_m2, glm_moe_dsa, deepseek_v4,
gemma4_moe, qwen3_moe, gpt_oss, llama, qwen3_next, glm4_moe, step3p7 -- M3 was
the only one missing it.
Requesting the fused path together with MTP raises instead of returning a dict
mtp_logits cannot consume, matching the MTP-under-PP guard earlier in the same
forward.
MiniMaxM3SparseForCausalLM has the same gap but is not in the architecture
registry, and its thd branch reshapes the return value, so it is left alone here.
Signed-off-by: Xinyu Li <xli74@kent.edu>
Five CPU tests over the existing tiny-config fixtures: - the recipe's own _supports_logits_to_keep probe returns True, which is the check that silently downgraded the loss when the kwarg was absorbed by **kwargs, so this is the regression that matters most; - logits_to_keep returns a mapping with hidden_states shaped [.., hidden_size] rather than [.., vocab_size], i.e. lm_head was skipped; - omitting it still returns the logits tensor, so the default path is unchanged; - MTP plus logits_to_keep raises NotImplementedError; - MTP without logits_to_keep still produces logits and mtp_per_depth_logits. Adds an mtp_vlm_model fixture: the shared mtp_model builds MiniMaxM3SparseForCausalLM, while the branch under test lives on the conditional-generation class. Signed-off-by: Xinyu Li <xli74@kent.edu>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ormat ruff format wants the closing paren of the multi-line signature at the method's indentation. Valid Python either way, but `ruff format --check` fails on it. Only this file is touched: upstream/main already has 12 files that ruff 0.16.2 would reformat, so those are left alone rather than folded into this PR. Signed-off-by: Xinyu Li <xli74@kent.edu>
Contributor
Author
|
/ok to test d25af4b |
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
3 participants
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 ?
debug: do not merge pr-3511
Changelog
Before your PR is "Ready for review"
Pre checks:
If you haven't finished some of the above items you can still open "Draft" PR.
Additional Information