Skip to content

feat(server): honor response_fields, stream_options and real prefill timings - #1476

Merged
inureyes merged 2 commits into
mainfrom
feature/issue-1441-native-completion-fields
Aug 27, 2026
Merged

inureyes merged 2 commits into
mainfrom
feature/issue-1441-native-completion-fields

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

Issue 1441 landed its route separation in #1467 and was deliberately left open, because closing it would have forced manifest entries into supported that were not. This PR closes the implementable half of what remained: response_fields, stream_options and stream_options.include_usage, the real prefill figures on timings_per_token frames, and the whole n_predict value domain including the n_predict: 0 prompt-only case, which is now verified against the pinned binary rather than assumed.

Issue 1441 stays open, and this body carries no closing keyword on purpose. POST /completion and POST /completions still carry divergences and therefore stay deferred under it; the validator requires a deferred entry's issue to be OPEN, so resolving it now would turn the llama-compat manifest job red for every open PR in the repository. What remains is listed at the bottom.

The seven entries and where they land

Entry Before After
field:response_fields deferred, no divergence recorded, simply unimplemented supported
field:stream_options deferred, no divergence recorded supported
field:stream_options.include_usage deferred, no divergence recorded supported
field:timings_per_token deferred, zeroed prompt_n / prompt_ms per frame supported
field:n_predict deferred, n_predict: 0 unverified supported
POST /completion deferred, 3 divergences deferred, 3 divergences (one reworded to name #1440, one added)
POST /completions deferred, 3 divergences deferred, 3 divergences (same)

Evidence: measured before and after

Every expectation is a capture of the pinned b10621 binary (build 10621, commit c1d0e7a00, Qwen3-0.6B-Q8_0.gguf) against mlxcel serving models/mlx/qwen3-0.6b. The schema descriptions are wrong about two of these fields, so the binary is the reference, not the source text.

response_fields

Before: silently ignored, all nine probes answered the full 16-key object. After: identical to b10621 on all nine.

Probe b10621 mlxcel before mlxcel after
["content","tokens_predicted"] ["content","tokens_predicted"] 16 keys ["content","tokens_predicted"]
["content","generation_settings/n_predict","timings/predicted_n"] same three keys, slash kept in the key, generation_settings/n_predict = 8 16 keys identical
["content","no_such_field"] ["content"] 16 keys ["content"]
["timings/predicted_n","content","index"] that order 16 keys that order
[""] {} 16 keys {}
["content","content"] ["content"] 16 keys ["content"]
"content" (a string) 200, 16 keys 200, 16 keys 200, 16 keys
["content/x","tokens/0"] {} 16 keys {}
streaming per-token frames unprojected, final frame projected not projected at all identical to b10621

The slash form is the trap: the description says generation_settings/n_predict "moves n_predict to the root", and the binary instead emits a root key literally named generation_settings/n_predict. The implementation follows the binary.

stream_options and include_usage

Probe b10621 mlxcel before mlxcel after
{"include_usage": true} vs absent byte-identical native body, so the field is inert inert, but as an unknown key that was never parsed inert, and declared and type-checked
"stream_options": "garbage" 200 200 (unknown key) 200
{"include_usage": "yes"} 400, Field 'include_usage': [json.exception.type_error.302] type must be boolean, but is string 200, value dropped 400, Field 'include_usage': type must be boolean, but is string

Inert on the native route is the correct behavior, not a shortcut: the native final frame already carries tokens_predicted, tokens_evaluated and the whole timings object whether or not the field is sent, which is why upstream's own /completion ignores it. mlxcel's OpenAI routes already honor the same block the way b10621's do.

timings_per_token

Streaming a fresh 22-token prompt with timings_per_token: true, per frame:

b10621 mlxcel before mlxcel after
tokens_evaluated 22 on every frame 0 until the final frame 22 on every frame
timings.cache_n 0 0 0
timings.prompt_n 22 on every frame 0 until the final frame 22 on every frame
timings.prompt_ms 176.368 on every frame 0.0 until the final frame 80.0 on every frame
timings.predicted_ms at frame 1 0.001 21.0 (it was counting prefill) 0.0

The mechanism is a one-shot GenerateEvent::Prefill snapshot emitted by SequenceInfo::mark_first_token when the scheduler stamps the first token, which reaches the streaming route before the first frame. The route could not previously learn the prefill figures until GenerateEvent::Done.

Two derivations inside timings were wrong against the binary and are corrected in the same change, so a client that graphs them will see different numbers:

  • prompt_n is tokens_evaluated - cache_n, not tokens_evaluated. Measured on a repeated prompt upstream: tokens_evaluated: 8, cache_n: 7, prompt_n: 1.
  • predicted_per_token_ms and predicted_per_second divide by predicted_n - 1, because predicted_ms starts at the first token. Measured: predicted_n: 4, predicted_ms: 15.132, predicted_per_token_ms: 5.044 (15.132 / 3) and predicted_per_second: 198.255 (1e3 / 15.132 * 3). The old test asserting ms / n was named timings_rates_are_derived_the_way_upstream_derives_them and was not.

n_predict

Probe b10621 mlxcel before mlxcel after
0, prompt "A fresh unique prompt zzz 12345 alpha beta gamma" content: " delta", tokens_predicted: 1, tokens_evaluated: 15, stop_type: "limit" identical, but unverified identical, now verified
0, streaming 2 frames, final tokens_predicted: 1 2 frames 2 frames
-1 with stop: [" 9"] tokens_predicted: 12, stop_type: "word", stopping_word: " 9" HTTP 422, invalid value: integer -1, expected usize tokens_predicted: 12, stop_type: "word", stopping_word: " 9"
-2 400, Field 'n_predict': Value must be between -1 <= value <= 2147483647, but got -2 HTTP 422, serde type error 400, same message text

The n_predict: 0 case is worth stating plainly, because the schema description says "no tokens will be generated but the prompt is evaluated into the cache" and the binary emits exactly one token. mlxcel emits the same one token on the same prompt, so the two agree with each other and both disagree with the description. -1 is upstream's "as many as the context allows" and now resolves through the existing effective-context clamp instead of being refused.

What changed

  • src/server/model_provider.rs: PrefillStats and GenerateEvent::Prefill, a drain_generation_events_with_prefill drain, and generate_streaming_native_reserved for the native route. The new variant does not open the bounded decode window, so it cannot start the hang timer while the first token is still being sampled.
  • src/server/batch/sequence.rs: SequenceInfo::mark_first_token, idempotent, replacing the two bare first_token_time = Some(...) assignments in scheduler.rs and speculative_burst.rs. prompt_ms uses created_at as its origin, the same origin take_generation_result hands to finish_with_cache, so the streamed figures and the final frame cannot disagree.
  • src/server/types/native_completion.rs: select_response_fields, and NativeTimings::new now takes tokens_evaluated and derives prompt_n and the corrected rates.
  • src/server/types/request.rs: n_predict becomes Option<i64> with resolve_n_predict, plus response_fields and stream_options held as raw values with response_field_paths and validate_stream_options, so upstream's tolerant-parse-then-reject behavior is reproducible rather than pre-empted by serde.
  • src/server/routes/native_completion.rs: projection applied to the non-streaming body and the final streaming frame through one project_native_response, and the streaming frames read the prefill snapshot.
  • src/server/llama_compat_tests.rs: the native-field claim gate gained a rule for entries with a non-null parent, which are modeled by a nested struct rather than a root key. It is documented as a deliberately weaker ratchet than the root-field rule, with the behavioral half asserted in native_route_tests.rs.
  • compat/llama-server/b10621/routes.json and docs/llama-server-compat.md.

What moved to #1440, and why

POST /completion and POST /completions carried a divergence reading "id_slot is always -1: the continuous-batching scheduler exposes no stable per-request slot number (#1441)". Exposing a slot number is #1440's scope ("Expand /slots to expose compatible slot state"), and #1440 is already an owner of routes.json, so the line now reads "... so exposing one is #1440's scope, not this entry's (#1440)". Only that divergence line moved: the entries stay filed under #1441, because their other divergences are #1441's.

What remains on #1441, and why it stays open

POST /completion and POST /completions keep three divergences:

  1. generation_settings echoes the settings mlxcel resolved rather than b10621's full task_params block (49 keys), so keys mlxcel has no analogue for are absent. Reporting an invented value for them would be worse than their absence.
  2. tokens_cached counts the prompt tokens mlxcel's KV prefix cache supplied, while b10621 reports the slot's cache occupancy after the request, which is tokens_evaluated + tokens_predicted - 1. Measured on a repeated 51-token prompt answered with 4 tokens: b10621 tokens_cached: 54, mlxcel its own cache-supplied count. This divergence is newly recorded by this PR; it was not in the list before, and I found it while measuring the cache-hit path.
  3. return_tokens, return_progress, verbose, n_indent, t_max_predict_ms and n_cmpl above 1 are refused with a diagnostic rather than honored. Each has its own not_applicable entry, and those do not block the issue.

Items 1 and 2 are the substantial remainder and are probably worth their own issue, since both are response-body fidelity work with no dependency on the field work here. I have not filed it; say the word and it can be filed with the measurements above.

Two smaller observations recorded here rather than in the manifest, because neither belongs to an entry this issue owns:

  • mlxcel's error envelope reports "code": null where b10621 reports the numeric status ("code": 400). It is the shared ErrorResponse across every mlxcel route, not a property of these fields, so recording it on field:stream_options would misattribute it.
  • On the cache-hit probe above, mlxcel's prompt prefix cache did not adopt a repeated 51-token prompt under its defaults (cache_n: 0) where b10621 reused 50 tokens. That is prompt-cache behavior, owned by the prompt-caching issues, not by these entries.

Test plan

  • cargo test --profile test-fast --features metal,accelerate --lib -- server:: (2207 passed, 0 failed, 8 ignored)
  • cargo test --profile test-fast --features metal,accelerate --lib -- server::batch:: (387 passed, 0 failed, 7 ignored)
  • cargo test --profile test-fast --features metal,accelerate --test llama_compat_manifest (4 passed)
  • cargo clippy --lib --bins --tests --features metal,accelerate --profile test-fast -- -D warnings
  • cargo fmt --all -- --check
  • python3 scripts/ci/check_llama_compat_manifest.py
  • python3 scripts/ci/check_cross_repo_refs.py
  • Side-by-side comparison of every behavior above against the pinned b10621 binary and models/mlx/qwen3-0.6b

Refs #1441, #1440

…timings

Four b10621 native request fields that `/completion` and `/completions` accepted and dropped are now honored, and every expectation below is a capture of the pinned b10621 binary (build 10621, commit c1d0e7a00) serving a real checkpoint rather than a reading of the schema, which is wrong about two of them.

`response_fields` projects the finished response down to the listed paths. Upstream walks each path splitting on `/` and stores the value under the WHOLE path string, so `generation_settings/n_predict` comes back as a root key literally named `generation_settings/n_predict`; the schema description claims the value is unnested to the root and the binary does not do that. Reproduced across nine probes: a missing path, a path that walks into a non-object, and an empty path are omitted without an error, a repeated path yields one key, a nested object can be selected whole, the result keeps the request's order, and a value that is not an array of strings returns the whole object with a 200 instead of a type error. Applied to the non-streaming body and to the final streaming frame only, which is where upstream applies it.

`stream_options` and `stream_options.include_usage` are declared and type-checked instead of being read past as unknown keys. Measured: on the native route the block is inert on both sides, because the native final frame already carries the counts and the whole `timings` object; a non-object `stream_options` is tolerated, and a non-boolean `include_usage` is refused with upstream's own wording.

`timings_per_token` frames now carry the real prefill figures. A one-shot `GenerateEvent::Prefill` snapshot, emitted by `SequenceInfo::mark_first_token` when the scheduler stamps the first token, reaches the streaming route before the first frame, so `cache_n`, `prompt_n`, `prompt_ms` and `tokens_evaluated` report from frame one what the final frame reports. They were zero on every frame until the last one.

`n_predict` accepts upstream's whole value domain instead of serde's. `-1` means "as many as the context allows" and is clamped to the effective context window rather than refused with a 422; `0` is verified against the binary, which emits exactly one token for it despite its description saying none, and mlxcel answers the same one token on the same prompt; a value outside `[-1, INT32_MAX]` is refused with upstream's wording.

Two derivations inside `timings` were also corrected against the binary: `prompt_n` is `tokens_evaluated - cache_n` (a cached prefix measured `prompt_n: 1` against `tokens_evaluated: 8` with `cache_n: 7`), and the generation rates divide by `predicted_n - 1` because `predicted_ms` starts at the first token, so `predicted_n: 4` with `predicted_ms: 15.132` reports `predicted_per_token_ms: 5.044`.

Manifest: `field:response_fields`, `field:stream_options`, `field:stream_options.include_usage`, `field:timings_per_token` and `field:n_predict` move from `deferred` to `supported` in `routes.json`. `POST /completion` and `POST /completions` stay `deferred` under #1441 with three divergences left: `generation_settings` is mlxcel's resolved set rather than upstream's `task_params`, `tokens_cached` counts what the prefix cache supplied rather than the slot's occupancy after the request, and the six fields mlxcel refuses. The `id_slot` divergence is reworded to name #1440, which owns slot exposure. The native-field claim gate in `llama_compat_tests.rs` gained a rule for subfield entries, whose parent block is modeled by a nested struct rather than a root key.

Refs #1441, #1440
@inureyes inureyes added status:review Under review type:bug Bug fixes, error corrections, or issue resolutions priority:high High priority impact:breaking Breaking change requiring migration area:architecture Architecture and code structure changes area:inference Generation, sampling, decoding (incl. speculative, DRY) labels Aug 27, 2026
`src/server/batch/xla_worker_tests.rs` is gated behind `xla-iree`, which no local gate compiles (`make verify` and the nightly build `metal,accelerate`), so its two exhaustive `GenerateEvent` matches were not updated with the rest when the `Prefill` variant landed. The `OpenXLA feature compile` CI job, which runs `cargo check --features cuda,xla-iree --all-targets`, is the gate that catches exactly this and it went red on the first push.

`receive_done` now ignores the snapshot alongside the token events; the withheld-EOS assertion panics on it, because that test drives the worker directly and no prefill snapshot can reach its channel.

Refs #1441
@inureyes inureyes added status:done Completed and removed status:review Under review labels Aug 27, 2026
@inureyes
inureyes merged commit e897a35 into main Aug 27, 2026
12 checks passed
@inureyes
inureyes deleted the feature/issue-1441-native-completion-fields branch August 27, 2026 17:24
inureyes added a commit that referenced this pull request Aug 28, 2026
#1441 separated the native `/completion` and `/embeddings` routes from the OpenAI routes across PR #1467 and PR #1476, and this moves what it did not finish so the issue can close against what it actually delivered.

Eight entries in `routes.json` move from #1441 to #1477. Two are the `POST /completion` and `POST /completions` route entries, still `deferred` behind four divergences: `generation_settings` carries mlxcel's 19 resolved keys against b10621's 49-key `task_params` block, `id_slot` is always -1 because the continuous-batching scheduler exposes no stable per-request slot number, `tokens_cached` counts the prompt tokens the KV prefix cache supplied rather than the slot occupancy after the request, and six native request fields are refused with a diagnostic rather than honored. The other six are the `field:n_cmpl`, `field:n_indent`, `field:return_progress`, `field:return_tokens`, `field:t_max_predict_ms` and `field:verbose` entries, which are `not_applicable` and therefore never blocked the gate, but whose "no equivalent" notes become false the moment #1477 honors the corresponding field, so ownership follows the work rather than the history.

The refused-fields divergence is the substantive part of the remainder: `return_tokens` and `return_progress` are named in #1441's own scope, so leaving them under a closed issue would have left real work with no owner in any wave of epic #1431. `id_slot` stays recorded but names #1440 as its owner, because slot exposure belongs to that issue. `generation_settings` shrinks on its own as #1436 and #1433 land their options, so #1477 re-measures it rather than chasing it now.

`pin.json` gains #1477 as an owner of the `routes` shard, without which the ownership check would reject every repointed entry, and the shard ownership table in the operator documentation follows. After this, no `deferred` entry references #1441.


Part of #1431.
inureyes added a commit that referenced this pull request Aug 30, 2026
…tive route (#1525)

## Summary

Settles two of the four divergences on `POST /completion` and `POST /completions`, both against the pinned b10621 binary rather than by argument, and rewrites the manifest entries for the rest with fresh measurement. **This pull request carries no closing keyword**: issue 1477 keeps the four request fields that are still refused and stays open, deliberately.

Two premises in that issue's body were wrong and are corrected in place, with the captures recorded in a comment on it. Implementing against them would have produced a `__verbose` block mlxcel invented and a JSON array behind a limit upstream refuses.

## What changed

- **`tokens_cached` is b10621's slot cache occupancy.** It reported the prompt tokens mlxcel's prefix cache supplied for the request; upstream reports the slot's occupancy after it, which six independent measurements agree is `tokens_evaluated + tokens_predicted - 1`. The subtraction saturates. `timings.cache_n` keeps the cache-supplied count, which is the different quantity `timings.prompt_n` is derived from and which PR #1476 verified against the binary; conflating them would have broken `prompt_n` silently. **Breaking for a client that read `tokens_cached` as a cache-hit figure**, with the migration in `docs/llama-server-compat.md`.
- **`verbose` is accepted and inert, as upstream is.** `__verbose` is written only from the OAI-compat response builders (`server-task.cpp` lines 404, 452, 521 and 1098, each inside a `to_json_oaicompat*`) and the native completion object IS `to_json_non_oaicompat()`. Captured against the pinned binary, a request with `verbose: true` returns exactly the key set of the same request without it. Refusing it was the divergence; `field:verbose` becomes `supported`.
- **The remaining entries are restated against measurement, not carried forward.** `field:n_cmpl` records that upstream's schema limit refuses rather than clamps and is bounded by the server's own slot count, so at one slot mlxcel's refusal is byte-equivalent to b10621's. `field:return_tokens` records the re-measurement the issue asked for: streaming frames carry per-frame ids unconditionally, with and without the field, and the final streaming frame carries `tokens: []` in both cases, so honoring it means an opt-in channel for the non-streaming half and an unconditional per-frame id for the streaming half. `field:return_progress`, `field:n_indent` and `field:t_max_predict_ms` each carry upstream's rule with the line in `server-context.cpp` and the number that pins it.
- **The `generation_settings` divergence is re-measured**: 25 of upstream's 49 keys are reported and the 24 absent ones are named with the issue that owns each (#1436 for the sampler stages, #1433 for `speculative.types`, #1439 and those two for the rest).

## Validation

Head to head against the pinned binary (build 10621, commit `c1d0e7a00`) serving `ggml-org/Qwen3-0.6B-GGUF`, mlxcel serving `qwen3-0.6b-4bit` at `--ctx-size 4096 --parallel 1`, same request body to both.

| case | binary | `tokens_evaluated` | `tokens_predicted` | `tokens_cached` | `e+p-1` | `cache_n` | `prompt_n` |
|---|---|---|---|---|---|---|---|
| limit stop, `n_predict 8` | b10621 | 5 | 8 | 12 | 12 | 0 | 5 |
| limit stop, `n_predict 8` | mlxcel | 5 | 8 | 12 | 12 | 0 | 5 |
| prompt only, `n_predict 0` | b10621 | 5 | 1 | 5 | 5 | 0 | 5 |
| prompt only, `n_predict 0` | mlxcel | 5 | 1 | 5 | 5 | 0 | 5 |
| longer run, `n_predict 24` | b10621 | 8 | 24 | 31 | 31 | 0 | 8 |
| longer run, `n_predict 24` | mlxcel | 8 | 24 | 31 | 31 | 0 | 8 |
| `verbose: true` | b10621 | 1 | 4 | 4 | 4 | 0 | 1 |
| `verbose: true` | mlxcel | 1 | 4 | 4 | 4 | 0 | 1 |

The `n_predict: 0` row is the underflow case the issue asked to measure before choosing a formula: upstream answers it with `tokens_predicted: 1`, so `5 + 1 - 1 = 5` and the saturating subtraction never fires in practice. The `verbose: true` row is the control for that change: the same request, the same numbers, and the key set unchanged.

Two further captures behind the restated entries, from the same session: `tokens_cached` also held at `15 + 4 - 1 = 18` on a fully cache-hit request (`cache_n` 14, `prompt_n` 1), so the figure is independent of the hit; and with `-b 64` on a 321-token prompt, `return_progress` emitted `processed` 0, 64, 128, 192, 256, 320, 321 against `total: 321`, one frame per prefill chunk, which maps directly onto mlxcel's own chunked prefill when that work is done.

## What remains on issue 1477

`return_tokens`, `return_progress`, `n_indent` and `t_max_predict_ms` are still refused, and each manifest entry now names the upstream rule, the source line, and the measurement rather than a general "no equivalent". `return_tokens` needs an opt-in id channel out of the scheduler plus unconditional per-frame ids on the streaming path; `return_progress` needs a per-chunk progress event mapped onto `PromptProgress`; the two stop rules need a second per-token guard on the sequence with upstream's newline and indentation conditions. `n_cmpl` above 1 stays `not_applicable`: shipping a partial array shape would be worse than the refusal, which at one slot is already what upstream answers.

## Test plan

- [x] `cargo fmt --all -- --check` and `cargo clippy --profile test-fast --features metal,accelerate --lib --tests --bins -- -D warnings`
- [x] `cargo test --profile test-fast --features metal,accelerate --lib server::` (2653)
- [x] `cargo test --profile test-fast --features metal,accelerate --test llama_compat_manifest` (4)
- [x] `python3 scripts/ci/check_llama_compat_manifest.py`
- [x] The head-to-head table above against the pinned binary

Tracked on issue 1477, which stays open.
@inureyes inureyes self-assigned this Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:architecture Architecture and code structure changes area:inference Generation, sampling, decoding (incl. speculative, DRY) impact:breaking Breaking change requiring migration priority:high High priority status:done Completed type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant