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

[Misc] Clean up MiniCPM-V/O code #15337

Merged
merged 8 commits into from
Mar 25, 2025
Merged

[Misc] Clean up MiniCPM-V/O code #15337

merged 8 commits into from
Mar 25, 2025
+531 −661

Conversation

DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Mar 22, 2025 •
edited by github-actions bot
Loading

Clean up the code before trying to support the model on V1

cc @HwwwwwwwH

@DarkLight1337 DarkLight1337 requested a review from Isotr0py March 22, 2025 16:44
@DarkLight1337 DarkLight1337 requested a review from ywang96 as a code owner March 22, 2025 16:44
@github-actions GitHub Actions
Copy link

👋 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 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 either: Add ready label to the PR or enable auto-merge.

🚀

@mergify mergify bot added the multi-modality Related to multi-modality (#4194) label Mar 22, 2025
DarkLight1337
DarkLight1337 commented Mar 22, 2025
View reviewed changes
tests/models/multimodal/processing/test_common.py
if msg is None:
assert a == b
else:
assert a == b, msg
Copy link
Member Author

@DarkLight1337 DarkLight1337 Mar 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

These changes are to let pytest show the non-matching items in more detail

DarkLight1337
DarkLight1337 commented Mar 22, 2025
View reviewed changes
vllm/model_executor/models/gemma3_mm.py
Comment on lines -298 to -299
assert isinstance(images, list)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unnecessary check since the data is parsed in the next line

DarkLight1337
DarkLight1337 commented Mar 22, 2025
View reviewed changes
tests/models/multimodal/processing/test_common.py
@@ -261,6 +264,7 @@ def _test_processing_correctness_mistral(
"TIGER-Lab/Mantis-8B-siglip-llama3",
"mistralai/Pixtral-12B-2409",
"mistral-community/pixtral-12b",
"openbmb/MiniCPM-Llama3-V-2_5",
Copy link
Member Author

@DarkLight1337 DarkLight1337 Mar 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Need this to test the else branch in _base_call_hf_processor

@DarkLight1337 DarkLight1337 marked this pull request as draft March 22, 2025 16:50
@DarkLight1337
Copy link
Member Author

Marking as draft until I can get the relevant tests to pass locally

@DarkLight1337 DarkLight1337 marked this pull request as ready for review March 24, 2025 04:00
@DarkLight1337
Copy link
Member Author

Ready to review now.

@Isotr0py
Copy link
Collaborator

Seems MiniCPM-V test is OOM on CI now... (https://buildkite.com/vllm/fastcheck/builds/17957#0195c650-42fc-4448-9309-7609ca41b3ed/205-3895)

@DarkLight1337
Copy link
Member Author

This simplified approach is not nearly as memory efficient, let's see how to optimize this while still keeping the code readable...

Old implementation:

INFO 03-24 08:10:12 [worker.py:267] model weights take 15.19GiB; non_torch_memory takes 0.06GiB; PyTorch activation peak memory takes 3.99GiB; the rest of the memory reserved for KV Cache is 0.55GiB.

New implementation:

INFO 03-24 07:52:04 [worker.py:270] model weights take 15.19GiB; non_torch_memory takes 0.07GiB; PyTorch activation peak memory takes 4.95GiB; the rest of the memory reserved for KV Cache is -0.42GiB.

@DarkLight1337
Copy link
Member Author

DarkLight1337 commented Mar 24, 2025 •
edited
Loading

Hmm this is odd, the memory usage remains exactly the same before get_embedding_with_vision is called in the model. I checked using torch.cuda.memory_allocated(), torch.cuda.memory_reserved() and torch.cuda.max_memory_reserved().

Old implementation:

# In GiB
allocated0 15.366820335388184
reserved0 15.44140625
max0 15.44140625
allocated1 15.500089168548584
reserved1 20.603515625
max1 20.603515625

New implementation:

# In GiB
allocated0 15.366820335388184
reserved0 15.44140625
max0 15.44140625
allocated1 15.508512020111084
reserved1 20.537109375
max1 21.255859375

@DarkLight1337
Fix embeds
7f0307a 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Update
1c45aa8 
Signed-off-by: DarkLight1337 <[email protected]>
DarkLight1337
DarkLight1337 commented Mar 24, 2025
View reviewed changes
vllm/model_executor/models/minicpmv.py
Comment on lines -1024 to -1031
num_slices = mm_data[modality][f"{modality}_num_slices"][b][
pos]
slice_start_idx = mm_slice_counts[modality]
slice_end_idx = slice_start_idx + num_slices
pixel_values_flat += mm_data[modality]["pixel_values"][b][
slice_start_idx:slice_end_idx]
tgt_sizes_flat += mm_data[modality]["tgt_sizes"][b][
slice_start_idx:slice_end_idx]
Copy link
Member Author

@DarkLight1337 DarkLight1337 Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Upon further inspection, the previous model implementation actually fails to extract all of the slices. (The last slice_end_idx does not match the length of mm_data[modality]["pixel_values"][b]). So the new implementation is more correct but also uses more memory.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll just disable the multi-image tests in test CI

@DarkLight1337
Clean
dd999b3 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 24, 2025
@DarkLight1337
Update tests
289619c 
Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Fix OOM
4789620 
Signed-off-by: DarkLight1337 <[email protected]>
@mergify mergify bot added the documentation Improvements or additions to documentation label Mar 25, 2025
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) March 25, 2025 09:32
@DarkLight1337 DarkLight1337 merged commit a9e879b into vllm-project:main Mar 25, 2025
39 checks passed
@DarkLight1337 DarkLight1337 deleted the minicpm-cleanup branch March 25, 2025 10:26
erictang000 pushed a commit to erictang000/vllm that referenced this pull request Mar 25, 2025
@DarkLight1337 @erictang000
[Misc] Clean up MiniCPM-V/O code (vllm-project#15337)
7505619 
Signed-off-by: DarkLight1337 <[email protected]>
wrmedford pushed a commit to wrmedford/vllm that referenced this pull request Mar 26, 2025
@DarkLight1337 @wrmedford
[Misc] Clean up MiniCPM-V/O code (vllm-project#15337)
ebedbda 
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: Wes Medford <[email protected]>
lengrongfu pushed a commit to lengrongfu/vllm that referenced this pull request Apr 2, 2025
@DarkLight1337
[Misc] Clean up MiniCPM-V/O code (vllm-project#15337)
70ca8ab 
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 approved these changes

@ywang96 ywang96 Awaiting requested review from ywang96 ywang96 is a code owner

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation multi-modality Related to multi-modality (#4194) 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.
2 participants
@DarkLight1337 @Isotr0py