fix: Align bitmask batch size with logits tensor dimension inside GuidedDecoder - #15026
fix: Align bitmask batch size with logits tensor dimension inside GuidedDecoder#15026Priyanshu31102003 wants to merge 2 commits into
Conversation
… registry context for btk verification Signed-off-by: jet1technology-tech <jet1technology@ryngo.in>
11a890a to
a86b18f
Compare
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. 📝 WalkthroughWalkthroughThis PR addresses a CUDA graph capture crash in guided decoding by defensively computing and clamping ChangesGuided Decoding and Model Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (1)
tensorrt_llm/_torch/pyexecutor/guided_decoder.py (1)
309-330: ⚡ Quick winConsider consolidating the clamping logic into a single expression.
After fixing the bug at lines 311-312, the three-step clamping pattern can be simplified for clarity:
♻️ Proposed refactor
if num_bitmask_tokens is None: num_bitmask_tokens = requests.num_bitmask_tokens if requests is not None else logits.size(0) -if logits.size(0) > num_bitmask_tokens: - num_bitmask_tokens = logits.size(0) - vocab_size_padded = self.vocab_size_padded if d2t is None else d2t.size( 0) assert vocab_size_padded % logits.size(1) == 0 tp_size = vocab_size_padded // logits.size(1) assert self.bitmask_size % tp_size == 0 tp_rank = self.rank % tp_size bitmask_start = tp_rank * self.bitmask_size // tp_size bitmask_end = bitmask_start + self.bitmask_size // tp_size if d2t is not None: d2t_start = tp_rank * vocab_size_padded // tp_size d2t_end = d2t_start + vocab_size_padded // tp_size d2t = d2t[d2t_start:d2t_end] -if self.bitmask.size(0) < num_bitmask_tokens: - num_bitmask_tokens = self.bitmask.size(0) - +num_bitmask_tokens = min(num_bitmask_tokens, logits.size(0), self.bitmask.size(0)) + torch.ops.trtllm.logits_bitmask(🤖 Prompt for 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. In `@tensorrt_llm/_torch/pyexecutor/guided_decoder.py` around lines 309 - 330, Replace the three-step clamping of num_bitmask_tokens with a single expression that computes the initial value (requests.num_bitmask_tokens if requests is not None else logits.size(0)), then clamps it to be at least logits.size(0) and at most self.bitmask.size(0); i.e. set num_bitmask_tokens = min(self.bitmask.size(0), max(logits.size(0), initial_value)). Apply this change where num_bitmask_tokens is defined/updated in guided_decoder.py so the later d2t handling and other logic using num_bitmask_tokens remain unchanged.
🤖 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 311-312: The comparison is reversed causing num_bitmask_tokens to
grow instead of being clamped to the logits batch size; change the logic around
logits and num_bitmask_tokens so that if num_bitmask_tokens is greater than
logits.size(0) you set num_bitmask_tokens = logits.size(0). Update the block
using logits, num_bitmask_tokens, logits_bitmask and self.bitmask in
guided_decoder.py to ensure you slice logits[:num_bitmask_tokens] and
self.bitmask[:num_bitmask_tokens, ...] with matching batch sizes (i.e., clamp
down num_bitmask_tokens to logits.size(0) before calling logits_bitmask).
---
Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/guided_decoder.py`:
- Around line 309-330: Replace the three-step clamping of num_bitmask_tokens
with a single expression that computes the initial value
(requests.num_bitmask_tokens if requests is not None else logits.size(0)), then
clamps it to be at least logits.size(0) and at most self.bitmask.size(0); i.e.
set num_bitmask_tokens = min(self.bitmask.size(0), max(logits.size(0),
initial_value)). Apply this change where num_bitmask_tokens is defined/updated
in guided_decoder.py so the later d2t handling and other logic using
num_bitmask_tokens remain unchanged.
🪄 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: d3abe478-c84f-4506-b295-944adde63110
📒 Files selected for processing (2)
examples/auto_deploy/model_registry/models.yamltensorrt_llm/_torch/pyexecutor/guided_decoder.py
… switches Signed-off-by: jet1technology-tech <jet1technology@ryngo.in>
a86b18f to
d562d41
Compare
|
Is this still needed now that #15023 is merged? |
|
@Priyanshu31102003 , I think the guided-decoding fix here has been superseded by #15023, which has already merged. #15023 addresses the root cause by making the guided-decoding bitmask layout follow the runtime draft length and passing Also, the Given #15023 is merged, I think we can close this PR as superseded. Please feel free to open a new one if the registry update is still needed. Thank you! |
Description
Fixes a
RuntimeError: bitmask must have the same batch size as logitscrash during CUDA graph capture warmup whendraft_len_scheduledrops to0.Changes
_apply_bitmask(guided_decoder.py) to synchronizenum_bitmask_tokenswith the actual batch size of thelogitstensor when speculative generation switches modes.self.bitmask.size(0)to structurally prevent any potential buffer overflows, safely addressing the architectural edge-case missing in other temporary patches.Closes #15022
Summary by CodeRabbit
New Features
Bug Fixes