[None][test] Half K25 Agg Multi Round to Solve Timeout Issue - #15083
Conversation
📝 WalkthroughWalkthroughThis PR reduces test iterations from 10 to 5 across performance sanity YAML configurations for K2.5 FP4 deployments (Dynamo-enabled, multi-node, and single-node variants), while maintaining the original concurrency settings for each test scenario. ChangesK2.5 FP4 Performance Test Configuration Updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🧹 Nitpick comments (3)
tests/scripts/perf-sanity/aggregated/k25_thinking_fp4_grace_blackwell.yaml (1)
29-31: 💤 Low valueVery low prompt count for low-concurrency test; verify adequacy.
Reducing iterations to 5 with
concurrency: 2yields only 10 total prompts. While consistent with the other files in this PR, this minimal sample size may not provide adequate coverage for detecting performance regressions on the Grace Blackwell platform. Consider whether this test should retain more iterations if the timeout issue is primarily in higher-concurrency scenarios.🤖 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/scripts/perf-sanity/aggregated/k25_thinking_fp4_grace_blackwell.yaml` around lines 29 - 31, The test "con2_iter5_8k1k" currently uses concurrency: 2 and iterations: 5 (only 10 total prompts); increase the iterations value (e.g., to at least 20 or match the iterations used by the sibling performance configs) so the low-concurrency run produces a larger sample size for meaningful regression detection; update the iterations key for the "con2_iter5_8k1k" entry (and keep concurrency: 2) to ensure adequate coverage.tests/scripts/perf-sanity/aggregated/k25_thinking_fp4_2_nodes_grace_blackwell.yaml (1)
30-32: 💤 Low valueVery low prompt count for low-concurrency test; verify adequacy.
Reducing iterations to 5 with
concurrency: 2yields only 10 total prompts for this 32k8k multi-node configuration. While this may address timeout issues, 10 prompts provides minimal statistical coverage for detecting performance regressions. Verify that this is sufficient for sanity checking or consider whether this test should retain more iterations (or whether alternative timeout mitigations are feasible).🤖 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/scripts/perf-sanity/aggregated/k25_thinking_fp4_2_nodes_grace_blackwell.yaml` around lines 30 - 32, The test case "con2_iter5_32k8k" currently sets concurrency: 2 and iterations: 5 (only 10 total prompts), which is likely too small for reliable perf sanity checks; update the YAML entry for the test named "con2_iter5_32k8k" to increase iterations (e.g., to 20 or another value that yields adequate sample size given concurrency: 2) or add an explicit comment/metadata justifying the low iteration count and any timeout mitigation used so reviewers can accept the reduced sample; ensure you modify the iterations field and/or add a clear inline note next to the name so the intent is obvious.tests/scripts/perf-sanity/aggregated/k25_thinking_fp4_blackwell.yaml (1)
29-31: 💤 Low valueVery low prompt count for low-concurrency tests; consider retaining more iterations.
Both
con2configurations now yield only 10 total prompts each (for both 8k1k and 32k8k variants). While this may address timeout issues, the minimal sample size may not provide adequate coverage for performance regression detection. Consider whether:
- These low-concurrency tests could retain 10 iterations (if timeout is primarily in high-concurrency configs)
- Alternative timeout mitigations exist (e.g., per-test timeout increases)
Also applies to: 95-97
🤖 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/scripts/perf-sanity/aggregated/k25_thinking_fp4_blackwell.yaml` around lines 29 - 31, The low-concurrency test entries (e.g., the YAML block with name "con2_iter5_8k1k" and its sibling 32k8k con2 entry) currently use iterations: 5 which yields too few prompts; change iterations from 5 to 10 for these con2_* entries to restore a larger sample size, or alternatively keep iterations at 10 and instead raise the per-test timeout if timeouts were the reason for reducing iterations — update the entries named "con2_iter5_8k1k" and the matching con2 32k8k entry accordingly.
🤖 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/scripts/perf-sanity/aggregated/dynamo_k25_thinking_fp4_blackwell.yaml`:
- Around line 32-34: Add a brief note (either as a YAML comment near the
con128_iter5_2k1k entry or in the PR description) that explains the timeout root
cause and why iterations were reduced: state that the original
con128_iter10_2k1k (iterations: 10) was hitting the transfer/response timeout
during key-value transfer/response handling (the failing component was the KV
transfer / RPC response path that exceeded the existing
kv_transfer_timeout_ms/timeout_iters), causing test runs to abort; explain that
reducing iterations to 5 prevents repeated back-to-back timeouts while still
providing 640 total prompts at concurrency 128 for this deployment variant, and
justify that 5 iterations maintain statistical signal because of fixed high
concurrency (128) and the test’s focus on median/percentile latency rather than
rare-tail extremes (or note what alternate threshold/logic—e.g., percentile
checks or additional runs—will compensate for the smaller sample).
---
Nitpick comments:
In
`@tests/scripts/perf-sanity/aggregated/k25_thinking_fp4_2_nodes_grace_blackwell.yaml`:
- Around line 30-32: The test case "con2_iter5_32k8k" currently sets
concurrency: 2 and iterations: 5 (only 10 total prompts), which is likely too
small for reliable perf sanity checks; update the YAML entry for the test named
"con2_iter5_32k8k" to increase iterations (e.g., to 20 or another value that
yields adequate sample size given concurrency: 2) or add an explicit
comment/metadata justifying the low iteration count and any timeout mitigation
used so reviewers can accept the reduced sample; ensure you modify the
iterations field and/or add a clear inline note next to the name so the intent
is obvious.
In `@tests/scripts/perf-sanity/aggregated/k25_thinking_fp4_blackwell.yaml`:
- Around line 29-31: The low-concurrency test entries (e.g., the YAML block with
name "con2_iter5_8k1k" and its sibling 32k8k con2 entry) currently use
iterations: 5 which yields too few prompts; change iterations from 5 to 10 for
these con2_* entries to restore a larger sample size, or alternatively keep
iterations at 10 and instead raise the per-test timeout if timeouts were the
reason for reducing iterations — update the entries named "con2_iter5_8k1k" and
the matching con2 32k8k entry accordingly.
In `@tests/scripts/perf-sanity/aggregated/k25_thinking_fp4_grace_blackwell.yaml`:
- Around line 29-31: The test "con2_iter5_8k1k" currently uses concurrency: 2
and iterations: 5 (only 10 total prompts); increase the iterations value (e.g.,
to at least 20 or match the iterations used by the sibling performance configs)
so the low-concurrency run produces a larger sample size for meaningful
regression detection; update the iterations key for the "con2_iter5_8k1k" entry
(and keep concurrency: 2) to ensure adequate coverage.
🪄 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: 09bc74e1-1892-4842-8ac1-5458a6e7e39c
📒 Files selected for processing (4)
tests/scripts/perf-sanity/aggregated/dynamo_k25_thinking_fp4_blackwell.yamltests/scripts/perf-sanity/aggregated/k25_thinking_fp4_2_nodes_grace_blackwell.yamltests/scripts/perf-sanity/aggregated/k25_thinking_fp4_blackwell.yamltests/scripts/perf-sanity/aggregated/k25_thinking_fp4_grace_blackwell.yaml
|
/bot run --disable-fail-fast --stage-list "DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-2,DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-2,DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-3,DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-4,GB200-8_GPUs-2_Nodes-PyTorch-PerfSanity-Node2-GPU8-Post-Merge-7" |
|
PR_Github #52703 [ run ] triggered by Bot. Commit: |
c50f30b to
0b7326a
Compare
|
/bot run --disable-fail-fast --stage-list "DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-2,DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-2,DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-3,DGX_B200-8_GPUs-PyTorch-PerfSanity-Post-Merge-4,GB200-8_GPUs-2_Nodes-PyTorch-PerfSanity-Node2-GPU8-Post-Merge-7" |
|
PR_Github #52707 [ run ] triggered by Bot. Commit: |
|
PR_Github #52703 [ run ] completed with state |
|
PR_Github #52707 [ run ] completed with state |
0b7326a to
ef7e2ea
Compare
|
/bot skip --comment "Only unwaive perf tests, no need to run the whole CI pipeline" |
|
PR_Github #52787 [ skip ] triggered by Bot. Commit: |
|
PR_Github #52787 [ skip ] completed with state |
3 participants
Summary by CodeRabbit
Release Notes
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.