[None][test] Update stress tests - #14454
Conversation
📝 WalkthroughWalkthroughThis PR enhances disaggregated integration testing by adding log capture and post-run fatal-pattern scanning, expanding/adding disaggregated YAML configurations, introducing speculative-model support and cancellation/stddev options for stress runs, and updating test lists/waivers and CI QA guidance. ChangesDisaggregated Stress Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/test_lists/qa/llm_function_stress.txt (1)
1-11:⚠️ Potential issue | 🟠 MajorUpdate
waives.txtfor the stress-test suite migration
tests/integration/test_lists/waives.txtstill hasSKIPentries forstress_test/stress_test.py::test_run_stress_test[...](lines 344-345). Ifllm_function_stress.txtno longer selects those nodeids, remove or update these waivers.- No waiver entries exist for
disaggregated/test_disaggregated.py::test_disaggregated_stress_test[...], even though the added params (deepseek_r1_v2_fp4_mtp_stress,gpt_oss_120b_eagle_stress,gpt_oss_120b_cutlass_stress,qwen3_5_4b_fp8_stress) are present intests/integration/defs/disaggregated/test_disaggregated.py. Add corresponding waivers if these disaggregated cases need them.- The new
llm_function_stress.txtnodeids match existing parametrizations intests/integration/defs/disaggregated/test_disaggregated.py.🤖 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/test_lists/qa/llm_function_stress.txt` around lines 1 - 11, The waivers file still contains SKIP entries for the old stress_test nodeids (stress_test/stress_test.py::test_run_stress_test[...]) that are no longer selected by the new list; remove or update those SKIP lines. Add SKIP entries for the new disaggregated parametrizations — e.g., nodeids matching disaggregated/test_disaggregated.py::test_disaggregated_stress_test[input8k-output1k-conc512-deepseek_r1_v2_fp4_mtp_stress], ...-gpt_oss_120b_eagle_stress, ...-gpt_oss_120b_cutlass_stress, and ...-qwen3_5_4b_fp8_stress — if those cases should be waived. Ensure the nodeids exactly match the parametrized names used by test_disaggregated_stress_test so the waiver system will match at runtime.tests/integration/defs/disaggregated/disagg_test_utils.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the NVIDIA copyright year on this modified file.
This file now has 2026 changes, but the header still says
2022-2024.As per coding guidelines,
**/*.{cpp,cc,h,hpp,py,cu,cuh}: Include NVIDIA copyright header on ALL new files; update year on modified files.🤖 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/disagg_test_utils.py` at line 1, Update the file header copyright range to include 2026 by changing the year span from "2022-2024" to "2022-2026" in the top-of-file comment (the SPDX/FileCopyrightText header) so the modified file reflects the new copyright year.
🤖 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 1995-1998: The failure message for the accuracy test references
non-existent log filenames; update the AssertionError raised when
accuracy_test_result is falsy (the block using accuracy_test_result) to point
users to the actual artifacts: "worker_{role}_{port}.log" and
"disagg_server.log" (or similar dynamic names used by the harness) instead of
"output_workers.log / output_disagg.log", so that the exception text directs
triage to the correct saved logs.
- Around line 2265-2271: The pytest.param block creating TestConfig is using a
hanging indent that triggers Flake8 E126; reformat the call so the continued
lines are properly aligned with the opening parenthesis or use a consistent
indentation style. Locate the pytest.param(...) that wraps TestConfig
(referencing TestConfig, model_path='gpt_oss/gpt-oss-120b',
speculative_model_path='gpt_oss/gpt-oss-120b-Eagle3') and adjust the indentation
of the inner lines and the closing parentheses so they align (e.g., align
arguments under the first argument after the opening parenthesis or place the
opening parenthesis at line end and indent subsequent lines consistently) to
satisfy E126.
- Around line 2302-2317: When test_config.speculative_model_path is provided we
must fail fast if the patch is a no-op: after building spec_model_dir and
loading patched_config, compare the original config's 'speculative_model'
entries under 'context_servers' and 'generation_servers' to the values in
patched_config (or track whether the loop that sets spec['speculative_model']
actually changed any entries); if no entry was updated, raise an exception or
assert to fail the test (use the existing variables config_file, patched_config,
spec_model_dir, and test_desc to locate context and produce a clear error).
Ensure this check runs before writing patched_path and replacing config_file so
the test fails immediately when the speculative-model patch had no effect.
- Around line 2018-2032: The current exception handler loads entire log files
with f.read().split("\n"), which can OOM; update the block that iterates over
[*ctx_workers, *gen_workers, disagg_server] and uses log_path to instead stream
the file tail without reading whole file (e.g., use a fixed-size ring buffer
like collections.deque(maxlen=30) or seek-from-end logic) before logging the
last 30 non-empty lines via logger.error; keep the same error handling around
open() and OSError.
---
Outside diff comments:
In `@tests/integration/defs/disaggregated/disagg_test_utils.py`:
- Line 1: Update the file header copyright range to include 2026 by changing the
year span from "2022-2024" to "2022-2026" in the top-of-file comment (the
SPDX/FileCopyrightText header) so the modified file reflects the new copyright
year.
In `@tests/integration/test_lists/qa/llm_function_stress.txt`:
- Around line 1-11: The waivers file still contains SKIP entries for the old
stress_test nodeids (stress_test/stress_test.py::test_run_stress_test[...]) that
are no longer selected by the new list; remove or update those SKIP lines. Add
SKIP entries for the new disaggregated parametrizations — e.g., nodeids matching
disaggregated/test_disaggregated.py::test_disaggregated_stress_test[input8k-output1k-conc512-deepseek_r1_v2_fp4_mtp_stress],
...-gpt_oss_120b_eagle_stress, ...-gpt_oss_120b_cutlass_stress, and
...-qwen3_5_4b_fp8_stress — if those cases should be waived. Ensure the nodeids
exactly match the parametrized names used by test_disaggregated_stress_test so
the waiver system will match at runtime.
🪄 Autofix (Beta)
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: 6fdc75d6-4dec-4fea-89ec-289e8f5bc504
📒 Files selected for processing (12)
tests/integration/defs/disaggregated/disagg_test_utils.pytests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp1_gentp1_qwen3_5_4b_fp8_tllm.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm_cutlass.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm_eagle.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yamltests/integration/defs/disaggregated/test_disaggregated.pytests/integration/defs/test_e2e.pytests/integration/test_lists/qa/llm_function_multinode.txttests/integration/test_lists/qa/llm_function_stress.txttests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (3)
- tests/integration/test_lists/qa/llm_function_multinode.txt
- tests/integration/defs/test_e2e.py
- tests/integration/test_lists/waives.txt
…LASS, and aiperf cancellation Build out the disaggregated stress test infrastructure on top of test_disaggregated_stress_test: * Add stress variants for DeepSeek-R1 v2 FP4 (with and without MTP), gpt-oss-120b (TRTLLM, Eagle3, and CUTLASS MoE backends) and Qwen3.5-4B-FP8, gated by the correct skip marks so each variant runs on the architectures that support it (Hopper, Blackwell, or both). * Fail the test eagerly when worker/server logs show fatal patterns (hangs, cluster-not-ready, internal server errors) and when the lm_eval accuracy run cannot complete, so silent regressions surface instead of being swallowed by aiperf's 500-as-success accounting. * Expose aiperf's --request-cancellation-rate / --request-cancellation-delay and --synthetic-input-tokens-stddev / --output-tokens-stddev through run_disaggregated_aiperf and TestConfig, then turn on 10% request cancellation, a 0.5s cancel delay, and 10% input/output token-length stddev for every stress variant so the runs exercise mid-flight client cancels and a realistic prompt-length distribution. When output_tokens_stddev > 0 the min_tokens/max_tokens extra-inputs are dropped so the sampling actually takes effect; defaults are backward-compatible for callers like test_llama4_long_context_kv_cache_overflow. * Enable attention DP in the disagg configs where EP equals TP, switch the Eagle3 gpt-oss stress config to CUTLASS so it can also run on Hopper, scale it to TP=EP=4, and update test list / waiver files accordingly. Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
…eq_len
Set input_tokens_stddev=0 in test_disaggregated_stress_test. With
input_tokens=8192 sitting only 40 tokens below the context server's
max_seq_len=8232, a 10% stddev caused sampled prompts to exceed
max_seq_len and the server returned 500s ("default_max_tokens
(-244) must be greater than 0"). Output stddev is kept at 10%
because the generation side has ~2k of headroom.
Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
|
@coderabbitai review my code |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/integration/defs/disaggregated/test_disaggregated.py (1)
97-121: 💤 Low valueConsider adding return type annotation.
The function is well-implemented but lacks a return type hint. Adding it would improve readability and tooling support.
Suggested annotation
-def scan_logs_for_fatal_errors(processes): +def scan_logs_for_fatal_errors(processes) -> dict[str, dict[str, int]]:🤖 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 97 - 121, Add an explicit return type annotation to the function scan_logs_for_fatal_errors: change its signature to include -> dict[str, dict[str, int]] so callers and linters know it returns a mapping of log path to pattern counts (matching the local findings variable). Ensure your project Python version supports PEP 585 annotations (or switch to typing.Dict if not) and update any related import/style if required..coderabbit.yaml (1)
40-42: 💤 Low valueVerify simplified test review guidance is intentional.
The test path instructions were simplified from a longer checklist (covering test list hygiene, coverage expectations, and performance-test coverage rules) to a concise two-line directive. While this makes the guidance more accessible, it removes specific rules that may have helped catch test-related issues.
Consider whether:
- The removed checklist items are now covered elsewhere (e.g., in CODING_GUIDELINES.md or CI documentation)
- Review quality might be affected by the loss of specific guidance on test list hygiene and performance coverage rules
- This change aligns with the team's desired review depth for test changes
🤖 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 @.coderabbit.yaml around lines 40 - 42, The simplified QA/test-review directive in .coderabbit.yaml removed the original checklist; restore explicit guidance by reintroducing key checklist items or linking to their canonical source: ensure the directive (the "Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM" block) includes concrete expectations for test list hygiene, coverage thresholds, and performance-test coverage rules, or add a clear pointer to CODING_GUIDELINES.md/CI docs and require concrete file lists (e.g., tests/unit/*.py, tests/integration/*, perf/tests/*) with labels like "sufficient/insufficient/follow-up" so reviewers know what to check and where to record results.
🤖 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_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm_cutlass.yaml`:
- Around line 43-57: The cuda_graph_config.batch_sizes list contains entries
larger than the server's max_batch_size (128) so remove any batch sizes >128
(256, 512, 768, 1024) or programmatically generate the batch_sizes up to the
max_batch_size used by the generation server; update the batch_sizes in the
cuda_graph_config block so all entries are <= the max_batch_size (128) to avoid
allocating unused CUDA graphs and free GPU memory for KV cache.
In `@tests/integration/test_lists/waives.txt`:
- Around line 329-330: The waivers added for llama-v3-8b-instruct-hf_tp1 only
shorten stress coverage for stress_time_300s_timeout_450s (GUARANTEED_NO_EVICT
and MAX_UTILIZATION); either document where equivalent longer-duration or
higher-parallelism coverage exists (e.g., reference the
disaggregated/test_disaggregated.py::test_disaggregated_stress_test
parametrizations listed in qa/llm_function_stress.txt) or add/extend stress
tests so the llama-v3-8b_tp1 workload still runs an equivalent
long-duration/high-parallelism scenario; update the waives.txt entry with a
comment pointing to the alternative test target if coverage is provided, or add
the missing stress-test parametrization to
stress_test/stress_test.py::test_run_stress_test for llama-v3-8b-instruct-hf_tp1
if no equivalent exists.
---
Nitpick comments:
In @.coderabbit.yaml:
- Around line 40-42: The simplified QA/test-review directive in .coderabbit.yaml
removed the original checklist; restore explicit guidance by reintroducing key
checklist items or linking to their canonical source: ensure the directive (the
"Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM"
block) includes concrete expectations for test list hygiene, coverage
thresholds, and performance-test coverage rules, or add a clear pointer to
CODING_GUIDELINES.md/CI docs and require concrete file lists (e.g.,
tests/unit/*.py, tests/integration/*, perf/tests/*) with labels like
"sufficient/insufficient/follow-up" so reviewers know what to check and where to
record results.
In `@tests/integration/defs/disaggregated/test_disaggregated.py`:
- Around line 97-121: Add an explicit return type annotation to the function
scan_logs_for_fatal_errors: change its signature to include -> dict[str,
dict[str, int]] so callers and linters know it returns a mapping of log path to
pattern counts (matching the local findings variable). Ensure your project
Python version supports PEP 585 annotations (or switch to typing.Dict if not)
and update any related import/style if required.
🪄 Autofix (Beta)
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: d1095b1d-e826-49d6-af1b-abf6b652f209
📒 Files selected for processing (13)
.coderabbit.yamltests/integration/defs/disaggregated/disagg_test_utils.pytests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp1_gentp1_qwen3_5_4b_fp8_tllm.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm_cutlass.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm_eagle.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm.yamltests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp4_gentp4_deepseek_r1_v2_fp4_tllm_mtp.yamltests/integration/defs/disaggregated/test_disaggregated.pytests/integration/defs/test_e2e.pytests/integration/test_lists/qa/llm_function_multinode.txttests/integration/test_lists/qa/llm_function_stress.txttests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (2)
- tests/integration/test_lists/qa/llm_function_multinode.txt
- tests/integration/defs/test_e2e.py
✅ Files skipped from review due to trivial changes (3)
- 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
- tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/integration/test_lists/qa/llm_function_stress.txt
- tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp1_gentp1_qwen3_5_4b_fp8_tllm.yaml
- tests/integration/defs/disaggregated/test_configs/disagg_config_ctxtp2_gentp2_gptoss_tllm_eagle.yaml
- tests/integration/defs/disaggregated/disagg_test_utils.py
|
/bot run --skip-test |
|
/bot run |
|
PR_Github #50478 [ run ] triggered by Bot. Commit: |
|
PR_Github #50479 [ run ] triggered by Bot. Commit: |
|
PR_Github #50478 [ run ] completed with state |
…g_ctxtp2_gentp2_gptoss_tllm_cutlass.yaml Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com>
|
PR_Github #50479 [ run ] completed with state |
|
/bot reuse-pipeline |
|
PR_Github #50523 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #50523 [ reuse-pipeline ] completed with state |
Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com> Signed-off-by: xinhe-nv <200704525+xinhe-nv@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Description
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.