Skip to content

patches: bench-probe-errors — the /tokenize alignment probe names its failure and sends the key - #165

Merged
mhenrichsen merged 4 commits into
syv-ai:mainfrom
TyroneNel:bench-probe-errors
Sep 22, 2026
Merged

mhenrichsen merged 4 commits into
syv-ai:mainfrom
TyroneNel:bench-probe-errors

Conversation

@TyroneNel

@TyroneNel TyroneNel commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

What

Adds patches/bench-probe-errors.patch, its patches/series line and its PATCHES.md row. The patch changes vllm bench serve's tokenizer-alignment probe: it sends the Authorization header the benchmark requests already send, and it classifies its failure instead of printing one line for every cause.

Upstream: vllm-project/vllm#58024.

Why

In 0.28.0, the probe posts args.model verbatim as the request model and swallows every exception into one line:

WARNING: /tokenize unavailable, skipping alignment.

bench/run_benchmarks.sh:25 passes --model $MODEL (a checkpoint path) with --served-model-name qwen3.8-27b, so the probe asks for a name the server does not serve and gets a 404 from the model check. The server is healthy and the benchmark runs, but the warning says the endpoint is unavailable. In my results tree, 314 of 602 benchmark logs carry that line, split exactly along dataset_name='random' versus 'custom'.

The probe also sends no key, so on a keyed server alignment is skipped in every run.

#170 fixes the caller in this repo. This patch fixes the message for everyone who reads it.

Runtime evidence

Patched function loaded from the fork commit, driven against a local aiohttp server (CPU only):

404  -> WARNING: /tokenize unavailable (404 Not Found: either this server has no /tokenize route, or it does not serve a model named `my-model` ...), skipping alignment.
401  -> WARNING: /tokenize unavailable (401 Unauthorized: the server requires an API key), skipping alignment.
500  -> WARNING: /tokenize unavailable (HTTP 500), skipping alignment.
down -> WARNING: http://127.0.0.1:18098 unreachable (ClientConnectorError(...)), skipping alignment.
200 with OPENAI_API_KEY -> probe sent: Bearer secret-key
200 with --header       -> probe sent: Bearer from-header

Verification

  • patch integrity (the workflow's git-apply job) applies the whole series to a pristine vllm-project/vllm checkout at the pin: passes on this PR.
  • patch -p1 --fuzz 0 --dry-run of this file against the installed tree in ghcr.io/syv-ai/hyperqwen:latest (06150174): applies, no fuzz.
  • verify.sh needs no new entry: its loop reads patches/series, and patches/_check_applied.py parses the patch file itself.

Not done: I have not rebuilt the image and restarted a server on this patch, so the runtime evidence above comes from the code path, not from a rebuilt server.

… and sends the key

vllm bench serve's tokenizer-alignment probe collapsed every failure into
one wrong message ("/tokenize unavailable") and sent no Authorization header,
so a keyed server 401'd it even with a valid model name. The patch sends the
same Bearer the benchmark requests carry and names the real cause: 404
(no route, or the model name was rejected — /v1/models lists the served
names), 401 (key required), unreachable, timeout, or the raw exception.

Independent of every other patch in the series (benchmarks/serve.py is
untouched by them), so it rides at the end of patches/series. Cut from the
extended cpuchip/vllm qwen38/0.28 branch, topic commit [qwen38]
bench-probe-errors; kind: fix, retires when upstream takes it.
fetch_spec_decode_metrics and fetch_diffusion_metrics GET /metrics with no
headers and return None on any non-200, so on a server bound with --api-key
the 401 is indistinguishable from 'speculative decoding is off' and the
benchmark's whole spec-decode block (acceptance length, accepted/drafted,
per-position acceptance) silently disappears from every run.

Both fetchers now take the headers the benchmark already built; the four
call sites in benchmark() pass extra_headers, the same dict the request path
uses. Unkeyed servers and the None-on-missing-metrics contract are unchanged.

Observed on hyperqwen d6e094a3 with a 48-char VLLM_API_KEY: GET /metrics
keyless 401, with the key 200, /health 200 keyless; the bench client's 401s
share an ephemeral port with its own POST /v1/completions, which is what
identifies it (rather than run_benchmarks.sh's curl) as the caller.
@TyroneNel

Copy link
Copy Markdown
Contributor Author

Amended: the patch now also fixes the /metrics scrapes.

fetch_spec_decode_metrics and fetch_diffusion_metrics GET /metrics with no headers and return None on any non-200, so against a server bound with --api-key the 401 is indistinguishable from "speculative decoding is off" — the benchmark's spec-decode block (acceptance length, accepted/drafted counts, per-position acceptance) silently disappears from every run on a keyed server. Same omission as the probe, quieter symptom.

Both fetchers take headers and pass it to the GET; the four call sites in benchmark() hand over extra_headers, the dict the request path already uses. Unkeyed behaviour and the None-on-missing-metrics contract are unchanged. Patch goes 5 hunks -> 13.

Runtime evidence (hyperqwen d6e094a3, 48-char VLLM_API_KEY):

GET /metrics  (no header)  -> 401
GET /metrics  (Bearer key) -> 200
GET /health   (no header)  -> 200

The 401s in the server log share an ephemeral port with the bench client's own POST /v1/completions, which is what identifies vllm bench serve — not run_benchmarks.sh's curl, whose scrapes come from separate one-shot ports and return 200.

Verification of the regenerated file: git apply --check clean against a pristine upstream benchmarks/serve.py; applying it reproduces the intended tree byte-for-byte; ast.parse clean; patches/_check_applied.py returns 0 against the patched tree and 1 against upstream.

Disclosure: this repo has no cpuchip/vllm checkout and docs/fork-workflow.md is absent here, so I could not regenerate through scripts/export-patch.sh. The diff was produced by reverse-applying the previous patch to recover pristine upstream, editing, and re-diffing — content-equivalent, but it needs a matching commit on the fork branch before the next export, or that export will revert it. Upstream vllm-project/vllm#58024 needs the same two functions.

… header

Threading extra_headers into fetch_spec_decode_metrics and
fetch_diffusion_metrics was not enough. That dict only ever carries the
--header pairs, so on a server bound with --api-key and no --header it is
empty, and all four /metrics scrapes still came back 401 -- which both
fetchers return as None, indistinguishable from 'speculative decoding is
off'. The spec-decode block then vanished from every run on a keyed server,
which is the exact symptom the patch set out to fix.

Both fetchers now build their scrape headers the way the /tokenize probe
does: start from whatever the caller passed, then add the Bearer from
OPENAI_API_KEY unless one is already set.

Observed on a keyed server: repeated 'GET /metrics 401 Unauthorized' from a
single long-lived bench-client session, while the harness's own keyed curl
scrapes on neighbouring ports returned 200.
TyroneNel added a commit to TyroneNel/qwen38-27b-rtx3090 that referenced this pull request Sep 22, 2026
@mhenrichsen

Copy link
Copy Markdown
Contributor

Merging this, with an independent check of the series rather than of the file alone.

Applied the whole patches/series to a pristine vllm-project/vllm checkout at the pin, in series order, with this repo's own patches/check_vllm_series.sh — and with the other four of your five stacked on top of it, since they all land in the same two files and will meet each other on the way in:

== pass 1: the whole series, GNU patch, patches/series order
   43 patches applied with exact context, 0 of them at an offset, 0 with fuzz
== pass 2: the ordered DFlash patches, git apply --check
patch integrity: OK

The premise checks out in the pinned source too, which is the part worth stating since neither of us has booted a rebuilt image: benchmarks/serve.py:76-120 posts model_id into /tokenize with no headers at all and swallows every exception into the one line you quote, and the two /metrics fetchers do the same with return None on non-200 — so on a keyed server the spec-decode block really is indistinguishable from "speculative decoding is off". That second omission is the one that mattered: a wrong warning costs a reader five minutes, but a silently absent acceptance block costs a measurement.

The amended shape is the right one. extra_headers alone would have fixed only the --header case, and starting from the caller's dict and then setdefault-ing the Bearer keeps --header authoritative, which is what someone passing a custom auth header expects.

On the regeneration disclosure: noted, and it is the right thing to have said. Reverse-applying to recover pristine upstream and re-diffing is content-equivalent — check_vllm_series.sh would have caught any drift, since it applies against a real upstream checkout rather than against the tree you edited — but the fork branch does need the matching commit before the next export-patch.sh run or the export will quietly revert it. @cpuchip, this is one for cpuchip/vllm: the bench-probe-errors branch there is behind what is now in patches/.

The remaining gap is the same for all five: nobody has booted a server from a rebuilt image carrying them. The image rebuilds on this merge, so that is coming; if anything in the series misbehaves at boot I will revert rather than patch forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants