server: generate asks the runner for pass-one metrics too - #271
Merged
Conversation
Upstream's v0.33.3 `IncludeIntermediateMetrics` was folded in for ChatHandler only. `/api/generate`'s structured-outputs double request therefore still reconstructed pass-one metrics by tokenizing text at the transition site, and a textual count cannot see image-embedding tokens -- the exact gap ADR 0010 documents (~2042 tokens per nemotron3 image, 256+ per gemma4 image). On a vision request the two endpoints disagreed on `prompt_eval_count` for the same work, and `eval_count` came from re-tokenized chunk text rather than the runner's own count. GenerateHandler now mirrors ChatHandler exactly: `includeIntermediateMetrics := req.Format != nil && currentFormat == nil` is computed after the deferring block, set on the `llm.CompletionRequest`, and the callback keeps the latest report in `firstPassMetrics` while blanking the non-terminal copies, so no mid-stream counter leaks into a streamed `/api/generate` chunk. At the transition site `reportedPassMetrics(firstPassMetrics)` is preferred and `transitionPassMetrics()` stays as the fallback for runners and paths that report nothing. What stays: the done-time fold is untouched -- ADR 0004's summing (each pass's prefill counts as prefill, each pass's decode as decode) and ADR 0010's `transitionPromptDelta` derivation, still computed only from a reconstructed pass because it needs a textual count on both sides. The runner side is untouched; both engines already honour the flag. `TestGenerateThinkFormatTransitionMetrics` keeps pinning the reconstruction fallback (its mock reports nothing on pass one); `TestGenerateThinkFormatTransitionMetricsReportedPassOne` is the new vision-shaped, streamed twin of the chat test -- pass one reports a cache-inclusive prefill carrying an image surplus, the folded counts must carry it, and the chunks before done must carry no metrics at all. ADR 0010 gains a dated status line; its decision is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on the v0.33.3 fold branch (#264) because the plumbing it uses arrives with the fold; GitHub retargets this to
mainwhen #264 merges. Not to be merged before #264.Upstream v0.33.3 added
llm.CompletionRequest.IncludeIntermediateMetricsand the fold wired it intoChatHandler(reportedPassMetrics()preferred over ADR 0010's textual reconstruction at the thinking→content transition).GenerateHandlerhas the same structured-outputs double request and the same cancellation problem but neither upstream nor the fold wired it there, so/api/generatestill reconstructed pass-one metrics by tokenizing text — blind to image-embedding tokens, the exact gap ADR 0010 documents.Change (
server/routes.go, +50/−21), mirroring ChatHandler line for line:includeIntermediateMetrics := req.Format != nil && currentFormat == nil, computed after thedeferringblock (so it rides PR server: chat transition restarts require a deferred pass one #238's gate exactly as chat's does), set on thellm.CompletionRequest.firstPassMetricsand non-terminal chunks are blanked, so no mid-stream counter leaks into streamed/api/generatechunks.pass1 = reportedPassMetrics(firstPassMetrics), falling back totransitionPassMetrics(...);transitionPromptDeltais still computed only from a reconstructed pass (it needs the textual count on both sides). The done-time arithmetic (ADR 0004 summing, ADR 0010 derivation) is byte-identical.Tests (
server/routes_generate_test.go, +175):TestGenerateThinkFormatTransitionMetricsunchanged and green (its mock reports nothing, so it still exercises the reconstruction fallback). NewTestGenerateThinkFormatTransitionMetricsReportedPassOne— streamed, one nemotron3 image (2042 tokens): pass one emits a non-terminal chunk carrying its cache-inclusive prefill (textual + 2042), then blocks until the transition cancels it; asserts pass one requested per-token metrics and pass two did not, every pre-donechunk carries no metrics, the foldedprompt_eval_countequals pass one's own prefill and strictly exceeds the textual count, andeval_count/durations sum per ADR 0004. Negative controls run and reverted: removing the blanking fails on a leaked chunk; removing thereportedPassMetricspreference fails oneval_count/durations (the prompt count alone cannot discriminate, by design — same as the chat twin).ADR 0010's status bullet gains a dated line saying both handlers now prefer the runner's reported pass-one metrics, reconstruction as fallback.
Verified (golang:1.26 container,
-u 1000:1000):go build ./... && go test -count=1 ./server/ ./llm/ ./api/all ok;go test -race -run ThinkFormat ./server/9/9; gofumpt clean; golangci-lint at repo root 0 issues.Follow-up for a decision, deliberately not made here: Chat's fold re-attributes
PromptEvalCachedCountfrom pass one (res.Metrics.PromptEvalCachedCount = pass1.PromptEvalCachedCount); Generate's fold never did, so/api/generatestill reports the continuation's cached count. Pre-existing, one line to close, but it is a fold-arithmetic change and out of this PR's scope.🤖 Generated with Claude Code