Normalize single element convolution arg lists - #22778
giuliocorradi wants to merge 1 commit into
Conversation
ATen lets a convolution's stride, padding, dilation and output_padding be given as a single value that applies to every spatial dimension, and torch.nn does exactly that for `padding="valid"`, which exports as `padding=[0]` rather than `[0, 0]`. The Vulkan convolution reads these as fixed width vectors (make_ivec2_from_list -> make_ivec2, which requires exactly 2 elements), so a 2D convolution written that way aborted at the first inference: make_ivec2 ... (ints.size() == 2) is false! The same convolution written `padding=0` lowers and runs correctly, so only the spelling of the argument decided whether the model worked. Add a NormalizeConvolutionArgs pass that broadcasts a single element list to the number of spatial dimensions, and run it at the start of the preprocess pipeline, where the graph is still ATen compliant. A 1D convolution is left alone, since one element already matches its one spatial dim. This is worth doing in the graph rather than the runtime because the normalized graph is what every later pass and the serializer see, but broadcasting in make_ivec2_from_list instead would also be a reasonable fix. `padding="valid"` is how HuggingFace writes SigLIP and CLIP patch embeddings, so every vision tower of that shape was affected. Test Plan: python -m unittest backends.vulkan.test.test_vulkan_passes -v 12 tests pass, including the new test_normalize_convolution_args_broadcasts_single_element_lists, which checks that a 2D convolution's `[0]` padding becomes `[0, 0]` and that a 1D convolution's stays `[0]`. Verified end to end on an AMD Radeon 8060S (RADV GFX1151), lowering each module and comparing the delegate's output against eager: case before after Conv2d(padding=0) ok ok Conv2d(padding=1) ok ok Conv2d(padding=(1, 1)) ok ok Conv2d(padding="valid") make_ivec2 ok, max|d| 6.0e-07 SigLIP patch embed, k=14 s=14, valid make_ivec2 ok, max|d| 1.2e-06 Conv1d(padding=1) ok ok Conv1d(padding="valid") ok ok backends.vulkan.test.test_vulkan_delegate shows an identical set of results before and after the change.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22778
Note: Links to docs will display an error until the docs builds have been completed.
|
|
Hi @giuliocorradi! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
This PR needs a
|
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped to Vulkan preprocessing, addresses the reported crash path, and includes a targeted regression test for the primary failing case.
Pull request overview
This PR fixes a Vulkan runtime abort triggered by ATen-compliant convolutions whose stride/padding/dilation/output_padding are exported as single-element lists (notably nn.Conv2d(padding="valid") exporting padding=[0]), by normalizing those argument lists early in the Vulkan preprocess pipeline.
Changes:
- Add a Vulkan preprocessing pass (
NormalizeConvolutionArgs) that broadcasts single-element convolution argument lists to the convolution’s spatial rank (leaving 1D conv unchanged). - Run the new normalization pass at the start of the Vulkan preprocess pass pipeline.
- Add a unit test covering the
padding="valid"export case (2D broadcast) and ensuring 1D conv remains unmodified.
File summaries
| File | Description |
|---|---|
| backends/vulkan/vulkan_preprocess.py | Inserts the new normalization pass early in the Vulkan preprocess pipeline to keep later stages/runtime inputs consistent. |
| backends/vulkan/_passes/normalize_convolution_args.py | Implements broadcasting of single-element convolution argument lists based on inferred spatial rank. |
| backends/vulkan/_passes/init.py | Exposes NormalizeConvolutionArgs via the Vulkan passes package. |
| backends/vulkan/test/test_vulkan_passes.py | Adds a regression test for padding="valid" exporting as a one-element list and verifies 2D broadcast / 1D no-op behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # 2D: one element padding must be broadcast to the two spatial dims. | ||
| edge_program = to_edge( | ||
| torch.export.export(Conv2dValid().eval(), (torch.randn(1, 3, 32, 32),)), | ||
| compile_config=EdgeCompileConfig(_check_ir_validity=False), | ||
| ) | ||
| ep = edge_program._edge_programs["forward"] | ||
| self.assertEqual(padding_of(ep), [0], "expected export to emit a 1D padding") | ||
|
|
||
| ep = _transform(ep, NormalizeConvolutionArgs()) | ||
| self.assertEqual(padding_of(ep), [0, 0]) | ||
|
|
||
| # 1D: a one element list already matches the single spatial dim, so it | ||
| # must be left alone. | ||
| edge_program = to_edge( | ||
| torch.export.export(Conv1dValid().eval(), (torch.randn(1, 3, 32),)), | ||
| compile_config=EdgeCompileConfig(_check_ir_validity=False), | ||
| ) | ||
| ep = _transform( | ||
| edge_program._edge_programs["forward"], NormalizeConvolutionArgs() | ||
| ) | ||
| self.assertEqual(padding_of(ep), [0]) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f947f27ed
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """A 2D convolution written `padding="valid"` exports a one element | ||
| padding list, which the Vulkan convolution cannot read. | ||
|
|
||
| make_ivec2_from_list -> make_ivec2 requires exactly 2 elements, so such | ||
| a convolution aborted at the first inference with |
There was a problem hiding this comment.
This lengthy docstring repeats the implementation rationale, runtime assertion, and downstream model context rather than concisely describing the behavior under test; the assertions below already make the 2-D broadcast and 1-D no-op expectations clear. Keeping the same explanation in both the pass and its test increases maintenance burden, so reduce this to a short behavioral description.
AGENTS.md reference: AGENTS.md:L46-L50
Useful? React with 👍 / 👎.
Fixes #22774
Summary
ATen lets a convolution's
stride,padding,dilationandoutput_paddingbe given as a single value that applies to every spatial dimension, andtorch.nndoes exactly that forpadding="valid", which exports aspadding=[0]rather than[0, 0].The Vulkan convolution reads these as fixed-width vectors —
make_ivec2_from_list→make_ivec2, which requires exactly two elements — so a 2D convolution written that way lowers fine and then aborts at the first inference:The identical convolution written
padding=0runs correctly, so only the spelling of the argument decided whether the model worked.This matters beyond one op:
padding="valid"is how HuggingFace writes SigLIP and CLIP patch embeddings —— so every vision tower of that shape hit it, with an assert deep in the runtime that gives no hint that padding spelling is the cause.
The change
A
NormalizeConvolutionArgspass that broadcasts a single-element list to the number of spatial dimensions, run at the start of the preprocess pipeline where the graph is still ATen compliant. A 1D convolution is left alone, since one element already matches its one spatial dim.On where to fix it: broadcasting in
make_ivec2_from_listwould also be reasonable and is smaller. I did it in the graph because the normalized graph is what every later pass and the serializer see, and because I can test a Python change against a stock runtime — I have no build of the Vulkan runtime here, so a C++ patch would have gone out unverified. Happy to move it if you prefer the runtime.Test plan
12 tests pass, including the new
test_normalize_convolution_args_broadcasts_single_element_lists, which asserts a 2D convolution's[0]padding becomes[0, 0]and a 1D convolution's stays[0].Verified end to end on an AMD Radeon 8060S (RADV GFX1151, RDNA 3.5), lowering each module and comparing the delegate's output against eager:
Conv2d(padding=0)Conv2d(padding=1)Conv2d(padding=(1, 1))Conv2d(padding="valid")make_ivec2abort"valid"make_ivec2abortConv1d(padding=1)Conv1d(padding="valid")backends.vulkan.test.test_vulkan_delegateproduces an identical set of results before and after (its failures in my environment are pre-existing and unrelated).Found while lowering openpi's π₀.₅ — whose vision tower is SigLIP — to the Vulkan delegate.