[https://nvbugs/6428087][fix] Extend the PP send payload with the flag (backward-compatible 2/3-tuple… - #16145
[https://nvbugs/6428087][fix] Extend the PP send payload with the flag (backward-compatible 2/3-tuple…#16145trtllm-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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR extends the ChangesPP ring broadcast and test waiver
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
NVBug 6428087 is closed as Bug - Fixed. The linked bug appears resolved elsewhere or for a reason that does not prove this PR is redundant. This PR should be judged on its own merits; repair-bot is not auto-closing it. |
The greedy host stop-criteria optimization (PR NVIDIA#15920) added a per-batch flag `use_host_stop_criteria` on `SampleStateTorch` that gates whether `update_requests` uses the host fast path or calls `process_draft_tokens`. In the PP execution loop, `SampleStateTorch` is constructed on non-last PP ranks via `_forward_step_inter_pp` without setting this flag, and only `sample_state.host` and per-request result diffs are shipped via ring send/recv from the last PP rank. Consequently, when the last PP rank determined `use_host_stop_criteria=True` and therefore skipped writing finish_reasons, earlier PP ranks still saw the default False and entered `process_draft_tokens`, which then indexed an empty finish_reasons list and raised `IndexError: list index out of range` from `finish_if_reason`. Extend the PP send payload with the flag (backward-compatible: recv accepts both 2- and 3-tuple payloads, and send only appends the flag when the sampler exposes the attribute) so that all ranks pick the same branch. Also remove the associated waiver. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
c9b9bea to
c180e55
Compare
|
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. |
| self.send_handles[microbatch_id] = self.dist.isend_object( | ||
| (sample_state.host, py_result_diffs), | ||
| (sample_state.host, py_result_diffs, | ||
| getattr(sample_state, "use_host_stop_criteria", None)), |
There was a problem hiding this comment.
use_host_stop_criteria does not exist on any SampleState class (nor at this PR's merge-base), so this getattr is always None, the receiver's guard never fires, and the change is a no-op -- while the waiver is removed, so the test will fail again as soon as it runs.
The flag you want is SampleStateTorch.single_step_greedy (sampler.py:1203): when set, sample_async skips write_finish_reasons (sampler.py:3045), and non-last ranks keep the False default from _forward_step_inter_pp, so they take the slow path and index an empty finish_reasons_list(). Please send that field and assign it unconditionally on recv; note SampleStateTRTLLM lacks it, so it probably belongs on the base SampleState.
brnguyen2
left a comment
There was a problem hiding this comment.
SampleStateTorch has no use_host_stop_criteria field — grep -rn use_host_stop_criteria matches only the five lines this PR adds. getattr(sample_state, "use_host_stop_criteria", None) therefore always sends None, the is not None guard never fires, and the patch is a behavioral no-op that still un-waives the test.
The flag that actually gates the two _update_requests branches is SampleStateTorch.single_step_greedy (sampler.py:1203, consumed at sampler.py:2751) — that's the one PP drops. Please re-verify the root cause before re-running the test stage; as written the waiver removal has nothing behind it.
| # SampleStateTorch carries a ``use_host_stop_criteria`` flag | ||
| # decided on the last PP rank; propagate it so ``update_requests`` | ||
| # picks the same branch on all ranks. Other samplers ship None. | ||
| sample_state.host, py_result_diffs, use_host_stop_criteria = self.dist.recv_object( |
There was a problem hiding this comment.
Two problems here beyond the missing attribute:
- The unpack is a strict 3-tuple, so this is not "backward-compatible 2/3-tuple handling" as the description claims — a 2-tuple payload raises
ValueError. (Fine in practice since all ranks run the same build, but the description and the code disagree.) - Sending
Noneas an in-band "field absent" sentinel and usinggetattr(..., None)on the send side hides exactly the bug this PR has: a typo'd or nonexistent attribute name silently degrades to the old behavior instead of failing. Reference the dataclass field directly (sample_state.single_step_greedy) so an attribute mismatch is anAttributeError, and branch onisinstance(sample_state, SampleStateTorch)if other sampler state types need to skip it.
StanleySun639
left a comment
There was a problem hiding this comment.
LGTM if the target test case can pass.
Summary
Test plan
Links
Dev Engineer Review
SampleStateTorch.use_host_stop_criteriaacross pipeline-parallel ranks.QA Engineer Review
tests/integration/test_lists/waives.txtchanged.TestDeepSeekV3Lite::test_nvfp4_4gpusCUTEDSL/tp2pp2 configuration linked to NVBug 6428087.test-db/orqa/files were modified.