Repository navigation
[Bugfix] Align vLLM Omni Request input signature with vLLM. - #4568
Conversation
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
timzsu
changed the title
|
@lishunyang12 Hi Shunyang, can you please have a look at this bugfix PR? |
|
test it in 0.23.0 version |
I can reproduce the bug on the latest main of vLLM-Omni (63ba11b) and v0.23.0rc1. Should I test against vLLM 0.23.0? |
the current main of vllm-omni has been updated to align with vllm 0.23.0 now |
@hsliuustc0106 I have reproduced the same failure with vLLM 0.23.0 and the latest vLLM-Omni. The fix is still needed. |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
I found one compatibility concern around preserving the base Request constructor contract.
| ): | ||
| prompt_embeds_tensor = self._maybe_decode_prompt_embeds(prompt_embeds) | ||
| super().__init__(prompt_embeds=prompt_embeds_tensor, *args, **kwargs) | ||
| super().__init__(*args, prompt_embeds=prompt_embeds_tensor, **kwargs) |
There was a problem hiding this comment.
This fixes the 4-arg warmup path, but I think OmniRequest still is not fully substitutable for vLLM's Request constructor. In vLLM 0.23.0, prompt_embeds is still a positional-capable base parameter after arrival_time, so a base-style call like Request(req_id, tokens, sp, None, 0, None, embeds) would leave embeds in *args here and then also pass prompt_embeds=prompt_embeds_tensor, causing Request.__init__ to receive prompt_embeds twice. Since vllm_omni/patch.py rebinds loaded vLLM Request references to OmniRequest, can we either mirror the base signature explicitly or detect/pop a positional prompt_embeds before forwarding the keyword override? It would also be good for the regression test to cover the positional prompt-embeds case.
There was a problem hiding this comment.
I have absorbed prompt_embeds into kwargs to solve this issue. PTAL
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
|
Will this change be back ported to 0.23.0 release? |
…ject#4568) Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
3 participants
Purpose
This PR fixes #4565. Now with vLLM-Omni installed, we can still initiate LLM engines for models with model runner v2.
Test Plan
vLLM Version: 0.23.0
Run the following snippet that produces the error to ensure that the error disappears.
Run a regression test
tests/test_omni_request.pyto lock this behavior.Test Result
The snippet that failed before passes now. The new regression test also passes.
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)