Skip to content

fix(cli): format structured responses, and never report a generation as silence - #1214

Merged
justinchuby merged 2 commits into
mainfrom
fix/cli-structured-responses-v2
Aug 18, 2026
Merged

justinchuby merged 2 commits into
mainfrom
fix/cli-structured-responses-v2

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Unstacked replacement for #1161, which GitHub will not let us merge: it was stacked on #1158, and the stack metadata survives even though that PR is now closed and its content is in main via #1200. gh pr merge, gh pr edit --base, and both async merge endpoints all refuse or 404. Same branch content, off main, no stack.

What it does

Formats structured model responses in the CLI, and fixes a silent-empty-reply bug found while reviewing it.

load_response_config succeeds from tokenizer_config.json alone, but the marker strings are only resolved to token ids later, against the tokenizer. If a template names a marker the tokenizer does not encode as a single special token, message_token_id stays None -- and ResponseStream starts in Header, whose only exit is message_token_id == Some(token.token_id). None never equals Some(_), so the parser is trapped in a state where every output carries include_in_reply: false. The caller gets an empty string and no error: a good generation reported as silence.

The fix withdraws structured mode entirely rather than degrading in place. Raw output shows the markers as literal text -- a poorer presentation, but an honest one. Silence is neither.

It is a mechanism, not a patch: bind_response_tokens is the single guarded entry point, and bind_token_ids is now module-private so no future caller can bind without the check. Only message_token_id is treated as load-bearing; withdrawing on the optional markers would drop structured mode for templates that parse fine.

Validation

  • onnx-genai-cli lib: 121 passed / 0 failed
  • engine lib: 508 / 0 / 1, matching main's baseline
  • Both new tests falsified: forcing is_parseable to true turns them RED on the exact assertion they claim to guard
  • The two repl_e2e attachment failures (missing required pipeline input 'encoder.pixel_values') reproduce identically on unmodified main and are unrelated to this branch

Supersedes #1161.

justinchuby and others added 2 commits August 17, 2026 21:27
Apply all metadata EOS token IDs in pipeline generation so ATEM models stop at end-of-turn. Parse tokenizer response templates to preserve and format generated channel markers while keeping only answer content in conversation history.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
`load_response_config` succeeds from `tokenizer_config.json` alone, but the
marker strings are only turned into ids later, against the tokenizer. When a
template names a marker the tokenizer does not encode as a single special
token, `message_token_id` stayed `None` -- and `ResponseStream` starts in
`Header`, whose only exit compares `message_token_id == Some(token.token_id)`.
`None` never equals `Some(_)`, so the parser was trapped in a state where every
output carries `include_in_reply: false`. The caller got an empty string and no
error: a good generation reported as silence.

Withdraw structured mode entirely instead of degrading in place. Raw output
shows the markers as literal text -- a poorer presentation, but an honest one.
Silence is neither.

Make it a mechanism rather than a patch: `bind_response_tokens` is the single
guarded entry point, and `bind_token_ids` is now module-private so no future
caller can bind without the check. Only `message_token_id` is load-bearing;
withdrawing on the optional markers would drop structured mode for templates
that parse fine.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
@justinchuby
justinchuby merged commit d1d15ef into main Aug 18, 2026
7 of 14 checks passed
@justinchuby
justinchuby deleted the fix/cli-structured-responses-v2 branch August 18, 2026 05:40
justinchuby added a commit that referenced this pull request Aug 18, 2026
… stale build (#1216)

A trap I walked into while validating #1214, worth writing down because
the failure mode is invisible.

Restoring a falsified file with `Copy-Item`/`Move-Item` preserves the
original `LastWriteTime`, so the restored file looks *older* than the
artifact cargo built from the broken version. cargo's freshness check is
mtime-based, sees nothing newer, and reuses the stale binary.

The direction it bit me in was benign -- I saw two red tests against
source I could read as correct. The dangerous mirror image is breaking
the code, restoring, seeing **green**, and concluding the test is
worthless. A falsification loop is exactly when you are toggling one
line and trusting the pass/fail signal completely, which makes it
exactly when a stale build does the most damage.

Fix is one line: `(Get-Item <path>).LastWriteTime = Get-Date` after any
restore.

Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
justinchuby added a commit that referenced this pull request Aug 18, 2026
`cargo fmt --all -- --check` was red on `main`: #1214 inserted
`bind_response_tokens` into two `use super::output::{...}` lists without
reflowing them. Pure formatting, no behaviour change; `--check` is clean
afterwards.

Note on #1204, which reports a trailing blank line at
`pipeline/mod.rs:3681` left by #1200: that no longer reproduces.
`main`'s copy is 3702 lines and ends on `}`, and `cargo fmt --all --
--check` does not flag it. That PR looks stale rather than wrong.

Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
@github-actions

Copy link
Copy Markdown

⚠️ Benchmark Change Detected

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
⚠️ gather/medium_f32_threads=1-internal/32768 4.39 µs 5.39 µs +22.8%
⚠️ matmul/medium_generic_f32_threads=8/32x512x512 919.96 µs 1.07 ms +16.1%
✅ gather/large_f32_threads=1-internal/131072 35.55 µs 40.11 µs +12.8%
✅ matmul/medium_generic_bf16_threads=8/32x512x512 366.17 µs 412.76 µs +12.7%
✅ matmul/medium_generic_f16_threads=8/32x512x512 30.35 µs 33.09 µs +9.0%
✅ gather/medium_bf16_threads=1-internal/32768 2.49 µs 2.68 µs +7.7%
✅ gather/medium_f16_threads=1-internal/32768 2.51 µs 2.67 µs +6.6%
✅ matmul/medium_generic_f16_threads=1/32x512x512 31.71 µs 33.27 µs +4.9%
✅ block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024 582.79 µs 610.82 µs +4.8%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.03 ms 1.06 ms +3.6%
✅ qwen3_sampling_processors/top_k_top_p_full_sort_baseline 5.86 ms 6.05 ms +3.3%
✅ gather/small_bf16_threads=1-internal/4096 501.6 ns 517.0 ns +3.1%
✅ sampling_latency/top_p_per_token 384.23 µs 393.62 µs +2.4%
✅ reduce_mean/medium_f32_threads=1-internal/65536 257.07 µs 263.25 µs +2.4%
✅ matmul/small_generic_bf16_threads=1/1x256x256 33.50 µs 34.26 µs +2.3%
✅ qwen3_sampling_processors/top_k_top_p_fast 672.85 µs 686.77 µs +2.1%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 539.38 µs 548.63 µs +1.7%
✅ qwen3_sampling_processors/top_p_fast_after_top_k 528.47 µs 535.72 µs +1.4%
✅ gather/small_f16_threads=1-internal/4096 538.0 ns 544.4 ns +1.2%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 82.13 µs 83.08 µs +1.2%
✅ add/large_f32_threads=1-internal/4194304 712.21 µs 718.10 µs +0.8%
✅ block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1 512.66 µs 516.19 µs +0.7%
✅ qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline 3.63 ms 3.65 ms +0.4%
✅ block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1 176.68 µs 177.14 µs +0.3%
✅ sampling_latency/greedy_per_token 3.28 µs 3.28 µs -0.1%
✅ reduce_mean/small_f32_threads=1-internal/4096 16.24 µs 16.22 µs -0.1%
✅ matmul/large_generic_bf16_threads=1/32x1024x1024 2.10 ms 2.09 ms -0.6%
✅ matmul/large_generic_f32_threads=8/32x1024x1024 4.15 ms 4.10 ms -1.1%
✅ matmul/small_generic_f32_threads=1/1x256x256 40.75 µs 40.18 µs -1.4%
✅ sampling_latency/top_k_per_token 52.94 µs 52.08 µs -1.6%
✅ tokenization/decode_tokens_per_second 6.63 ms 6.51 ms -1.9%
✅ logit_processing/seven_processor_chain_per_step 328.58 µs 321.16 µs -2.3%
✅ matmul/large_generic_f32_threads=1/32x1024x1024 9.89 ms 9.61 ms -2.9%
✅ gather/small_f32_threads=1-internal/4096 734.0 ns 711.9 ns -3.0%
✅ sampling_latency/min_p_per_token 221.11 µs 212.39 µs -3.9%
✅ matmul/small_generic_f16_threads=8/1x256x256 35.36 µs 33.84 µs -4.3%
✅ matmul/small_generic_bf16_threads=8/1x256x256 36.46 µs 34.76 µs -4.7%
✅ grammar_masking/llguidance_compute_mask/32 85.32 µs 81.00 µs -5.1%
✅ matmul/large_generic_f16_threads=8/32x1024x1024 96.84 µs 90.41 µs -6.6%
✅ kv_cache/alloc_dealloc_pages 40.65 µs 37.92 µs -6.7%
✅ add/large_bf16_threads=1-internal/4194304 2.01 ms 1.86 ms -7.6%
✅ block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024 60.56 µs 55.79 µs -7.9%
✅ qwen3_sampling_processors/top_k_full_sort_baseline 2.38 ms 2.17 ms -8.7%
✅ tokenization/encode_tokens_per_second 438.36 µs 395.78 µs -9.7%
✅ matmul/large_generic_bf16_threads=8/32x1024x1024 1.73 ms 1.56 ms -9.7%
✅ matmul/small_generic_f32_threads=8/1x256x256 43.49 µs 39.12 µs -10.0%
✅ matmul/small_generic_f16_threads=1/1x256x256 38.79 µs 32.99 µs -15.0%
🟢 add/small_f32_threads=1-internal/1024 238.7 ns 199.8 ns -16.3%
🟢 matmul/medium_generic_f32_threads=1/32x512x512 2.98 ms 2.45 ms -17.9%
🟢 add/small_f16_threads=1-internal/1024 603.6 ns 492.6 ns -18.4%
🟢 block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024 68.02 µs 53.92 µs -20.7%
🟢 add/small_bf16_threads=1-internal/1024 581.4 ns 451.0 ns -22.4%
🟢 add/large_f16_threads=1-internal/4194304 2.17 ms 1.68 ms -22.6%
🟢 add/medium_f32_threads=1-internal/262144 32.58 µs 25.04 µs -23.1%
🟢 qwen3_sampling_processors/top_k_partial_selection 185.84 µs 141.52 µs -23.8%
🟢 gather/large_f16_threads=1-internal/131072 17.74 µs 13.50 µs -23.9%
🟢 gather/large_bf16_threads=1-internal/131072 16.05 µs 11.75 µs -26.8%
🟢 add/medium_bf16_threads=1-internal/262144 153.63 µs 104.56 µs -31.9%
🟢 add/medium_f16_threads=1-internal/262144 190.66 µs 104.86 µs -45.0%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.5.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 3.62 3.48 5.71 }
What this cannot catch
  • Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
  • Sub-threshold regressions that compound over multiple PRs
  • Performance changes that only manifest under GPU execution
  • Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)

@codecov

codecov Bot commented Aug 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.90%. Comparing base (2f8ba9b) to head (7607fa1).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #1214    +/-   ##
========================================
  Coverage   79.89%   79.90%            
========================================
  Files         359      359            
  Lines      157798   157898   +100     
  Branches   157798   157898   +100     
========================================
+ Hits       126076   126165    +89     
- Misses      27110    27120    +10     
- Partials     4612     4613     +1     
Flag Coverage Δ
mlas 85.39% <ø> (+0.03%) ⬆️
offline 79.79% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants