fix(logprobs): preserve None/Some(0)/Some(k) contract end-to-end (#720)#722
fix(logprobs): preserve None/Some(0)/Some(k) contract end-to-end (#720)#722n-WN 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: eabbd5dc9c
ℹ️ 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".
| token_tx: req.token_tx, | ||
| logprobs: req.logprobs, | ||
| echo: req.echo, | ||
| logprobs: req.logprobs.unwrap_or(0), |
There was a problem hiding this comment.
Preserve
logprobs=0 on DeepSeek admission
For DeepSeek-V2-Lite requests, this unwrap_or(0) collapses the new Some(0) contract back into the disabled value. Admission later only rejects req.logprobs > 0, and token emission always sends logprob: None, so a client request with logprobs=0 is accepted but silently returns no chosen-token logprobs instead of honoring or rejecting it. Keep the Option through admission, or reject is_some() for this unsupported path.
Useful? React with 👍 / 👎.
| logprobs: req.logprobs, | ||
| echo: req.echo, | ||
| logprobs: req.logprobs.unwrap_or(0), | ||
| echo: req.prompt_logprobs.is_some(), |
There was a problem hiding this comment.
Suppress DeepSeek prompt-logprobs echo on rejection
Mapping any prompt_logprobs request onto the old echo flag leaves the stub echo path active: DeepSeek admission rejects these requests, but the rejection handling still calls send_prompt_echo(&pending), which emits TokenEvent::PromptTokens with all None logprobs before Rejected. Thus prompt_logprobs=0/1 still publishes the all-None prompt-logprobs payload this change is intended to remove; keep the unsupported request flag separate from echo emission or skip echo for rejected requests.
Useful? React with 👍 / 👎.
eabbd5d to
eeb59cc
Compare
…ninfer-project#720) The engine contract collapsed `logprobs: 0` and "logprobs disabled" into one value, dropped `prompt_logprobs` at the wire boundary, and the bridge silently discarded `TokenEvent::PromptTokens`. As a result `echo=true`+`logprobs` and explicit `prompt_logprobs` requests hit HTTP 500 in response assembly, and `logprobs=-1` was silently mapped to 0 instead of failing fast. Engine contract (openinfer-engine): - `GenerateRequest.logprobs: usize` -> `Option<usize>` (None = disabled, Some(0) = chosen-token logprob only, Some(k) = +k alts) - `GenerateRequest.echo: bool` -> `prompt_logprobs: Option<usize>` vLLM frontend: - wire.rs: `requested_logprobs` preserves None/Some(0)/Some(k); `prompt_logprobs` is now read; `-1` (full-vocab sentinel) rejected early via `unsupported_sampling` for both fields, before any GPU work. - bridge.rs: submit maps `prompt_logprobs=Some(k)` to the engine prompt-logprobs request; `TokenEvent::PromptTokens` is encoded into `new_prompt_logprobs_tensors` with vLLM semantics (scored positions only, leading None position omitted; single-token prompt sends no payload). Qwen3 (only crate that computes prompt logprobs): - scheduler/plan/resolve gate on `prompt_logprobs.is_some()`; all echo invariants kept (whole-prompt single forward, no chunking, no prefix cache match, dedicated Prefill routing, all-position logits). Honor-or-reject for crates without prompt-logprob support: - Kimi-K2: reject prompt logprobs at admission (lm_head runs on last position only); row options take `Option<usize>`. - Qwen3.5: reject at admission instead of emitting all-None PromptTokens. - GLM5.2: admission message now names completion/prompt logprobs. - DeepSeek-V2-Lite: reject prompt logprobs (was silent all-None echo stub). Tests: - wire: None/0/k/-1 matrix for both fields. - bridge: PromptTokens -> wire payload (multi-token + single-token cases). - sim frontend_e2e: HTTP regression for echo+logprobs (was HTTP 500), explicit prompt_logprobs, logprobs=0, -1 early rejection, single-token prompt. - Model crates: scheduler/plan/executor tests migrated to Option fields; batch-invariance gates updated for Option step-item logprobs. CPU logprob computation is unchanged; GPU-side reduction is openinfer-project#719. Verified: - cargo check + clippy --all-targets: 9 crates clean - openinfer-vllm-frontend: 27 unit tests - openinfer-sim frontend_e2e: 19 HTTP regression tests - model-crate lib tests: 150 passed - qwen3 dflash_speculative_gate on H100: 4 passed - qwen3 hf_golden_gate (logprobs vs HF golden, bf16 tolerance): 1 passed
eeb59cc to
817e963
Compare
FeathBow
left a comment
There was a problem hiding this comment.
Both upstream fixes required by this integration have landed:
However, all four vLLM Rust crates in this PR and current main remain pinned to 8e61b646, 462 commits behind f25953c. The OpenInfer build therefore still contains both the single-token prompt-logprobs failure and the zero-column payload panic. Consequently, the E2E coverage added by #722 cannot establish that the actual vLLM integration is correct.
Let's keep #722 blocked and revisit it after vLLM publishes the next version and OpenInfer refreshes all four crate revisions plus the lockfile. The Rust frontend and integration gates must then be rerun against that updated pin.
| if ids.len() > 1 { | ||
| let positions: Vec<PositionLogprobs> = ids | ||
| .iter() | ||
| .zip(logprobs.iter()) |
There was a problem hiding this comment.
zip silently truncates when ids.len() != logprobs.len(), while the following filter_map drops any non-leading position whose logprob is None. Both cases produce fewer wire positions than prompt positions, violating the positional contract and potentially shifting subsequent values.
We could validate equal lengths and permit None only for the protocol-defined leading position. Any other mismatch or missing value should terminate the request with an explicit error instead of degrading the payload.
| } | ||
|
|
||
| if req.echo { | ||
| if req.prompt_logprobs.is_some() { |
There was a problem hiding this comment.
Once prompt logprobs are requested, this path must not synthesize a successful all-None result. extract_prompt_logprobs converts both device extraction and D2H failures to None via .ok(), and the fallback below turns that missing result into vec![None; prompt_len]; the bridge then drops those positions.
IMO We could make extraction and result propagation fallible and emit TokenEvent::Error instead of converting real computation failures into missing logprobs.
Closes #720
Summary
The engine contract collapsed
logprobs: 0and "logprobs disabled" into one value, droppedprompt_logprobsat the wire boundary, and the bridge silently discardedTokenEvent::PromptTokens. As a result,echo=true+logprobsand explicitprompt_logprobsrequests hit HTTP 500 in response assembly, andlogprobs=-1was silently mapped to 0 instead of failing fast.Changes
Engine contract (
openinfer-engine)GenerateRequest.logprobs: usize → Option<usize>—None= disabled,Some(0)= chosen-token logprob only,Some(k)= chosen + k alternativesGenerateRequest.echo: bool → prompt_logprobs: Option<usize>vLLM frontend
wire.rs:requested_logprobspreservesNone/Some(0)/Some(k);prompt_logprobsis now read;-1(full-vocab sentinel) rejected early viaunsupported_samplingfor both fields, before any GPU workbridge.rs: submit mapsprompt_logprobs=Some(k)to the engine prompt-logprobs request;TokenEvent::PromptTokensis encoded intonew_prompt_logprobs_tensorswith vLLM semantics (scored positions only, leadingNoneposition omitted; single-token prompt sends no payload)Qwen3 (the only crate that computes prompt logprobs)
prompt_logprobs.is_some(); all echo invariants kept: whole-prompt single forward, no chunking, no prefix-cache match, dedicated Prefill routing, all-position logitsHonor-or-reject for crates without prompt-logprob support (#236 precedent)
Option<usize>NonePromptTokensNoneecho stub)Tests
None/0/k/-1matrix for both fieldsPromptTokens→ wire payload (multi-token and single-token cases)frontend_e2e: HTTP regression forecho+logprobs(was HTTP 500), explicitprompt_logprobs,logprobs=0,-1early rejection, single-token promptOptionstep-item logprobsCPU logprob computation is unchanged; the GPU-side reduction is #719.
Verification
cargo check+cargo clippy --all-targets(9 crates)openinfer-vllm-frontendunit testsopeninfer-sim --test frontend_e2e(HTTP regression, CI gate)--libtests (qwen3/qwen35/kimi/glm52/deepseek)dflash_speculative_gate(H100)hf_golden_gate— logprobs vs HF golden, bf16 tolerance (H100)qwen35 / kimi-k2 golden gates self-skip on this box (model weights not present); their migrated test code compiles under
clippy --all-targets.