fix(frontend): unify stop-token semantics across model schedulers - #865
fix(frontend): unify stop-token semantics across model schedulers#865RicardoMin wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57270613ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
will review later |
|
The upstream limitation is already fixed by vLLM PR #47707 (merged at I locally bumped the five vLLM Rust crates to Could you please open a separate PR for the vLLM pin bump? Please keep the lockfile update narrowly scoped and include the zero-generated-token flush regression plus frontend compile/tests. With that bump in place, #865 can rely on the upstream fix instead of carrying a local detokenizer workaround. |
|
Thanks for confirming. I’ll open a separate PR to bump the five vLLM Rust crates to Once that PR lands, I’ll rebase #865 onto the updated main so it can rely on the upstream detokenizer fix. |
Signed-off-by: RicardoMin <17879681016@163.com>
ad9499f to
1428bfd
Compare
Description
This PR introduces a typed stop contract for stepped engines and implements it end-to-end for Qwen3, currently the only model using the stepped-engine path.
I discovered the issue while validating Qwen3 through the OpenAI-compatible completions API: explicitly configured
stop_token_idsdid not terminate generation when the matching token was produced.Problem
The stepped frontend did not carry primary EOS and request-level
stop_token_idsas independent stopping conditions. As a result, Qwen3 could not distinguish whether generation stopped because of EOS or because of an explicit request stop token.The intended contract is:
ignore_eosstop_token_idsstopfalsetrueignore_eosshould only control primary EOS handling. It should not disable explicit stop tokens supplied by the request.Changes
StopPolicyandStopCausecontracts for stepped engines.max_tokens.stop_reason, while primary EOS has nostop_reason.min_tokensbefore scheduler submission because stepped engines do not support its required logits masking yet.Scope
This PR changes the stepped frontend contract and its Qwen3 implementation only.
Qwen3.5, Kimi-K2, DeepSeek-V2-Lite, GLM5.2, and Gemma4 still use the legacy
EngineHandlepath, so their runtime behavior is unchanged. If this contract direction is accepted, those model paths can be migrated and validated separately.Validation
cargo fmt --all -- --checkgit diff --checkThe real HTTP coverage included explicit stop tokens with both values of
ignore_eos, length termination, streaming,include_stop_str_in_output, and primary-EOS behavior.For
stop_token_ids = [17], both values ofignore_eosproduced:Primary EOS was independently verified:
ignore_eos = false151645;finish_reason = stop;stop_reason = nullignore_eos = true151645and later finished withfinish_reason = lengthKnown upstream limitation
The repository currently pins vLLM revision
8e61b646e2d157f9b93451fa048f9c8530c8a67b.In that revision, when the first generated token is itself a stopping token, the detokenizer may incorrectly flush prompt text into the completion. This is a separate upstream issue fixed by vLLM PR #47707, so this PR does not add a local workaround.