[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 |
3 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.