Repository navigation
[Pooling] Cap max-length padding for chunked embeddings - #56505
yewentao256 merged 5 commits into
Conversation
Signed-off-by: Taneem Ibrahim <taneem.ibrahim@gmail.com>
yewentao256
left a comment
There was a problem hiding this comment.
We now firstly padding, then chunk. So 513 -> 512 + 1.
Shall we chunk first then padding? So 513 -> 512 + 512(1 padding to 512)
Correct - the current code does turn 513 tokens into chunks of 512 and 1. However, padding after chunking would turn the last 1-token chunk into 512 tokens. That means processing 1,024 tokens instead of 513, and the mostly empty second chunk could have too much influence on the final embedding. For this narrow fix, I think keeping the current order is safer. The current PR behavior of 513 tokens becoming 512 + 1 preserves the original long-input embedding. Padding each chunk would need a separate and broader change that keeps track of how many real tokens each chunk contained. For example, it would require coordinated changes to padding, real-token tracking, aggregation weights, usage reporting, and accuracy validation. |
yewentao256
left a comment
There was a problem hiding this comment.
I am not sure the 1 token request would generate correct output, could you test on this?
Using I tested both the one-token input and the 513-token boundary that produces a 512+1 split on MRV2 with an H100. The one-token padded output matched an explicitly constructed token-plus-511-pads reference (max_abs=4.66e-10, cosine 1.0). The 513-token result matched both do_not_pad (max_abs=9.31e-10) and a manual weighted aggregation of independent 512-token and 1-token chunks (max_abs=7.21e-09, cosine 1.0). Mixed batching also matched the individual results. Therefore, the one-token tail is handled correctly and I don’t think an additional code change is needed. |
|
The current vLLM behavior is:
This PR change only caps the padding target from
The cap is min(max_model_len, max_embed_len), preserving a smaller configured embedding limit. However, chunking first and padding each chunk would introduce new semantics rather than preserve existing behavior. I think that might be a risky one to do. |
yewentao256
left a comment
There was a problem hiding this comment.
LGTM, thanks for the work!
|
✅ @taneem-ibrahim, CI is now available for this PR.
|
|
/ci run |
|
✅ Triggered Buildkite CI #89170 for commit |
Purpose
Fix
padding="max_length"with chunked embeddings. Padding incorrectly used the aggregatemax_embed_len, creating artificial pad-only chunks. The fix caps padding at the smaller ofmax_model_lenandmax_embed_lenwhile preserving the original validation limit.Reproducer
Start vLLM:
VLLM_USE_V2_MODEL_RUNNER=1 \ vllm serve intfloat/multilingual-e5-small \ --runner pooling \ --dtype bfloat16 \ --enforce-eager \ --max-model-len 512 \ --pooler-config \ '{"pooling_type":"MEAN","use_activation":true,"enable_chunked_processing":true,"max_embed_len":10000}' \ --gpu-memory-utilization 0.8In another terminal run:
Output
Main
Branch
Test Plan and Results
AI assistance disclosure
OpenAI Codex (GPT-5) assisted with the implementation.