Repository navigation
OpenVINO: fix wrong outputs when a partition returns a copy of its input - #24466
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24466
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 6 Pending, 1 Unrelated FailureAs of commit e71822f with merge base 893d815 ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 19m 36s —— View job The partitioner change fixes the reported stale-output case, but when a channels-last copy also has users inside the partition, those users now read its result in the wrong layout and give wrong values without any error, so this needs changes before merging. Detailed reviewPR Review: #24466SummaryThe PR makes Code Quality
Testing
RecommendationRequest Changes The case of a copy used both inside and outside the partition now gives wrong values without an error. The PR's own docstring describes this risk. It is cheap to turn into an export error and to cover with a test. The check that no output comes directly from an input is optional, but it would make any op missing from |
|
@suryasidd @cavusmustafa @ynimmaga @anzr299 @daniil-lyakhov could you take a look at this one? It fixes wrong results when an OpenVINO partition returns one of its inputs unchanged, for example The bug also reproduces in plain OpenVINO (2025.2, 2026.4.1 and nightly), with no ExecuTorch involved. Binding the outputs before the inputs avoids it, which may help find the root cause. Thanks! |
When an OpenVINO partition returned a copy of one of its inputs, for example `x.clone()` or `x[:, :]`, the copy came back with stale memory, and so did anything else the partition computed from that input. OpenVINO converts these copies to nothing, so the converted model returns its input directly. ExecuTorch's runtime binds the output buffers after the inputs, and in that order OpenVINO's CPU plugin does not write such an output and can overwrite the input with the output buffer's old contents. Every OpenVINO release tried behaves this way, from 2025.2 to the current nightly. Binding the outputs first avoids it, but only because of an undocumented detail of the plugin. The partitioner now leaves such a copy (aten.clone, dim_order_ops._clone_dim_order or aten.alias_copy) out of the partition when its result leaves the partition, so it runs on the CPU kernels. Copies used only inside the partition stay there, since OpenVINO skips them anyway. The OpenVINO op tests can now pass an EdgeCompileConfig, to also cover lowering with dim order turned off, which gives aten.clone.
ce2f519 to
e71822f
Compare
|
Thanks for the review. Answers by item:
|
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 23m 30s —— View job No blocking issues found. Two text fixes remain: the PR description says both new tests fail without the change, but one passes on main, and the workaround's docstring links no upstream OpenVINO issue. Detailed reviewPR Review: #24466SummaryThe PR leaves a Code Quality
RecommendationApprove Neither point blocks merging. Both are text fixes, one to the PR description and one to the docstring. |
Hey @shoumikhin, i have reproduced the bug, i will raise it with the core OpenVINO team. Should I create another PR to fix the other issue about non-default dim order ? |
|
@pytorchbot cherry-pick --onto cherry-pick-24396-by-pytorch_bot_bot_ -c critical |
Cherry picking #24466The cherry pick PR is at #24478 and it is recommended to link a critical cherry pick PR with an issue. Details for Dev Infra teamRaised by workflow job |
What is wrong today
When an OpenVINO partition returns a copy of one of its inputs, ExecuTorch gets stale memory back. For example:
Lowered with
OpenvinoPartitionerfor CPU,yholds whatever its buffer held before, andtorch.sin(y)is wrong too. OpenVINO can even overwritex, so later ops that readxgo wrong as well. The same happens forx.detach(),x[:, :], and chains likex.clone().clone(). This made a robot policy exported with LeRobot return wrong actions.The cause is in OpenVINO. Its PyTorch frontend converts
aten.cloneandaten.alias_copyto nothing, so the converted model returns its input directly. ExecuTorch's runtime sets its output tensors after its input tensors. In that order the CPU plugin does not write such an output, and it can overwrite the input with the old contents of the output buffer. A plain OpenVINO script, without ExecuTorch, shows the same problem in every release tried, from 2025.2.0 to 2026.4.1 and a 2026.5 nightly.What this change does
The partitioner leaves such a copy out of the partition when its result leaves the partition, so ExecuTorch's CPU kernel makes the copy. This covers
aten.clone,aten.alias_copy, anddim_order_ops._clone_dim_order(which becomesaten.clonebefore conversion), including chains of them. A copy used only inside the partition stays there. OpenVINO skips it anyway, and taking it out could hand OpenVINO an input in a dim order it does not read.Not touched here: the backend still reads and writes partition inputs and outputs in the default dim order, so a channels-last tensor at the partition edge can give wrong values.
Setting the output tensors first also avoids the bug, but only through an undocumented detail of the plugin, so this change does not rely on it.
With dim order turned off, a channels-last copy of an input that leaves the partition now fails at export instead of giving wrong values.
execute_layer_testgets an optionalcompile_config, to test lowering with dim order turned off, where a clone isaten.clone.Testing
backends/openvino/tests/ops/test_copy.py.test_copy_of_input_as_outputfails without the partitioner change and passes with it.test_copy_of_input_used_insidechecks that a copy used only inside the partition stays there.