Skip to content

Conversation

@DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Dec 11, 2025

Purpose

  • Handle multiple dictionary inputs in MultiModalItemTracker.all_mm_data and its async version.
  • Update InputProcessor._validate_single_prompt to handle dictionary inputs correctly.

FIX #29778

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 adds support for multiple dictionary-based input embeddings, which is a great enhancement. The core logic changes in chat_utils.py and input_processor.py seem to correctly handle the main use case of multiple dictionary embeddings. The new tests for this functionality are also valuable.

However, I've identified two critical issues. First, the refactoring seems to have broken the existing functionality for handling cached embeddings (where image_embeds is null but a uuid is provided), as the new _get_embeds_data function does not handle None values. Second, the new test for empty dictionary embeddings ({}) is internally inconsistent and reveals a bug that will cause a validation failure. An empty dictionary is treated as one item, creating a placeholder, but its data length is considered zero, leading to a mismatch with the UUID count. Please see my detailed comments for suggestions on how to address these issues.

@DarkLight1337 DarkLight1337 marked this pull request as ready for review December 11, 2025 19:01
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 11, 2025
@mergify
Copy link

mergify bot commented Dec 11, 2025

Hi @DarkLight1337, the pre-commit checks have failed. Please run:

uv pip install pre-commit
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

Tip

Is mypy or markdownlint failing?
mypy and markdownlint are run differently in CI. If the failure is related to either of these checks, please use the following commands to run them locally:
# For mypy (substitute "3.10" with the failing version if needed)
pre-commit run --hook-stage manual mypy-3.10
# For markdownlint
pre-commit run --hook-stage manual markdownlint

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@DarkLight1337 DarkLight1337 changed the title [Frontend] Support multiple dict input embeddings for online chat [Bugfix] Dictionary input embeddings for online chat Dec 11, 2025
@DarkLight1337 DarkLight1337 changed the title [Bugfix] Dictionary input embeddings for online chat [Bugfix] Dictionary MM embeddings for online chat Dec 11, 2025
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Copy link
Member Author

/gemini review

Signed-off-by: DarkLight1337 <[email protected]>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 introduces a bugfix to correctly handle dictionary-based multimodal embeddings, particularly for online chat scenarios. The changes involve updating MultiModalItemTracker and InputProcessor to manage multiple dictionary inputs. New tests have been added to cover these cases. My review focuses on the robustness of the new logic. I've identified a potential KeyError in _get_embeds_data if input dictionaries have inconsistent keys and have suggested a validation to prevent this.

Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 removed the ready ONLY add when PR is ready to merge/full CI is needed label Dec 11, 2025
@DarkLight1337
Copy link
Member Author

Test currently fails, will resume work tomorrow

@mergify
Copy link

mergify bot commented Dec 11, 2025

Hi @DarkLight1337, the pre-commit checks have failed. Please run:

uv pip install pre-commit
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

Tip

Is mypy or markdownlint failing?
mypy and markdownlint are run differently in CI. If the failure is related to either of these checks, please use the following commands to run them locally:
# For mypy (substitute "3.10" with the failing version if needed)
pre-commit run --hook-stage manual mypy-3.10
# For markdownlint
pre-commit run --hook-stage manual markdownlint

Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Copy link
Member Author

Tests should pass now

Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 12, 2025
@DarkLight1337 DarkLight1337 merged commit b09806e into vllm-project:main Dec 13, 2025
49 checks passed
@DarkLight1337 DarkLight1337 deleted the multi-dict-embed branch December 13, 2025 07:48
Lucaskabela pushed a commit to Lucaskabela/vllm that referenced this pull request Dec 15, 2025
joa-stdn pushed a commit to joa-stdn/vllm that referenced this pull request Dec 15, 2025
weiyu0824 pushed a commit to weiyu0824/vllm that referenced this pull request Dec 16, 2025
quanliu1991 pushed a commit to quanliu1991/vllm that referenced this pull request Dec 22, 2025
Majid-Taheri pushed a commit to Majid-Taheri/vllm that referenced this pull request Dec 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Using image_embeds to input multimodal image embedding results causes array out-of-bounds during processing.

2 participants