[#15182][fix] Fix embedding vocab mask for handling rejection sampling in Kimi-K2.5 - #15233
Conversation
Signed-off-by: chungen04 <b09901027@ntu.edu.tw>
📝 WalkthroughWalkthroughThis PR extends the input token masking logic in the embedding operation to support non-tensor-parallel mode. The ChangesEmbedding Masking for Non-Tensor-Parallel Mode
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
zhaoyangwang-nvidia
left a comment
There was a problem hiding this comment.
Thanks for the fix — this correctly closes the crash on the non-TP (attention-DP) path. Two small follow-ups:
- Add a comment on where the -1 comes from. It's not obvious why masking is needed here. flashinfer's chain_speculative_sampling pads non-accepted tokens with -1, and these invalid ids reach the embedding lookup — that's what this guards against. Suggested:
elif tp_mode is None:
# flashinfer's rejection kernel (chain_speculative_sampling) pads non-accepted
# tokens with -1. In non-TP mode the full vocab is local, so mask out-of-range
# ids (e.g. -1) over [0, weight.shape[0]) to avoid an OOB embedding lookup.
input_, input_mask = get_masked_input_and_mask(input_, 0, weight.shape[0])
- Could you add the same guard for ROW mode? ROW also holds the full vocab on each rank, so it has the same -1 exposure but isn't masked. Note the ROW all-gather padding is in an elif that's mutually exclusive with the mask-cleanup, so it needs to be decoupled:
input_mask = None
if tp_mode == TensorParallelMode.COLUMN:
input_, input_mask = get_masked_input_and_mask(input_, vocab_start_index, vocab_end_index)
elif tp_mode is None or tp_mode == TensorParallelMode.ROW:
# Full vocab present locally -> mask out-of-range ids (e.g. -1).
input_, input_mask = get_masked_input_and_mask(input_, 0, weight.shape[0])
output = F.embedding(input_, weight)
if input_mask is not None:
output.masked_fill_(input_mask, 0)
# ROW: pad hidden dim for all-gather (independent of masking).
if tp_mode == TensorParallelMode.ROW and gather_output:
if tp_rank == tp_size - 1 and padding_size > 0:
output = F.pad(output, (0, padding_size))
return output
|
Thank you for the feedback. I am taking @zhaoyangwang-nvidia 's recommendation here as it also guards >= vocab_size vocab id ( |
Signed-off-by: chungen04 <b09901027@ntu.edu.tw>
It is Ok to reuse |
Signed-off-by: chungen04 <b09901027@ntu.edu.tw>
|
@yuxianq resolved. I was thinking about making the condition of |
|
/bot run --disable-fail-fast |
|
PR_Github #53817 [ run ] triggered by Bot. Commit: |
|
PR_Github #53817 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #53867 [ run ] triggered by Bot. Commit: |
|
PR_Github #53867 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #53872 [ run ] triggered by Bot. Commit: |
|
PR_Github #53872 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #54026 [ run ] triggered by Bot. Commit: |
|
PR_Github #54026 [ run ] completed with state
|
|
/bot run |
|
PR_Github #54295 [ run ] triggered by Bot. Commit: |
|
PR_Github #54295 [ run ] completed with state
|
|
Should we rebase the branch? |
|
/bot run --disable-fail-fast |
|
PR_Github #54425 [ run ] triggered by Bot. Commit: |
|
PR_Github #54425 [ run ] completed with state
|
|
/bot run |
|
PR_Github #54528 [ run ] triggered by Bot. Commit: |
|
PR_Github #54528 [ run ] completed with state |
|
Hi @yuxianq this PR is ready to merge, please help to merge it. |
…ampling in Kimi-K2.5 (NVIDIA#15233) Signed-off-by: chungen04 <b09901027@ntu.edu.tw> Co-authored-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> Signed-off-by: GitLab CI Bot <gitlab-ci@nvidia.com>
…ampling in Kimi-K2.5 (NVIDIA#15233) Signed-off-by: chungen04 <b09901027@ntu.edu.tw> Co-authored-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> Signed-off-by: GitLab CI Bot <gitlab-ci@nvidia.com>
[None][fix] fix for rejection sampling warmup crash (upstream Pr NVIDIA#15233)
Summary by CodeRabbit
Description
Fix #15182
Quick recap of the issue One-model Eagle3 speculative decoding with
use_rejection_sampling: truecrashes during the first generation step (warmup during engine launch) with a device-side assert, where the issue is specific to Kimi (not to the models supported as indicated in #12588):Summary of cause The root cause (examined by logging) is that the draft model's token embedding receives flashinfer's -1 "rejected token" vocab id, and on models such as Kimi-K2.6, the embedding does not vocab-mask its input, so F.embedding(-1) faults. Models such as Llama-4-Maverick, running correctly with
use_rejection_sampling: trueabsorb the same -1 via the existing vocab-mask and never crash.Code tracing of the root cause
use_rejection_sampling: true.Rejection acceptance calls flashinfer's
chain_speculative_samplingviarejection_sampling_one_model(tensorrt_llm/_torch/speculative/one_model_sampler.py). Its returned output_token_ids pads rejected positions with -1 (flashinfer sampling.py, chain_speculative_sampling docstring: "rejected samples are padded with -1").This path is taken in
_sample_and_accept_draft_tokens_rejection(tensorrt_llm/_torch/speculative/interface.py), and the raw accepted_tokens (still containing -1) is returned unchanged.prepare_1st_drafter_inputsbuilds the draft model's input_ids by flattening all accepted-token slots, including the rejected (-1) ones src:That tensor flows into the draft loop (eagle3.py
_run_draft_forward), which calls the draft token embedding (modeling_speculative.py:414,Eagle3DraftModel.forward):The assert listed in the original crashing log is in tensorrt_llm/_torch/modules/embedding.py,
pre_comm_embedding_ops:input_got -1, which cause the kernel to got caught in assertion.tl;dr: Llama guarded the -1 vocab ids via the vocab id mapping to TP shards, which is enabled by default, but not for other models such as DeepSeek-V3.
The case for Llama-4-Maverick: the tracing stack is as follows.
use_rejection_sampling, the -1 vocab ids were emitted as usual.tp_modesrc.get_masked_input_and_maskmethod indicates that the -1 vocab id is not mapped to any shard in TP ranks. output is eventually mask filled with 0 src. This path naturally handles out of range IDs.For Kimi / DeepSeek-V3:
use_rejection_sampling, the -1 vocab ids were emitted upon rejection.embed_tokensin DeepSeek-V3 is not initialized with TP modeling_deepseekv3.py#L1759. The -1 vocab id was directly used to index the embedding table, causing the crash.Proposed fix
Derived from the trace, there are a few possible fixes:
embed_tokensinclass DeepseekV3Model.tensorrt_llm/_torch/modules/embedding.py,pre_comm_embedding_ops: apply the existing get_masked_input_and_mask in the replicated (tp_mode is None) case as well, using the local vocab range [0, weight.shape[0]).The second was what current PR adopts. This is a minimal change that reuses the same masking primitive Llama-4 already relies on, and it does not touch the rejection sampler, the draft loop, or accepted_tokens (where -1 remains a meaningful vocab id that downstream code reads via
num_accepted_tokens, which is not altered here).Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.