Replace StreamChunk with explicit token/done/error events - #96
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes the server-sent events (SSE) streaming protocol across v1 and v2+ endpoints by replacing the overloaded “chunk” payload with an explicit discriminated event protocol (type: token|done|error). It also refactors model/executor internals to preserve token identity (including empty decoded strings) and to report authoritative token counts without re-tokenizing output text.
Changes:
- Introduce explicit stream event schemas (
StreamTokenEvent,StreamDoneEvent,StreamErrorEvent) and update v1 + v2+ streaming endpoints to emit per-token events immediately followed by a separate terminaldone(orerror) event. - Refactor decoding/generation to yield structured
GenerationSteprecords and to use sampled token counts (including empty decoded tokens) rather than re-tokenizing output strings. - Update bench client + tests to validate and exercise the new protocol (token index contiguity, done counts, terminal semantics).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/scripts/bench/test_runners.py | Updates bench-stream tests to use explicit token/done/error events and adds protocol-validation expectations. |
| tests/model/test_hf_runner_smoke.py | Adjusts smoke test assertions to the new GenerationStep-based streaming output. |
| tests/model/test_hf_runner_behavior.py | Adds coverage for empty decoded tokens and for sampled-count reporting; updates EOS behavior assertions. |
| tests/executor/test_sinks.py | Updates TokenEvent construction after dropping is_first/is_last. |
| tests/executor/test_schedule_engine.py | Updates scheduler-engine expectations for explicit token events + separate done event. |
| tests/executor/test_events.py | Updates event emission tests for new EOS semantics (no EOS token event) and finish_reason on DoneEvent. |
| tests/api/test_routes.py | Adds schema-level tests for the explicit SSE event models; updates stream expectations and FinishReason usage. |
| tests/api/test_collector.py | Updates DoneEvent construction to require finish_reason; updates TokenEvent construction. |
| server/model/types.py | Introduces FinishReason as a string Enum in model types. |
| server/model/hf_runner.py | Adds GenerationStep and refactors decode loop + generation APIs to yield structured steps and sampled token counts. |
| server/executor/types.py | Removes FinishReason from executor types; drops TokenEvent.is_first/is_last; requires DoneEvent.finish_reason. |
| server/executor/events.py | Updates event emission: EOS produces no TokenEvent; terminal steps emit DoneEvent with required finish_reason. |
| server/api/v1.py | Updates v1 streaming endpoint to emit explicit token/done/error SSE events. |
| server/api/schema.py | Replaces StreamChunk with explicit Stream*Event models and a discriminated union. |
| server/api/routes.py | Updates v2+ streaming generator to emit explicit token/done/error SSE events immediately. |
| scripts/bench/runners.py | Updates bench stream parsing/accumulation to enforce explicit protocol and report protocol errors. |
| scripts/bench/models.py | Updates bench result docs to reflect the new terminal done event source for server metrics. |
| scripts/bench/cli.py | Updates comment wording from “final chunks” to “done events”. |
| benchmarks/README.md | Updates protocol documentation to describe explicit token/done events and client-side validation rules. |
Suppressed comments (1)
scripts/bench/runners.py:176
- The PR description says
doneevents have non-optional timing metrics, but the accumulator currently accepts missing/nulltotal_ms,queue_wait_ms,execution_ms, andtokens_per_s(they becomeNonevia_float_or_none). That makes protocol regressions harder to detect and can also lead to downstream calculations mixing clocks.
self.server_ttft_ms = _float_or_none(chunk.get("ttft_ms"))
self.server_total_ms = _float_or_none(chunk.get("total_ms"))
self.server_queue_wait_ms = _float_or_none(chunk.get("queue_wait_ms"))
self.server_execution_ms = _float_or_none(chunk.get("execution_ms"))
self.server_tokens_per_s = _float_or_none(chunk.get("tokens_per_s"))
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+132
to
+136
| if step.is_token: | ||
| if ttft_ms is None: | ||
| ttft_ms = ns_to_ms(now_ns() - start_ns) | ||
| assert step.index is not None | ||
| token_event = StreamTokenEvent( |
Comment on lines
+139
to
+146
| token_str = chunk.get("token_str") | ||
| index = chunk.get("index") | ||
| if not isinstance(token_str, str): | ||
| raise StreamProtocolError("token event requires token_str") | ||
| if index != self.client_token_count: | ||
| raise StreamProtocolError( | ||
| f"expected token index {self.client_token_count}, got {index!r}" | ||
| ) |
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
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.
Why
In
decode_loop, we had two different terminal shapes:max_new_tokensmarked the last real token as terminal — and that token's decoded text could legitimately be"".v1 relayed both through the same
StreamChunkconstruction, so for any stream past the first token the two cases arrived at the client as byte-identical JSON. Nothing on the wire discriminated them, and a chunk-count heuristic only relocates the error: counting the token-less terminal chunk fixes max-tokens termination but breaks EOS-at-the-last-permitted-step. The bench client's rule (skip a token-less terminal chunk) was the better of two wrong defaults, but still undercounted in that narrow case.Surfacing v1's own tally was not a fix either: v1 counted non-empty token strings, dropping every empty-decoding token, so it would have handed the client a differently-wrong number wearing the authoritative
output_tokens_source: "server"label — worse than the honest client fallback. The discrimination had to originate indecode_loop, which is what this PR does: it removes the ambiguous terminal shape from the protocol entirely.What changed
The SSE streaming endpoints now emit an explicit event protocol instead of overloaded chunks: every payload carries a
typediscriminator, each sampled non-EOS token is delivered immediately as its own event, and a separate terminaldoneevent carries the authoritative token count, finish reason, and timing metrics. This removes the terminal-chunk heuristics the client previously needed to distinguish the v1 stream from v2+.StreamChunkwithStreamTokenEvent/StreamDoneEvent/StreamErrorEvent: discriminated union ontype,extra="forbid", and done events requirefinish_reason,prompt_tokens,output_tokens, and non-optional timing metricsdoneis sent as a separate terminal eventdecode_loopyields structuredGenerationSteprecords: EOS produces a completion step with no token event, and a token whose decoded text is empty still carries its index and is countedgenerate_textreports the sampled token count instead of re-tokenizing the output text, which undercounted when the decoded text did not retokenize cleanlyTokenEventdropsis_first/is_last;DoneEventgainsfinish_reason;FinishReasonmoves toserver/model/types.pyas a str enumprotocol_error