Skip to content

Conversation

lgeiger
Copy link
Contributor

@lgeiger lgeiger commented Aug 29, 2025

Purpose

This changes the Idefics2VisionTransformer model to use in-place adds where possible. This is relevant since vision encoders currently run outside of torch compile so things like this won't be automatically optimised away. See also #18922.

Related to: #23884

Test Plan

vllm serve openbmb/MiniCPM-V-4_5 --trust-remote-code

Test Result

In an internal benchmark with many image inputs this has a surprisingly big performance impact. I'm seeing a 5.5% - 6.2% increase in throughput with the openbmb/MiniCPM-V-4_5 model running on a L40s GPU.

@DarkLight1337 let me know if you'd like me to do a bit of search and replace and apply similar changes in the other model definitions as well which might also benefit text only models when run in eager mode.

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 performance optimization in Idefics2VisionTransformer by replacing out-of-place additions with in-place additions. The changes in Idefics2VisionEmbeddings and Idefics2EncoderLayer are correct and safe, as they operate on locally-scoped tensors, avoiding side effects. This modification should lead to reduced memory allocation and improved throughput as described in the pull request, which is a valuable improvement for inference performance. The changes are well-implemented. As you suggested in the description, applying this optimization to other models in the codebase would likely be beneficial as well.

@DarkLight1337
Copy link
Member

Thanks for the optimization. Let's open a separate PR with a benchmark for each model

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) August 29, 2025 11:32
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 29, 2025
auto-merge was automatically disabled August 29, 2025 12:11

Head branch was pushed to by a user without write access

@lgeiger lgeiger force-pushed the inplace-adds-idefics branch from 4c086b6 to c727b73 Compare August 29, 2025 12:11
@lgeiger
Copy link
Contributor Author

lgeiger commented Aug 29, 2025

Rebased to re-trigger CI...

@vllm-bot vllm-bot merged commit 0a2f4c0 into vllm-project:main Aug 29, 2025
38 of 41 checks passed
@lgeiger lgeiger deleted the inplace-adds-idefics branch August 29, 2025 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants