Skip to content

[https://nvbugs/6472256][fix] Fix disagg stress cluster flapping and DeepSeek R1 FP4 ctx OOM; add aiperf error-rate gate - #17427

Open
moraxu wants to merge 9 commits into
NVIDIA:mainfrom
moraxu:dev-mguzek-nvbug-6472256-disagg-stress-test
Open

[https://nvbugs/6472256][fix] Fix disagg stress cluster flapping and DeepSeek R1 FP4 ctx OOM; add aiperf error-rate gate#17427
moraxu wants to merge 9 commits into
NVIDIA:mainfrom
moraxu:dev-mguzek-nvbug-6472256-disagg-stress-test

Conversation

@moraxu

@moraxu moraxu commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Dev Engineer Review

  • Stress runners use production discovery timings.
  • enforce_aiperf_error_rate() validates exports, handles cancellations, excludes non-request records, and checks expected records.
  • DeepSeek R1 FP4 context configurations set max_num_tokens to 8320 to reduce transient workspace OOM risk.
  • B200 waivers were removed for the DeepSeek R1 FP4 and Qwen3 32B FP8 stress tests.
  • No configuration or API consistency issues were identified.

QA Engineer Review

  • Added GPU-free tests for error storms, cancellations, invalid exports, incomplete accounting, metadata fallback, disabled checks, and record filtering.
  • Registered all 11 aiperf gate tests in test-db/l0_cpu.yml and qa/llm_function_core.txt.
  • The affected B200 stress tests are no longer waived.
  • The reported 35,000-request run had 0 errors, 0 OOMs, and GSM8K accuracy of 0.9568.
  • Verdict: sufficient.

Description

Root cause & fix (two independent issues).

  1. The reported failure ("Cluster is not ready" 500-storm, 2026-07-17 run): spurious cluster-membership flapping. The CI harness injected heartbeat_interval_sec=1 / inactive_timeout_sec=2 (production defaults: 5/10) while the worker heartbeat task, the cluster-storage /expire handler, and the expiry sweep all share asyncio event loops saturated at 512-concurrency; any >1s stall spuriously expired a worker, flipping is_ready() false, and closed-loop clients amplified each window into thousands of fast 500s. Since [https://nvbugs/6440089][test] Rework disagg mixed-stress accuracy metric and harden the test #16445 removed the log patterns and the test was waived, a recurrence would have passed silently (the aiperf path had no error-rate gate). Fix from this PR: stress runners now use production discovery timings; a non-cancellation error-rate gate on aiperf's per-record export restores coverage; the B200 deepseek_r1_v2_fp4_stress and qwen3_32b_fp8_stress waives (both under this NVBug) are removed. A dedicated qwen3_32b_fp8_stress validation run on 8xB200 is queued; the H100 waive for that test stays (separate bug, nvbugs/6312828).

  2. Found while validating: deterministic ctx-worker CUDA OOM on current main. Bisect across rc21/rc22/rc23/rc24 wheels, 4 targeted commit reverts, and era-matched deps (13 probes) proved it is not a code regression: the config is memory-borderline on 192GB B200 — weights 107.5 GiB/GPU + KV pool (0.8 fraction) + ~7 GiB CUDA/NCCL leave less headroom than real serving transients (~15–18 GiB, dominated by a 6.3–8 GiB TRTLLM-Gen FP4 MoE workspace at 16640-token chunks), while KV-pool sizing only observes a 10.77 GiB dynamic peak during estimation. Shortfall at OOM: 1–4 GiB; the 2026-07-17 pass was environment margin. Fix from this PR: ctx moe_config.max_num_tokens: 8320 halves the MoE transient, applied to both the non-MTP and MTP DeepSeek R1 FP4 disagg configs (identical ctx sizing carries the same transient). Validation: full 35,000-request stress run passed — 0 errors, 0 OOM, GSM8K 0.9568.

  3. Review-driven hardening of the error-rate gate: a broken export can no longer read as a clean pass (empty exports, wholesale JSON parse failures, all-cancelled record sets, and record counts below 0.9x the requested load all raise); cancellation detection falls back to the per-record metadata.was_cancelled field so aiperf error-shape drift cannot reclassify the ~10% intentional cancellations as server errors; non-request records are excluded from the denominator; when --request-count is not passed, the expected record count is derived as max(10, concurrency * 2) mirroring aiperf 0.8.0; the validated record schema is pinned to the aiperf==0.8.0 requirements-dev pin in the gate docstring.

Test Coverage

  • 11 GPU-free unit tests in tests/integration/defs/disaggregated/test_aiperf_gate.py, registered in test-db/l0_cpu.yml (pre-merge) and qa/llm_function_core.txt. They feed the gate synthetic profile_export.jsonl cases, including a replay of the original nvbugs/6472256 500-storm (fires at 13.6% vs the 5% threshold), a healthy run with 10% cancellations (passes), and the empty/corrupt/all-cancelled/incomplete/schema-drift failure modes.
  • Full-scale validation on 8xB200: test_disaggregated_stress_test[input8k-output1k-conc512-deepseek_r1_v2_fp4_stress], 35,000 requests — 1 passed in 1:28:52, [aiperf-gate] non-cancellation errors: 0/31561 (0.00%), cancelled: 3439 (~3500 expected at cancellation_rate=10), GSM8K accuracy 0.9568 (threshold 0.92), zero OOM, zero "Cluster is not ready".
  • qwen3_32b_fp8_stress B200 validation run queued; result will be posted on the review thread before merge.

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-compatible or api-breaking. For api-breaking, include BREAKING in 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.

Signed-off-by: Michal Guzek <mguzek@nvidia.com>
@moraxu

moraxu commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Disaggregated tests now use production service-discovery timings, support cluster overrides, report fatal-log context, and enforce aiperf error-rate limits. Two FP4 configurations limit context-server tokens to 8320. Two B200 stress-test waivers are removed.

Changes

Disaggregated test reliability

Layer / File(s) Summary
Production discovery settings
tests/integration/defs/disaggregated/test_disaggregated.py
Cluster setup accepts service-discovery overrides. Aiperf and mixed-stress paths use 5-second heartbeats and a 10-second inactive timeout.
Aiperf validation and diagnostics
tests/integration/defs/disaggregated/test_disaggregated.py, tests/integration/defs/disaggregated/test_aiperf_gate.py, tests/integration/test_lists/qa/llm_function_core.txt, tests/integration/test_lists/test-db/l0_cpu.yml
Aiperf exports are checked for completeness and error rate. Intentional cancellations and metadata-only records are excluded. Tests cover malformed exports, truncated lines, cancellation metadata, and threshold handling. Fatal-log output includes bounded context.
FP4 stress readiness
tests/integration/defs/disaggregated/test_configs/*deepseek_r1_v2_fp4_tllm*.yaml, tests/integration/defs/disaggregated/test_disaggregated.py, tests/integration/test_lists/waives.txt
Both FP4 context-server configurations set max_num_tokens to 8320. The KV-overflow regression disables aiperf error-rate enforcement. The DeepSeek R1 v2 FP4 and Qwen3 32B FP8 B200 waivers are removed.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant run_disaggregated_aiperf
  participant fatal_log_scanner
  participant enforce_aiperf_error_rate
  run_disaggregated_aiperf->>fatal_log_scanner: scan saved logs
  fatal_log_scanner-->>run_disaggregated_aiperf: return first fatal-log context
  run_disaggregated_aiperf->>enforce_aiperf_error_rate: validate request errors
  enforce_aiperf_error_rate-->>run_disaggregated_aiperf: pass or raise threshold failure
Loading

Suggested labels: ci: full pre-merge approved

Suggested reviewers: schetlur-nv, qijune, bowenfu

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the cluster-flapping fix, DeepSeek R1 FP4 context OOM fix, and aiperf error-rate gate.
Description check ✅ Passed The description includes complete Description, Test Coverage, and PR Checklist sections with detailed rationale, changes, and validation results.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/integration/defs/disaggregated/test_disaggregated.py`:
- Around line 2385-2389: Replace the assert checking export_path in the
disaggregated test with an explicit FileNotFoundError, preserving the existing
diagnostic message and path context so missing exports fail immediately
regardless of Python optimization.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 11b7339f-4141-4c3e-a8b6-0b61bafffe4e

📥 Commits

Reviewing files that changed from the base of the PR and between 81db688 and e5a3c90.

📒 Files selected for processing (3)
  • tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm.yaml
  • tests/integration/defs/disaggregated/test_disaggregated.py
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt

Comment thread tests/integration/defs/disaggregated/test_disaggregated.py Outdated
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #64668 [ run ] triggered by Bot. Commit: e5a3c90 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.

The root-cause write-up is excellent and the cluster-timing fix looks right — the harness was the only thing injecting 1s/2s, and every stress path now goes through PRODUCTION_CLUSTER_TIMINGS.

Two scope concerns worth settling before merge:

The error-rate gate is on by default for every stress param, but only one was validated. max_error_rate=0.05 now applies to all nine test_disaggregated_stress_test configs (gpt-oss ×4, GLM-5, Qwen3.5-4B, Qwen3-32B, DeepSeek MTP) plus the unwaived non-MTP DeepSeek run, and all of them pass cancellation_rate=10. If aiperf classifies a cancelled request as anything other than code == 499 / type == "RequestCancellationError" — no error object at all, or a different type string — 10% of requests land in the numerator and every stress test fails at once. Please paste the [aiperf-gate] line from the 35k validation run so the cancelled: count can be confirmed at ~3500, and pin the aiperf version this record schema comes from. Also worth noting the gate has never been observed firing; a small unit test feeding enforce_aiperf_error_rate a synthetic profile_export.jsonl (clean / all-cancelled / 500-storm) would prove it catches the failure it exists for, and costs no GPU.

The two DeepSeek configs have diverged. disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yaml has the same ctx sizing (max_num_tokens: 16640, 0.8 KV fraction, TRTLLM MoE) and does not get the chunking. If the analysis holds it is exposed to the same transient, and it is not waived.

Description hygiene: the description covers the diff accurately; test-only change, so no docs/changelog owed.

Comment thread tests/integration/defs/disaggregated/test_disaggregated.py
Comment thread tests/integration/defs/disaggregated/test_disaggregated.py
Comment thread tests/integration/test_lists/waives.txt Outdated
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #64668 [ run ] completed with state ABORTED. Commit: e5a3c90

Link to invocation

@fredricz-20070104 fredricz-20070104 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.

Review summary - CONCERNS

Verdict: Test-only change with a solid root-cause write-up; the timing fix looks right, but the error-rate gate is enabled by default across many stress configs while only one was validated, so it should not merge until that blast radius is confirmed safe.

Concerns

  1. [MAJOR] tests/integration/defs/disaggregated/test_disaggregated.py:2454 - error-rate gate on by default for every stress config, only one validated

    • What is wrong: run_disaggregated_aiperf now defaults max_error_rate=0.05, and enforce_aiperf_error_rate only excludes a request from the numerator when error.code == 499 OR error.type == "RequestCancellationError". This default now applies to all stress configs (gpt-oss x4, GLM-5, Qwen3.5-4B, Qwen3-32B, DeepSeek MTP, plus the unwaived DeepSeek non-MTP), every one running cancellation_rate=10 (~10% intentionally cancelled).
    • How it fails: if the aiperf version in CI records a cancelled request with any other schema (no error object, a null code, or a different type string), those ~10% cancellations land in the numerator, error_rate ~= 0.10 > 0.05, and every stress test fails at once with an AssertionError. Only one config was validated end-to-end.
    • Suggested fix: paste the [aiperf-gate] line from the 35k validation run (confirm cancelled ~= 3500 and errors == 0), pin the aiperf version this record schema comes from, and widen cancellation detection (also treat status/http 499 and message-based cancellation) or enable the gate per-config until the schema is confirmed for each.
  2. [MAJOR] tests/integration/defs/disaggregated/test_disaggregated.py:2411 - empty / all-cancelled export passes the gate silently

    • What is wrong: the function returns early when considered <= 0.
    • How it fails: a catastrophic run producing an empty profile_export.jsonl (file exists, zero request records) or a run where every record is classified as cancelled yields total == 0/considered == 0 and returns without gating — the exact 500-storm/total-failure case the gate exists to catch passes silently.
    • Suggested fix: distinguish "no records at all" from "all cancelled" and fail when total is 0 or considered is implausibly small relative to the requested load.

Minor notes (non-blocking)

  • tests/integration/defs/disaggregated/test_disaggregated.py:2385 - assert os.path.exists(export_path) is stripped under python -O; use an explicit FileNotFoundError preserving the diagnostic message (also flagged by CodeRabbit).
  • tests/integration/defs/disaggregated/test_disaggregated.py:2401 - total counts every JSONL line; if the export contains non-request metadata records the denominator is inflated and the rate under-reported. Filter to request records or confirm the export is request-only.

QA view

  • Test coverage: partial - the changes are all test infra; the new enforce_aiperf_error_rate gate has no unit test. A cheap synthetic-JSONL test (clean / all-cancelled / 500-storm / empty) would prove it catches the failure it exists for and expose the cancellation-schema risk above. The gate has reportedly never fired.
  • SM coverage: architecture-aligned - the OOM/chunking fix is B200-192GB (sm100, nvfp4) specific and the unwaived stress test runs on B200 (skip_pre_blackwell, full:B200). No cross-arch gap on the changed path.
  • Test code: assert-optimized-out; default gate applied globally with only one config validated; silent pass on empty export; no unit test for the gate; the DeepSeek MTP sibling config (..._mtp.yaml) keeps identical ctx sizing but gets neither the chunking nor a waiver, so it is exposed to the same OOM.
  • Test time: significant - waives.txt re-enables a full 35,000-request 512-concurrency B200 stress run; exact runtime not measurable from the diff.
  • Needs /qa-verify: yes - unwaive + never-fired default-on gate + shared test-infra change; QA should confirm the [aiperf-gate] output (cancelled ~= 3500, non-cancellation errors 0) on DeepSeek and at least one gpt-oss config and confirm no spurious failures from the 10% cancellation traffic.

Does this actually fix nvbugs/6472256?

Partial. The 'Cluster is not ready' storm was triggered by the harness injecting 1s/2s discovery timings that spuriously expired workers under 512-concurrency saturation; routing stress runs through PRODUCTION_CLUSTER_TIMINGS (5s/10s) removes that harness-injected trigger and the new gate prevents a silent recurrence, so the unwaived DeepSeek case is addressed. Still uncovered: (a) the product behaviour under genuine event-loop saturation is unchanged - the harness simply stops artificially inducing it; (b) qwen3_32b_fp8_stress remains waived under the same bug id though the timing fix is claimed to cover it; (c) the DeepSeek MTP config is neither chunked nor waived.

Possible new issues

  • Mass spurious CI failures across all stress configs if aiperf's cancelled-request schema differs from the two checked keys (see concern 1).
  • Empty/all-cancelled export bypasses the gate (concern 2).
  • Re-enabling the 35k stress test lengthens the B200 suite.

What I could not verify

  • The exact aiperf profile_export.jsonl record schema for the pinned version - whether cancelled requests always carry code==499 or type=="RequestCancellationError", and whether the file contains non-request metadata lines.
  • The full set of run_disaggregated_aiperf callers and which stress params actually route through it vs run_disaggregated_mixed_stress.
  • The DeepSeek MTP config contents (not in this diff) and whether it is exercised in the same B200 list.

Automated review by NVCortex Lite, run by @fredricz-20070104.

@fredricz-20070104 fredricz-20070104 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.

Review summary - Approve (non-blocking)

Approving so this is not blocked on me. The points raised in my review comment above are non-blocking — please read them and address what you agree with before merging.

Worth doing before this is relied on: Bug fix that unwaives a stress test, adds a never-fired error-rate gate applied by default to many configs, and modifies shared test infrastructure. A human QA should confirm the [aiperf-gate] output on the pinned aiperf version (cancelled count ~= 3500, non-cancellation errors 0) across at least the DeepSeek and one gpt-oss config, and verify the gate does not spuriously fail on the 10% cancellation traffic.

Automated review by NVCortex Lite, run by @fredricz-20070104.

moraxu added 4 commits August 10, 2026 15:44
…erf export instead of assert

Addresses CodeRabbit review on NVIDIA#17427: assert statements are stripped
under python -O, which would defer a missing-export failure to open()
without the intended diagnostic.

Signed-off-by: Michal Guzek <mguzek@nvidia.com>
…E in MTP config too

Addresses brnguyen2's review on NVIDIA#17427:
- The gate no longer degrades into a silent pass on a broken export:
  empty exports, wholesale JSON parse failures, all-cancelled record
  sets, and record counts far below the requested load now raise, with
  decode failures counted and reported in the [aiperf-gate] line.
- Docstring pins the validated record schema to aiperf==0.8.0 (the
  requirements-dev.txt pin) and records the reference gate output from
  the 35000-request DeepSeek R1 FP4 validation run
  (0/31561 non-cancellation errors, cancelled: 3439 vs ~3500 expected).
- GPU-free unit tests (test_aiperf_gate.py) feed the gate synthetic
  profile_export.jsonl cases: the nvbugs/6472256 500-storm replay
  (fires), healthy run with cancellations (passes), and the
  empty/corrupt/all-cancelled/incomplete failure modes.
- disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yaml gets the
  same ctx moe_config.max_num_tokens chunking as the non-MTP config: its
  ctx sizing is identical, so it carries the same 7-8 GiB TRTLLM-Gen FP4
  MoE workspace transient and is a live OOM candidate on 192GB B200.

Signed-off-by: Michal Guzek <mguzek@nvidia.com>
…clude non-request records

Addresses fredricz-20070104's review on NVIDIA#17427 (the blast-radius concern
for the default-on gate across all stress configs at cancellation_rate=10):
- Cancellation classification no longer hinges solely on
  error.code == 499 / error.type == "RequestCancellationError": the
  per-record metadata.was_cancelled field is honored as a fallback, so a
  future aiperf that records cancellations with a different error shape
  (or no error object) cannot push ~10% intentional cancellations into
  the error numerator and trip every stress test at once.
- Records carrying neither metrics nor an error object (future
  non-request metadata lines) are excluded from the denominator instead
  of diluting the reported rate; the [aiperf-gate] line now reports the
  skipped count.
- Unit tests cover both: drifted-schema cancellations at 10% must pass
  the 5% threshold, and 10% real errors must still fire even when
  padded with 9x non-request records.

Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Follow-up to brnguyen2's review on NVIDIA#17427: the B200 qwen3_32b_fp8_stress
waive references the same NVBug this PR fixes, and the cluster-timing fix
applies to it (same run_disaggregated_aiperf path), so it is unwaived
alongside the DeepSeek param. A dedicated validation run on 8xB200 is
queued; the H100 waive for the same test stays (separate bug, 6312828).

Signed-off-by: Michal Guzek <mguzek@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/integration/defs/disaggregated/test_aiperf_gate.py (1)

64-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add type annotations to all helper and test functions. The repository guideline requires annotations on every function. The missing test docstrings are not an issue because these functions are not externally usable interfaces.

Test coverage summary: needs follow-up. The 11 tests cover error storms, cancellations, malformed exports, incomplete accounting, and denominator filtering. No test_aiperf_gate.py entry exists in the test lists. The GPU-backed deepseek_r1_v2_fp4_mtp_stress scenario is already listed in tests/integration/test_lists/qa/llm_function_stress.txt. A CBTS coverage report is unavailable, so coverage of max_num_tokens: 8320 cannot be established.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/defs/disaggregated/test_aiperf_gate.py` around lines 64 -
152, Add complete parameter and return type annotations to every helper and test
function in test_aiperf_gate.py, including the visible tests and helpers such as
_record, _write_export, and enforce_aiperf_error_rate. Use appropriate types for
pytest fixture arguments, record data, paths, and return values while preserving
existing behavior.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/integration/defs/disaggregated/test_aiperf_gate.py`:
- Around line 32-152: Add type annotations to every helper and test function in
the diff, including parameter and return types; all test functions must
explicitly return None. Add Google-style docstrings to _record, _write_export,
test_missing_export_raises, test_empty_export_fails, and
test_all_cancelled_fails, preserving the existing behavior and documentation for
functions that already have docstrings.

---

Nitpick comments:
In `@tests/integration/defs/disaggregated/test_aiperf_gate.py`:
- Around line 64-152: Add complete parameter and return type annotations to
every helper and test function in test_aiperf_gate.py, including the visible
tests and helpers such as _record, _write_export, and enforce_aiperf_error_rate.
Use appropriate types for pytest fixture arguments, record data, paths, and
return values while preserving existing behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ceacd03d-e30a-44a2-a226-e3b3e0c0abde

📥 Commits

Reviewing files that changed from the base of the PR and between e5a3c90 and fbf5971.

📒 Files selected for processing (4)
  • tests/integration/defs/disaggregated/test_aiperf_gate.py
  • tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yaml
  • tests/integration/defs/disaggregated/test_disaggregated.py
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/integration/defs/disaggregated/test_disaggregated.py

Comment thread tests/integration/defs/disaggregated/test_aiperf_gate.py Outdated
@moraxu

moraxu commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

@fredricz-20070104 addressing your review: (1) blast-radius of the default-on gate — dcbf25c widens cancellation detection to honor the per-record metadata.was_cancelled field as a fallback beyond code=499/type=RequestCancellationError, so error-shape drift cannot reclassify the ~10% intentional cancellations as errors across the stress configs; the schema is pinned to aiperf==0.8.0 in the docstring, and the 35k DeepSeek validation gate line was: [aiperf-gate] non-cancellation errors: 0/31561 (0.00%), cancelled: 3439, threshold: 5.00%. (2) empty/all-cancelled exports — 94bedd3 makes empty exports, wholesale parse failures, all-cancelled record sets, and incomplete accounting (total < 0.9x requested) raise instead of passing. Your minor notes are covered too: the assert is now an explicit FileNotFoundError (04df351), and non-request records are excluded from the denominator (dcbf25c). The gate now has GPU-free unit tests (test_aiperf_gate.py) including the 500-storm replay proving it fires.

@moraxu

moraxu commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65146 [ run ] triggered by Bot. Commit: fbf5971 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65146 [ run ] completed with state FAILURE. Commit: fbf5971
/LLM/main/L0_MergeRequest_PR pipeline #52941 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

@moraxu

moraxu commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65173 [ run ] triggered by Bot. Commit: fbf5971 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65173 [ run ] completed with state FAILURE. Commit: fbf5971
/LLM/main/L0_MergeRequest_PR pipeline #52967 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

…72256-disagg-stress-test

Signed-off-by: Michal Guzek <mguzek@nvidia.com>

# Conflicts:
#	tests/integration/defs/disaggregated/test_disaggregated.py
@coderabbitai

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/integration/defs/disaggregated/test_aiperf_gate.py`:
- Line 83: Update the pytest.raises match patterns at the three affected
assertions in test_aiperf_gate.py to use raw string literals and escape the dot
in profile_export.jsonl, preserving the existing FileNotFoundError matching
behavior.
- Around line 64-152: Register the new test module containing
test_fires_on_error_storm and the other aiperf gate tests in both
test-db/l0_cpu.yml and qa/llm_function_core.txt. Add the
disaggregated/test_aiperf_gate.py entry using the existing formatting and
ordering conventions, without modifying or removing other test registrations.

In `@tests/integration/defs/disaggregated/test_disaggregated.py`:
- Around line 2457-2581: Register the existing
test_llama4_long_context_kv_cache_overflow test in the QA LLM function-core list
and the applicable hardware-specific l0_*.yml test database entry. Keep the
existing stress-test registrations unchanged and use the same registration
format and hardware scope as comparable Llama4 long-context tests.
- Around line 2735-2738: The disaggregated test gate currently passes
request_count directly, allowing None to disable completeness validation. In the
surrounding request-generation/setup flow, resolve the effective request count
used by AIPerf when request_count is omitted, then pass that resolved value as
expected_records to enforce_aiperf_error_rate; alternatively make
--request-count explicit and reuse the same value.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 97f9837a-b2d2-47dc-b7f8-1cd56a256f13

📥 Commits

Reviewing files that changed from the base of the PR and between fd2edba and be6e0ed.

📒 Files selected for processing (5)
  • tests/integration/defs/disaggregated/test_aiperf_gate.py
  • tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm.yaml
  • tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yaml
  • tests/integration/defs/disaggregated/test_disaggregated.py
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yaml
  • tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm.yaml

Comment thread tests/integration/defs/disaggregated/test_aiperf_gate.py Outdated
Comment thread tests/integration/defs/disaggregated/test_aiperf_gate.py Outdated
Comment thread tests/integration/defs/disaggregated/test_disaggregated.py
Comment thread tests/integration/defs/disaggregated/test_disaggregated.py Outdated
moraxu added 2 commits August 10, 2026 22:51
…aiperf gate tests

Addresses CodeRabbit's review of the gate unit tests on NVIDIA#17427: every
helper and test function is now annotated (tests return None), the
previously bare functions have Google-style docstrings, and the regex
match= patterns containing metacharacters are raw strings (Ruff RUF043).

Signed-off-by: Michal Guzek <mguzek@nvidia.com>
…est mode; register gate tests

Addresses CodeRabbit's follow-up review on NVIDIA#17427:
- When request_count is None, aiperf 0.8.0 derives the request count as
  max(10, concurrency * 2) (--num-dataset-entries only sizes the prompt
  pool), so the gate now mirrors that derivation instead of silently
  disabling its completeness check in the default request mode.
- The 11 aiperf gate tests are registered in test-db/l0_cpu.yml
  (GPU-free, pre-merge) and qa/llm_function_core.txt so they actually
  run in CI/QA.

Skipped with reason: the raw-regex RUF043 comment was already fixed in
f4dd1fd (stale review anchor), and registering the pre-existing
test_llama4_long_context_kv_cache_overflow RCCA repro is out of scope
for this PR.

Signed-off-by: Michal Guzek <mguzek@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/integration/test_lists/qa/llm_function_core.txt`:
- Around line 824-834: Add the standard NVIDIA copyright header to the modified
test list file and set the copyright year to 2026, preserving the existing test
entries unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a8831d8f-1fc5-4eb7-a413-6db93bf02732

📥 Commits

Reviewing files that changed from the base of the PR and between f4dd1fd and f060429.

📒 Files selected for processing (3)
  • tests/integration/defs/disaggregated/test_disaggregated.py
  • tests/integration/test_lists/qa/llm_function_core.txt
  • tests/integration/test_lists/test-db/l0_cpu.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/integration/defs/disaggregated/test_disaggregated.py

Comment thread tests/integration/test_lists/qa/llm_function_core.txt
@moraxu

moraxu commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65334 [ run ] triggered by Bot. Commit: f060429 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65334 [ run ] completed with state FAILURE. Commit: f060429
/LLM/main/L0_MergeRequest_PR pipeline #53113 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

@moraxu

moraxu commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65366 [ run ] triggered by Bot. Commit: f060429 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65366 [ run ] completed with state FAILURE. Commit: f060429

Link to invocation

@moraxu

moraxu commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65368 [ run ] triggered by Bot. Commit: f060429 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65368 [ run ] completed with state SUCCESS. Commit: f060429
/LLM/main/L0_MergeRequest_PR pipeline #53137 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

…ests in l0_cpu.yml

The Check Test List CI stage validates test-db entries against collected
test IDs, so the bare module path was rejected as invalid; expand it into
the 11 ::-qualified test names (matching the qa list entries, which
validated fine).

Signed-off-by: Michal Guzek <mguzek@nvidia.com>
@moraxu

moraxu commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65387 [ run ] triggered by Bot. Commit: cf26ed1 Link to invocation

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.

6 participants