[https://nvbugs/6428124][fix] Include use_host_stop_criteria in the PP send/recv payload in… - #16134
[https://nvbugs/6428124][fix] Include use_host_stop_criteria in the PP send/recv payload in…#16134trtllm-agent wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughModified the pipeline-parallel ring broadcast of ChangesPP Ring Broadcast Flag Propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PrevPPRank
participant CurrentPPRank
participant NextPPRank
PrevPPRank->>CurrentPPRank: recv_object(host, py_result_diffs, use_host_stop_criteria)
CurrentPPRank->>CurrentPPRank: set sample_state.use_host_stop_criteria if attribute exists
CurrentPPRank->>NextPPRank: isend_object(host, py_result_diffs, use_host_stop_criteria)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
A fix was being worked on: #16096 |
26e810c to
8d406d5
Compare
8d406d5 to
72fdc5d
Compare
BowenFu
left a comment
There was a problem hiding this comment.
LGTM — adds use_host_stop_criteria to the PP ring send/recv payload symmetrically (send uses getattr(...,False), recv guards with hasattr); only affects multi-PP-rank sample-state broadcast, and PP=1/default bypasses the loop entirely.
72fdc5d to
d8f8ccf
Compare
Nemotron-H LoRA PP=2 crashed with IndexError in finish_if_reason because the last PP rank produced a SampleStateTorch with use_host_stop_criteria=True (and host.finish_reasons=None), but only sample_state.host was sent through _ring_broadcast_sample_state. Non-last ranks kept the constructor default False and tried to index into an empty finish_reasons list from process_draft_tokens. Include the flag in the PP send/recv payload and restore it on the receiver. Use getattr/hasattr to stay compatible with the TRT-LLM sampler flavor which lacks this field. Remove the corresponding waiver. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Signed-off-by: handongl <handongl@nvidia.com>
d8f8ccf to
812d4e0
Compare
Summary
Test plan
Links
Summary by CodeRabbit
Bug Fixes
Chores