Skip to content

fix(server): let a package's declared sampling stand when the caller is silent (replaces #1236) - #1250

Merged
justinchuby merged 2 commits into
mainfrom
serverstack-1236
Aug 18, 2026
Merged

justinchuby merged 2 commits into
mainfrom
serverstack-1236

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Replacement for #1236, rebased onto current main after #1197/#1224 landed as squashes (the stacked base diverged). Same fix, plus one added guard test, re-validated on top of the merged stack.

The defect

temperature and top_p carried the OpenAI schema's documented 1.0 defaults via #[serde(default = ...)], so the server could not tell a caller who chose 1.0 from a caller who said nothing. Both were then handed to resolve_sampling_defaults as explicit overrides, so a model that declares its own sampling regime lost it on every request. An absent field is "no opinion", not the documented default.

The fix

Both fields become Option<f32>; an absent one is passed as None, so the package's declared value wins — the same treatment top_k already had. An explicit value (including temperature: 0, greedy) still wins. Validation (validate_sampling_range) now judges only what the caller actually sent. The same fix applies to legacy /v1/completions.

Verification (mine, on this branch atop merged #1197 + #1224)

  • 211 lib + 31 http + 11 bundle tests pass; cargo clippy -p onnx-genai-server --all-targets -- -D warnings clean.
  • Falsified the core claim (revert hunk → RED → restore → GREEN): reverting chat_sampling_overrides to forward Some(1.0) for an absent field made a_silent_request_keeps_the_declared_temperature_and_top_p see Some(1.0) instead of None, and silent_chat_request_honors_model_do_sample override the declared 0.6 with 1.0; restored, the declared value survives.
  • Absent-vs-default trap, added test: an_explicit_default_valued_temperature_still_overrides_the_package asserts a caller that explicitly sends temperature: 1.0 (equal to the old schema default) still overrides a package declaring 0.6. This documents that absence (None → package wins) and an explicitly-sent default (Some(1.0) → caller wins) are now distinct — the exact distinction this PR exists to make.
  • End to end (qwen05b-q4, native): a silent request answers normally; legacy /v1/completions at temperature: 0 is bit-identical across two runs, so the greedy path is untouched; an explicit temperature: 1.0 is accepted and a negative one rejected.

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

justinchuby and others added 2 commits August 18, 2026 07:48
…is silent

`temperature` and `top_p` carried the OpenAI schema's documented 1.0
defaults, so serde could not tell a caller who chose 1.0 from a caller
who said nothing. Both were then forwarded to the engine as explicit
overrides, and a package that declares its own sampling regime — this
model declares top_p 0.95 — had it silently replaced by the full
distribution.

An agent client sends neither field. OpenCode's requests carry no
sampling controls at all, so every turn it drove was sampled from the
whole tail rather than from the nucleus the package asked for, which is
how a long agent turn degenerates.

Both fields are now optional, and an absent one is passed to
`resolve_sampling_defaults` as "no opinion" so the declared value wins,
exactly as `top_k` already did. Validation likewise judges only what the
caller actually sent. `temperature: 0` still means greedy, and an
explicit value still beats the package.

The same defect and the same fix apply to legacy `/v1/completions`,
whose doc comment had recorded the mistaken premise outright: that the
schema "has no unspecified state".

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
…rrides the package

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby merged commit 20aa31a into main Aug 18, 2026
6 of 14 checks passed
@justinchuby
justinchuby deleted the serverstack-1236 branch August 18, 2026 14:50
justinchuby added a commit that referenced this pull request Aug 18, 2026
CI's format lane had fallen behind — `cargo fmt --all -- --check` was
failing on `main` at `7d669c76`.

The drift came from several changes landing in the same window: the
session-state unification (#1255), the server stack
(#1197/#1249/#1250/#1254), and the native CUDA vision kernels (#1251).

**No logic changes.** This is `cargo fmt --all` and nothing else — 7
source files, plus two `.squad/decisions/inbox` notes that were already
sitting uncommitted in the tree.

Verified locally: `cargo fmt --all -- --check` exits clean on this
branch.

Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
@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.94%. Comparing base (ccc24ba) to head (86df22a).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1250   +/-   ##
=======================================
  Coverage   79.93%   79.94%           
=======================================
  Files         360      360           
  Lines      158320   158333   +13     
  Branches   158320   158333   +13     
=======================================
+ Hits       126554   126577   +23     
+ Misses      27151    27142    -9     
+ Partials     4615     4614    -1     
Flag Coverage Δ
mlas 85.05% <ø> (ø)
offline 79.84% <ø> (+<0.01%) ⬆️

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

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

@github-actions

Copy link
Copy Markdown

🔴 Benchmark Regression 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
🔴 block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024 42.19 µs 114.67 µs +171.8%
🔴 block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024 50.49 µs 128.85 µs +155.2%
🔴 block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024 538.93 µs 1.18 ms +119.5%
🔴 block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1 69.27 µs 129.42 µs +86.8%
🔴 matmul/large_generic_bf16_threads=8/32x1024x1024 1.30 ms 2.27 ms +74.1%
🔴 block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1 384.75 µs 609.44 µs +58.4%
🔴 matmul/large_generic_f16_threads=8/32x1024x1024 87.30 µs 121.00 µs +38.6%
🔴 add/large_f16_threads=1-internal/4194304 1.91 ms 2.55 ms +33.5%
⚠️ gather/small_bf16_threads=1-internal/4096 546.6 ns 706.4 ns +29.2%
⚠️ add/large_bf16_threads=1-internal/4194304 2.08 ms 2.56 ms +23.0%
⚠️ reduce_mean/small_f32_threads=1-internal/4096 15.59 µs 19.04 µs +22.1%
⚠️ gather/small_f32_threads=1-internal/4096 669.3 ns 786.1 ns +17.5%
⚠️ matmul/large_generic_bf16_threads=1/32x1024x1024 2.09 ms 2.44 ms +16.8%
⚠️ sampling_latency/top_p_per_token 400.41 µs 465.79 µs +16.3%
⚠️ qwen3_sampling_processors/top_k_partial_selection 153.28 µs 178.23 µs +16.3%
⚠️ gather/medium_f32_threads=1-internal/32768 4.70 µs 5.45 µs +16.0%
⚠️ gather/small_f16_threads=1-internal/4096 479.8 ns 553.9 ns +15.5%
✅ add/large_f32_threads=1-internal/4194304 762.73 µs 873.99 µs +14.6%
✅ sampling_latency/min_p_per_token 216.03 µs 246.18 µs +14.0%
✅ qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline 3.77 ms 4.26 ms +12.9%
✅ reduce_mean/medium_f32_threads=1-internal/65536 248.91 µs 278.67 µs +12.0%
✅ matmul/medium_generic_f16_threads=1/32x512x512 38.84 µs 43.31 µs +11.5%
✅ logit_processing/seven_processor_chain_per_step 337.65 µs 371.94 µs +10.2%
✅ matmul/medium_generic_f32_threads=1/32x512x512 2.59 ms 2.85 ms +9.9%
✅ matmul/small_generic_f16_threads=1/1x256x256 39.15 µs 42.94 µs +9.7%
✅ matmul/small_generic_bf16_threads=8/1x256x256 38.73 µs 42.46 µs +9.6%
✅ matmul/small_generic_f32_threads=8/1x256x256 46.55 µs 50.94 µs +9.4%
✅ gather/large_f16_threads=1-internal/131072 15.03 µs 16.36 µs +8.9%
✅ matmul/small_generic_f32_threads=1/1x256x256 43.69 µs 47.45 µs +8.6%
✅ tokenization/encode_tokens_per_second 429.91 µs 463.44 µs +7.8%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 691.85 µs 743.58 µs +7.5%
✅ qwen3_sampling_processors/top_k_full_sort_baseline 2.34 ms 2.51 ms +7.2%
✅ grammar_masking/llguidance_compute_mask/32 86.08 µs 92.21 µs +7.1%
✅ matmul/small_generic_bf16_threads=1/1x256x256 37.21 µs 39.56 µs +6.3%
✅ sampling_latency/greedy_per_token 3.42 µs 3.59 µs +5.0%
✅ matmul/medium_generic_f16_threads=8/32x512x512 47.53 µs 49.76 µs +4.7%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 93.30 µs 96.71 µs +3.7%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.01 ms 1.04 ms +3.5%
✅ matmul/small_generic_f16_threads=8/1x256x256 42.99 µs 44.23 µs +2.9%
✅ matmul/medium_generic_f32_threads=8/32x512x512 1.58 ms 1.61 ms +1.7%
✅ matmul/medium_generic_bf16_threads=8/32x512x512 612.67 µs 623.20 µs +1.7%
✅ sampling_latency/top_k_per_token 57.76 µs 58.33 µs +1.0%
✅ add/medium_bf16_threads=1-internal/262144 122.28 µs 123.30 µs +0.8%
✅ kv_cache/alloc_dealloc_pages 43.64 µs 43.79 µs +0.4%
✅ gather/large_f32_threads=1-internal/131072 43.66 µs 43.49 µs -0.4%
✅ gather/medium_bf16_threads=1-internal/32768 2.75 µs 2.71 µs -1.5%
✅ qwen3_sampling_processors/top_p_fast_after_top_k 603.70 µs 579.67 µs -4.0%
✅ matmul/large_generic_f32_threads=8/32x1024x1024 6.66 ms 6.32 ms -5.1%
✅ tokenization/decode_tokens_per_second 8.53 ms 7.92 ms -7.1%
✅ matmul/large_generic_f32_threads=1/32x1024x1024 11.34 ms 10.40 ms -8.3%
✅ add/small_f32_threads=1-internal/1024 254.0 ns 229.9 ns -9.5%
✅ qwen3_sampling_processors/top_k_top_p_full_sort_baseline 7.05 ms 6.35 ms -10.0%
✅ qwen3_sampling_processors/top_k_top_p_fast 833.39 µs 728.96 µs -12.5%
✅ add/small_f16_threads=1-internal/1024 627.4 ns 541.4 ns -13.7%
✅ gather/medium_f16_threads=1-internal/32768 3.30 µs 2.82 µs -14.5%
🟢 add/medium_f32_threads=1-internal/262144 34.24 µs 27.23 µs -20.5%
🟢 add/small_bf16_threads=1-internal/1024 644.4 ns 468.0 ns -27.4%
🟢 gather/large_bf16_threads=1-internal/131072 29.13 µs 20.04 µs -31.2%
🟢 add/medium_f16_threads=1-internal/262144 199.91 µs 120.22 µs -39.9%

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: { 5.05 3.81 7.13 }
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)

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