Skip to content

[https://nvbugs/6428063][fix] Include use_host_stop_criteria in the PP ring-broadcast payload (sender uses… - #16136

Open
trtllm-agent wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6428063
Open

[https://nvbugs/6428063][fix] Include use_host_stop_criteria in the PP ring-broadcast payload (sender uses…#16136
trtllm-agent wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6428063

Conversation

@trtllm-agent

@trtllm-agent trtllm-agent commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause: PR [None][perf] Move greedy stop checks to host #15920's new SampleStateTorch.use_host_stop_criteria flag was set only on the last PP rank; non-last PP ranks received sample_state.host with finish_reasons=None but kept the local default False, hitting the process_draft_tokens branch which indexed an empty list.
  • Fix: Include use_host_stop_criteria in the PP ring-broadcast payload (sender uses getattr default False; receiver assigns via hasattr guard) so all PP ranks agree on the sampler fast-path decision; also removed the corresponding waiver line.
  • Automated fix generated by repair-bot

Test plan

  • Verify fix on the same GPU type as the original failure
  • Check for regressions in related tests

Links

Dev Engineer Review

  • Updated the PP ring-broadcast payload to preserve SampleStateTorch.use_host_stop_criteria across pipeline-parallel ranks.
  • Used getattr(..., False) for send-side compatibility and an hasattr guard on receive.
  • Removed the related integration-test waiver.
  • No exported or public API declarations changed.
  • The changes are consistent with the reported failure and do not introduce an unintended scope change.

QA Engineer Review

  • Modified tests/integration/test_lists/waives.txt.
  • Removed the waiver for accuracy/test_llm_api_pytorch.py::TestDeepSeekV3Lite::test_nvfp4_4gpus, tracked by nvbugs/6428063.
  • No test-db or QA entries were modified.
  • Verdict: needs follow-up because CBTS coverage data is unavailable.

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 42cbe468-402a-45d1-9344-3b46128a31b4

📥 Commits

Reviewing files that changed from the base of the PR and between 71f025e and 5dcd222.

📒 Files selected for processing (2)
  • tensorrt_llm/_torch/pyexecutor/py_executor.py
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (1)
  • tensorrt_llm/_torch/pyexecutor/py_executor.py

Walkthrough

This PR adds use_host_stop_criteria to the PP ring broadcast payload and removes an obsolete integration-test waiver.

Changes

Ring Broadcast Protocol Update

Layer / File(s) Summary
Propagate use_host_stop_criteria in ring broadcast
tensorrt_llm/_torch/pyexecutor/py_executor.py
The receive path unpacks and applies use_host_stop_criteria. The send path includes the value and defaults to False when unavailable.

Test Waiver Cleanup

Layer / File(s) Summary
Remove obsolete waiver
tests/integration/test_lists/waives.txt
Removes the waiver for TestDeepSeekV3Lite::test_nvfp4_4gpus referencing nvbugs/6428063.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 5dcd2

The change synchronizes sampler stop-criteria behavior across pipeline-parallel ranks and removes the related waiver; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: lancelly

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title identifies the bug, fix type, tracking ID, and primary change to the PP ring-broadcast payload.
Description check ✅ Passed The description explains the root cause, fix, test plan, and bug link; the repository checklist is not included.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@trtllm-agent
trtllm-agent force-pushed the repair-bot-bug6428063 branch 3 times, most recently from 41705fc to e2bfdb4 Compare July 13, 2026 17:45
PR NVIDIA#15920 introduced a fast path in TorchSampler that skips writing
finish_reasons to host when all requests are greedy, single-beam, and
have no stop words. In pipeline-parallel mode with disable_overlap_scheduler=False,
only the last PP rank runs sample_async and knows the fast path was taken;
non-last PP ranks construct SampleState in _forward_step_inter_pp with the
default use_host_stop_criteria=False. The ring-broadcast payload sent only
sample_state.host, so non-last ranks ended up with finish_reasons=None but
use_host_stop_criteria=False, falling through to process_draft_tokens ->
finish_if_reason which indexed an empty list and raised IndexError.

Include use_host_stop_criteria in the PP ring-broadcast payload so all ranks
agree on which update_requests branch to take. Guard with getattr/hasattr so
SampleStateTRTLLM (which lacks this field) is unaffected. Remove the waiver
for the affected test.

Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants