Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[2/N] Proper handling of placeholders in merged multi-modal processor #10485

Merged
merged 31 commits into from
Nov 23, 2024
Merged

[2/N] Proper handling of placeholders in merged multi-modal processor #10485

merged 31 commits into from
Nov 23, 2024

Conversation

DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Nov 20, 2024 •
edited by github-actions bot
Loading

Part of #10114

This PR updates #10044 to handle transformers v4.47 processors (huggingface/transformers#30962), which directly contain placeholder tokens in the processed input_ids.

I have also updated our code to properly replace targets with placeholders inside the processed prompt, and added unit tests for it.

@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 20, 2024
@github-actions GitHub Actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@DarkLight1337
Disable yapf
a7e831d 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 changed the title [Bugfix] Handle transformers v4.47 processors [Bugfix] Handle transformers v4.47 in merged multi-modal processors Nov 20, 2024
@DarkLight1337
Add comment
5fcb3d3 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Test invariants
47d3dcc 
Signed-off-by: DarkLight1337 <[email protected]>
@ywang96 ywang96 self-assigned this Nov 20, 2024
@DarkLight1337
Optimize
ad1675e 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Optimize 2
4b23817 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 changed the title [Bugfix] Handle transformers v4.47 in merged multi-modal processors [Bugfix] Handle transformers v4.47 and fix placeholder replacement in merged multi-modal processors Nov 20, 2024
@DarkLight1337 DarkLight1337 changed the title [Bugfix] Handle transformers v4.47 and fix placeholder replacement in merged multi-modal processors [Bugfix] Handle transformers v4.47 and fix placeholder search in merged multi-modal processors Nov 20, 2024
@DarkLight1337 DarkLight1337 changed the title [Bugfix] Handle transformers v4.47 and fix placeholder search in merged multi-modal processors [Bugfix] Handle transformers v4.47 and fix placeholder matching in merged multi-modal processors Nov 20, 2024
@DarkLight1337
Rename
a4b4108 
Signed-off-by: DarkLight1337 <[email protected]>
ywang96
ywang96 reviewed Nov 21, 2024
View reviewed changes
Copy link
Member

@ywang96 ywang96 left a comment

Choose a reason for hiding this comment

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

Left two comments - PTAL!

vllm/multimodal/processing.py Outdated Show resolved Hide resolved
vllm/multimodal/processing.py Outdated Show resolved Hide resolved
@DarkLight1337
Add test
43043ca 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 marked this pull request as draft November 21, 2024 07:48
@DarkLight1337
Iterate
dc619cc 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 changed the title [Bugfix] Handle transformers v4.47 and fix placeholder matching in merged multi-modal processors [Bugfix] Handle transformers v4.47 and fix placeholder replacements in merged multi-modal processors Nov 21, 2024
@DarkLight1337
Reorganize
44f1566 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Setup tests
3baa22a 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Fix extra matches
98590cc 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 changed the title [2/N] Handle transformers v4.47 and fix placeholder replacements in merged multi-modal processors [2/N] Handle transformers v4.47 and fix placeholder replacements in merged multi-modal processor Nov 22, 2024
@mergify mergify bot added the documentation Improvements or additions to documentation label Nov 22, 2024
@DarkLight1337
Fix docs
4b7d00a 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Avoid repeated decode
c8eb070 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Fix docs
6ab14df 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 changed the title [2/N] Handle transformers v4.47 and fix placeholder replacements in merged multi-modal processor [2/N] Proper implmentation of placeholder replacements in merged multi-modal processor Nov 22, 2024
@DarkLight1337 DarkLight1337 changed the title [2/N] Proper implmentation of placeholder replacements in merged multi-modal processor [2/N] Proper implementation of placeholder replacements in merged multi-modal processor Nov 22, 2024
@DarkLight1337 DarkLight1337 changed the title [2/N] Proper implementation of placeholder replacements in merged multi-modal processor [2/N] Proper handling of placeholders in merged multi-modal processor Nov 22, 2024
@DarkLight1337
Cleanup
a29daa4 
Signed-off-by: DarkLight1337 <[email protected]>
ywang96
ywang96 approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@ywang96 ywang96 left a comment

Choose a reason for hiding this comment

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

Overall LGTM and I left a few final comments!

vllm/multimodal/processing.py Outdated Show resolved Hide resolved
vllm/multimodal/processing.py Show resolved Hide resolved
vllm/multimodal/processing.py Outdated Show resolved Hide resolved
@DarkLight1337
Update
d8d6764 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) November 23, 2024 03:02
@DarkLight1337
Update
f269bee 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Update
bbdd0e2 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Add sanity check
b1429e6 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Comment
5a05879 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Fix
acc183e 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Merge branch 'main' into fix-mm-processor
3ec2157
@DarkLight1337
Inline
fea6854 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Update
7eb0311 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Update
4c8dd61 
Signed-off-by: DarkLight1337 <[email protected]>
@youkaichao youkaichao disabled auto-merge November 23, 2024 05:24
@youkaichao youkaichao merged commit c8acd80 into vllm-project:main Nov 23, 2024
44 of 47 checks passed
@DarkLight1337 DarkLight1337 deleted the fix-mm-processor branch November 23, 2024 05:28
mfournioux pushed a commit to mfournioux/vllm that referenced this pull request Nov 28, 2024
@DarkLight1337 @mfournioux
[2/N] handling placeholders in merged multi-modal processor (vllm-pro…
a6f54da 
…ject#10485)

Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: Maxime Fournioux <[email protected]>
sleepwalker2017 pushed a commit to sleepwalker2017/vllm that referenced this pull request Dec 13, 2024
@DarkLight1337
[2/N] handling placeholders in merged multi-modal processor (vllm-pro…
27eec60 
…ject#10485)

Signed-off-by: DarkLight1337 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Isotr0py Isotr0py Isotr0py left review comments

@ywang96 ywang96 ywang96 approved these changes

Assignees

@ywang96 ywang96

Labels
documentation Improvements or additions to documentation ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DarkLight1337 @Isotr0py @ywang96 @youkaichao