[None][fix] Propagate host stop criteria across PP ranks - #16096
[None][fix] Propagate host stop criteria across PP ranks#16096mingyangHao wants to merge 3 commits into
Conversation
|
/bot run |
|
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)
📝 WalkthroughWalkthroughThe ChangesHost stop-criteria flag relocation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #58132 [ run ] triggered by Bot. Commit: |
|
PR_Github #58132 [ run ] completed with state
|
chienchunhung
left a comment
There was a problem hiding this comment.
Thanks for the fix!
|
/bot run |
|
PR_Github #58162 [ run ] triggered by Bot. Commit: |
|
PR_Github #58162 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #58241 [ run ] triggered by Bot. Commit: |
|
PR_Github #58241 [ run ] completed with state |
841ce21 to
8d0cbb0
Compare
Reland the host-side greedy stop-criteria fast path from NVIDIA#15920 after its temporary revert in NVIDIA#16163. Signed-off-by: Mingyang Hao <200044211+mingyangHao@users.noreply.github.com>
Signed-off-by: Mingyang Hao <200044211+mingyangHao@users.noreply.github.com>
87b65a1 to
c0c45c7
Compare
Remove the PP sampler waivers introduced by NVIDIA#16103, NVIDIA#16105, NVIDIA#16127, and NVIDIA#16146. Also remove overlapping PP and gpu2 waivers from NVIDIA#16109, plus the PP2 waiver added by NVIDIA#16169, so the restored pre-NVIDIA#15920 sampler path is exercised in CI. Signed-off-by: Mingyang Hao <200044211+mingyangHao@users.noreply.github.com>
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #58414 [ run ] triggered by Bot. Commit: |
|
PR_Github #58414 [ run ] completed with state
|
Description
PR #15920 moved greedy stop-criteria checks to the host. In pipeline-parallel execution,
use_host_stop_criteriawasstored on the outer
SampleStateTorch, while_ring_broadcast_sample_state()only broadcastssample_state.host.As a result, non-last PP ranks reconstructed the sample state with
use_host_stop_criteria=Falseand attempted to processdevice finish reasons, even though the host fast path intentionally leaves
finish_reasonsunset.This change:
use_host_stop_criteriaintoSampleStateTensorsHostTorch, which is already broadcast between PP ranks.update_requests()to read the flag from the broadcast host state.Performance Impact
No measurable performance impact is expected. This change adds only one boolean to the existing serialized PP payload and
introduces no additional CUDA kernels, synchronization, device-to-host copies, or collectives. The host stop-criteria fast
path introduced by #15920 remains unchanged.
Validation
Passed:
TestFinishReasons::test_host_stop_criteria_fast_path— 4 casestest_tinyllama_logits_processor_2gpu[1-2]on 2× B200test_tinyllama_logits_processor_tp2pp2on 4× B200Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Summary by CodeRabbit