-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Bugfix] Merge MM embeddings by index instead of token IDs #16229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bugfix] Merge MM embeddings by index instead of token IDs #16229
Conversation
…ken ID Signed-off-by: DarkLight1337 <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: Roger Wang <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the tpu issues, thanks @DarkLight1337 !
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: DarkLight1337 <[email protected]>
We will not include this in the v0.11 release because of the breaking change. But it should be fine to merge this into main branch since the release branch has been cut already. |
Signed-off-by: DarkLight1337 <[email protected]>
upstream PR: vllm-project/vllm#16229 Fix is still in progress, don't merge yet --------- Signed-off-by: Agata Dobrzyniewicz <[email protected]> Signed-off-by: Chendi Xue <[email protected]> Co-authored-by: Chendi Xue <[email protected]>
upstream PR: vllm-project/vllm#16229 Fix is still in progress, don't merge yet --------- Signed-off-by: Agata Dobrzyniewicz <[email protected]> Signed-off-by: Chendi Xue <[email protected]> Co-authored-by: Chendi Xue <[email protected]> Signed-off-by: Iryna Boiko <[email protected]>
…ect#16229) Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: NickLucche <[email protected]> Signed-off-by: Roger Wang <[email protected]> Co-authored-by: NickLucche <[email protected]> Co-authored-by: Roger Wang <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: NickLucche <[email protected]> Signed-off-by: Roger Wang <[email protected]> Co-authored-by: NickLucche <[email protected]> Co-authored-by: Roger Wang <[email protected]> Signed-off-by: yewentao256 <[email protected]>
…ect#16229) Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: NickLucche <[email protected]> Signed-off-by: Roger Wang <[email protected]> Co-authored-by: NickLucche <[email protected]> Co-authored-by: Roger Wang <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
This PR fixes a mismatch in merging multi-modal embeddings when the model itself generates embedding placeholder tokens such as
<image>
. Although this error mainly occurs in V1, it can possibly occur in V0 as well. This PR focuses on the V1 case.For V0 users, you can work around this by setting
top_p
so that the model has no chance of generating such tokens.FIX #15677
FIX #15764
FIX #23891
FIX #23954
FIX #24456
Breaking change for model developers
This PR has updated
SupportsMultiModal.get_input_embeddings
to support passingis_multimodal
mask and added a default implementation so that there is no need to override it in most cases. OOT/WIP models should either remove their override to use the default implementation, or update their override to acceptis_multimodal
anddo_language_embed_multimodal
arguments.Text-only model developers should ensure that their models have implemented
get_input_embeddings
to continue using them in vLLM.Breaking change for model runner plugins
In order to continue supporting multimodal models, you should update
_gather_mm_embeddings
method to build up and return theis_mm_embed
mask, then pass it to the model.