fix(grpc): router-authoritative string stop sequences for SGLang skip_tokenizer_init workers - #1877
fix(grpc): router-authoritative string stop sequences for SGLang skip_tokenizer_init workers#1877gongwei-130 wants to merge 1 commit into
Conversation
github-actions
Bot
added
grpc
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesSGLang Stop Sequence Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RequestBuildingStage
participant StopResolver
participant Tokenizer
participant SGLangWorker
RequestBuildingStage->>StopResolver: mutable request and tokenizer
StopResolver->>Tokenizer: encode string stops
Tokenizer-->>StopResolver: token IDs
StopResolver-->>RequestBuildingStage: normalized request
RequestBuildingStage->>SGLangWorker: dispatch request
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves issue #227, where SGLang gRPC workers reject string stop sequences when running with skip_tokenizer_init=True. It introduces a helper function resolve_sglang_string_stops that clears string stops from SGLang requests and converts single-token stops into stop_token_ids as an optimization, leaving multi-token stops to be handled by the router-side decoder. This helper is integrated into the chat, completion, generate, and message request building stages, and is accompanied by comprehensive unit tests. There are no review comments, so we have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/routers/grpc/regular/stages/chat/request_building.rs`:
- Around line 133-138: The repeated explanatory comment around
resolve_sglang_string_stops is duplicated across the request-building stages, so
trim the call site near proto_request handling to a short reference instead of
keeping the full 5-line block. Update the comment in the request_building stage
using the resolve_sglang_string_stops helper name as the anchor, and keep only a
brief note that points to its existing doc comment so the
chat/completion/generate/messages paths stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bbf22c3-3106-4500-b6e7-a40082c9b3ab
📒 Files selected for processing (5)
model_gateway/src/routers/grpc/common/stages/helpers.rsmodel_gateway/src/routers/grpc/regular/stages/chat/request_building.rsmodel_gateway/src/routers/grpc/regular/stages/completion/request_building.rsmodel_gateway/src/routers/grpc/regular/stages/generate/request_building.rsmodel_gateway/src/routers/grpc/regular/stages/messages/request_building.rs
| // issue #227: SGLang gRPC workers run with skip_tokenizer_init and | ||
| // reject string `stop` sequences. Resolve them router-side (drop the | ||
| // strings, convert single-token stops to stop_token_ids) before | ||
| // dispatch; the router-side StopSequenceDecoder handles text trimming. | ||
| helpers::resolve_sglang_string_stops(&mut proto_request, ctx.tokenizer_arc().as_ref()); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Correctly wired after sampling defaults, before PD metadata injection.
The repeated 5-line explanatory comment block is duplicated verbatim across all four request-building stages (chat/completion/generate/messages). Since resolve_sglang_string_stops already carries a thorough doc comment at its definition, consider trimming each call site to a one-line reference (e.g. // issue #227: see resolve_sglang_string_stops doc) to reduce comment-maintenance drift across 4 files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model_gateway/src/routers/grpc/regular/stages/chat/request_building.rs`
around lines 133 - 138, The repeated explanatory comment around
resolve_sglang_string_stops is duplicated across the request-building stages, so
trim the call site near proto_request handling to a short reference instead of
keeping the full 5-line block. Update the comment in the request_building stage
using the resolve_sglang_string_stops helper name as the anchor, and keep only a
brief note that points to its existing doc comment so the
chat/completion/generate/messages paths stay consistent.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// Only SGLang is affected: the vLLM servicer forces `detokenize=bool(stop)`, | ||
| /// TRT-LLM tokenizes stop words server-side, and the MLX proto has no | ||
| /// string-`stop` field. Non-SGLang requests are left untouched. | ||
| pub(crate) fn resolve_sglang_string_stops( |
There was a problem hiding this comment.
🔴 Important: HarmonyRequestBuildingStage also builds SGLang requests via build_generate_request_from_chat → build_grpc_sampling_params_from_chat, which sets stop: stop_sequences (string stops extracted from the Chat request). But resolve_sglang_string_stops is never called on the Harmony path — so Harmony Chat requests carrying string stop sequences against an SGLang worker will still get the 400 rejection from SamplingParams.verify().
The Harmony stage (harmony/stages/request_building.rs) builds the proto at line 148, injects Harmony stop token IDs at line 334, then jumps to PD metadata — there's no call to this function in between. The Responses path is safe (it hardcodes stop: vec![] at sglang_scheduler.rs:445), but the Chat path is exposed.
Suggested fix: add a resolve_sglang_string_stops(&mut proto_request, ctx.tokenizer_arc().as_ref()) call in HarmonyRequestBuildingStage::execute after the proto is built (before or after the Harmony stop token injection block).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9cf09a1e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Always drop the string stops from the SGLang request: the worker cannot | ||
| // handle them under skip_tokenizer_init and the router-side decoder is the | ||
| // source of truth for string-stop matching/trimming. | ||
| let stop_strings = std::mem::take(&mut params.stop); |
There was a problem hiding this comment.
When a SGLang request has a multi-token stop sequence, this removes it from the backend request and no replacement stop_token_id is added. I checked the regular response processors: they derive finish_reason/stop_reason from complete.finish_reason() and matched_stop_json(), not from StopSequenceDecoder::is_stopped, so these requests can be truncated by the router but still report length/end_turn and omit stop_sequence after the worker runs to max tokens or EOS. Please propagate the router-side stop match into the response metadata, or avoid clearing stops without an equivalent signal.
Useful? React with 👍 / 👎.
| let id = ids[0]; | ||
| if !params.stop_token_ids.contains(&id) { | ||
| params.stop_token_ids.push(id); |
There was a problem hiding this comment.
For Messages API stop_sequences that encode to one token, this appends only a numeric stop_token_id. The SGLang servicer returns that as a numeric matched token, but the non-streaming Messages processor only treats matched_stop_json().as_str() as a stop sequence, so a request stopped by a user sequence like "." is returned as end_turn with stop_sequence: null; streaming similarly cannot fill stop_sequence. Keep a mapping from the inserted token id to the original string, or skip this optimization for Messages.
Useful? React with 👍 / 👎.
slin1237
left a comment
slin1237
left a comment
There was a problem hiding this comment.
it doesn't look like the fix should be here?
And even if we wanna add a code like this for SGLang, the code shouldn't be in helper
Also. Please combine all the if statement to single if.
And follow pull request template.
1935463 to
3608abf
Compare
gongwei-130
changed the title
3608abf to
88f4402
Compare
…it workers
SGLang gRPC workers run with skip_tokenizer_init=True (the router owns the
tokenizer). The router forwarded string `stop` sequences as-is into the gRPC
SamplingParams, and upstream SGLang's SamplingParams.verify() rejects them in
that mode ("stop=[...] is unavailable when skip_tokenizer_init=True"),
returning a 400 for any request carrying the OpenAI-compatible `stop`
parameter.
The router already matches string stops itself via StopSequenceDecoder (it
detokenizes worker output and trims), so the worker never needs the raw
strings. Add resolve_sglang_string_stops(), applied after building the gRPC
request in the chat/completion/messages/generate stages, which:
- clears the string `stop` list on SGLang requests (fixes the 400
unconditionally; the router-side decoder still trims the text); and
- converts any stop string that maps to a single token into stop_token_ids
so the worker can still halt early for the common case (e.g. ["."], ["\n"]).
Multi-token stops cannot be represented in the flat stop_token_ids field,
so those rely on the router-side decoder.
Only SGLang is affected: the vLLM servicer forces detokenize=bool(stop),
TRT-LLM tokenizes stop words server-side, and MLX has no string-stop field.
Adds 8 unit tests covering single/multi-token, mixed, dedup, empty/unknown,
no-tokenizer, no-op, and non-SGLang-untouched cases.
Signed-off-by: gongwei-130 <56567052+gongwei-130@users.noreply.github.com>
88f4402 to
c07b7e9
Compare
| // reject string `stop` sequences. Resolve them router-side (drop the | ||
| // strings, convert single-token stops to stop_token_ids) before | ||
| // dispatch; the router-side StopSequenceDecoder handles text trimming. | ||
| helpers::resolve_sglang_string_stops(&mut proto_request, ctx.tokenizer_arc().as_ref()); |
There was a problem hiding this comment.
🔴 Important: The PR description says "Native /generate: its streaming path has no decoder, so a cleared stop would stream untrimmed text. Keeps the pre-existing 400." — but this call does the opposite: it clears string stops, removing the 400.
The non-streaming generate path has a StopSequenceDecoder (processor.rs:378), so text gets trimmed correctly there. But the streaming generate path (streaming.rs:834–945) directly accumulates decoded text with no stop-sequence matching at all:
// streaming.rs:867-873
let chunk_text = tokenizer
.decode(chunk.token_ids(), true)
.unwrap_or_default();
let accumulated_text = accumulated_texts.entry(index).or_default();
accumulated_text.push_str(&chunk_text);For multi-token string stops on a streaming /generate request, this changes the behavior from:
- Before: SGLang rejects the request with a 400 (explicit failure)
- After: SGLang accepts it, generates past the stop sequence, and streams untrimmed text containing the stop string (silent incorrect output)
Single-token stops that convert to stop_token_ids are likely fine (worker halts before emitting that token), but any multi-token stop will silently produce wrong output.
Consider either (a) gating this call on !ctx.input.request_type.is_streaming() so streaming generate preserves the pre-existing 400, or (b) adding a StopSequenceDecoder to the generate streaming path to match chat/completions/messages.
Engines that can't match string `stop` sequences server-side previously handled them in three divergent ways: SGLang gRPC (skip_tokenizer_init) had a dedicated resolver that dropped the strings and converted single-token stops to stop_token_ids; the ZMQ vLLM path silently dropped strings with no early-stop conversion; and the ZMQ TokenSpeed path rejected any request carrying `stop` with a 400. Consolidate all of them onto one helper, resolve_string_stops, applied in the chat/completion/generate/messages request-building stages. For backends whose engine sees token ids only — SGLang on any transport, and every direct-ZMQ backend — it drops the string `stop` list (the router-side StopSequenceDecoder trims the text) and forwards single-token stops as stop_token_ids for early stopping; multi-token stops fall to the decoder. gRPC vLLM (servicer detokenizes), TRT-LLM (server-side), and MLX keep their strings untouched, gated by the backend's is_zmq() flag. This fixes the TokenSpeed 400 (stop strings now work over ZMQ) and gives the ZMQ vLLM path the single-token early-stop optimization it lacked. The finish_reason=stop synthesis added earlier is already backend-neutral, so the two halves together give consistent stop-string behavior across gRPC-SGLang, ZMQ-vLLM, and ZMQ-TokenSpeed. Supersedes #1877 (SGLang-only input-side fix). Adds unit tests for the shared resolver and for TokenSpeed accepting residual stop strings. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
Closing — superseded by #2057 (
ProtoGenerateRequest::Sglang(req) => {
if let Some(params) = req.sampling_params.as_mut() {
let stops = std::mem::take(&mut params.stop);
encode_single_token_stops(stops.clone(), &mut params.stop_token_ids, tokenizer);
return stops;
}
}Same approach — drop the string
Nothing here is worth carrying forward on top of that. Thanks to whoever picked the approach up. |
5 participants
Summary
SGLang gRPC workers run with
skip_tokenizer_init=True(the router owns the tokenizer). The router forwards stringstopsequences as-is into the gRPCSamplingParams, and SGLang'sSamplingParams.verify()rejects them in that mode:So any request carrying the OpenAI-compatible
stopparameter against an SGLang gRPC worker gets a 400. This PR makes the router authoritative for string stops end to end: the 400 is gone, output text is trimmed correctly,finish_reason/matched_stop/ Anthropicstop_reason+stop_sequencematch what vLLM reports for the same request, and streaming requests abort the worker when the router matches a stop — so multi-token stops no longer over-generate tomax_tokens(no inflatedusage.completion_tokens, no wasted GPU time).SGLang-specific: the vLLM servicer forces
detokenize=bool(stop), TRT-LLM tokenizes stop words server-side, MLX has no string-stopfield.Design
The router already decodes worker output through a
StopSequenceDecoderper request. This PR completes that into full ownership of string stops:1. Drain string stops from SGLang requests (
resolve_sglang_string_stopsincommon/stages/helpers.rs), called from the chat / completions / messages request-building stages — exactly the paths whose streaming and non-streaming handlers both run a trimming decoder. As an optimization, a stop string that encodes to a single token is forwarded as astop_token_idsentry so the worker halts immediately (skipped whenignore_eos=true, where SGLang skips allstop_token_idsmatching — seeReq._check_token_based_finish).2. Record what matched —
StopSequenceDecodernow tracksMatchedStop::Sequence(String) | TokenId(u32)when it stops.3. Refine response metadata (
utils::refine_stop_metadata, applied at all six response sites: chat/completions/messages × streaming/non-streaming):finish_reason="stop",matched_stop= the original stop string (was:"length"+ null);stop_token_idspass through unchanged.Also gates the streaming Messages
stop_reasonon an actual string match (a numeric matched_stop isend_turn, notstop_sequencewith a null sequence), and removes the completions endpoint's stream-vs-non-streamfinish_reasondivergence.4. Streaming early termination — when the router decoder matches a string stop on an SGLang stream (n=1), the loop records usage from the chunk's cumulative counters, emits the final chunks, and breaks without
mark_completed(); dropping the stream sends the Abort RPC (AbortOnDropStream). Gated to SGLang streams and actualSequencematches — vLLM/TRT-LLM paths are untouched.Deliberately not drained (documented
NOTEs at the call sites)<|return|>/<|call|>, corrupting Harmony parsing. Keeps the pre-existing 400./generate: its streaming path has no decoder, so a cleared stop would stream untrimmed text. Keeps the pre-existing 400.Known remaining limitations (in the helper docstring)
finish_reason/matched_stopare correct, but the worker still generates to EOS/max_tokensbefore the response is assembled. Needs early abort inresponse_collection— follow-up.Tests
resolve_sglang_string_stops: 12 unit tests (single/multi-token, mixed, dedup, empty/unknown, whitespace single-token via a wrapper tokenizer, two distinct single tokens,ignore_eos, missingsampling_params, no tokenizer, non-SGLang untouched)refine_stop_metadata: 5 unit testsStopSequenceDecoder::matched_stop: 2 unit testssmglib suite green; clippy/fmt cleanEnd-to-end verification (B200, real SGLang gRPC worker)
Same host, same engine image and weights, only the router build swapped:
stop=["."](non-stream) → 200,finish_reason="stop",matched_stop="."(the string, not a token id), text trimmed at the stopfinish_reason="stop",matched_stop=<stop string>, text trimmed exactly before the stopusage.completion_tokens=3vs 488 for the identical request non-streamed (worker log showsReceive abort request: ...), confirming the Abort path worksThis is the same change merged in our production deployment after review and e2e validation.