Skip to content

Split inference includeLogits bool into orthogonal tokens + logits axes - #297

Open
stikves wants to merge 6 commits into
apple:mainfrom
stikves:sukru/logits-tokens-seam
Open

stikves wants to merge 6 commits into
apple:mainfrom
stikves:sukru/logits-tokens-seam

Conversation

@stikves

@stikves stikves commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A code-health cleanup of the inference layer. It replaces the InferenceOptions.includeLogits: Bool flag with two orthogonal request axes, so all four engines — sequential, pipelined, static/ANE, and VLM — describe a forward step the same way:

  • tokens: TokenRequest (.none / .sample) — should the engine sample and return a token?
  • logits: LogitsRequest (.none / .lastPosition / .allPositions) — which positions' logits should it surface?

A few presets cover the everyday combinations:

Preset tokens logits Use
.prefill .none .none warm the KV cache
.extend (alias .default) .sample .none decode
.eval .none .allPositions perplexity / evaluation
.guided .none .lastPosition constrained decoding
.sampleWithLogits .sample .lastPosition decode and keep the last-row logits

Motivation

The single boolean had to answer two unrelated questions — whether to sample a token, and which logits (if any) to return — and it couldn't answer either well:

  • The pipelined engine samples on the GPU and never brings logits back to the CPU, so it can only return a token. A logits flag has no way to express that.
  • Perplexity and evaluation need logits at every position, while constrained decoding needs only the last row. A boolean can't tell those apart.
  • Logit inspection and scoring want a token and its logits together.

Splitting the request into an action axis and an output axis lets a caller ask for exactly what it needs, and lets an engine turn down a request it can't serve up front rather than fail partway through a forward pass.

Compatibility

The default, (tokens: .sample, logits: .none), is equivalent to the old includeLogits: false, so decode is unchanged. Every call site now uses a preset, and engines derive returnsLogits = (logits != .none).

It does remove the public includeLogits property, which is a breaking change; the replacements are the two enums, the tokens and logits properties, and the preset factories.

Verification

This is a behavior-preserving refactor, and the output is byte-identical to the base commit:

  • 39 unit tests pass, including coverage of every preset and the default.
  • A golden byte-identical greedy-decode check passes on a 135M fp16 model and a 0.8B hybrid model (both the sequential and pipelined engines), and on a static/accelerator model.
  • The pipelined engine's rejection of logit-returning requests is unchanged, checked against the base binary.

Follow-ups

Intentionally left for separate changes:

  • Narrow the CPU logits readback for .lastPosition and .none — a decode-throughput win the sequential and VLM paths don't take yet.
  • Skip sampling entirely when tokens is .none on the decode path.
  • Support .allPositions on the static and pipelined engines.
  • The engine-consolidation and warmup work that builds on this contract.

@stikves
stikves marked this pull request as ready for review September 27, 2026 20:58
@stikves stikves self-assigned this Sep 27, 2026
Replace InferenceOptions.includeLogits: Bool with two orthogonal axes —
tokens: TokenRequest {.none, .sample} and logits: LogitsRequest
{.none, .lastPosition, .allPositions} — plus presets (.prefill / .extend /
.eval / .guided / .sampleWithLogits). Default (.sample, .none) equals the old
includeLogits: false, so decode is byte-identical. All call sites migrated to
presets; engines derive returnsLogits = (logits != .none); the pipelined engine
rejects logits != .none (was includeLogits).
…binations

Add an InferenceOptions.returnsLogits computed property (equivalent to
logits != .none) and route all engines (sequential, static-shape, VLM,
pipelined guard) and the MockEngine through it instead of open-coding the
comparison. Document on the raw initializer that the (tokens, logits) space
is intentionally permissive with no invalid combinations, that the presets
cover the meaningful pairings, and that (tokens: .sample, logits: .allPositions)
is the one representable pair without a dedicated preset. Pure readability
refactor; behavior unchanged.
/// `eval`, `guided`, and `sampleWithLogits` presets below cover all the
/// meaningful pairings; prefer them to constructing options by hand. The
/// only representable pair without a dedicated preset is
/// `(tokens: .sample, logits: .allPositions)` — sampling a token while also

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grepped for .tokens, .logits, .allPositions and .lastPosition in swift/ and internal/. Outside InferenceEngine.swift, the only readers are the tests. Every engine (sequential, VLM, static-shape, pipelined) reads returnsLogits, which is just logits != .none.

So tokens: .none and .allPositions change nothing at runtime, so these are declared but no engine reads them 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will be handled in a follow up.

(The engines do extra work today)

/// Max tokens to generate. Nil = until EOS or context limit.
public var maxTokens: Int?
/// Include raw logits in each `InferenceOutput`. May incur GPU→CPU copy cost.
public var includeLogits: Bool

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Public API breaking when we remove this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's mark this deprecated


/// Presets for the common (tokens, logits) pairs.
extension InferenceOptions {
/// Warm the KV cache; no token or logits generation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.prefill is documented as "no token or logits generation" but an engine still samples even though .guided and .eval set tokens: .none

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating documentation

Reintroduce the old InferenceOptions.includeLogits boolean as a deprecated
initializer parameter and computed property that bridge to the (tokens, logits)
axes: true -> logits: .lastPosition, false -> .none, with tokens still sampled.
This avoids a hard public-API break from splitting the bool into the orthogonal
axes — external callers keep compiling, with a deprecation warning pointing to
the new API. Addresses review feedback.
Reword the .prefill preset from "no token or logits generation" to describe
what the request needs. The (tokens, logits) axes express intent, and engines
do not yet gate sampling on tokens: .none, so the old wording over-claimed
runtime behavior. Addresses review feedback.
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.

3 participants