[Bugfix] Fix broken v0 multimodal inference#19814
[Bugfix] Fix broken v0 multimodal inference#19814Isotr0py wants to merge 1 commit intovllm-project:mainfrom
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.
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 Tensorsthat 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 Nonetomultimodal_embeddings(a boolean evaluation) across numerous multimodal model implementations. This ensures that the embedding merging logic is correctly skipped whenmultimodal_embeddingsis 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.
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.
| 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.
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.
| ) -> 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.
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.mdandexamplesfor a new model.Purpose
multimodal_embeddings is not Noneto 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