Add --duration flag to perf for time-budgeted benchmarking#1168
Add --duration flag to perf for time-budgeted benchmarking#1168xieofxie wants to merge 3 commits into
Conversation
Run the benchmark for a wall-clock budget (seconds) instead of a fixed --iterations count. After warmup, inference loops until the duration elapses (always at least one benchmark iteration). Ideal with --monitor, whose PDH counters need time to emit real utilization data. Rejected with --op-tracing, which runs its own fixed iteration count. Works across the single-model, monitored, and per-module paths. The live --monitor chart shows elapsed/total time progress in duration mode. The reported iteration count reflects the actual timed sample count instead of the unused --iterations default. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Needed for #1149 |
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Took a pass through this — the overall approach looks good. The warmup→timed-loop split, the at-least-one-iteration guarantee, the --op-tracing conflict, and the genai ignored-flag warning are all correct, and the tests cover the important paths.
A few minor nits (none blocking):
perf.py L732-734 — logger.info("Running benchmark: %d iterations + %d warmup", ...) still logs the fixed iterations count in --duration mode, so it reads e.g. "Running benchmark: 100 iterations + 10 warmup" for a run that's actually time-bounded. Might be worth branching to log the time budget there.
Two more inline below.
Address PR review nits: - Log duration/warmup (not iterations) at benchmark start in duration mode. - _run_simple_loop now logs elapsed/total seconds in duration mode instead of a meaningless iteration count over total_iterations. - LiveMonitorDisplay reads the loop's shared _BenchmarkClock start instead of self-stamping on first update(), so the time progress bar tracks the exact budget the loop stops on rather than lagging by one inference. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review @DingmaomaoBJTU. Addressed all three nits in 6b3f744:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The duration-mode loop in _run_simple_loop does not use the yielded index, so name it _ to satisfy CodeQL's unused-loop-variable check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Found one termination bug and one runtime-dispatch inconsistency in the new flag handling; details inline.
| ) | ||
| @click.option( | ||
| "--duration", | ||
| type=click.FloatRange(min=0, min_open=True), |
There was a problem hiding this comment.
Click FloatRange accepts non-finite values because comparisons such as nan <= 0 are false. That value reaches _benchmark_indices, where elapsed >= duration_sec is permanently false, so winml perf --duration nan never terminates; inf likewise creates an unbounded run. Please reject non-finite durations (for example with math.isfinite) and add a CLI regression test.
| # --duration replaces the fixed iteration count with a wall-clock budget. | ||
| # Op-tracing runs its own fixed, small iteration count, so the two are | ||
| # mutually exclusive. | ||
| if duration is not None and op_tracing: |
There was a problem hiding this comment.
This validation runs before the winml-genai dispatch, but both --duration and --op-tracing are registered in _GENAI_IGNORED_FLAGS. Passing either alone warns and continues, while passing both now fails before the ignored-flag warning. Please move the conflict check to the WinML path (after the GenAI early return), or explicitly exclude the GenAI runtime, so ignored options remain consistently non-fatal.
3 participants
Summary
Adds a
--duration <seconds>flag towinml perf. When set, the benchmark runs for a wall-clock time budget instead of a fixed--iterationscount. After warmup, inference loops until the duration elapses (always running at least one benchmark iteration so stats are never empty).This is ideal with
--monitor, whose PDH hardware counters need time to emit real utilization data — a small fixed iteration count often finishes before the counters produce meaningful samples.Details
--duration FLOAToption (must be> 0), plumbed throughBenchmarkConfig.duration._benchmark_indices()helper centralizes the loop: warmup first, then either a fixed iteration count (default) or a timed loop. Used by the single-model, monitored, and per-module paths.--op-tracing(op-tracing runs its own fixed, small iteration count) — rejected with a clearUsageError.--monitorchart shows elapsed/total time progress (Time: 12.3/30s) in duration mode instead of an iteration counter.--iterationsdefault. For per-module runs (where each module runs a different count under a shared budget), the count is recorded per instance and the top-level value isnull; both single-model and module reports also recordduration_sec.--durationis added to the genai "ignored flags" warning (genai benchmarks by token generation).Tests
--durationhelp/forwarding/default,--op-tracingconflict, non-positive rejection._benchmark_indicesiteration mode, timed budget, and at-least-one-iteration guarantee (deterministic via a faked clock).LiveMonitorDisplaytime-based vs iteration-based progress.benchmark_info.iterationsreports configured count without duration and actual sample count with duration.All perf suites pass (
test_perf_cli,test_perf_module,test_perf_composite,test_ep_monitor): 200 passed, 2 NPU-only skips;ruffclean.