[https://nvbugs/6460072][fix] Split the caller — for use_lm_head_tp_in_adp and is_all_greedy_sample, keep… - #16440
Conversation
📝 WalkthroughWalkthroughEagle3 now routes all-greedy LM-head-TP-in-ADP draft-token selection through the TP-aware sampler, slices results to ChangesEagle3 draft sampling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/speculative/eagle3.py`:
- Around line 1254-1257: Update the comment near the plain-TP and
LM-head-TP-in-ADP routing explanation to reference `_forward_linear_draft_loop`
instead of `_sample_draft_tokens_common`, preserving the existing description of
pre-trim padded logits and `mapping_lm_head_tp`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f2d42e53-7ade-4ba6-ba37-8e0ef615b6dd
📒 Files selected for processing (1)
tensorrt_llm/_torch/speculative/eagle3.py
|
/bot run --disable-fail-fast |
|
PR_Github #59562 [ run ] triggered by Bot. Commit: |
use_lm_head_tp_in_adp and is_all_greedy_sample, keep…use_lm_head_tp_in_adp and is_all_greedy_sample, keep…
use_lm_head_tp_in_adp and is_all_greedy_sample, keep…use_lm_head_tp_in_adp and is_all_greedy_sample, keep…
|
PR_Github #59562 [ run ] completed with state
|
46f8aae to
a28ed86
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59846 [ run ] triggered by Bot. Commit: |
…-Eagle greedy draft under attention DP The draft_sampler consolidation dropped the ADP+LM-head-TP branch: under enable_lm_head_tp_in_adp the MTP shared head returns the LM-head-TP group's row-stacked, vocab-sharded logits, but the worker trimmed rows [:token_count] (group rank 0's requests, not this rank's) and greedy sampling fell through to a plain per-rank argmax over a local vocab shard, producing wrong draft tokens on every rank. Restore the group-wide distributed argmax in greedy_sample_draft_with_tp_gather: combine each rank's local (global index, max) over mapping_lm_head_tp, slice this rank's own row segment at tp_rank * max_num_requests, and only then trim the padding to token_count in the caller. Advanced (rejection) draft sampling asserts it never sees this layout, as rejection is config-gated off under attention DP. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>
a28ed86 to
3a94f75
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59846 [ run ] completed with state
|
|
PR_Github #59888 [ run ] triggered by Bot. Commit: |
|
PR_Github #59888 [ run ] completed with state |
Summary
use_lm_head_tp_in_adp and is_all_greedy_sample, keep padded logits, callself.draft_sampler(logits, draft_model.mtp_layers[0].shared_head.mapping_lm_head_tp)(which all-gathers local argmax/max across mapping_lm_head_tp ranks and picks global argmax), then trim result to token_count. Non-greedy path unchanged.Test plan
Links
Summary by CodeRabbit