Repository navigation
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@BugenZhao Did you have any chance to check this? |
…e client The sampler computes a sampled token's vocab rank as `(logprobs >= sampled_logprob).sum(-1)`, which is 0 when the sampled logprob is NaN because every IEEE-754 comparison against NaN is false. The engine-core client treated rank 0 as a corrupt wire payload and bailed during decode of the *batched* EngineCoreOutputs frame, which tripped the shared output-reader path and made the client record a permanent health error - killing every in-flight and subsequent request until process restart. Clamp rank 0 to the lowest valid rank, and map non-finite logprob values to the same -9999.0 sentinel the Python frontend uses (vllm-project#48585), so a NaN state degrades to one request's model-quality problem instead of a server outage. Tests: `cargo test -p vllm-engine-core-client` (100 passed), including new cases pinning rank-0 clamping, NaN-sentinel mapping, and survival of co-scheduled requests sharing the bad row's frame. Co-authored-by: Kimi (AI coding assistant) Signed-off-by: Islam <islam.almersawi@openinnovation.ai>
| // Map non-finite logprobs (from corrupt logits) to the same -9999.0 | ||
| // sentinel the Python frontend uses, so the serving layer can still | ||
| // serialize the response. | ||
| let logprob = if logprob.is_finite() { | ||
| logprob | ||
| } else { | ||
| -9999.0 | ||
| }; |
There was a problem hiding this comment.
I think this is not necessary? We can preserve the original value within the frontend pipeline, but only clamp it when it's going to be serialized in the serving layer, which we already have:
vllm/rust/src/server/src/routes/openai/utils/logprobs.rs
Lines 294 to 296 in b0898a4
Keep rank 0 and non-finite engine logprobs intact, leaving sentinel conversion at the OpenAI response boundary. Cover sample and prompt logprobs in a shared output batch and verify the existing HTTP NaN/negative-infinity conversion. Co-authored-by: Codex Signed-off-by: Bugen Zhao <i@bugenzhao.com>
257301a to
3344086
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #89228 for commit |
|
✅ @almersawi, CI is now available for this PR.
|
vllm-project#51026) Co-authored-by: Bugen Zhao <i@bugenzhao.com> Signed-off-by: Islam <islam.almersawi@openinnovation.ai> Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Purpose
Fix a total-outage bug in the Rust frontend's engine-core client: a single request asking for
logprobs/prompt_logprobswhile the model produces NaN logits permanently kills the client —/healthreturns 503 and every subsequent request (including ones with no logprobs) fails withengine-core error: messagepack ext value decode failed: token_ranks must be >= 1 for decoded engine-core logprobsuntil process restart.Rank 0 is an engine output that must be accepted: the sampler computes the rank as
(logprobs >= sampled_logprob).sum(-1)(vllm/v1/sample/sampler.py), which is 0 when the sampled logprob is NaN because every>=comparison against NaN is false. The decode failure is frame-scoped (one bad row fails the whole batchedEngineCoreOutputspayload), happens on the shared output-reader path, and is recorded as a sticky health error, so a per-request data anomaly becomes a server-wide outage.Fix: in
PositionLogprobs::from_decoded_row, accept and preservesampled_rank = 0along with the engine's original logprob values. This keeps decoding valid for the other requests in the batch. The existing Rust OpenAI serving helper converts NaN and negative infinity to-9999.0when constructing HTTP responses; engine-core decoding preserves the numerical results for all consumers.Why this does not duplicate existing PRs:
Healthy payloads retain their existing behavior. Model evaluation N/A: this changes wire-payload decode tolerance while preserving the engine's numerical output.
AI assistance was used for the original fix and the follow-up changes and tests. The follow-up preserves rank 0 and raw engine values and adds coverage for the existing HTTP conversion.
Test Plan
Two regression tests cover the layer boundary:
zero_sampled_rank_does_not_fail_frame_matesdecodes a sharedEngineCoreOutputsbatch, preserving rank 0, NaN, and negative infinity in both sample and prompt logprobs while keeping the healthy request intact.chat_logprobs_clamps_nan_and_negative_infinity_for_jsonverifies that the existing OpenAI response conversion emits-9999.0for NaN and negative infinity while preserving a finite logprob.Test Result
cargo nextest run --locked -p vllm-engine-core-client -p vllm-server -E 'package(vllm-engine-core-client) | test(routes::openai::utils::logprobs::tests)': 118 passed.cargo clippy --locked -p vllm-engine-core-client -p vllm-server --all-targets --all-features -- -D warnings: passed.cargo fmt --all -- --check,git diff --check, and commit-time pre-commit hooks: passed.Commands ran from
rust/with a fresh shortTMPDIRfor tests and an existing Cargo target directory. No GPU was required.Model evaluation N/A: the change preserves the engine's numerical output and adjusts frontend decode tolerance.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model. (N/A — internal decode-tolerance fix, no docs affected)