[Frontend][VLM] Add support for multiple multi-modal items in the OpenAI frontend #7923
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge). To run full CI, you can do one of these:
🚀 |
|
Can you update the test pipeline so that |
There was a problem hiding this comment.
Overall this looks good to me as an initial step. What do you think @ywang96 ?
As we add more models, we will eventually have to address how to let the user customize https://github.com/vllm-project/vllm/pull/7923/files#diff-31e6bd0df09a47b5587701203d558701ac46e4f85bf7db83632da9990eaef198R120-R145
Will review this PR tonight! |
There was a problem hiding this comment.
Thanks for working on this @petersalas!
Overall this looks good to me and I've tested it with Phi-3.5-vision-instruct in my dev environment, but please take a look at my comment regarding the case when the user specifies placeholder themselves in the prompt. Thanks!
| "--dtype", "bfloat16", "--max-model-len", "4096", "--enforce-eager", | ||
| "--trust-remote-code", "--limit-mm-per-prompt", | ||
| f"image={MAXIMUM_IMAGES}" |
There was a problem hiding this comment.
Can you add "--max-num-seqs", "5" to this? I'm a bit worried this might not launch on the CI machines we have.
| conversation, mm_futures = parse_chat_messages( | ||
| conversation, mm_future = parse_chat_messages( | ||
| request.messages, model_config, tokenizer) | ||
|
|
||
| if mm_futures: | ||
| if mm_future: |
There was a problem hiding this comment.
Let's name it mm_data_future to be consistent with the change in serving_chat.py
| if placeholder_token_str is not None: | ||
| if placeholder_token_str in text_prompt: | ||
| logger.warning( | ||
| "Detected multi-modal token string in the text prompt. " | ||
| "Skipping prompt formatting.") |
There was a problem hiding this comment.
IMO we should keep this logic. This is really useful in case the default placeholder padding logic (i.e, to the front of the prompt) doesn't work well with the model, which happens to some models in practice actually (chameleon, deepseek-vl, etc).
We could simply check if the first placeholder exists in the user prompt and assume the user intends to add the placeholder themselves if that's the case, and verify prompt contains exactly what's in mm_placeholders as substrings. (perhaps we could use dictionary instead of list for mm_placeholders?)
Let me know if you have any strong opinion about this!
There was a problem hiding this comment.
The (unfortunately convoluted) prompt_fragments loop above is how I tried to generalize this to multiple placeholders. The intent is that any placeholders that aren't present in the text get prepended before the text prompt of that message such that the behavior is the same as the current behavior with n=1, but complicated by a) not wanting to double count one placeholder for two mm items and b) possibly having different placeholders for each mm item. (See the unit tests for some contrived examples.)
Does that make sense? I'm happy to do something different/simpler though.
There was a problem hiding this comment.
That makes sense, but I think we should be able to do something simpler. I also realized we should apply this formatting after we apply chat template to the whole conversation instead of a part of the conversation since the multimodal data item limit is for the final prompt we're sending to the model. We should add a test for such cases like one below
conversation = [{
"role": "user",
"content": [{
"type": "image_url",
"image_url": {"url": image_url}
}, {
"type": "image_url",
"image_url": {"url": image_url}
} {
"type": "text",
"text":
"What's in <|image_1|> and how does it compare to the other one?"
}]
},
{
"role": "assistant",
"content": "Some stuff."
},
{
"role": "user",
"content": [{
"type": "image_url",
"image_url": {"url": image_url}
}, {
"type": "image_url",
"image_url": {"url": image_url}
}, {
"type": "text",
"text": "How about these two images compared to the ones above?"
}]}
]
Here are the pseudocode I'm thinking
mm_placeholders: Dict[str, int] # key: placeholder_str, value: count
text_prompt: str # text_prompt = "\n".join(texts) for each message in a conversation
# Check if the prompt has any of placeholders
missing_placeholders = []
for key in mm_placeholders:
# If placeholders exist in prompt, leave them as is but deduct the count
mm_placeholders[key] -= text_prompt.count(key)
# Raise error if more placeholders are present in the prompt
if mm_placeholders[key] < 0:
raise
# Add the rest to missing placeholders list
missing_placeholders += mm_placeholders[key] * [key]
final_prompt = "\n".join(missing_placeholders + [text_prompt]) # final prompt for "text" field in each messageHow does this look?
There was a problem hiding this comment.
The one thing I wasn't sure about was a natural way to track the position within the former messages list once it's been stringified from the template -- unless I'm misunderstanding what you're proposing I think that would end up putting the images before the first message (e.g. system prompt) rather than at the start of the message in which they appeared, right?
(In any event, using count() is certainly simpler than what I wrote above, so whatever we decide I'll rewrite it to use that.)
There was a problem hiding this comment.
Ah yes you're right, and it looks like we're already passing mm_tracker across messages in a conversation so the count shouldn't be an issue! (I thought it was initialized for each message, sorry for not reading your code too carefully 😅 )
| prompt_fragments = [text_prompt] | ||
| missing_placeholders: List[str] = [] | ||
| for placeholder in placeholders: | ||
| for fragment_index, fragment in enumerate(prompt_fragments): | ||
| index = fragment.find(placeholder) | ||
| if index >= 0: | ||
| # This part of the text prompt already has a placeholder. | ||
| # Remove it from the text prompt fragments so that we don't | ||
| # consider it for later placeholders. | ||
| prompt_fragments.pop(fragment_index) | ||
| before = fragment[:index] | ||
| after = fragment[index + len(placeholder):] | ||
| if before: | ||
| prompt_fragments.insert(fragment_index, before) | ||
| if after: | ||
| prompt_fragments.insert(fragment_index + 1, after) | ||
| break |
There was a problem hiding this comment.
I'm a little confused about what we're doing here. Isn't text_prompt already a string (and thus prompt_fragments is a list of one string)?
Did you mean to iterate over texts instead of text_prompt = "\n".join(texts)?
|
Hey @petersalas! To speed things up, I guess we can actually keep your code for the parsing logic as is for now since it's functionally working, and I will make a PR later on to refactor that logic. Could you address my other comments so we can get this in? Thanks! |
|
Sorry for the delay, have been traveling. I might be able to push a version with the changes this afternoon, but I won't be offended if you want to pull/change my commits in a different PR 😀 |
This adds support for handling multiple image_urls (or audio_urls) in the OpenAI frontend.
Multi-modal item collection is consolidated in
MultiModalItemTrackerwhich:This also changes
tests/entrypoints/openai/test_vision.pyto use Phi3V to exercise the support for multiple image inputs end-to-end and adds unit tests forparse_chat_messages.PR Checklist (Click to Expand)
Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.
PR Title and Classification
Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:
[Bugfix]for bug fixes.[CI/Build]for build or continuous integration improvements.[Doc]for documentation fixes and improvements.[Model]for adding a new model or improving an existing model. Model name should appear in the title.[Frontend]For changes on the vLLM frontend (e.g., OpenAI API server,LLMclass, etc.)[Kernel]for changes affecting CUDA kernels or other compute kernels.[Core]for changes in the core vLLM logic (e.g.,LLMEngine,AsyncLLMEngine,Scheduler, etc.)[Hardware][Vendor]for hardware-specific changes. Vendor name should appear in the prefix (e.g.,[Hardware][AMD]).[Misc]for PRs that do not fit the above categories. Please use this sparingly.Note: If the PR spans more than one category, please include all relevant prefixes.
Code Quality
The PR need to meet the following code quality standards:
format.shto format your code.docs/source/if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.Notes for Large Changes
Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with
rfc-requiredand might not go through the PR.What to Expect for the Reviews
The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:
action-requiredlabel on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.Thank You
Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!