[None][test] Enable warmup request for gen_only perf sanity lanes - #17098
Conversation
Signed-off-by: Eric Tsai <ertsai@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe performance sanity tests now read and persist a ChangesWarmup-aware benchmark execution
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 (1)
tests/integration/defs/perf/test_perf_sanity.py (1)
980-983: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both command branches.
Test
ClientConfig._to_default_benchmark_cmd()withwarmup=Falseandwarmup=True. Assert that--no-test-inputis present only whenwarmupis disabled. Also cover thegen_onlyparser path that setswarmup=True.As per path instructions, this test review must report changed test functions, test-list membership, and a coverage verdict.
🤖 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/perf/test_perf_sanity.py` around lines 980 - 983, Add regression tests for ClientConfig._to_default_benchmark_cmd() covering warmup=False and warmup=True, asserting --no-test-input appears only when warmup is disabled; also exercise the gen_only parser path that enables warmup. Report the changed test functions, test-list membership, and the resulting coverage verdict.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/perf/test_perf_sanity.py`:
- Line 897: Extend ClientConfig.to_match_keys() and ClientConfig.to_db_data() to
include warmup as a persisted boolean so baseline identity distinguishes
warmup-enabled and cold runs. When loading existing rows that lack the field,
treat warmup as false for backward compatibility, and ensure matching uses the
normalized boolean consistently.
---
Nitpick comments:
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 980-983: Add regression tests for
ClientConfig._to_default_benchmark_cmd() covering warmup=False and warmup=True,
asserting --no-test-input appears only when warmup is disabled; also exercise
the gen_only parser path that enables warmup. Report the changed test functions,
test-list membership, and the resulting coverage verdict.
🪄 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: 3da2bcf3-1bd9-47ab-98a5-0dccc44c6cca
📒 Files selected for processing (1)
tests/integration/defs/perf/test_perf_sanity.py
|
Nice diagnosis of the cold-start TTFT, and the
Nothing breaks the cycle — the gate is re-evaluated every iteration but (A) can only be satisfied by requests the client is blocked from sending.
Of the 37 Options, roughly in order of preference:
Also worth coordinating with #16961 — it is rewriting how this fill target is derived ( Non-blocking: CodeRabbit's I have not reproduced the hang — this is read from the gate source, so please sanity-check it against a |
…mup flag in perf DB Signed-off-by: Eric Tsai <ertsai@nvidia.com>
|
Pushed the option 3 fix (warmup scoped to After pushing, though, I started questioning my own assumption that con1 is the only exposed shape — so I probed it: I cloned the con1024 lane config with only
(TPOT is flat or slightly better on v2 across the board — the gap is entirely the one-time connection setup landing on the TTFT baseline, which scales with So the honest framing is: the cold-start problem lives on any lane with a small (On option 2 — re-reading "add 1 to |
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
Option 3 looks right, and your read of option 2 is correct — concurrency + 1 doesn't help, because the client awaits the warmup response before sending the measured load, so the gate never sees more than 1 queued request regardless of the queue size. Scratch that option.
On the durable fix: agreed that con4/con16 @ short ISL would need option 1, but I wouldn't take the executor change now — every gen_only lane in tree today is either con1 or a minutes-scale baseline, so the con1 scoping is exact rather than merely convenient, and the comment you added is what keeps it honest. Worth revisiting when such a lane is actually proposed.
Verified: --no-test-input is still unconditionally seeded and only dropped under if not self.warmup, so every other lane's command is byte-identical; b_warmup in to_db_data() without a match key is safe since warmup is fully determined by s_benchmark_mode + l_concurrency, both already match keys. Not blocking on the one-time TTFT step change those 8 con1 lanes will show against their stored baselines.
|
I'm not familiar with this code, I'll defer to other reviewers in the trt-llm-qa review group |
|
/bot run --disable-fail-fast |
|
PR_Github #63277 [ run ] triggered by Bot. Commit: |
|
PR_Github #63277 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #63317 [ run ] triggered by Bot. Commit: |
|
PR_Github #63317 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #63364 [ run ] triggered by Bot. Commit: |
|
PR_Github #63364 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
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. |
|
PR_Github #63917 [ run ] triggered by Bot. Commit: |
|
PR_Github #63917 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #63995 [ run ] triggered by Bot. Commit: |
|
PR_Github #63995 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64157 [ run ] triggered by Bot. Commit: |
|
PR_Github #64157 [ run ] completed with state |
…anes (NVIDIA#17098)" This reverts commit 77bc7f0. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
…anity lanes NVIDIA#17098 enabled benchmark_serving's initial test request as a warmup for gen_only lanes; NVIDIA#18011 reverted it, because gen_only does not measure TTFT and the extra ctx->gen handover leaves a stale mSenderFutures entry that the CTX worker's blocking idle KV-transfer poll then waits on. Two other disagg lanes do want it, for two reasons that come to the same thing -- a one-time cold-start cost that otherwise lands inside the measured window: * e2e pays for the KV cache transceiver's lazy connection setup (ZMQ mesh + NIXL metadata registration) on the first handover, so until that has happened once the transfer runs well below steady-state bandwidth. * ctx_only forces osl=1, so the first cold prefill lands directly in the headline TTFT with nothing to amortize it. The initial test request is excluded from the reported metrics and reuses input_requests[0], so it carries the lane's own ISL/OSL -- which is what makes it an effective warmup rather than a token-sized probe. The effect scales as setup_cost/num_requests: measured on GB300 disagg e2e lanes, median TTFT drops ~49% at 8 requests and ~0.26% at 10240, so short lanes gain and long ones are unaffected. warmup is passed to ClientConfig as a constructor argument rather than through client_config_data, so no lane yaml can enable it. b_warmup is reported but is deliberately not a baseline match key -- warmup is a measurement-quality knob, not part of case identity, and forking history would hide the improvement in its own series -- and that is only sound while the value stays fully determined by benchmark_mode. b_warmup records the effective value: to_cmd dispatches to three builders and only the built-in benchmark_serving one has a test request to suppress, so a warmup requested on an agentx or nv_sa lane is recorded as False rather than claiming a warmup that never ran. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
6 participants
Dev Engineer Review
ClientConfigadds disabled-by-defaultwarmupsupport and persists it asb_warmup.gen_onlylanes withconcurrency == 1.--no-test-inputonly when warmup is enabled.QA Engineer Review
tests/integration/defs/perf/test_perf_sanity.py.test-db/orqa/entries changed.Description
gen_only perf-sanity lanes measure as few as a single request:
iterationsisforced to 1 for gen_only mode. With only one measured request,
one-time cold-start costs land entirely on the measured TTFT. Concretely, with
the KV cache manager V2 (python cache transceiver), the lazy connection setup
(ZMQ mesh + NIXL metadata registration, ~6s) shows up as a fake -43% output
throughput regression on the GLM-5 con1 gen_only lane, while steady-state
performance is actually on par or better than V1.
Fix: stop passing
--no-test-inputfor gen_only lanes, so thatbenchmark_serving's built-in initial single-prompt test run (excluded frommetrics) acts as a warmup request.
Validation on the GLM-5 con1 gen_only lane (GB300, 8k1k, tep8 mtp3, KV cache
manager V2 cpp):
Test Coverage
perf/test_perf_sanity.py::test_e2e[disagg_upload-gen_only-*]lanes exercisethe changed path directly; all other lanes are unaffected (warmup defaults to
off).
disagg_upload-gen_only-gb300_glm-5-fp4_8k1k_con1_ctx1_dep2_gen1_tep8_eplb0_mtp3_ccb-NIXL(results above).
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.