[None][test] Restrict gen-worker per-iter mean to steady-state iterations - #16298
Conversation
📝 WalkthroughWalkthroughThe performance sanity test now averages numeric device step times only from iterations at least 5 that match each gen-worker log’s maximum ChangesSteady-state parsing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/defs/perf/test_perf_sanity.py (1)
201-242: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCompare each file’s observed max before declaring the logs settled.
parse_gen_worker_device_step_timeonly waits fortotal_countto repeat, but that count can stay unchanged while a later poll sees a highernum_generation_tokensmaximum. That can stop the loop on a stale batch and return the wrong steady-state mean. Return each file’smax_ngenfrom_scan_gen_worker_device_step_timeand require both the count and maxima to stabilize.🤖 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 201 - 242, The polling logic in parse_gen_worker_device_step_time must stabilize per-file maxima as well as total_count. Update _scan_gen_worker_device_step_time to return each file’s observed max_ngen alongside its means and count, then track and compare those maxima between polls; continue polling whenever either the total count or any file’s max_ngen changes, and only compute the final steady-state result after both remain unchanged.
🧹 Nitpick comments (1)
tests/integration/defs/perf/test_perf_sanity.py (1)
201-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit test coverage for the new two-pass scan.
No tests exercise
_scan_gen_worker_device_step_time's new max-num_generation_tokensfiltering (ties at the max, all-N/A lines,iter < 5exclusion, tail rows with lower token counts than the plateau, multi-file averaging). Given the algorithm's added complexity (two passes, dynamic per-file max, Welford accumulation), targeted tests usingtmp_path-backed fakegen_server_{i}.logfiles would materially increase confidence without much effort. A file such astests/unittest/.../test_perf_sanity_device_step_time.py(or a small test class colocated in this module) mocking log content per scenario would suffice; coverage for this specific logic currently appears insufficient.As per path instructions, "Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM... suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR."
🤖 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 201 - 242, Add targeted unit tests for _scan_gen_worker_device_step_time using tmp_path-backed gen_server_{i}.log fixtures, covering max-num_generation_tokens filtering, ties at the maximum, all-N/A input, exclusion of iter values below 5, lower-token tail rows, and multi-file mean aggregation. Keep the tests focused on the two-pass scan and Welford results, colocated in the existing module or a dedicated test_perf_sanity_device_step_time.py file.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.
Outside diff comments:
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 201-242: The polling logic in parse_gen_worker_device_step_time
must stabilize per-file maxima as well as total_count. Update
_scan_gen_worker_device_step_time to return each file’s observed max_ngen
alongside its means and count, then track and compare those maxima between
polls; continue polling whenever either the total count or any file’s max_ngen
changes, and only compute the final steady-state result after both remain
unchanged.
---
Nitpick comments:
In `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 201-242: Add targeted unit tests for
_scan_gen_worker_device_step_time using tmp_path-backed gen_server_{i}.log
fixtures, covering max-num_generation_tokens filtering, ties at the maximum,
all-N/A input, exclusion of iter values below 5, lower-token tail rows, and
multi-file mean aggregation. Keep the tests focused on the two-pass scan and
Welford results, colocated in the existing module or a dedicated
test_perf_sanity_device_step_time.py file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 681b7445-649c-4204-b0b6-742ad836b80f
📒 Files selected for processing (1)
tests/integration/defs/perf/test_perf_sanity.py
…ions
The disagg gen_only Mean Gen Worker Per-Iter Device Step Time metric
currently averages every iter >= 5. Tail iterations near the end of a
run have a shrinking num_generation_tokens as sequences finish, so their
device step time is systematically lower than the steady-state cost and
drags the mean down.
Filter to steady state by, for each gen_server_{i}.log:
1. Scanning iter >= 5 rows once to find the maximum
num_generation_tokens in that file (the steady-state batch);
2. Averaging prev_device_step_time only over rows whose
num_generation_tokens equals that max.
The scanner stays O(1) memory (two file passes, Welford mean in the
second). The regex now also captures num_generation_tokens. The
settle-detection contract is unchanged: total_count is the number of
included rows, and once it is non-zero and stable across two polls we
return the metric.
Signed-off-by: chenfeiz0326 <chenfeiz@nvidia.com>
5935833 to
15b122d
Compare
|
/bot run --disable-fail-fast --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-2" |
|
PR_Github #58901 [ run ] triggered by Bot. Commit: |
|
PR_Github #58901 [ run ] completed with state |
…egex Rework gen-worker per-iter mean parsing to address three issues raised in review of the previous approach: 1. Settle contract: total_count is again monotonic. The previous iteration counted only rows whose num_generation_tokens equaled the file's running max, so a mid-flush max_ngen jump could hold the count flat for two consecutive polls and trigger early return on a non-steady-state sample. total_count now sums every iter >= 5 row with a numeric prev_device_step_time (rows are append-only across polls) while the per-ngen bucket dict is used only after the poll loop settles. 2. Bucket selection: mode ngen with tie-break to the larger value replaces strict == max. A single-iter ngen spike could previously collapse the mean to 1-2 samples (ds-r1: 253 -> 252 iters, mean 53.98 -> 53.30 ms). The mode is the sustained batch by definition and is robust to that spike; ties resolve to the larger ngen so we still pick the upper of any tied clusters when they exist. 3. Regex order coupling: num_generation_tokens is now captured by a separate regex applied to the same line, not chained after prev_device_step_time. num_generation_tokens is always present in the states dict and its name is stable, but its position within the line is not, so the previous chained pattern silently dropped rows whose states dict was printed first. The scan is now a single pass building per_file_by_ngen: List[Dict[int, Tuple[int, float]]] (count + Welford mean per ngen bucket), removing the second read of large gen logs. Signed-off-by: chenfeiz0326 <chenfeiz@nvidia.com>
7676afe to
74998f7
Compare
|
/bot run --disable-fail-fast --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-2" |
|
PR_Github #58919 [ run ] triggered by Bot. Commit: |
|
PR_Github #58919 [ run ] completed with state
|
Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
c4018bf to
b8dedde
Compare
|
/bot skip --comment "Only update gen only test's metric calculation, no need to run the whole CI pipeline" |
|
PR_Github #58945 [ skip ] triggered by Bot. Commit: |
|
PR_Github #58945 [ skip ] completed with state |
The disagg gen_only perf-sanity regression metric
mean_gen_worker_per_iter_device_step_time is parsed from each
gen_server_{i}.log by parse_gen_worker_device_step_time(). Since NVIDIA#16298
introduced steady-state ngen bucketing, a gen worker whose
prev_device_step_time lines are all present and numeric can still yield a
None metric if num_generation_tokens fails to match _NUM_GEN_TOKENS_RE on
every row (absent from the states dict, or a non-bare-int render). A None
metric then trips PerfSanityTestConfig.check_test_failure() with a
misleading "missing 'prev_device_step_time'" RuntimeError even though the
data was logged and the workload passed (nvbugs 6487036 / 6487040).
Fix: _scan_gen_worker_device_step_time now also accumulates a per-file
un-bucketed all-iter Welford mean over every iter>=5 numeric
prev_device_step_time row. _mean_at_mode_ngen prefers the steady-state
mode-ngen bucket (unchanged for healthy logs) but falls back to that
all-iter mean for any worker whose buckets are empty, so a present metric
is never dropped to None. The fallback path logs a print_info. Returns
None only when no usable row exists in any worker at all.
Verified behavior-preserving on a real passing gen_server_0.log (v32
gen_only mtp3 on lyris GB200: 12.91 ms before and after). Adds
test_gen_worker_device_step_time_parser.py (10 CPU-only cases covering the
mode-bucket path, iter<5 / N/A skips, the missing / unparsable
num_generation_tokens fallback, mixed-worker averaging, and the None
cases).
Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
5 participants
Summary
The disagg
gen_onlyMean Gen Worker Per-Iter Device Step Time metric intests/integration/defs/perf/test_perf_sanity.pycurrently averagesprev_device_step_timeover every log line whoseiterfield is>= 5.Near the end of a run,
num_generation_tokensshrinks as sequences finish,so those tail iterations have systematically shorter device step times and
drag the mean below the steady-state cost we actually want to track.
This PR restricts the mean, per
gen_server_{i}.log, to iterations at themode (most-frequent)
num_generation_tokensvalue observed foriter >= 5rows in that file, with ties broken toward the larger ngen:gen_server_{i}.logbuilds aDict[num_generation_tokens, (count, Welford mean of prev_device_step_time)]over
iter >= 5rows with a numericprev_device_step_time, andindependently counts every such row into
total_count.most rows is selected (ties → largest ngen) and its mean is taken as
that file's contribution. The per-file means are then averaged.
num_generation_tokensis captured by a separate regex applied to thesame line — the name is stable, but its position within the line relative
to
prev_device_step_timeis not, so chaining the two into one patternwould silently drop any row whose
statesdict was printed first.total_countis the number ofiter >= 5rows with a numericprev_device_step_timeacross all files — not the size of the modebucket. This keeps the settle signal monotonic across polls (rows only ever
get appended as NFS flushes), so an in-flight ngen shift while the tail is
still flushing cannot hold the count flat for two consecutive polls and
trigger a premature return on a non-steady-state sample.
Note on
iter >= 5:iterhere is the literal value of theiterfieldin the log line, not a running line count. Gen-worker logs emit many
iter = 0prefill/scheduling lines and a warmupiter = 1line beforegeneration begins, and
< 5drops all of them before we ever look atnum_generation_tokens.Method comparison
iter >= 5iter >= 5andnum_generation_tokens == mode(ngen)(ties → largest ngen)(iter, prev_device_step_time)(iter, prev_device_step_time)from_DEVICE_STEP_TIME_REplusnum_generation_tokensfrom an independent_NUM_GEN_TOKENS_REapplied to the same line (position-independent)gen_server_{i}.logngenspike== maxwould have collapsed the file mean to 1–2 samples)parse_gen_worker_device_step_timeiter >= 5rows — monotonic across polls, so a mid-flush ngen shift cannot spoof "stable"Impact on real logs
Sanity-checked on two real gen worker logs:
The gpt_oss case shows the failure mode this fix addresses: the tail
included 1329 iterations with
num_generation_tokensranging 1–960, allwith shorter device step times, pulling the mean ~5% below the real
steady-state cost. The new metric reports the correct steady-state value.
Review defects addressed
Three defects were flagged on the earlier revision of this PR; the current
code addresses each:
total_countwas the size of the filtered (mode/
== max) bucket, so a mid-runmax_ngenjump could hold the count flat for two consecutive polls andreturn a mean computed on a non-steady-state sample. Fixed by making
total_countcount everyiter >= 5row with a numeric device time(monotonic across polls) and moving the ngen bucket selection after
the settle poll drains.
== maxwith no sample-size protection. Replaced by modengen (ties → largest ngen). A single-iter spike above the sustained
batch used to lock
max_ngenonto that spike and collapse the mean to1–2 samples; the mode is unaffected.
num_generation_tokensto appear afterprev_device_step_timeonthe line.
num_generation_tokensis now captured by a separate regexapplied to the same line, so ordering does not matter.
Test plan
the reported
d_mean_gen_worker_per_iter_device_step_timematchesthe manual computation on the collected gen_server logs.
workloads whose tail is small, and produce a higher, more accurate
value for workloads with long tails.
statesdict captured, iter<5 dropped,N/Adevice-time dropped,settle counter monotonic across a mid-flush
max_ngenjump, modewins over a one-off ngen spike, tie-break resolves to the larger
ngen, and rows missing
num_generation_tokensare still countedtoward the settle signal.