Repository navigation
feat(cli): show per-turn stats by default for generate - #325
Conversation
The REPL has shown stats by default on a TTY since #289, but one-shot generate did not: it only emitted anything when --profile was passed explicitly. So the command Justin uses for native-vs-ORT benchmarking printed no timing, no token counts, and no context usage unless you knew to ask. That is the wrong default for a tool whose charter is a developer instrument rather than a consumer product. Text generate now defaults to the compact stats line on a TTY, reusing the REPL's repl_input_mode and initial_repl_show_stats rather than adding a second TTY check that could disagree, and emitting through the shared emit_stats_line so there is one format rather than two. generate --no-stats opts out, matching run --no-stats, and --profile suppresses the compact line so the two cannot double-print. Stats go to stderr and generated text stays on stdout. That split is what keeps piped generate byte-stable for the scripts and e2e tests that consume it, and it is now covered by a test asserting piped stdout is byte -identical. Image and audio generate, and transcribe, deliberately do not get the compact token stats: tokens per second is meaningless for image generation, so those paths keep --profile for steps, RTF and audio metrics rather than gaining a misleading line. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 023d5a3f-c551-4aa8-afbe-a329af15fbdd
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
- Coverage 81.04% 80.28% -0.76%
==========================================
Files 306 306
Lines 116658 116736 +78
Branches 116658 116736 +78
==========================================
- Hits 94544 93724 -820
- Misses 18200 19103 +903
+ Partials 3914 3909 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
我说错了,repl有stats就行了。oneshot不需要默认展示 |
✅ Benchmarks — No RegressionComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
There was a problem hiding this comment.
Pull request overview
This PR updates the onnx-genai CLI so generate (text mode) shows the same compact per-turn stats line by default on an interactive TTY, matching the REPL behavior, while preserving the stdout/stderr contract for scriptable usage.
Changes:
- Add
generate --no-statsopt-out and default-on compact stats for TTY text generation (stats on stderr, generated text on stdout). - Reuse the existing REPL TTY detection (
repl_input_mode/initial_repl_show_stats) and the shared stats emitter (emit_stats_line) to avoid divergent behavior/formatting. - Add regression coverage ensuring piped
generatestdout remains byte-identical with and without the default-stats behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the unified “stats on TTY, stderr-only” behavior and the piping contract. |
| crates/onnx-genai-cli/tests/multimodal_cli_e2e.rs | Adds an e2e regression test asserting piped generate stdout stays pure generated text. |
| crates/onnx-genai-cli/src/output.rs | Refactors stats emission gating into a helper and adds a small unit test. |
| crates/onnx-genai-cli/src/lib.rs | Introduces --no-stats for generate and adds a CLI parsing test. |
| crates/onnx-genai-cli/src/generate.rs | Implements TTY-gated default stats for text generate, and explicitly excludes image/audio outputs. |
| crates/onnx-genai-cli/README.md | Updates CLI crate docs to reflect default stats behavior and stderr routing. |
| .squad/agents/zhora/history.md | Records the design decision/contract for default stats in generate. |
| assert!( | ||
| !stdout(&base).contains("tok/s") && !stdout(&base).contains("ttft"), | ||
| "stats must not contaminate piped stdout: {}", | ||
| stdout(&base) | ||
| ); |
The stdout-contamination check asserted on the substrings "tok/s" and "ttft". A model can legitimately generate either as prose, so the test could fail for a reason unrelated to what it guards -- the same flavour of mistake as the help test fixed earlier today, which compared merged stdout+stderr including timestamps and was flaky by construction. Replaces the blocklist with a shape match for the bracketed compact stats line: numeric in/out, backend, numeric tok/s, and numeric ttft in ms. The equality check against --no-stats is kept alongside it. Equality is what actually guarantees piped stdout is byte-stable and cannot miss a contaminant nobody thought of; the shape match makes a failure point directly at stats leakage rather than at unspecified drift. Together they give completeness and a precise diagnosis, which is why both are worth having. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 023d5a3f-c551-4aa8-afbe-a329af15fbdd
Drops the stray .msg.txt scratch file, which leaked onto main from my own commit-message workflow and is removed there by #335. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 023d5a3f-c551-4aa8-afbe-a329af15fbdd
Resolves an additive conflict in output.rs: #325 added should_emit_stats_line and its test while this branch added budget_cap_notice and its test, in the same two places. Both are kept. Verified after resolution: build, 96 CLI lib tests, clippy, fmt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 023d5a3f-c551-4aa8-afbe-a329af15fbdd
The gap
The REPL has shown stats by default on a TTY since #289. One-shot
generatedid not — it emitted nothing unless--profilewas passed explicitly. So the command being used for native-vs-ORT benchmarking printed no timing, no token counts, and no context usage unless you already knew to ask for it.Wrong default for a tool whose charter (#271) is explicitly a developer instrument rather than a consumer product.
Change
Text
generatenow defaults to the compact stats line on a TTY.Reuses rather than reimplements, which was the main constraint:
repl_input_mode/initial_repl_show_statsfor TTY detection — a second TTY check that could disagree with the first is the failure mode hereemit_stats_line— one format, not twogenerate --no-statsopts out, matchingrun --no-stats.--profilesuppresses the compact line so the two cannot double-print.The stdout/stderr contract
Stats go to stderr; generated text stays on stdout. That is what keeps piped
generatebyte-stable for the scripts and e2e tests that consume it — a stats line on stdout would break every script capturing model output.Covered by a test asserting piped stdout is byte-identical. This is not theoretical: the same class of regression broke #289 on its first pass, and a flaky test that compared merged streams had to be fixed earlier today.
Image, audio, and transcribe deliberately excluded
They do not get the compact token stats. Tokens per second is meaningless for image generation, so those paths keep
--profilefor steps, RTF and audio metrics rather than gaining a line that reads as authoritative and is not.Stating this explicitly so it reads as a decision rather than an oversight.
Gates
cargo build/test --lib/fmt --check/clippy -D warningspass. Model-loading e2e could not run locally (ORT API mismatch 27 vs 17 — #315 addresses that); thecli-ortCI lane covers Linux and Windows.Part of the CLI improvement track — see #271.