fix(server): let a package's declared sampling stand when the caller is silent by justinchuby · Pull Request #1236 · justinchuby/onnx-genai

justinchuby · 2026-08-18T08:38:18Z

Stacked on #1224.

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:

temperature: Some(request.temperature),
top_p: Some(request.top_p),

An explicit override beats the package's declaration by design, so a model that declares its own sampling regime lost it on every request. This model declares top_p: 0.95; every OpenAI client silently widened it to 1.0, the full distribution.

The old doc comment stated the mistaken premise outright — that the schema "always supplies them ... and has no 'unspecified' state". It does have one: the field is simply absent.

An agent client is exactly the caller that hits this. OpenCode's captured requests carry no sampling controls at all, so every turn it drove sampled from the whole tail rather than the nucleus the package asked for.

The fix

Both fields become Option<f32>, and an absent one is passed through as None — "no opinion" — so the declared value wins. This is precisely the treatment top_k already had, and the comment explaining why was already sitting three lines below:

Treat an absent top_k as unspecified so a package that declares one keeps it, instead of every OpenAI client silently widening the model to the full vocabulary.

Validation now judges only what the caller actually sent, via one shared validate_sampling_range.

The same defect and the same fix apply to legacy /v1/completions.

What still behaves exactly as before

temperature: 0 means greedy and beats a package declaring do_sample: true.
Any explicit value still overrides the package.
A model that declares nothing falls back to the neutral 1.0 as before.

One existing test asserted the old behaviour directly (assert_eq!(options.temperature, 1.0) with the comment "the schema always supplies a value"); it now asserts the declared 0.6 survives.

Verification

210 lib + 31 http + 11 bundle tests pass; clippy --all-targets clean.
Live 30B model, native CUDA: silent requests answer normally, and temperature: 0 returns bit-identical output across runs, so the greedy path is untouched.

codecov · 2026-08-18T13:08:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (fix/reasoning-turn-budget-and-privacy@060e45e). Learn more about missing BASE report.

Additional details and impacted files

@@                           Coverage Diff                            @@
##             fix/reasoning-turn-budget-and-privacy    #1236   +/-   ##
========================================================================
  Coverage                                         ?   79.90%           
========================================================================
  Files                                            ?      359           
  Lines                                            ?   157905           
  Branches                                         ?   157905           
========================================================================
  Hits                                             ?   126174           
  Misses                                           ?    27119           
  Partials                                         ?     4612

Flag	Coverage Δ
mlas	`85.49% <ø> (?)`
offline	`79.79% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2026-08-18T14:10:25Z

🔴 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
🔴	`tokenization/decode_tokens_per_second`	6.60 ms	10.10 ms	+53.0%
🔴	`sampling_latency/top_p_per_token`	410.31 µs	589.33 µs	+43.6%
🔴	`tokenization/encode_tokens_per_second`	424.34 µs	608.14 µs	+43.3%
🔴	`qwen3_sampling_processors/top_k_top_p_full_sort_baseline`	5.87 ms	8.02 ms	+36.6%
🔴	`qwen3_sampling_processors/top_k_partial_selection`	149.01 µs	196.43 µs	+31.8%
⚠️	`sampling_latency/min_p_per_token`	221.47 µs	287.12 µs	+29.6%
⚠️	`qwen3_sampling_processors/top_p_fast_after_top_k`	542.59 µs	678.70 µs	+25.1%
⚠️	`qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline`	3.70 ms	4.56 ms	+23.5%
⚠️	`sampling_latency/greedy_per_token`	3.77 µs	4.62 µs	+22.5%
⚠️	`block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1`	177.12 µs	216.70 µs	+22.3%
⚠️	`matmul/medium_generic_f16_threads=1/32x512x512`	36.30 µs	43.95 µs	+21.1%
⚠️	`qwen3_sampling_processors/top_k_full_sort_baseline`	2.28 ms	2.75 ms	+20.6%
⚠️	`kv_cache/alloc_dealloc_pages`	41.36 µs	49.65 µs	+20.0%
⚠️	`block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024`	97.26 µs	115.30 µs	+18.5%
⚠️	`sampling_latency/top_k_per_token`	55.00 µs	64.57 µs	+17.4%
⚠️	`matmul/large_generic_f16_threads=8/32x1024x1024`	99.40 µs	114.80 µs	+15.5%
✅	`matmul/large_generic_bf16_threads=8/32x1024x1024`	2.04 ms	2.34 ms	+14.6%
✅	`qwen3_sampling_processors/top_k_top_p_fast`	700.03 µs	792.26 µs	+13.2%
✅	`logit_processing/seven_processor_chain_per_step`	334.32 µs	376.56 µs	+12.6%
✅	`add/large_bf16_threads=1-internal/4194304`	1.82 ms	2.04 ms	+11.7%
✅	`block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024`	121.58 µs	135.38 µs	+11.3%
✅	`block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024`	1.24 ms	1.38 ms	+11.0%
✅	`matmul/large_generic_bf16_threads=1/32x1024x1024`	2.13 ms	2.37 ms	+10.9%
✅	`matmul/small_generic_f16_threads=1/1x256x256`	35.87 µs	38.59 µs	+7.6%
✅	`reduce_mean/small_f32_threads=1-internal/4096`	15.67 µs	16.81 µs	+7.2%
✅	`matmul/small_generic_f32_threads=8/1x256x256`	52.97 µs	56.09 µs	+5.9%
✅	`matmul/large_generic_f32_threads=8/32x1024x1024`	5.89 ms	6.24 ms	+5.9%
✅	`matmul/medium_generic_f32_threads=8/32x512x512`	1.75 ms	1.85 ms	+5.7%
✅	`matmul/medium_generic_f32_threads=1/32x512x512`	2.45 ms	2.59 ms	+5.7%
✅	`add/medium_bf16_threads=1-internal/262144`	110.95 µs	116.77 µs	+5.2%
✅	`matmul/large_generic_f16_threads=1/32x1024x1024`	92.14 µs	96.88 µs	+5.1%
✅	`matmul/medium_generic_f16_threads=8/32x512x512`	41.75 µs	43.82 µs	+5.0%
✅	`matmul/medium_generic_bf16_threads=8/32x512x512`	552.84 µs	578.44 µs	+4.6%
✅	`matmul/medium_generic_bf16_threads=1/32x512x512`	588.18 µs	614.04 µs	+4.4%
✅	`matmul/large_generic_f32_threads=1/32x1024x1024`	9.99 ms	10.42 ms	+4.3%
✅	`reduce_mean/medium_f32_threads=1-internal/65536`	258.36 µs	267.86 µs	+3.7%
✅	`matmul/small_generic_bf16_threads=8/1x256x256`	39.02 µs	39.80 µs	+2.0%
✅	`matmul/small_generic_bf16_threads=1/1x256x256`	37.69 µs	37.58 µs	-0.3%
✅	`grammar_masking/llguidance_compute_mask/32`	81.32 µs	81.00 µs	-0.4%
✅	`reduce_mean/large_f32_threads=1-internal/262144`	1.13 ms	1.11 ms	-1.8%
✅	`gather/medium_f16_threads=1-internal/32768`	2.66 µs	2.60 µs	-2.2%
✅	`gather/small_f32_threads=1-internal/4096`	747.5 ns	729.7 ns	-2.4%
✅	`add/medium_f16_threads=1-internal/262144`	113.99 µs	111.15 µs	-2.5%
✅	`matmul/small_generic_f16_threads=8/1x256x256`	41.01 µs	39.35 µs	-4.0%
✅	`gather/medium_f32_threads=1-internal/32768`	4.72 µs	4.53 µs	-4.2%
✅	`gather/large_bf16_threads=1-internal/131072`	15.99 µs	15.25 µs	-4.6%
✅	`add/large_f32_threads=1-internal/4194304`	831.44 µs	779.41 µs	-6.3%
✅	`gather/medium_bf16_threads=1-internal/32768`	2.78 µs	2.59 µs	-6.7%
✅	`matmul/small_generic_f32_threads=1/1x256x256`	43.76 µs	40.49 µs	-7.5%
✅	`block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1`	635.35 µs	584.99 µs	-7.9%
✅	`gather/small_f16_threads=1-internal/4096`	639.0 ns	586.7 ns	-8.2%
✅	`gather/large_f16_threads=1-internal/131072`	16.05 µs	14.62 µs	-9.0%
✅	`gather/large_f32_threads=1-internal/131072`	47.90 µs	42.11 µs	-12.1%
✅	`add/medium_f32_threads=1-internal/262144`	29.62 µs	25.84 µs	-12.8%
✅	`add/large_f16_threads=1-internal/4194304`	2.03 ms	1.77 ms	-12.8%
🟢	`gather/small_bf16_threads=1-internal/4096`	626.0 ns	515.9 ns	-17.6%
🟢	`add/small_f16_threads=1-internal/1024`	640.1 ns	501.9 ns	-21.6%
🟢	`add/small_bf16_threads=1-internal/1024`	628.8 ns	470.3 ns	-25.2%
🟢	`add/small_f32_threads=1-internal/1024`	307.3 ns	223.5 ns	-27.3%

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: { 8.64 5.90 6.24 }

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)

…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

…is silent (replaces #1236) (#1250) 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e

justinchuby · 2026-08-18T14:50:38Z

Superseded by #1250, which is this commit rebased onto current main after #1197/#1224 landed as squashes (the stacked base diverged). #1250 adds one guard test (an explicit temperature of 1.0, equal to the old schema default, still overrides a package's declared 0.6 — the absent-vs-default trap), was validated (211 lib + 31 http + 11 bundle tests, clippy clean, silent-override claim falsified revert->RED->restore->GREEN, e2e on qwen05b-q4 incl. temperature:0 bit-identical), and merged as 20aa31a. Closing in favour of it.

…aces #1248) (#1254) Replacement for #1248, rebased onto current `main` after #1197/#1224/#1236 landed as squashes (the stacked base diverged). The original functional commit plus two added tests, re-validated on top of the merged stack. ## Symptom A model driven with flat tool names (`bash`, `edit`, `glob`, `grep`, `read`, `write`) emits a namespaced call such as `glob.glob`, which the client rejects: it can only dispatch a name it offered. ## Cause (as claimed by the PR, and what I could and could not confirm) The caller offers flat tool *definitions* over the API. The instruction text the model reads is rendered by the **package's own chat template**, which the PR says assumes namespaced names, takes `name.split('.')[0]`, and renders `# Valid recipients: "self", "glob.*", ..., "user".` — so the model follows a grammar that is wrong for a flat offer and writes `glob.glob`. **I could not inspect that template with my own eyes.** It ships inside the model package (Muse Glimmer / harmony-style), which is not in this repository or in my local model set; searching the repo and local packages for the `split('.')` / "Valid recipients" grammar found nothing. What I *can* confirm from the server side corroborates the addressing scheme: the ATEM parser recognises `to=functions.<name>` channels and `<atem:invoke name="...">` blocks (`extract_atem_tool_calls`, `ATEM_USER_CHANNEL = "to=user<|message|>"`), so a namespaced recipient like `functions.glob` producing a call named `glob.glob` is consistent with the diagnosis. **Template fix vs dispatch fix — my conclusion:** resolving at dispatch is the right layer, and this is a considered choice, not merely the first available. The template belongs to the model author and is rendered in the jinja sandbox; the server cannot rewrite an arbitrary third-party package's template at runtime, and the namespaced grammar is not universally wrong (it is correct for a caller that offers namespaced tools — only a flat offer breaks it). The server is the **only** component that sees both the caller's offer and the model's emitted call, so it is the only place that can reconcile them without editing model files. A template correction for flat offers would still be worthwhile in the package, but it cannot be the server's runtime responsibility. ## Fix When a parsed call names a tool that is not on offer but whose **final `.`-segment exactly equals an offered tool's whole name**, the call is resolved to that offered tool; anything else is left exactly as the model spelled it. Applied at the three call sites that already have the request in scope (buffered, streaming, and the logprob path), via `ParsedAssistantOutput::aligned_to` / `align_tool_calls`. **Ambiguity is impossible by construction, not merely avoided:** the resolution target is always the single final segment string, and it resolves only if that exact string is an offered (unique) name — there is never a choice between two candidates. When two offered tools merely share a final segment (`fs.read`, `net.read`) and the model writes a third namespace (`svc.read`), the bare segment `read` is offered by neither, so the call is left alone for the caller to reject. ## Verification (mine, on this branch atop the merged stack) - **216 lib + 31 http + 11 bundle tests pass**; `cargo clippy -p onnx-genai-server --all-targets -- -D warnings` clean. - **Falsified the positive claim** (revert → RED → restore → GREEN): neutering `align_tool_calls` to a no-op made `a_namespaced_call_resolves_to_the_offered_tool` and the new full-parse-path test return `glob.glob` instead of `glob`. - **Confirmed the negative tests have teeth, and characterised their limits honestly:** a no-op revert leaves the negatives green (they guard against *over*-resolution, not absence). Dropping the exact-offer guard makes `an_offered_name_is_left_alone` go RED (`fs.read` → `read`), which is exactly what that test protects. - **Added tests:** `a_shared_final_segment_is_not_resolved_arbitrarily` (the ambiguity case above) and `a_namespaced_atem_call_dispatches_through_the_full_parse_path`, which drives a raw `<atem:invoke name="glob.glob">` turn through the real `parse_assistant_output` → `align_tool_calls` path and asserts the dispatched name is `glob`. - **End-to-end note:** I could not reproduce the OpenCode/Muse path — no local model emits ATEM namespaced calls — so the end-to-end evidence is the full-parse-path unit test above (the server's actual code path), clearly labelled as such rather than an HTTP round-trip. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e

justinchuby mentioned this pull request Aug 18, 2026

fix(server): resolve a namespaced tool call to the offered tool #1248

Closed

justinchuby force-pushed the fix/honour-declared-sampling branch from ea24844 to cba7254 Compare August 18, 2026 14:44

justinchuby mentioned this pull request Aug 18, 2026

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

Merged

justinchuby closed this Aug 18, 2026

justinchuby mentioned this pull request Aug 18, 2026

fix(server): resolve a namespaced tool call to the offered tool (replaces #1248) #1254

Merged

justinchuby mentioned this pull request Aug 18, 2026

Making reasoning visible spans #1197, not just #1224 — the buffered path filters independently #1256

Closed

Navigation Menu

Repository navigation

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

fix(server): let a package's declared sampling stand when the caller is silent#1236
justinchuby wants to merge 1 commit into
fix/reasoning-turn-budget-and-privacyfrom
fix/honour-declared-sampling

justinchuby commented Aug 18, 2026

Uh oh!

codecov Bot commented Aug 18, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 18, 2026 •

edited

Loading

Uh oh!

justinchuby commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

justinchuby commented Aug 18, 2026

The defect

The fix

What still behaves exactly as before

Verification

Uh oh!

codecov Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔴 Benchmark Regression Detected

Uh oh!

justinchuby commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov Bot commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading