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
gongwei-130
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
github-actions
Bot
added
grpc
📝 WalkthroughWalkthroughAdds a new helper ChangesSGLang Stop Sequence Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RequestBuildingStage
participant Helpers as resolve_sglang_string_stops
participant Tokenizer
RequestBuildingStage->>Helpers: proto_request, tokenizer
Helpers->>Helpers: check request is Sglang and stop non-empty
alt tokenizer available
Helpers->>Tokenizer: encode each stop string
Tokenizer-->>Helpers: token id(s)
Helpers->>Helpers: single-token stops appended to stop_token_ids (deduped)
else no tokenizer
Helpers->>Helpers: log warning
end
Helpers->>Helpers: clear sampling_params.stop
Helpers-->>RequestBuildingStage: mutated proto_request
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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.
slin1237
force-pushed
the
fix/sglang-string-stops
branch
from
1935463 to
3608abf
Compare
…r_init 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. Make the router authoritative for string stops end to end:
1. resolve_sglang_string_stops (common/stages/helpers.rs) drains string stops
from SGLang requests in the chat/completions/messages request-building
stages — exactly the paths whose streaming AND non-streaming handlers both
run a trimming StopSequenceDecoder. Stop strings that encode to a single
token are forwarded as stop_token_ids so the worker halts early (skipped
under ignore_eos=true, where SGLang ignores stop_token_ids entirely).
Harmony (gpt-oss) and native /generate are deliberately NOT drained
(documented NOTEs): they have no trimming decoder, and a converted stop
token could truncate Harmony's <|return|>/<|call|> terminals.
2. StopSequenceDecoder records WHICH stop matched (MatchedStop::Sequence |
TokenId) so responses can report it.
3. utils::refine_stop_metadata applies the decoder's verdict at all six
response sites (chat/completions/messages x streaming/non-streaming):
decoder-matched string stop -> finish_reason="stop" + matched_stop is the
original stop string; a numeric matched_stop from a converted single-token
stop is mapped back to the user's string; EOS and user stop_token_ids pass
through unchanged. Also gates the streaming Messages stop_reason on an
actual string match (numeric matched_stop is end_turn, not
stop_sequence:null), and removes the completions endpoint's
stream-vs-non-stream finish_reason divergence.
4. Streaming early termination: when the router decoder matches a string stop
on an SGLang stream (n=1), record usage from the chunk's cumulative
counters, emit the final chunks, and break WITHOUT mark_completed() — the
stream drop sends the Abort RPC (AbortOnDropStream), so the worker no
longer generates to EOS/max_tokens (fixes usage.completion_tokens
overcount and latency for multi-token stops). Gated to SGLang streams and
Sequence matches; other backends halt on string stops server-side.
Tests: 12 unit tests for resolve_sglang_string_stops, 5 for
refine_stop_metadata, 2 for MatchedStop recording.
Verified end-to-end on a B200 against a real SGLang gRPC worker: stop=["."]
returns 200 with matched_stop="." (string), a multi-token stop returns
finish_reason="stop" + the stop string with the text trimmed, and the
streaming path aborts the worker (usage.completion_tokens 3 vs 488 for the
same request non-streamed; worker log shows "Receive abort request").
Signed-off-by: gongwei-130 <56567052+gongwei-130@users.noreply.github.com>
gongwei-130
changed the title
gongwei-130
force-pushed
the
fix/sglang-string-stops
branch
from
3608abf to
88f4402
Compare
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.