Skip to content

[https://nvbugs/6428069][chore] Unwaive the pp=2 disagg tests - #18135

Open
lori-ren wants to merge 5 commits into
NVIDIA:mainfrom
lori-ren:chore/unwaive-host-stop-criteria-bugs
Open

[https://nvbugs/6428069][chore] Unwaive the pp=2 disagg tests#18135
lori-ren wants to merge 5 commits into
NVIDIA:mainfrom
lori-ren:chore/unwaive-host-stop-criteria-bugs

Conversation

@lori-ren

@lori-ren lori-ren commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Description

use_host_stop_criteria (added by #15920) was reverted wholesale by #16163, so the mechanism
behind nvbugs/6428069 no longer exists — grep -rn use_host_stop_criteria returns nothing on
main. The 8 waivers it left behind have been skipping these cases since 2026-07-08.

Un-waiving all 8 was tried first. CI build #56392 ran them on
DGX_H200-8_GPUs-PyTorch-Post-Merge-1 and split them exactly by pipeline-parallel degree:

shape cases result
pp = 2 test_tp_pp_symmetric[GSM8K-tp2pp2], [MMLU-tp2pp2], test_disaggregated_ctxtp2pp2_gentp2pp2 3/3 passed
pp = 4 test_ctx_pp_gen_tp_asymmetric[…ctx_pp=4] ×4, test_disaggregated_ctxpp4_genpp4 5/5 failed

This PR now un-waives only the 3 that CI confirmed passing. The 5 pp=4 lines stay
waived.

The pp=4 failures are a different defect from 6428069: a 300 s HangDetector fire with the
NIXL KV transfer never completing. IndexError, finish_reasons and use_host_stop_criteria
appear 0 times across all five logs. Per-rank stacks show 3 of 4 context ranks blocked in
a world-size allgather inside _gen_consensus (transceiver.py:491) while the 4th is blocked
in wait_on_pp_send_handles (py_executor.py:3287) — a collective-participation deadlock
that a deeper pipeline makes reachable. Those 5 lines still cite 6428069 and should be
re-pointed at a bug for that hang.

Test Coverage

The three un-waived cases, on DGX_H200-8_GPUs-PyTorch-Post-Merge-1:

  • accuracy/test_disaggregated_serving.py::TestLlama3_1_8BInstruct::test_tp_pp_symmetric[GSM8K-tp2pp2]
  • accuracy/test_disaggregated_serving.py::TestLlama3_1_8BInstruct::test_tp_pp_symmetric[MMLU-tp2pp2]
  • disaggregated/test_disaggregated.py::test_disaggregated_ctxtp2pp2_gentp2pp2[TinyLlama-1.1B-Chat-v1.0]

All three passed in build #56392. Re-run with
/bot run --extra-stage "DGX_H200-8_GPUs-PyTorch-Post-Merge-1" (needs the
ci: full pre-merge approved label, or the multi-GPU stages are skipped).

Note for reviewers

waives.txt has neighbouring entries under nvbugs/6655360, 6644475 and 6611817 covering
other variants of these same test functions. Those are a different defect again — a
disaggregated-server hang/timeout at launch on H100 4-GPU — and are intentionally left alone.

PR Checklist

  • 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.

Dev Engineer Review

  • Removed three confirmed-passing pp=2 disaggregated test waivers.
  • Limited the change to tests/integration/test_lists/waives.txt.
  • Kept five pp=4 waivers for separate NIXL KV-transfer hangs.
  • No code, API, configuration, or error-handling changes were made.
  • The waiver format and scope are consistent.
  • No duplicate or invalid entries were identified.

QA Engineer Review

  • Modified tests/integration/test_lists/waives.txt.
  • Removed two Llama 3.1 8B skips and one TinyLlama skip.
  • No test-db/ or qa/ files were modified.
  • No test functions changed.
  • CI confirmed the three pp=2 tests pass after use_host_stop_criteria was removed.
  • CBTS coverage data is unavailable.

Verdict: needs follow-up

@coderabbitai

coderabbitai Bot commented Aug 24, 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: 8ae4e066-aed0-45f8-8c75-95dd15bb1085

📥 Commits

Reviewing files that changed from the base of the PR and between c77f319 and 3034ef9.

📒 Files selected for processing (1)
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The integration waiver list removes two Llama 3.1 8B symmetric skips and one TinyLlama disaggregated-serving skip. The Llama MMLU tp2pp1 skip remains.

Changes

Integration test waiver updates

Layer / File(s) Summary
Remove obsolete generation waivers
tests/integration/test_lists/waives.txt
Removes the Llama 3.1 8B symmetric GSM8K and MMLU tp2pp2 skips, plus the TinyLlama disaggregated context, tensor, and pipeline-parallel skip.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 3034e

This change re-enables three previously waived pp=2 tests without altering product or runtime behavior; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required NVBugs and type format. It clearly identifies the primary change: unwaiving the pp=2 disaggregated tests.
Description check ✅ Passed The description explains the reason for the change, identifies the three tests being unwaived, documents CI coverage and results, and explains why the five pp=4 waivers remain. It includes the require…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
Full details: Description check

Explanation

The description explains the reason for the change, identifies the three tests being unwaived, documents CI coverage and results, and explains why the five pp=4 waivers remain. It includes the required Description, Test Coverage, and PR Checklist sections.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Signed-off-by: Lori Ren <lorir@nvidia.com>

# Conflicts:
#	tests/integration/test_lists/waives.txt
@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --extra-stage "DGX_H200-8_GPUs-PyTorch-Post-Merge-1"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68795 [ run ] triggered by Bot. Commit: e55eb41 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68795 [ run ] completed with state SUCCESS. Commit: e55eb41
/LLM/main/L0_MergeRequest_PR pipeline #56195 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --extra-stage "DGX_H200-8_GPUs-PyTorch-Post-Merge-1" --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68949 [ run ] triggered by Bot. Commit: e55eb41 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68949 [ run ] completed with state FAILURE. Commit: e55eb41
/LLM/main/L0_MergeRequest_PR pipeline #56332 completed with status: 'UNSTABLE'

CI Report

⚠️ Multi-GPU Label Required:
Multi-GPU tests require the ci: full pre-merge approved label on this PR. Ask a member of NVIDIA/trt-llm-ci-approvers to add the label, then re-trigger CI with the same bot command (no rebase needed).

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --extra-stage "DGX_H200-8_GPUs-PyTorch-Post-Merge-1" --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69010 [ run ] triggered by Bot. Commit: e55eb41 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69010 [ run ] completed with state SUCCESS. Commit: e55eb41
/LLM/main/L0_MergeRequest_PR pipeline #56392 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

…nly the pp=2 cases CI confirmed

Signed-off-by: Lori Ren <lorir@nvidia.com>
Signed-off-by: Lori Ren <lorir@nvidia.com>

# Conflicts:
#	tests/integration/test_lists/waives.txt
@lori-ren lori-ren changed the title [https://nvbugs/6428069][chore] Unwaive nvbug/6428069 related tests [https://nvbugs/6428069][chore] Unwaive the pp=2 disagg tests confirmed passing by CI Aug 27, 2026
@coderabbitai

coderabbitai Bot commented Aug 27, 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.

@lori-ren lori-ren changed the title [https://nvbugs/6428069][chore] Unwaive the pp=2 disagg tests confirmed passing by CI [https://nvbugs/6428069][chore] Unwaive the pp=2 disagg tests Aug 27, 2026
@coderabbitai

coderabbitai Bot commented Aug 31, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants