Skip to content

Conversation

@Isotr0py
Copy link
Member

@Isotr0py Isotr0py commented Nov 17, 2025

Purpose

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.

@mergify mergify bot added the qwen Related to Qwen models label Nov 17, 2025
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 replaces nn.Conv2d with vLLM's Conv2dLayer across multiple model files. The refactoring is mostly correct, but there are several critical errors in vllm/model_executor/models/chameleon.py. In this file, torch.Conv2dLayer is used instead of Conv2dLayer, which will lead to an AttributeError as Conv2dLayer is not a member of the torch module. I've provided suggestions to fix these issues. Please apply the corrections to ensure the model works as expected.

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".

Comment on lines +290 to 293
self.patch_embedding = Conv2dLayer(
in_channels=config.num_channels,
out_channels=self.embed_dim,
kernel_size=self.patch_size,

Choose a reason for hiding this comment

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

P0 Badge Pass unsupported padding="valid" to Conv2dLayer in Siglip embeddings

Conv2dLayer stores the passed padding as-is and forwards it to torch.nn.functional.conv2d, which only accepts integers or tuples. The new code now hands it the string "valid", causing a runtime TypeError when embedding images. Compute the numeric padding or drop the argument to preserve the previous behaviour.

Useful? React with 👍 / 👎.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good bot, thanks! :)

Comment on lines +71 to 74
self.patch_embedding = Conv2dLayer(
in_channels=config.num_channels,
out_channels=self.embed_dim,
kernel_size=self.patch_size,

Choose a reason for hiding this comment

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

P0 Badge Siglip2 NaViT embedding uses Conv2dLayer with string padding

Similar to the Siglip change, Conv2dLayer does not translate string paddings. Passing padding="valid" will be forwarded directly to F.conv2d, triggering a TypeError when the model processes images. Replace the string with an explicit numeric padding (likely 0) before constructing the layer.

Useful? React with 👍 / 👎.

Comment on lines +64 to 67
self.patch_embedding = Conv2dLayer(
in_channels=config.num_channels,
out_channels=self.embed_dim,
kernel_size=self.patch_size,

Choose a reason for hiding this comment

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

P0 Badge Idefics2 vision embeddings now call Conv2dLayer with padding string

The new Conv2dLayer wrapper forwards padding directly to F.conv2d and does not implement the "valid" shortcut that nn.Conv2d provided. Using the string here will cause a runtime failure when forward runs. Replace with the correct numeric padding (0) or add conversion logic.

Useful? React with 👍 / 👎.

Comment on lines +208 to 211
self.patch_embedding = Conv2dLayer(
in_channels=config.num_channels,
out_channels=self.embed_dim,
kernel_size=self.patch_size,

Choose a reason for hiding this comment

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

P0 Badge Keye vision embeddings pass unsupported padding="valid"

Conv2dLayer’s constructor only handles integer/tuple padding values. Passing the string "valid" like nn.Conv2d previously allowed will lead to an exception in forward when the convolution executes. Replace the string with the equivalent numeric padding.

Useful? React with 👍 / 👎.

Copy link
Member

Choose a reason for hiding this comment

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

Can you update the type annotation to account for this?

Comment on lines +423 to 426
self.patch_embedding = Conv2dLayer(
in_channels=config.num_channels,
out_channels=self.embed_dim,
kernel_size=self.patch_size,

Choose a reason for hiding this comment

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

P0 Badge PaddleOCR vision embeddings forward padding string to Conv2dLayer

The padding argument is now the literal string "valid", but Conv2dLayer passes self.padding straight to F.conv2d, which expects integers or tuples. This will raise a TypeError when embeddings are computed. Compute the numeric padding instead of relying on the string shortcut.

Useful? React with 👍 / 👎.

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

/gemini review

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 systematically replaces all instances of torch.nn.Conv2d with vllm.model_executor.layers.conv.Conv2dLayer across various model implementations. This is a consistent and necessary refactoring to leverage vLLM's custom convolution layer, which likely offers optimizations or specific functionalities tailored for the framework. The changes are applied uniformly, including updating import statements and isinstance checks where appropriate. No functional issues or bugs were identified in this mechanical replacement.

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

/gemini review

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 systematically replaces all instances of torch.nn.Conv2d with vLLM's custom Conv2dLayer across various model implementations. This is a crucial step towards unifying convolution operations within the vLLM framework, likely enabling custom optimizations or distributed computing features. The changes in vllm/model_executor/layers/conv.py correctly extend the ConvLayerBase to handle string-based padding ("same", "valid") and include a necessary validation check for strided convolutions with "same" padding. This ensures correctness and prevents undefined behavior. The widespread adoption of Conv2dLayer across numerous models demonstrates a consistent application of this architectural change.

Comment on lines +65 to +66
if padding == "same" and any(s != 1 for s in stride):
raise ValueError("padding='same' is not supported for strided convolutions")
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The addition of this validation check is crucial for correctness. padding='same' behavior is not well-defined for strided convolutions in all frameworks, and explicitly disallowing it prevents potential silent miscalculations or unexpected output dimensions. This improves the robustness of the Conv2dLayer.

@Isotr0py Isotr0py enabled auto-merge (squash) November 17, 2025 15:22
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 17, 2025
@Isotr0py Isotr0py merged commit e4bb268 into vllm-project:main Nov 18, 2025
51 checks passed
Victor49152 pushed a commit to Victor49152/vllm that referenced this pull request Nov 20, 2025
@Isotr0py Isotr0py deleted the replace-conv branch November 20, 2025 07:10
bhagyashrigai pushed a commit to odh-on-pz/vllm-upstream that referenced this pull request Nov 20, 2025
Amir-19 pushed a commit to Amir-19/vllm that referenced this pull request Nov 21, 2025
bigPYJ1151 pushed a commit that referenced this pull request Nov 25, 2025
bringlein pushed a commit to bringlein/vllm that referenced this pull request Nov 26, 2025
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
kitaekatt pushed a commit to kitaekatt/vllm that referenced this pull request Dec 1, 2025
iboiko-habana pushed a commit to vllm-project/vllm-gaudi that referenced this pull request Dec 3, 2025
Due to the latest changes from upstream, gemma3 is failing to compile on
HPU
vllm-project/vllm#27772
vllm-project/vllm#28842

-replace unfold to view/reshape
-replace text embedding to avoid dynamic shape
-remove merge_multimodal replacement since masked_scatter issue is fixed
-enable back gemma3 model test

---------

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

Labels

qwen Related to Qwen models 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.

2 participants