Skip to content

[None][infra] Unwaive perf-sanity disagg gen_only cases - #17350

Merged
chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:user/chenfeiz/unwaive-perf-sanity-gen-only
Aug 7, 2026
Merged

[None][infra] Unwaive perf-sanity disagg gen_only cases#17350
chenfeiz0326 merged 1 commit into
NVIDIA:mainfrom
chenfeiz0326:user/chenfeiz/unwaive-perf-sanity-gen-only

Conversation

@chenfeiz0326

@chenfeiz0326 chenfeiz0326 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Removes all 20 perf/test_perf_sanity.py disagg gen_only SKIP waivers so post-merge re-runs those cases against the now-fixed harness.

Why these were waived

Builds 2873 / 2884 / 2876 recorded results-timeout.xml "Test terminated unexpectedly" for these cases (nvbugs 6541361 / 6542166 / 6561566 / 6550133, and the duplicate 6551021). These were not workload failures — the gen_only benchmark passed end-to-end (all ranks exit 0, "N passed", benchmark_status="Done", 0 failed requests), then the GEN slurm step idled ~76–78 min in teardown until the 90-min stage watchdog killed it.

Root cause (already fixed on main)

wait_for_gen_log_sentinels(), added in 99bdffc4c3 (#16717), polled gen_server_{i}.done bounded by DEFAULT_TIMEOUT (10800s) — longer than the 5400s stage watchdog — so a stuck/un-reaped multi-node gen srun turned an optional log-flush sync into a stage cancellation. A companion defect derived testOutputDir from a positional test-list index that could disagree with the pytest-split shard, so the sentinel was polled in the wrong directory.

Both are merged to main:

Scope / caveat

  • Removes only disagg gen_only perf-sanity waivers (20 lines). Non-gen_only perf-sanity waivers (ctx_only, e2e, aggregated) are left untouched.
  • A few removed lines cite bugs that also tracked genuine gen_only failures (6490049 gen_only Server/Client error; 6374872). If those recur, the post-merge run will re-file an accurate, harness-fix-independent waiver — which is the correct signal, since the current waiver conflates them with the teardown hang.

Testing

Per the NVBugs waiver-removal guidance, the affected post-merge stages must be re-run rather than skipped, e.g.:

/bot run --stage-list "GB200-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-3 GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-3"

(full stage list follows from the 20 unwaived cases).

Dev Engineer Review

  • Removed 20 obsolete gen_only skip waivers from tests/integration/test_lists/waives.txt.
  • The entries cover disaggregated-upload configurations for B200, GB200, and GB300 DeepSeek, GPT-OSS, Kimi, Qwen, and GLM tests.
  • Added 3 GB200 DeepSeek V32 and GPT-OSS configurations with updated bug references.
  • The waiver-file format and scope are consistent with the stated objective.
  • The change has no production code or public API impact.
  • The waivers can be removed because the related harness issues were fixed in #17134 and #17140.

QA Engineer Review

  • Modified tests/integration/test_lists/waives.txt.
  • Removed obsolete disaggregated-upload gen_only entries and added updated GB200 entries.
  • No test-db/ or qa/ files were modified.
  • Verdict: needs follow-up. CBTS coverage data is unavailable.

@coderabbitai

coderabbitai Bot commented Aug 6, 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: 50b3380c-e05d-4a45-8994-2103d11b0ae2

📥 Commits

Reviewing files that changed from the base of the PR and between a6ea52f and 5bf2d46.

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

Walkthrough

Updates disaggregated-upload gen_only waivers in tests/integration/test_lists/waives.txt. The change removes obsolete model entries and adds three GB200 DeepSeek V32 and GPT-OSS entries with updated bug references.

Changes

Performance test waiver updates

Layer / File(s) Summary
Update disaggregated-upload waivers
tests/integration/test_lists/waives.txt
Replaces obsolete waiver entries with two GB200 DeepSeek V32 entries and one GB200 GPT-OSS entry. Each entry retains skip status and uses an updated bug reference.

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

Possibly related PRs

Suggested labels: ci: full pre-merge approved

Suggested reviewers: yiqingy0, yuanjingx87, xinhe-nv

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format and clearly identifies the removal of infrastructure waivers for perf-sanity disaggregated gen_only cases.
Description check ✅ Passed The description explains the affected waivers, root causes, fixes, scope, and testing plan, although it does not use all template headings.
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.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #64279 Bot args parsing error: usage: /bot [-h]
{run,kill,skip,submit,reviewers,reuse-pipeline,reuse-review} ...
/bot: error: unrecognized arguments: These 20 stages are the post-merge disagg gen_only perf-sanity cases unwaived by this PR (one case per stage; -k = pytest-split least_duration group index against tests/integration/defs/.test_durations). Running them here re-validates the teardown-hang fix (#17134 + #17140) against ToT before the waivers are dropped.

Link to invocation

@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-1,DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-3,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-5,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB200-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-3,GB200-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-4,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-3,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-4,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-5,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-6,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-7,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-8,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-1,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-2,GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-4,GB300-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #64283 [ run ] triggered by Bot. Commit: 9b4617a Link to invocation

@BowenFu BowenFu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16 of the 20 removals are well-evidenced and I'd take them as-is — #17134 (721c46b8c2) and #17140 (22184ba7cb) are both on main, the sentinel wait really is bounded at GEN_LOG_SENTINEL_TIMEOUT=120 s well under the 5400 s watchdog and gated on benchmark_mode == "gen_only" (test_perf_sanity.py:112,1337-1376,1527-1530), and the shard/testOutputDir mismatch is fixed in jenkins/scripts/perf/submit.py:235-275,705-744. The scope is honest too: only gen_only disagg lines go, ctx_only/e2e/aggregated stay.

The four that cite 6374872 and 6490049 are a different case, and I don't think the "post-merge will re-file an accurate waiver" argument covers them. #16717 — the commit that introduced the sentinel and that this PR cites as the root cause — says in its own message that those were deliberately kept:

that still regress independent of the parse race (6478615, 6374872, and three under the 6490049 umbrella) also stay waived.

So the author of the harness change had already separated the teardown hang from these four and concluded they fail for their own reasons. Nothing since 99bdffc4c3 touches those four configurations, so 721c46b8c2/22184ba7cb don't establish that they're fixed — removing them re-reds four cases that are already known-red for a cause this PR doesn't address.

Mitigating, and why I'm not making a big deal of it: all four are stage: post_merge (l0_gb200_multi_nodes_perf_sanity_*, l0_gb300_multi_nodes_perf_sanity_*), so nobody's PR gets blocked either way. Suggestion below restores just those four and leaves the other 16 removed.

Comment thread tests/integration/test_lists/waives.txt Outdated
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #64283 [ run ] completed with state FAILURE. Commit: 9b4617a
/LLM/main/L0_MergeRequest_PR pipeline #52182 (Partly Tested) 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

@brnguyen2 brnguyen2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the comments below are optional touch-ups, not blockers.

Confirmed both fixes (721c46b8c, 22184ba7c) are ancestors of this head, and #17140's gating on benchmark_mode == "gen_only" matches the scoping here. Two things before merge:

  • All four cited bugs are still open. Has the /bot run --stage-list re-run actually been executed and passed? Without that, the merged fix is the only evidence and these come back red post-merge.
  • The 6374872 line is the weak one — that bug tracks a capacity-class failure, not the teardown timeout, and no part of #17134/#17140 addresses it. Consider keeping that single waiver until the stage re-run proves otherwise.

The perf-sanity disagg gen_only cases were waived after builds 2873/2884/2876
recorded "Test terminated unexpectedly" (nvbugs 6541361 / 6542166 / 6561566 /
6550133 / 6551021). Those were not workload failures: the gen_only benchmark
PASSED end-to-end (all ranks exit 0, "N passed", benchmark_status="Done"), then
the GEN slurm step idled ~76-78 min in teardown until the 90-min stage watchdog
killed it, emitting results-timeout.xml with an empty passed_test_list.

Root cause was the gen-log sentinel wait added in 99bdffc (NVIDIA#16717):
wait_for_gen_log_sentinels() polled gen_server_{i}.done under a timeout of
DEFAULT_TIMEOUT (10800s), longer than the 5400s stage watchdog, so a stuck /
un-reaped multi-node gen srun turned an optional log-flush sync into a
stage cancellation. A companion defect derived testOutputDir from a positional
test-list index that could disagree with the pytest-split shard, so the
sentinel was polled in the wrong directory.

Both defects are fixed and merged to main:
  - NVIDIA#17134 (721c46b) aligns the launcher's testOutputDir with the pytest
    shard.
  - NVIDIA#17140 (22184ba) bounds the wait to GEN_LOG_SENTINEL_TIMEOUT (120s, well
    below the watchdog), falls back to parsing the current gen log on timeout,
    and gates the whole path on benchmark_mode == "gen_only".

This removes 15 perf/test_perf_sanity.py disagg gen_only SKIP waivers whose
bugs tracked only the harness defect above, so post-merge re-runs those cases
against the fixed harness.

Four gen_only waivers are deliberately KEPT, because their bugs track genuine
workload failures that the harness fixes do not address:
  - gb200_deepseek-v32-fp4_32k4k_con2048_..._eplb288_mtp1 (nvbugs/6374872)
  - gb200_deepseek-v32-fp4_8k1k_con4096_..._eplb256_mtp0  (nvbugs/6490049)
  - gb200_gpt-oss-120b-fp4_8k1k_con4_..._tp4_eplb0_mtp0   (nvbugs/6490049)
  - gb300_kimi-k25-thinking-fp4_8k1k_con1024_..._mtp3     (nvbugs/6490049)

Non-gen_only perf-sanity waivers (ctx_only, e2e, aggregated) are left
untouched. In particular the e2e gb300_kimi-k25-thinking con4096 waiver that
this branch originally removed is no longer part of this change: NVIDIA#16920
(b21a3b4) already unwaived that exact case on main under nvbugs/6490049.

Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
@chenfeiz0326
chenfeiz0326 force-pushed the user/chenfeiz/unwaive-perf-sanity-gen-only branch from 9b4617a to 5bf2d46 Compare August 7, 2026 03:32
@coderabbitai

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

@chenfeiz0326

Copy link
Copy Markdown
Collaborator Author

/bot skip --comment "Only unwaive perf tests, no need to run the whole CI pipeline"

@chenfeiz0326
chenfeiz0326 enabled auto-merge (squash) August 7, 2026 03:38
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #64481 [ skip ] triggered by Bot. Commit: 5bf2d46 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #64481 [ skip ] completed with state SUCCESS. Commit: 5bf2d46
Skipping testing for commit 5bf2d46

Link to invocation

@chenfeiz0326
chenfeiz0326 merged commit 8836813 into NVIDIA:main Aug 7, 2026
11 checks passed
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.

5 participants