fix(vllm): pool embedding worker output via PoolingParams(task="embed") - #10248
Conversation
The text-embedding worker built ``PoolingParams()`` with no task. On vLLM 0.21, ``encode()`` with ``task=None`` resolves to per-token output, so the worker returned the full ``n_tokens x hidden`` hidden-state matrix (un-normalized) instead of one pooled, L2-normalized vector per input. The OpenAI ``/v1/embeddings`` response dimension then scaled with input length (e.g. ~80*1024 for an 80-token prompt) instead of the model's native 1024. Setting ``task="embed"`` selects the pooled+normalized embedding, matching vLLM's own embedding server. Verified on GB200 with Qwen3-Embedding-0.6B: the response is a fixed 1024-dim vector for any input length, with ||v|| = 1.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
WalkthroughThe embeddings request handler in the vLLM integration now explicitly configures vLLM's pooling behavior by passing ChangesEmbeddings Pooling Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Address review: move ``from vllm import PoolingParams`` out of ``EmbeddingWorkerHandler.generate`` up to the module-level vllm imports, per the repo's all-imports-at-top guideline. Also clarify in the comment that normalization follows the model's pooler default (``use_activation`` left unset, matching vLLM's own embedding server) rather than being forced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
|
Confirmed the bug is live on both vLLM 0.21.0 and 0.22.0 (main pins 0.22.0). Built a dynamo image on the 0.22.0 base and ran the unpatched worker —
So it returns |
|
Can confirm this resolves embedding model issues on 1.3.0.dev.1 images. Using this patch on 1.3.0 dev.1 image: Usage in Dockerfile: |
The embedding worker honored OpenAI `dimensions` by truncating the
pooled vector itself (`embedding[:dimensions]`) without re-normalizing,
so a Matryoshka-reduced vector came back with norm < 1. Forward
`dimensions` into `PoolingParams(task="embed", dimensions=...)` instead:
vLLM's pooler truncates *and* re-normalizes (correct MRL) and rejects
models that don't declare Matryoshka support, matching bare `vllm serve`.
- handlers.py: build PoolingParams with `dimensions` when requested; drop
the post-hoc slice + exceeds-dim check (vLLM now owns both).
- agg_embed.sh: launch the default Qwen3-Embedding-0.6B (Matryoshka-capable
but not declared in its HF config) with
`--hf-overrides '{"is_matryoshka": true}'` so `dimensions` requests are
accepted; guarded to the default model.
- tests: cover dimensions forwarding (and its absence); note the override
in the serve test's dimensions case.
Addresses the review comment about setting dimensions=dimensions on #10248.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
…pooling-task Signed-off-by: Tzu-Ling <tzulingk@nvidia.com> # Conflicts: # components/src/dynamo/vllm/tests/test_vllm_worker_handler.py
This comment has been minimized.
This comment has been minimized.
Devin review on #10248 flagged that removing the post-hoc truncation dropped the clear ValueError when a client requests more `dimensions` than the model produces. vLLM rejects an unsupported `dimensions` only for models that declare a `matryoshka_dimensions` list; a model enabled via `--hf-overrides '{"is_matryoshka": true}'` (no list) is only validated for `dimensions >= 1`, and the pooler then silently clamps an oversized request to the model's native size (`embeddings[..., :d]`). Re-add a post-encode guard that raises the same clear `dimensions=N exceeds model embedding dimension M` error when the returned vector is shorter than requested, without re-introducing client-side truncation (vLLM still owns the Matryoshka reduction). Adds a unit test for the oversized case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
The new oversized-dimensions guard (handlers.py) raises when the returned vector is shorter than the requested `dimensions`. The existing `test_dimensions_forwarded_to_pooling_params` mocked a 3-float encode output while requesting dimensions=128, so the guard now (correctly) rejects it. Make the stub return a 128-dim vector to simulate vLLM's pooler having already applied the Matryoshka reduction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Overview:
The dynamo text-embedding worker returns the wrong output on vLLM 0.21: it emits the full per-token hidden-state matrix (
n_tokens × hidden, un-normalized) instead of one pooled, L2-normalized embedding per input. This one-line fix sets the pooling task to"embed", restoring correct OpenAI/v1/embeddingsoutput.Details:
EmbeddingWorkerHandler.generatebuildspooling_params = PoolingParams()with notask. On vLLM 0.21,engine.encode()withtask=Noneresolves to per-token output, so the worker serializes the entire hidden-state sequence. The/v1/embeddingsresponse dimension then scales with input length instead of being the model's fixed embedding dimension.Qwen/Qwen3-Embedding-0.6B(hidden_size=1024),input=["hi"]returns dim 2048,"hello world"→ 3072, an 80-token prompt → ~81,920 — i.e.n_tokens × 1024— and the vectors are un-normalized.vllm serveand the model's sentence-transformers config (pooling_mode_lasttoken,2_Normalize).PoolingParams(task="embed")selects the pooled + normalized embedding, matching vLLM's own embedding server.dimensionsparameter intoPoolingParams(task="embed", dimensions=...)rather than truncating the pooled vector in the handler. vLLM's pooler then performs the Matryoshka reduction correctly (truncate then L2-renormalize) and validates model support, matching barevllm serve; the previous post-hoc slice did not re-normalize (returned ‖v‖ < 1). Models whose HF config doesn't declare Matryoshka (e.g.Qwen3-Embedding) are launched with--hf-overrides '{"is_matryoshka": true}'inagg_embed.sh.Verification (GB200,
Qwen/Qwen3-Embedding-0.6B):"hi""hello world"After the fix the response is a fixed 1024-dim vector for any input length, with ‖v‖ = 1.0.
Where should the reviewer start?
components/src/dynamo/vllm/handlers.py—EmbeddingWorkerHandler.generate, thePoolingParams(task="embed")line.Related:
vLLM version (updated):
mainnow pins vLLM 0.23.0 (#10723) and this branch is rebased onto it. The pooling/MRL internals the fix relies on —PoolingParams(task="embed", dimensions=...), theis_matryoshkagate inPoolingParams.verify(), and the pooler's truncate-then-normalize order inEmbeddingPoolerHead— are unchanged between 0.22.0 and 0.23.0, and the fix is functionally verified on 0.23.0 in CI (theembedding_aggserve test, including thedimensions:128case). The 0.21 / 0.22 tables further down are retained as historical; the 0.23.0 latency refresh is in the section immediately below.vLLM 0.23.0 latency (refreshed):
Rebuilt a dynamo image on the 0.23.0 base (arm64) and re-ran the sweep on a single GB200. Same node/GPU, back-to-back, identical engine args (
--dtype bfloat16 --max-model-len 3096 --runner pooling --pooler-config '{"pooling_type":"MEAN","use_activation":false}' --no-enable-prefix-caching) and identicalaiperfparams (Qwen/Qwen3-Embedding-0.6B, dim 1024, ISL 80,--request-count 200, warmup 15). Small batches (15/64) rate-limited (--request-rate 10); large (128–1024) closed-loop--concurrency 1.dyn= dynamo embedding worker (pooling-fixed,task="embed"), baseline serialization.p50 latency (ms):
p90 latency (ms):
p99 latency (ms):
Takeaways (0.23.0): the pooling-fixed dynamo worker is at/below bare
vllm serveat p50 and p90 for every batch (e.g. batch-1024 p50 781 vs 1075 ms) — no median-latency regression on the shipped version, output dim = 1024. bare vLLM keeps a much tighter tail: dynamo's p99 grows at batch ≥64 (the frontend↔worker IPC hop + queueing), consistent with the 0.21/0.22 finding. The dyn↔vLLM comparison is apples-to-apples within this same-node 0.23.0 run; absolute values are not directly comparable to the historical 0.21/0.22 tables below (different harness/node).Verified on vLLM 0.22.0 + 0.21 vs 0.22 sweep (historical):
Built a dynamo image on the 0.22.0 base and re-ran the verification + the full uniform-count sweep:
pyproject.tomlpins): unpatched worker returns per-token output —"hi"→ dim 2048,"hello world"→ 3072, 100-word → ~104,448 (= n_tokens × 1024), un-pooled. Withtask="embed": a fixed 1024-dim for any input length, ‖v‖=1.0. So this is a live correctness fix on the shipped version, not just hardening.Same node (single GB200),
Qwen/Qwen3-Embedding-0.6B, dim 1024, ISL 80,--request-count 200(uniform). Small batches (15/64) rate-limited; large (128–1024) closed-loop--concurrency 1.dyn= dynamo embedding worker, pooling-fixed (task="embed"), baseline serialization.p50 latency (ms):
p90 latency (ms):
p99 latency (ms):
Takeaways: 0.21 ≈ 0.22 for both bare vLLM and the (pooling-fixed) dynamo worker. At p50 dynamo is within ~10–25% of bare
vllm serve; bare vLLM keeps a much tighter tail (p99@1024 ≈ 0.52 s vs dynamo ≈ 1.4 s — the frontend↔worker IPC hop). The pooling fix is the only change that matters here; serialization optimizations (the now-closed #10231 / #10220) stay within noise at correct embedding size.🤖 Generated with Claude Code