-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[Bugfix] Fix broken v0 multimodal inference #19814
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
Conversation
Signed-off-by: Isotr0py <2037008807@qq.com>
👋 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 🚀 |
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.
Summary of Changes
Hello @Isotr0py, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request provides a critical bug fix for the v0 multimodal inference pipeline, specifically addressing an issue where text-only inputs would cause a runtime error due to incorrect handling of multimodal embeddings. The core change refines conditional logic across various model implementations to ensure robust behavior when multimodal data is not present, thereby improving the stability and correctness of multimodal inference.
Highlights
- Bug Fix: Multimodal Inference: This pull request resolves a
RuntimeError: torch.cat(): expected a non-empty list of Tensors
that occurred in v0 multimodal inference when processing text-only inputs. The issue stemmed from an incorrect conditional check that attempted to merge non-existent multimodal embeddings. - Conditional Logic Refinement: The fix involves changing the conditional check from
multimodal_embeddings is not None
tomultimodal_embeddings
(a boolean evaluation) across numerous multimodal model implementations. This ensures that the embedding merging logic is correctly skipped whenmultimodal_embeddings
is an empty tensor or similar falsy value, which is the expected behavior for text-only inputs. - Test Stability: The changes also address a failing sampler test on the main branch, contributing to overall system stability.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request addresses a bug in multimodal inference where an empty list of embeddings, when not None
, would still proceed to the merging step, causing errors. The fix involves changing the condition from multimodal_embeddings is not None
to the more Pythonic if multimodal_embeddings:
. This correctly handles cases where multimodal_embeddings
is None
or an empty list/tuple, preventing downstream errors like IndexError
or torch.cat()
issues.
The change is consistently applied across numerous model files and the documentation, which is appropriate for a fix to a common utility pattern. The solution is clear, concise, and directly addresses the described problem. This is a good improvement for robustness and correctness.
@@ -69,7 +69,7 @@ Further update the model as follows: | |||
# model as one of the requirements of basic vLLM model implementation. | |||
inputs_embeds = self.language_model.get_input_embeddings(input_ids) | |||
|
|||
if multimodal_embeddings is not None: | |||
if multimodal_embeddings: |
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.
The change from multimodal_embeddings is not None
to if multimodal_embeddings:
in this documentation example correctly reflects the corresponding code changes. This updated condition is more Pythonic and robustly handles cases where multimodal_embeddings
might be an empty list or tuple (which evaluates to False
), in addition to None
.
@@ -683,7 +683,7 @@ def get_input_embeddings( | |||
multimodal_embeddings: Optional[MultiModalEmbeddings] = None, | |||
) -> torch.Tensor: | |||
inputs_embeds = self.language_model.get_input_embeddings(input_ids) | |||
if multimodal_embeddings is not None: | |||
if multimodal_embeddings: |
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.
This change from if multimodal_embeddings is not None:
to if multimodal_embeddings:
is a good fix.
As per the PR description, the previous condition (is not None
) would evaluate to True
if multimodal_embeddings
was an empty list (e.g., []
). This would lead to merge_multimodal_embeddings
being called with an empty list for multimodal_embeddings
.
If multimodal_embeddings
is an empty list but input_ids
still contains placeholder tokens, this could lead to an IndexError
within merge_multimodal_embeddings
when trying to access multimodal_embeddings[0]
. The torch.cat()
error mentioned in the PR description might be another symptom depending on the exact path taken within merge_multimodal_embeddings
.
The new condition if multimodal_embeddings:
correctly evaluates to False
for both None
and empty lists/tuples, thus preventing merge_multimodal_embeddings
from being called inappropriately. This makes the code more robust and Pythonic.
Oh, didn't notice #19715 |
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.Purpose
multimodal_embeddings is not None
to skip merging embeddings for text-only, while it has been changed to empty list in v0 for text-only input, which caused error:Test Plan
Test Result
(Optional) Documentation Update