[Bugfix][Benchmark] Check readiness before tokenizer init in rust vllm-bench by tlrmchlsmth · Pull Request #51863 · vllm-project/vllm

tlrmchlsmth · 2026-08-11T19:05:27Z

Summary

Apply --ready-check-timeout-sec to the startup-dependent requests that can run before the benchmark readiness probe:

retry /v1/models discovery when --model is omitted
retry the /tokenize probe when tokenizer loading falls back to the server
retain the dataset-derived readiness request immediately before tokenizer verification and the timed run

This keeps the readiness request representative of the benchmark payload, including token IDs, multimodal content, and pooling inputs. Local tokenizer loading and dataset generation can still overlap server startup.

Duplicate work

Searched open PRs for ready-check-timeout, tokenizer readiness, and server-side tokenization readiness. No duplicate fix was found.

Testing

cargo test -p vllm-bench — 150 passed, 10 ignored
cargo clippy -p vllm-bench --all-targets -- -D warnings
cargo fmt --all -- --check
pre-commit run

AI assistance

AI assistance was used to implement and test this change. I reviewed every changed line and can explain and maintain the implementation.

Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Tyler Michael Smith <tlrmchlsmth@gmail.com>

claude

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

esmeetu

Ran an AI-assisted review (Claude Code) of this PR; findings below as inline comments, most severe first.

TL;DR: The PR moves the ready check earlier but leaves two real gaps: (1) the unretried /v1/models fetch still runs before the readiness wait in both orchestrators, so the startup race this PR targets still reproduces whenever --model is omitted; and (2) replacing the dataset-derived probe with a synthetic one ("test", output_len: 1) makes the check fail forever against healthy servers with --extra-body min_tokens or --skip-tokenizer-init, while also passing trivially when the real request shape (multimodal / token-id prompts) would be rejected — converting an upfront abort into a full run that saves completed=0 garbage results. Secondary findings cover the now-stale readiness probe, serialized boot-wait vs dataset generation, rerank payload knowledge duplicated in ready_checker, and missing test coverage for the new helper.

esmeetu · 2026-08-12T03:15:06Z

        (id, Some(name))
    };

+    run_initial_ready_check(config, &client, &model_id, &model_name).await?;


[correctness — confirmed] Model auto-resolution still hits the server before this ready check, so the readiness timeout never applies when --model is omitted.

When --model is omitted while the server is still starting (the exact scenario this PR targets), get_first_model_from_server (line 352) sends a single unretried GET /v1/models (request.send().await?) before run_initial_ready_check is reached, so the benchmark exits immediately with a connection-refused error instead of waiting --ready-check-timeout-sec. Same gap in run_multi_turn_benchmark (multi_turn.rs:77 via get_first_model).

Consider moving the ready check above model resolution (probing a health-ish endpoint first), or retrying the /v1/models fetch under the same timeout.

esmeetu · 2026-08-12T03:15:06Z

+        prompt: Arc::from("test"),
+        api_url: config.api_url.clone(),
+        prompt_len: 1,
+        output_len: 1,


[correctness — confirmed] The synthetic probe hardcodes output_len: 1 (rendered as max_tokens: 1) while still merging the user's extra_body, so --extra-body '{"min_tokens": 32}' (a common way to force output length) makes every probe fail with 400 min_tokens must be less than or equal to max_tokens against a perfectly healthy server. wait_for_endpoint treats each 400 as "not ready" and retries every 5s, so the run stalls for the full --ready-check-timeout-sec and then aborts with EndpointTimeout.

The removed code used the first dataset request's expected_output_len as max_tokens (normally >= min_tokens), so this configuration previously worked.

esmeetu · 2026-08-12T03:15:06Z

+        ])
+    });
+    let test_input = RequestFuncInput {
+        prompt: Arc::from("test"),


[correctness — confirmed] The probe sends a text prompt ("test") without prompt_token_ids, which breaks the ready check against servers running with --skip-tokenizer-init: such servers reject string prompts on /v1/completions with a 400 on every attempt, so the check retries until --ready-check-timeout-sec expires and aborts with EndpointTimeout.

The removed single-turn ready check cloned first.prompt_token_ids into the probe, so this setup previously worked (e.g. random dataset with token-id prompts, which sets prompt="" and relies on prompt_token_ids).

esmeetu · 2026-08-12T03:15:06Z

+            Arc::<str>::from("test document"),
+        ])
+    });
+    let test_input = RequestFuncInput {


[correctness — confirmed] The ready check no longer exercises the real benchmark request shape — multi_modal_content, chat_messages_json, and prompt_token_ids from the first dataset request are all dropped. Shape incompatibilities that previously aborted upfront now produce a full benchmark run with 0 completed requests.

Example: a multimodal dataset (VisionArena via openai-chat) against a model that rejects image input. Previously every probe got the server's 400 and the run aborted early with the underlying error in BenchError::EndpointTimeout. Now the text-only probe succeeds immediately, all N multimodal requests fail during the run, and — since metrics/calculator.rs has no all-requests-failed guard — the user waits out the whole run and gets a saved JSON result with completed=0 and zero/NaN metrics. Same applies to token-id probes if the server rejects token-array inputs.

Deriving the probe from the first dataset request (as before) while keeping the new early placement would need the dataset ready first — which conflicts with the goal here — but the probe could at least carry the request-shape-relevant fields once the dataset exists, e.g. by keeping a second shape-check before the timed run (see the staleness comment on benchmark.rs).

esmeetu · 2026-08-12T03:15:06Z

    };

+    run_initial_ready_check(config, &client, &model_id, &model_name).await?;
+


[correctness — plausible] With the probe moved before tokenizer download and dataset generation (which can take minutes for HF tokenizer fetch / ShareGPT / 100k-prompt random datasets), readiness is stale by the time the timed run starts:

reqwest's default pool_idle_timeout (90s, not overridden by this client builder) evicts the probe's connection in the interim, so for datasets that skip the /tokenize sample verification and with default num_warmups=0, the first benchmark request absorbs a fresh TCP/TLS handshake — inflating TTFT max/percentiles on small runs.

A server that crashes or restarts during dataset preparation — previously caught by the just-before-run probe — now surfaces as failed requests mid-benchmark instead of an upfront abort.

Same applies to multi_turn.rs:86.

esmeetu · 2026-08-12T03:15:06Z

        (id, Some(name))
    };

+    run_initial_ready_check(config, &client, &model_id, &model_name).await?;


[efficiency] This serializes the server-boot wait before tokenizer load and dataset generation instead of overlapping them. On main, dataset generation (minutes for large random/HF datasets) ran while the server booted, and the ready check at the end consumed only the remaining boot time; now wall-clock startup is boot_time + generation_time instead of roughly max of the two.

A cheaper shape that keeps the fix: spawn the ready check as a concurrent task here and await it before the first server-dependent step (server-side tokenizer fallback, /tokenize verification, warmup).

esmeetu · 2026-08-12T03:15:06Z

+        return Ok(());
+    }
+
+    let prompt_list = (config.backend == BackendKind::VllmRerank).then(|| {


[simplification] This rerank special case duplicates the pooling backend's payload contract (prompt_list = [query, doc1, ...], enforced in backends/pooling.rs::build_payload) at the ready-check layer. If another structured-input backend is added or rerank's minimum-input requirement changes, this probe silently becomes invalid and every run against a healthy server spins until the full --ready-check-timeout-sec, then dies with EndpointTimeout.

A probe-input constructor owned by the backend layer (e.g. a method on BackendKind/Backend in backends/mod.rs next to the payload builders) would keep the knowledge in one place.

esmeetu · 2026-08-12T03:15:06Z

 use crate::error::{BenchError, Result};

+/// Wait for the configured backend before tokenizer and dataset initialization.
+pub async fn run_initial_ready_check(


[test-coverage] run_initial_ready_check ships with no test coverage for its skip conditions or the rerank special case. The guard (return Ok on dry_run or ready_check_timeout_sec == 0) and the VllmRerank two-element prompt_list construction are pure, unit-testable logic. A future edit that inverts the dry_run guard would make --dry-run start issuing network requests (breaking the "dry runs stay offline" invariant documented in benchmark.rs) with nothing failing in CI.

esmeetu · 2026-08-12T03:15:06Z

+use crate::config::BenchConfig;
 use crate::error::{BenchError, Result};

+/// Wait for the configured backend before tokenizer and dataset initialization.


[conventions] The move dropped the original inline comments — // Ready check (benchmark.rs) and // Ready check with a simple single request (multi_turn.rs) — contrary to rust/AGENTS.md: "When refactoring or reconstructing code, always preserve the original comments and documentation VERBATIM, if applicable." The note that the multi-turn probe is intentionally "a simple single request" is lost.

Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Tyler Michael Smith <tlrmchlsmth@gmail.com>

esmeetu

LGTM

esmeetu · 2026-08-19T02:18:19Z

/ci run

github-actions · 2026-08-19T02:18:54Z

✅ Triggered Buildkite CI #84519 for commit aa0a648b4891.

…m-bench (vllm-project#51863) Signed-off-by: Tyler Michael Smith <tlrmchlsmth@gmail.com> Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Wenhua Cheng <wenhua.cheng@intel.com>

…m-bench (vllm-project#51863) Signed-off-by: Tyler Michael Smith <tlrmchlsmth@gmail.com> Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Zhu, Zufang <zufang.zhu@intel.com>

…m-bench (vllm-project#51863) Signed-off-by: Tyler Michael Smith <tlrmchlsmth@gmail.com> Co-authored-by: OpenAI Codex <codex@openai.com>

[Bugfix][Benchmark] Check readiness before tokenizer init

51fea48

Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Tyler Michael Smith <tlrmchlsmth@gmail.com>

tlrmchlsmth requested a review from esmeetu as a code owner August 11, 2026 19:05

claude Bot reviewed Aug 11, 2026

View reviewed changes

mergify Bot added rust bug Something isn't working labels Aug 11, 2026

esmeetu reviewed Aug 12, 2026

View reviewed changes

[Bugfix][Benchmark] Retry startup-dependent requests

aa0a648

Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Tyler Michael Smith <tlrmchlsmth@gmail.com>

esmeetu approved these changes Aug 19, 2026

View reviewed changes

esmeetu added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 19, 2026

esmeetu enabled auto-merge (squash) August 19, 2026 02:19

esmeetu merged commit aabc1a0 into vllm-project:main Aug 19, 2026
29 of 30 checks passed

		};

		run_initial_ready_check(config, &client, &model_id, &model_name).await?;

Uh oh!

Conversation

tlrmchlsmth commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Duplicate work

Testing

AI assistance

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

esmeetu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esmeetu left a comment

Choose a reason for hiding this comment

Uh oh!

esmeetu commented Aug 19, 2026

Uh oh!

github-actions Bot commented Aug 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tlrmchlsmth commented Aug 11, 2026 •

edited

Loading