[None][fix] Fix logits post processor for beam search in PyTorch backend#16010
Conversation
Signed-off-by: Guiju Zhang <7135567+cascade812@users.noreply.github.com>
📝 WalkthroughWalkthroughRefactors logit post-processing in PyTorchModelEngine into a new ChangesLogits Post-Processing Refactor
BART Test Waiver Cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ModelEngine as PyTorchModelEngine
participant ScheduledRequests
participant ApplyHelper as _apply_logits_processors
ModelEngine->>ScheduledRequests: get context requests
ModelEngine->>ModelEngine: compute beam_width=1, token_ids via get_tokens(0)
ModelEngine->>ApplyHelper: apply processors on context logits slice
ApplyHelper-->>ModelEngine: updated logits_tensor
ModelEngine->>ScheduledRequests: get generation requests
ModelEngine->>ModelEngine: compute per-request beam_width, per-beam token_ids
ModelEngine->>ApplyHelper: apply processors on generation logits slice
ApplyHelper-->>ModelEngine: updated logits_tensor
ModelEngine->>ModelEngine: update logits_row_offset
Possibly related PRs
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/pyexecutor/model_engine.py`:
- Around line 5870-5873: The logit post processor call in `model_engine.py`
accepts processors with either 4 or 5 parameters, but the invocation currently
always passes 5 positional arguments. Update the `lp(...)` call to match the
validated arity by branching on the processor signature (for example, using the
same `lp_params` check around `lp`) so 4-argument processors are called with 4
args and 5-argument processors receive the extra `None` argument.
🪄 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: 2948cce2-6d41-48a7-b0fd-3949364f07ac
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/model_engine.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
/bot run |
|
PR_Github #58033 [ run ] triggered by Bot. Commit: |
|
PR_Github #58033 [ run ] completed with state
|
|
/bot run |
|
PR_Github #58079 [ run ] triggered by Bot. Commit: |
|
PR_Github #58079 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #58091 [ run ] triggered by Bot. Commit: |
|
PR_Github #58091 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #58182 [ run ] triggered by Bot. Commit: |
|
PR_Github #58182 [ run ] completed with state |
…end (NVIDIA#16010) Signed-off-by: Guiju Zhang <7135567+cascade812@users.noreply.github.com>
Summary
Problem
The bug occurs when beam search (
beam_width > 1) is combined with logits post processors. In_execute_logit_post_processors, the PyTorch backend indexed the logits tensor by request index (logits_tensor[idx]), which assumes one logits row per request. With beam search, each generation request contributesbeam_widthrows, so the row indexing was wrong: processors were applied to the wrong rows and only to a single beam.This is why the bart beam2 tests failed while t5 passed — bart registers a
_BartForcedTokensLogitsProcessor(to force decoder start/EOS tokens), while t5 has no logits post processor, so t5 never hit this path.Fix
logits_row_offsetacross context and generation requests, advancing it bybeam_widthper request (1 for context requests), so each request maps to its correct logits rows.beam_widthrows of a generation request at once via the new_apply_logits_processorshelper, passing per-beam token ids (request.get_tokens(beam_idx)), with logits shaped(beam_width, 1, vocab)to match the TRT backend'sLogitsProcessorinterface.