Add --duration flag to perf for time-budgeted benchmarking - #1168
Merged
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>
Contributor
Author
|
Needed for #1149 |
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
Collaborator
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>
Contributor
Author
|
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
requested changes
Jul 23, 2026
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
Collaborator
There was a problem hiding this comment.
Found one termination bug and one runtime-dispatch inconsistency in the new flag handling; details inline.
…tion # Conflicts: # src/winml/modelkit/commands/_live_chart.py # src/winml/modelkit/commands/perf.py # tests/unit/session/test_ep_monitor.py
Reject non-finite --duration values (nan/inf) via a Click callback since FloatRange lets them through, and move the --duration/--op-tracing conflict check into the WinML-only path so both flags stay non-fatal (ignored) for winml-genai. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
Collaborator
There was a problem hiding this comment.
Overall well-structured feature with good test coverage. 1 bug (warmup progress bar in duration mode) and several minor suggestions - see inline comments.
The warmup progress bar scaled by total_iterations, which in duration mode includes the unused default iteration count, so warmup 5/10 rendered as ~5%. Split warmup into its own branch in _render_status so it scales by the warmup count, and make the iteration-mode benchmark bar match its label. Also: clarify the --duration help text as a minimum budget, note +inf (vs the range-rejected -inf) in the validation docstring, group --duration with the other run-control flags in the genai ignored-flags map, and log benchmark-only progress in the simple loop. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DingmaomaoBJTU
approved these changes
Jul 27, 2026
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! All review comments addressed cleanly:
- Warmup progress bar bug fixed with correct branch restructuring
- Progress log now excludes warmup from denominator
- Help text clarifies minimum-budget semantics
- Docstring accurately describes +inf/-inf behavior
- Dict entries regrouped logically
- Regression test added for the warmup bar fix
Nice work!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.