[https://nvbugs/6472256][fix] Fix disagg stress cluster flapping and DeepSeek R1 FP4 ctx OOM; add aiperf error-rate gate - #17427
Conversation
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
|
/bot run |
WalkthroughThe disaggregated tests now use production service-discovery timings, support cluster overrides, show fatal-log context, and enforce aiperf error-rate limits. The FP4 configuration limits context-server tokens, and its stress-test waiver is removed. ChangesDisaggregated test reliability
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
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (3)
tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm.yamltests/integration/defs/disaggregated/test_disaggregated.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
| export_path = os.path.join(artifact_dir, "profile_export.jsonl") | ||
| assert os.path.exists(export_path), ( | ||
| f"aiperf per-record export not found at {export_path}; cannot enforce " | ||
| "the request error-rate gate. If this aiperf version/export level does " | ||
| "not produce it, pass max_error_rate=None explicitly.") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Replace the assertion with an explicit exception.
Python removes assert statements when it runs with -O. A missing export then fails later at open() without the intended diagnostic. Raise FileNotFoundError explicitly.
Proposed fix
- assert os.path.exists(export_path), (
- f"aiperf per-record export not found at {export_path}; cannot enforce "
- "the request error-rate gate. If this aiperf version/export level does "
- "not produce it, pass max_error_rate=None explicitly.")
+ if not os.path.exists(export_path):
+ raise FileNotFoundError(
+ f"aiperf per-record export not found at {export_path}; cannot enforce "
+ "the request error-rate gate. If this aiperf version/export level does "
+ "not produce it, pass max_error_rate=None explicitly.")As per coding guidelines, “raise ValueError rather than assertions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export_path = os.path.join(artifact_dir, "profile_export.jsonl") | |
| assert os.path.exists(export_path), ( | |
| f"aiperf per-record export not found at {export_path}; cannot enforce " | |
| "the request error-rate gate. If this aiperf version/export level does " | |
| "not produce it, pass max_error_rate=None explicitly.") | |
| export_path = os.path.join(artifact_dir, "profile_export.jsonl") | |
| if not os.path.exists(export_path): | |
| raise FileNotFoundError( | |
| f"aiperf per-record export not found at {export_path}; cannot enforce " | |
| "the request error-rate gate. If this aiperf version/export level does " | |
| "not produce it, pass max_error_rate=None explicitly.") |
🤖 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_disaggregated.py` around lines 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.
Source: Coding guidelines
|
PR_Github #64668 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
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.
| [0, error.get("message", "")]) | ||
| entry[0] += 1 | ||
| considered = total - cancelled | ||
| if considered <= 0: |
There was a problem hiding this comment.
This early return makes the gate silently pass in the cases you most want it to fail. total only counts lines that parsed as JSON, so if aiperf writes an empty export, gets killed mid-write, or changes the export path/nesting in a future version, total == 0 → considered <= 0 → clean pass. Same for the json.JSONDecodeError: continue above: a wholesale format change degrades to "no errors found" instead of an error.
Suggest counting decode failures and asserting the export is substantive before computing the rate:
assert total > 0, f"{export_path} contained no parseable records ({malformed} malformed lines)"
assert malformed <= 0.01 * (total + malformed), ...
if considered <= 0:
raise AssertionError(f"all {total} records were cancellations")| code = error.get("code") | ||
| err_type = error.get("type") | ||
| if code == 499 or err_type == "RequestCancellationError": | ||
| cancelled += 1 |
There was a problem hiding this comment.
The whole gate hinges on this classification being exhaustive: these stress runs use --request-cancellation-rate 10, so if aiperf reports a cancelled request under any other shape (a different type string, a null code, or no error object with a separate status field), ~10% of records become "errors" and every stress test trips the 5% threshold on the first run.
Please record the aiperf version this record schema was validated against in the docstring, and post the [aiperf-gate] output line from the 35k validation run so the cancelled: count can be checked against the expected ~3500.
| # KV pool on 192GB B200 and intermittently OOMs the ctx worker under | ||
| # 512-concurrency 8k prefill load (memory estimation only observes | ||
| # ~10.8 GiB dynamic peak, so the KV pool leaves no slack for it). | ||
| max_num_tokens: 8320 |
There was a problem hiding this comment.
disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yaml has an identical context server (max_num_tokens: 16640, free_gpu_memory_fraction: 0.8, TRTLLM MoE backend, same weights) and does not get this chunking. By the analysis in the description it carries the same workspace transient, and deepseek_r1_v2_fp4_mtp_stress is not waived — so it is a live OOM candidate on B200. Either apply the same moe_config.max_num_tokens there or say in the PR why the MTP variant's headroom differs (enable_attention_dp: false and max_draft_len: 1 change the picture, but in which direction?).
| full:B200/accuracy/test_llm_api_pytorch.py::TestQwen3_30B_A3B::test_dummy_load_format SKIP (https://nvbugs/6525059) | ||
| full:B200/disaggregated/test_disaggregated.py::test_disaggregated_stress_test[input8k-output1k-conc512-deepseek_r1_v2_fp4_stress] SKIP (https://nvbugs/6472256) | ||
| full:B200/disaggregated/test_disaggregated.py::test_disaggregated_stress_test[input8k-output1k-conc512-glm5_nvfp4_tp4_ep4_dp_stress] SKIP (https://nvbugs/6544407) | ||
| full:B200/disaggregated/test_disaggregated.py::test_disaggregated_stress_test[input8k-output1k-conc512-qwen3_32b_fp8_stress] SKIP (https://nvbugs/6472256) |
There was a problem hiding this comment.
qwen3_32b_fp8_stress is still waived under the same NVBug this PR fixes. The cluster-timing fix applies to it too (it runs through the same run_disaggregated_aiperf path), so either it should be unwaived alongside the DeepSeek param, or — if it fails for an unrelated reason — it needs its own bug ID. As it stands, nvbugs/6472256 can't be closed by this PR.
|
PR_Github #64668 [ run ] completed with state |
fredricz-20070104
left a comment
fredricz-20070104
left a comment
There was a problem hiding this comment.
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
-
[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_aiperfnow defaultsmax_error_rate=0.05, andenforce_aiperf_error_rateonly excludes a request from the numerator whenerror.code == 499ORerror.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 runningcancellation_rate=10(~10% intentionally cancelled). - How it fails: if the aiperf version in CI records a cancelled request with any other schema (no
errorobject, a nullcode, or a differenttypestring), 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 (confirmcancelled~= 3500 anderrors== 0), pin the aiperf version this record schema comes from, and widen cancellation detection (also treatstatus/http 499 and message-based cancellation) or enable the gate per-config until the schema is confirmed for each.
- What is wrong:
-
[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 yieldstotal == 0/considered == 0and 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
totalis 0 orconsideredis implausibly small relative to the requested load.
- What is wrong: the function returns early when
Minor notes (non-blocking)
tests/integration/defs/disaggregated/test_disaggregated.py:2385-assert os.path.exists(export_path)is stripped underpython -O; use an explicitFileNotFoundErrorpreserving the diagnostic message (also flagged by CodeRabbit).tests/integration/defs/disaggregated/test_disaggregated.py:2401-totalcounts 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_rategate 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.txtre-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.jsonlrecord schema for the pinned version - whether cancelled requests always carrycode==499ortype=="RequestCancellationError", and whether the file contains non-request metadata lines. - The full set of
run_disaggregated_aiperfcallers and which stress params actually route through it vsrun_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
left a comment
fredricz-20070104
left a comment
There was a problem hiding this comment.
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.
5 participants
Dev Engineer Review
enforce_aiperf_error_rate()with cancellation-aware error handling.max_num_tokensto8320to reduce transient TRTLLM-Gen workspace OOM risk.QA Engineer Review
tests/integration/defs/disaggregated/test_disaggregated.py.tests/integration/test_lists/waives.txt.0.9568.Description
Root cause & fix (two independent issues).
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/expirehandler, and the expiry sweep all share asyncio event loops saturated at 512-concurrency; any >1s stall spuriously expired a worker, flippingis_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; test unwaived.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: 8320halves the MoE transient. Validation: full 35,000-request stress run passed — 0 errors, 0 OOM, GSM8K 0.9568.Test 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.