[https://nvbugs/6487040][test] Robustify gen_only per-iter device-step-time metric parsing#16717
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe gen-worker step-time parser now retains usable samples without parseable ChangesGen-worker device step-time parsing
MTP worker entrypoint
Performance waiver cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant GenWorkerLogs
participant Scanner
participant Aggregator
Parser->>GenWorkerLogs: read gen_server_*.log files
Parser->>Scanner: scan usable rows
Scanner-->>Parser: return per-file scans and total count
Parser->>Aggregator: select settled or timeout result
Aggregator-->>Parser: return bucket mean or all-iteration fallback
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_gen_worker_device_step_time_parser.py`:
- Around line 30-34: Update the isolated parser loader used by test_perf_sanity
to accurately document that it reads and executes a source slice rather than
using importlib. At the exec call, add a narrowly scoped Ruff S102 suppression
with justification that _TPS_PATH is a fixed, repository-local trusted path;
avoid broad lint disables or unrelated changes.
- Around line 214-229: Strengthen
test_scan_counts_all_usable_rows_regardless_of_ngen by assigning distinct
step-time values to the bucketed and fallback-only row groups, while preserving
their separate num_generation_tokens states. Assert _mean_at_mode_ngen() returns
the mean for the selected ngen bucket rather than all_mean, and update expected
aggregate values accordingly; complete the required coverage review for this
test change.
🪄 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: 2b12569b-ab7d-4b3f-8135-ad7d00ad5c39
📒 Files selected for processing (2)
tests/integration/defs/perf/test_gen_worker_device_step_time_parser.pytests/integration/defs/perf/test_perf_sanity.py
|
This is a The 10 cases map to 5 post-merge Disagg-PerfSanity stages (these stages shard
/bot run --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-1,DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-3,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-4,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-5,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-2,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-1,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-2" |
|
/bot run --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-1,DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-3,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-4,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-5,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-2,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-1,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-2" |
|
PR_Github #60958 [ run ] triggered by Bot. Commit: |
|
PR_Github #60958 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-1,DGX_B200-16_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE1-GPU8-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU2-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-2,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-3,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-4,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-5,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-2,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-1,GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU2-GEN1-NODE2-GPU8-Post-Merge-2" |
|
PR_Github #60997 [ run ] triggered by Bot. Commit: |
|
PR_Github #60997 [ run ] completed with state |
|
/bot run --disable-fail-fast --stage-list "GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-2,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-2,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-3,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-4,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-5,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-6,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-7,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-8" |
|
PR_Github #61272 [ run ] triggered by Bot. Commit: |
|
PR_Github #61272 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-4,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-8,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-5,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3" |
|
PR_Github #61284 [ run ] triggered by Bot. Commit: |
|
PR_Github #61284 [ run ] 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>
… test - Fix loader docstring: it reads a fixed source slice and exec()s it; it does not use importlib. Document the trusted, repository-local path and add a narrowly-scoped `# noqa: S102` for the intentional exec. - Strengthen test_scan_counts_all_usable_rows_regardless_of_ngen: use distinct step times (10.0 bucketed vs 99.0 fallback-only) and assert _mean_at_mode_ngen() returns the bucket mean, so the test can no longer pass if aggregation wrongly selects the all-iter mean when a bucket exists. Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
…f-sanity cases Remove the SKIP waivers for the two disagg gen_only NIXL perf-sanity cases now that the device-step-time parser fix is in place, so CI validates them: - gb200 deepseek-v32-fp4 8k1k con1 ...tep8 eplb0 mtp3 (nvbugs/6487040) - gb200 deepseek-v32-fp4 8k1k con4096...dep32 eplb256 mtp0 (nvbugs/6487036) Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
… bot rerun Un-waive the 3 still-waived affected cases from nvbug 6487036 so the /bot run can validate the prev_device_step_time parser fix: - disagg_upload-gen_only-gb200_deepseek-r1-fp4_8k1k_con1024_ctx1_dep4_gen1_dep32_eplb0_mtp3_ccb-NIXL - disagg_upload-gen_only-gb200_deepseek-r1-fp4_8k1k_con1_ctx1_dep4_gen1_tep8_eplb0_mtp3_ccb-NIXL - disagg_upload-gen_only-gb200_gpt-oss-120b-fp4_8k1k_con128_ctx1_tp1_gen1_tp4_eplb0_mtp0_ccb-NIXL Signed-off-by: Chenfei Zhang <chenfeiz@nvidia.com>
91be512 to
daa4def
Compare
|
/bot run --disable-fail-fast --stage-list "GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1,GB200-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-3,GB200-8_GPUs-2_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU1-GEN1-NODE1-GPU4-Post-Merge-1,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-5,GB200-36_GPUs-9_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE8-GPU32-Post-Merge-1" |
|
PR_Github #61501 [ run ] triggered by Bot. Commit: |
|
PR_Github #61501 [ run ] completed with state
|
Dev Engineer Review
parse_gen_worker_device_step_timeparsing/aggregation to avoid falseNonewhennum_generation_tokenscannot be parsed despite validprev_device_step_timesamples:_scan_gen_worker_device_step_timenow returns per-file(by_ngen, all_iter_count, all_iter_mean)whereall_iter_meanis a Welford mean over all numericprev_device_step_timerows withiter >= 5, independent of whethernum_generation_tokensparsed._mean_at_mode_ngennow:num_generation_tokensmode bucket (by_ngen) when non-emptyall_iter_meanwhenby_ngenis empty butall_iter_countindicates usable rowsNoneonly when no usable numericprev_device_step_timerows exist across any scanned fileprint_infowhen fallback is usedparse_gen_worker_device_step_timepolling/settling loop now uses the scanner’s monotonictotal_count(rawiter>=5usable row count) to decide when logs have flushed before computing the mean.tests/integration/defs/perf/test_gen_worker_device_step_time_parser.py:gen_server_*.logfilestests/integration/defs/perf/test_perf_sanity.pyandexec()-ing it into a minimal namespace (stubbingprint_info) to avoid heavy integration dependency import chainsiter < 5, ignoring non-numericprev_device_step_time(e.g.,N/A), fallback whennum_generation_tokensis missing/unparsable, mixed-worker averaging,Nonewhen no usable data exists,Nonewhen logs are missing, and scan counting correctness.MTPEagleDynamicTreeWorker.forward(...)→MTPEagleDynamicTreeWorker._forward_impl(...)intensorrt_llm/_torch/speculative/mtp_dynamic_tree.pywith the NVTX decorator preserved.QA Engineer Review
tests/integration/defs/perf/test_gen_worker_device_step_time_parser.pytest_healthy_bare_int_uses_mode_ngen_buckettest_iters_below_five_are_skippedtest_na_device_step_time_is_ignoredtest_missing_num_generation_tokens_falls_back_to_all_iter_meantest_unparsable_num_generation_tokens_falls_back(parametrized)test_mixed_workers_bucket_and_fallback_averagetest_no_usable_lines_returns_nonetest_missing_log_file_returns_nonetest_scan_counts_all_usable_rows_regardless_of_ngentests/integration/defs/perf/test_perf_sanity.py(updatedprev_device_step_timegen-worker scan/aggregation logic used by the metric parser)tests/integration/test_lists/waives.txtperf/test_perf_sanity.py::test_e2ewaiver entries fordisagg_upload-gen_onlyongb200DeepSeek by removing a subset of SKUs associated with nvbugs/6487040 (as part of aligning CI expectations with the parser fix).tests/integration/test_lists/(test-db/ for CI, qa/ for manual QA): not indicated for the new pure-CPU unit tests; the waiver removals are intended to re-enable/validate the previously skippedgen_onlydisagg perf-sanity cases.Description
The disagg
gen_onlyperf-sanity regression metricmean_gen_worker_per_iter_device_step_timeis parsed from eachgen_server_{i}.logbyparse_gen_worker_device_step_time()(
tests/integration/defs/perf/test_perf_sanity.py). Since #16298 restricted themean to steady-state iterations by bucketing rows on
num_generation_tokens, agen worker whose
prev_device_step_timelines are all present and numeric canstill yield a
Nonemetric whennum_generation_tokensfails to match_NUM_GEN_TOKENS_REon every row (the key is absent from thestatesdict, orrendered in a non-bare-int form). A
Nonemetric then tripsPerfSanityTestConfig.check_test_failure()with a misleadingRuntimeError: ... is missing 'prev_device_step_time'even though the value waslogged and the workload passed. This is the false-failure class reported in
nvbugs 6487036 (
disagg_gen_missing_prev_device_step_time) and 6487040(
disagg_gen_prev_device_step_time_parse_mismatch), children of the L0_PostMerge#2846 triage bug 6484688.
Re-running the 6487040 case (
disagg_upload-gen_only-gb200_deepseek-v32-fp4_8k1k_con1_ctx1_dep4_gen1_tep8_eplb0_mtp3_ccb-NIXL)on GB200 passed (metric
12.91 ms), so the failure is intermittent — theexact per-line trigger from #2846 was not captured. This change hardens the
parser so the intermittent extraction miss can no longer turn a present metric
into a spurious CI failure, rather than chasing a specific unreproduced render.
Fix:
_scan_gen_worker_device_step_timenow also accumulates a per-fileun-bucketed all-iter Welford mean over every
iter >= 5numericprev_device_step_timerow._mean_at_mode_ngenstill prefers the steady-statemode-
ngenbucket (behavior unchanged for healthy logs) but falls back to thatall-iter mean for any worker whose buckets are empty, so a present metric is
never dropped to
None. The fallback path emits aprint_info. The parserreturns
Noneonly when no usable row exists in any worker at all — restoring thepre-#16298 guarantee while keeping the steady-state refinement.
Test Coverage
tests/integration/defs/perf/test_gen_worker_device_step_time_parser.py(10 CPU-only cases): healthy bare-int mode-bucket;
iter < 5andN/Askips;missing / unparsable
num_generation_tokens-> all-iter fallback (notNone);mixed bucketed + fallback worker averaging; and the
Nonecases (no usablelines, missing file). The test importlib-loads the parser block with a stubbed
print_infoso it runs standalone without the module's GPU/DB import chain.gen_server_0.logcaptured fromthe v32
gen_onlymtp3case on lyris GB200:12.91 msbefore and after thechange (the mode-
ngenpath, unchanged).PR Checklist