Repository navigation
fix(coordinator): rewrite cached tokens in streamed Responses usage - #3596
Conversation
31a776c to
034f9db
Compare
blacksoxx
left a comment
There was a problem hiding this comment.
Checked this at 034f9db.
setCachedTokens only looked for a top-level usage, and a streamed Responses reply carries its usage inside the response object of the response.completed and response.incomplete events, which is also where the EPP's openai parser reads it from. Recursing into response when the frame has no top-level block covers both terminal events, and a response.created frame, whose usage is null, falls out of the type assertion and is left unchanged, as the third row shows. The chat and completions paths are untouched since their frames carry usage at the top level. On the constant: "response" appears nowhere else in the request paths except as the openai parser's struct tag, so FieldResponse is its first named home.
The two terminal-event rows fail on main and pass on the branch; go test -race for pkg/common/usage, pkg/common/request and pkg/coordinator/steps, go build ./..., go vet and golangci-lint v2.14.0 are clean.
I would approve this; the decision is with the maintainers.
|
@elevran, could you review this when you have time? It fixes the cached-token count in streamed Responses replies. It has one approval, all checks pass, and the branch still merges cleanly with main. |
|
@wguo7 This PR has merge conflicts with gh pr checkout 3596
git fetch origin main
git rebase origin/main
# resolve conflicts
git push --force-with-lease |
034f9db to
aa0fc90
Compare
|
@blacksoxx @wguo7 has rebased this PR and it is now conflict-free. Please take another look and re-approve when ready. |
|
Rebased onto main to resolve the conflict with #3636. Since CI passes, and locally the |
A streamed /v1/responses reply carries its usage inside the response object of the response.completed event, so the P/D usage rewriter left decode's cached_tokens in place of the prefill count. Signed-off-by: William Guo <guowilliam3@gmail.com>
aa0fc90 to
5e02ca7
Compare
|
This replaces my earlier rebase note. #3751 conflicted, so I rebased again and dropped the The PR changes only
The two Responses event cases in |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The P/D usage rewriter, used by the coordinator decode step and the NIXL sidecar connector, reads only a top-level
usagefield. Theresponse.completedevent of a streamed/v1/responsesreply nests usage underresponse, so it kept decode'scached_tokens(#3577).observealready finds thatresponse.usageblock for the prompt-token parity check (#3751), and with this change the cached-token rewrite applies to it too. The Messagesmessage.usageblock stays observe-only.response.incompletecarries usage the same way and is covered too. Events whoseresponse.usageis null, such asresponse.created, are left unchanged. The rewriter already buffers an SSE line split across writes (TestResponseWriter_BuffersStreamedFramesSplitAcrossWrites), so a largeresponse.completedevent is parsed whole.Which issue(s) this PR fixes:
Fixes #3577
Release note (write
NONEif no user-facing change):Test plan:
response.completedandresponse.incompleteevents have their cached tokens rewritten (fail on main, pass here), and aresponse.createdevent with null usage passes through unchanged.go testpasses forpkg/common/...,pkg/coordinator/...andpkg/sidecar/proxy(TestServeMetrics_ListenErrorskipped, since it also hangs on macOS on main), andmake lintreports 0 issues.make presubmitwas not run.