[#15022][fix] Guided decoding (xgrammar) + EAGLE-3 + draft_len_schedule reaching 0 crashes during CUDA graph capture, "bitmask must have the same batch size as logits" - #15023
Conversation
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. 📝 WalkthroughWalkthroughThe PR threads a runtime draft length parameter through guided decoder APIs to bound draft-token processing. GuidedDecoder and CapturableGuidedDecoder now accept an optional ChangesRuntime Draft Length Parameter
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pyexecutor/guided_decoder.py`:
- Around line 341-344: Validate that runtime_draft_len does not exceed
self.max_num_draft_tokens before constructing GuidedRequests: if
runtime_draft_len is not None and runtime_draft_len > self.max_num_draft_tokens,
raise a ValueError (or clamp and log) so requests =
GuidedRequests.from_scheduled_requests(...) cannot create
requests.max_num_draft_tokens larger than the buffers allocated in __init__;
this prevents buffer overflows in _copy_bitmask and _apply_bitmask.
- Around line 484-487: Validate that runtime_draft_len does not exceed the
preallocated capacity before using it: inside GuidedDecoder (the constructor
block where runtime_draft_len is used to set num_draft_tokens and call
GuidedRequests.from_scheduled_requests) add a check comparing runtime_draft_len
(when not None) against self.max_num_draft_tokens and raise a clear ValueError
(or clamp/handle per project convention) if runtime_draft_len >
self.max_num_draft_tokens; mirror the same validation logic used in
GuidedDecoder.add_batch to prevent buffer overflows during
_copy_bitmask/_apply_bitmask.
🪄 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: 6e2a708e-3128-4228-9570-48ff238ea55a
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/guided_decoder.pytensorrt_llm/_torch/pyexecutor/model_engine.py
283e092 to
b55920a
Compare
|
Reviewing the current patch, the concern raised by @coderabbitai regarding potential buffer overflows if Instead of overriding I have submitted a bullet-proof fix addressing this edge-case safely here: #15026 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 15026 --repo NVIDIA/TensorRT-LLM --json title,body,files,additions,deletions,state
gh pr diff 15026 --repo NVIDIA/TensorRT-LLMLength of output: 5928
The approach in the current PR ( |
fe65ffa to
e760fa2
Compare
|
Despite there are no explicit guarding and clamping of
|
|
Could you clarify the intended order between this PR and #15026? |
|
@achartier I am not sure about the content and author in #15026 , looks like he proposed a PR quickly soon after my issue & PR, but there are some design choices to be justified there. |
|
/bot run |
|
PR_Github #52859 [ run ] triggered by Bot. Commit: |
|
PR_Github #52859 [ run ] completed with state
|
|
Please follow these steps to setup pre-commit and run it on this PR: https://github.com/NVIDIA/TensorRT-LLM/blob/main/CONTRIBUTING.md |
e760fa2 to
f82e6eb
Compare
|
@achartier Done, please check |
|
/bot run |
|
PR_Github #52871 [ run ] triggered by Bot. Commit: |
|
PR_Github #52871 [ run ] completed with state
|
|
@chungen04 Could you double check? |
|
@achartier Done, please check. Looks like I missed a message saying yapf was installing on my end. |
|
/bot run |
|
PR_Github #53089 [ run ] completed with state
|
7b35bde to
819227e
Compare
|
/bot run |
Infra issue. I rebased and relaunched the pipeline. |
|
PR_Github #53121 [ run ] triggered by Bot. Commit: |
|
PR_Github #53121 [ run ] completed with state
|
Signed-off-by: chungen04 <b09901027@ntu.edu.tw>
819227e to
0e4f0b4
Compare
|
/bot run |
|
PR_Github #53239 [ run ] triggered by Bot. Commit: |
|
PR_Github #53239 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53371 [ run ] triggered by Bot. Commit: |
|
PR_Github #53371 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53438 [ run ] triggered by Bot. Commit: |
|
PR_Github #53438 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53599 [ run ] triggered by Bot. Commit: |
|
PR_Github #53599 [ run ] completed with state
|
|
/bot run |
|
PR_Github #53655 [ run ] triggered by Bot. Commit: |
|
PR_Github #53655 [ run ] completed with state |
|
Oh, here we go. Thank you @achartier for the support! |
@coderabbitai summary
Description
Addressing Issue #15022.
With dynamic draft length (
draft_len_schedule), the model engine tracks this asruntime_draft_len:py_executor.py:2346-2362 per iteration, from the schedule
When
runtime_draft_lenis, say, 1, the target model emits only 1 + 1 = 2 logit rows per generation request. But now, the guided decoder always laid out its bitmask for the static maxguided_decoder.py:335-337
So the guided decoder expected
max_num_draft_tokens + 1rows per request, while the model produced onlyruntime_draft_len + 1.That row count flows into num_bitmask_tokens:
guided_decoder.py:110-116
and is then used to index both the bitmask and the logits tensor:
guided_decoder.py:328-332 (_apply_bitmask)
Because
num_bitmask_tokens(computed from the static max) is larger than the actual number of logit rows the model produced, the indexing reads past the end of logits and crash.At #10860, it let
runtime_draft_lendrop below the static max per iteration, butguided_decoder.pywas not updated. So after #10860, the guided decoder kept laying out its bitmask formax_num_draft_tokenswhile the target emitted onlyruntime_draft_len + 1rows per request.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.