[suryasidd]

Summary

OpenVINO computes in the default dim order. When a partition input or output was channels-last, it read or wrote the bytes as if they were contiguous, and ExecuTorch returned wrong values without any error. Elementwise ops hide this, since they produce the same bytes either way, so it showed up mostly with reductions, convs and other ops that depend on the layout.

The lowering now adds a convert_layout transpose for each partition input and output in another dim order. The runtime passes OpenVINO the sizes in memory order, both when it binds tensors and when it resizes dynamic outputs. Copies used only inside a partition can now stay there for OpenVINO to fold.

Added a new method _partition_inputs to keep preprocess under flake8's complexity limit.

Test plan

Added a new test for testing non-default dim order. Each new edge case in test_dim_order fails without the fix, with wrong values and no error. With the fix everything is passing. Also checked with the reproducer in #24325 under 13. The reproducer now works without errors.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24797

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 4bad4d2 with merge base 2d8cd35 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[Copilot]

🟢 Approval recommended

No active review issues remain for the changes addressing channels-last tensor handling at OpenVINO partition boundaries.

No open issues 🎉

What changed in this PR

This PR fixes channels-last tensor handling at OpenVINO partition boundaries, addressing silent incorrect results in layout-sensitive operations.

Changes:

• Adds boundary transposes during lowering and maps runtime tensor shapes to and from memory order.
• Adds layout regression tests covering partition inputs, outputs, dynamic sizes, and size-one dimensions.
• Refactors input metadata collection and updates layout-related comments and type annotations.

File	Description
exir/​tensor.py	Corrects tuple annotations for strides and sizes.
backends/​openvino/​tests/​ops/​test_dim_order.py	Adds channels-last boundary regression tests.
backends/​openvino/​tests/​ops/​test_copy.py	Updates the comment explaining partition-internal copies.
backends/​openvino/​tests/​ops/​base_openvino_op_test.py	Allows tests to specify dynamic shapes during export.
backends/​openvino/​runtime/​OpenvinoBackend.cpp	Maps tensor shapes between logical and memory order and validates output rank.
backends/​openvino/​preprocess.py	Collects input layout metadata and inserts boundary transposes.
backends/​openvino/​partitioner.py	Updates comments to reflect the new layout handling.

_{Review effort: Balanced}

[shoumikhin]

@claude review effort=max

[claude]

Claude finished @shoumikhin's task in 31m 46s —— View job

---

PR Review: #24797

This PR adds transposes at channels-last OpenVINO partition edges, but the runtime now binds every blob in memory order with no version marker, so some older .pte files that used to work now fail with a bare Error::Internal; the README, tests and partitioner also still reflect the old behavior (Needs Discussion).

Detailed review

Summary

Preprocess now adds a convert_layout for each partition input or output whose dim order is not the default, and the runtime passes OpenVINO the sizes of those tensors in memory order.

Problems found:

• The contract between the lowering and the runtime has no version and is never checked, which breaks some .pte files that used to work.
• The README still documents the bug this PR fixes.
• The new tests depend on where the partitioner puts a clone, and never assert it.
• copy_output adds a heap allocation on every call.
• The partitioner keeps a dim-order rule whose stated reason this PR removed.

Nothing was executed, because this runner can't run Python, pytest or git fetch. All findings come from reading the code.

Code Quality

• The partitioner keeps a dim-order rule whose only stated reason this PR deleted.
  • partitioner.py:139 (node.meta["val"].is_contiguous()) still sends a non-contiguous copy of a constant to the CPU, where the constant is stored twice (README.md:130-131).
  • The deleted docstring text ("since OpenVINO writes every output in the default one") was the reason, and the new output transpose makes it false.
  • test_copy.py:155 still says "OpenVINO writes every output in the default dim order".
  • The docstring (:109-110) says "not in the default dim order", but the code checks is_contiguous(). These differ for size-1 dims: a channels-last (2,3,1,1) tensor is is_contiguous() but has dim order (0,2,3,1).
  • The same applies to a returned copy of a partition input that changes dim order. "OpenVINO would return the input itself" (:104) no longer holds once that output gets a Transpose.
  • Either drop the rule (and flip the two channels-last cases at test_copy.py:155-161), or document the reason that remains.
  • I'm not confident that dropping it is safe, because I couldn't run OpenVINO.
  • Fix this →
• Output indices count every graph output, but input indices count only user inputs (preprocess.py:165-170 vs :105).
  • output_node().args[0] lists BUFFER_MUTATION outputs first.
  • The OpenVINO model comes from edge_program.module(), which returns only user outputs.
  • This is latent, because partitions made by the partitioner don't own mutated buffers.
  • It would fire on a whole-program to_backend (as at test_copy.py:254-260) of a program that mutates a buffer: the transposes would land on the wrong outputs.
  • Filter by graph_signature.output_specs with OutputKind.USER_OUTPUT.
  • Fix this →

Testing

• The tests depend on where a clone runs, but never assert it.
  • The "input to …" cases (test_dim_order.py:26-29) get a channels-last partition input only because returning y makes _remove_copies_of_inputs move the clone to the CPU.
    • The comment at :22-23 says the clone runs on the CPU, but not why.
    • If that rule changes (see the partitioner item under Code Quality), these cases silently become contiguous-input tests and still pass.
    • Assert that dim_order_ops::_clone_dim_order is in execution_plan[0].operators, as test_copy.py:46-47 does.
  • test_copy_of_input_used_inside (test_copy.py:49-55): the new comment says the clone stays in the partition, but after this PR the values are right either way, so the test no longer checks that. Assert plan.operators == [], as test_copy.py:129-130 does.
• Coverage gaps for the new paths:
  • No channels-last program input goes straight to the partition, although that is the usual NHWC deployment flow. A channels-last sample input to execute_layer_test should cover it.
  • No 5-D channels_last_3d case, although dim_order_from_stride canonicalizes 5-D too (exir/tensor.py:110).
  • No SymInt input next to a channels-last input. That would check that the int_inputs and input_dim_orders positions line up with ppp.input(i).
• Fix this →

API Design

• backends/openvino/README.md still documents the bug this PR fixes.
  • Lines 111-114 say only returned copies "in the default dim order" are handled.
  • Lines 138-141 say that a copy made with .to(), a copy in another dim order such as channels_last, or a copy of a value computed inside the partition "can come back with wrong values … without an error". The new output, conv output and outputs in both orders cases are exactly these.
  • The README also never says that partition edges in a non-default dim order now get a transpose.
  • Fix this →

Backward Compatibility

• Old .pte files with a partition edge in a non-default dim order can stop running, and nothing is logged.
  • create_ov_tensor (backends/openvino/runtime/OpenvinoBackend.cpp:764-769) and copy_output (:651-656) now reorder sizes by tensor.dim_order() for every tensor.
  • The blob is still only compiled.export_model() (backends/openvino/preprocess.py:218-220), so the runtime cannot tell a blob with the new transposes from one lowered before this PR.
  • Trigger:
    • Take a .pte lowered before this PR and run it on the new runtime with the same OpenVINO. It needs a channels-last edge whose memory-order shape differs from its logical shape, in a case where the old code happened to give correct values.
    • Two examples are an elementwise-only partition (sin of a channels-last (2,3,6,5) input) and the size-1 shapes from test_channels_last_with_size_one_dims.
    • The runtime now binds [2,6,5,3] / [2,1,1,3] to ports compiled as [2,3,6,5] / [2,3,1,1], and OpenVINO refuses the shape on a static port.
    • execute then returns Error::Internal without an ET_LOG (:526-528, :568-570).
    • Upgrading ExecuTorch without re-exporting is enough to hit this.
  • New blobs are fragile too.
    • The two sides agree only because _non_default_dim_order (preprocess.py:93-99) and TensorSpec.from_tensor (exir/tensor.py:248) both call dim_order_from_stride(stride, shape).
    • Nothing checks this at load or execute time.
    • If the two drift apart, the result is a shape error, or silently wrong values again when the shapes coincide or are dynamic.
  • Suggestion:
    • Follow the XNNPACK pattern (backends/xnnpack/runtime/XNNHeader.h): prefix the processed bytes with a magic/version header, or with the dim order of each port.
    • Reorder sizes only for blobs that have the header, and keep the old behavior for blobs without one.
    • At minimum, log when infer_request_set_{input,output}_tensor_by_index fails, and say in the README that OpenVINO .pte files must be re-exported.
  • Fix this →

Performance

• copy_output heap-allocates on every execute, for every dynamic output (OpenvinoBackend.cpp:653).
  • create_ov_tensor in the same file uses a stack buffer instead.
  • resize_tensor(Tensor, ArrayRef<int64_t>) copies the sizes into a stack std::array<SizesType, kTensorDimensionLimit> anyway (runtime/core/exec_aten/util/tensor_util.h:1221-1236).
  • Fill a bounds-checked std::array<exa::SizesType, kTensorDimensionLimit> directly, and call the ArrayRef<SizesType> overload. :642 already ties the rank to output.dim().
  • Fix this →

Specific Comments

• backends/openvino/preprocess.py:162-164: the comment covers inputs and outputs but sits above the output code only. It also doesn't say why output orders are read before DimOrderOpsRevertPass and input orders after it.
• backends/openvino/preprocess.py:103-104: the docstring lists the dim order before the int positions, but the tuple is (types, shapes, int_inputs, dim_orders). It also doesn't say that only non-default orders are returned.
• backends/openvino/preprocess.py:98: the outer tuple(...) is redundant, since dim_order_from_stride already returns a tuple.
• backends/openvino/tests/ops/test_dim_order.py:63-64: say what the case guards against, namely that preprocess and the runtime pick the same dim order for ambiguous strides. It passes on the old code too.
• exir/tensor.py:54: the PR fixes the parameter annotations, but the return annotation Tuple[bytes] is the misleading one, since the function returns ints.
• PR description:
  • "Each new edge case … fails without the fix" isn't true for input to sin, contiguous and the six size-one subtests, whose bytes are the same either way.
  • "Copies used only inside a partition can now stay there" describes no change in behavior: partitioner.py:128 already looked only at nodes used outside the partition.
  • _partition_inputs is a module-level function, not a method.

Recommendation

Needs Discussion

The open question is whether OpenVINO .pte files must keep running on a newer runtime.

• If yes, the blob needs a version marker before the runtime changes how it binds tensors.
• If no, document that they must be re-exported, and log the bind failure.

Fix the README and the test assertions in this PR. The rest are minor.

Review tasks

• Gather context (diff, CLAUDE.md, pr-review skill)
• Independent reviews (3 reviewers) across all 7 changed files
• Consolidate and deduplicate findings
• Fact-check each finding against the code
• Post final review

[github-actions]

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[suryasidd]

Thanks for the review. Addressed the comments in the latest commit.

• Added a log message if a input or output bind failed due to channels-last format at the edge.
• Added a dim order section in the README.md, removed the parts about channels-last copies not handled.
• Removed the is_contiguous() check.
• Output dim orders now counts only user outputs. It is similar to inputs now.
• Addressed copy_output heap allocation, used stack
• Added more test cases to cover the cases mentioned in the review.

[shoumikhin]

@claude review effort=max

[claude]

Claude finished @shoumikhin's task in 37m 41s —— View job

---

PR Review: #24797

The runtime now passes sizes in memory order for every edge that is not in the default dim order, but nothing in the blob says whether it was lowered with the new transposes, so existing .pte files break in ways the README doesn't describe: some that give correct values on main now fail to bind, and dynamic outputs silently get a permuted shape (Request Changes).

Detailed review

Summary

This PR adds a transpose, when lowering, to each OpenVINO partition input and output that is not in the default dim order, and the runtime now passes their sizes in memory order. This re-review covers d691c32.

• Nothing tells the runtime whether a blob has the transposes, so .pte files exported before this PR change behavior in ways the README doesn't describe.
• The partitioner docstring and the README disagree on which returned copies are handled.

I couldn't run Python or the tests on this runner, so everything below comes from reading the code.

Testing

• Several cases assert nothing about the program (backends/openvino/tests/ops/test_dim_order.py:48-51, :76-91).

  • check_case asserts only for the input* cases. "output", "conv output", "outputs in both orders" and "contiguous" would still pass if the CPU computed them, and then no output transpose would run.
  • test_channels_last_input_after_int_input asserts nothing. It would still pass if the clone stayed in the partition, and then the partition would never get a channels-last input after the int.

  Assert operators == [] for the non-input cases, and assert _clone_dim_order in the int test.

• No 5-D output (test_dim_order.py:69-74). The 5-D test reduces to 2-D, so convert_layout never runs on a 5-D output. The comment at :70-71 covers only inputs, but a partition also returns a channels_last_3d tensor whenever it returns an elementwise op of one. Return x * 2 as well.

  Fix this →

• Low confidence, not run: outputs that the transposes make equal to an input.

  • A partition that returns y.permute(0, 2, 3, 1) of a channels-last input y becomes Parameter → Transpose → Transpose → Result, and the two transposes cancel.
  • A whole-program to_backend that returns a channels-last input unchanged becomes Parameter → Transpose → Identity → Transpose → Result.

  _copy_returned_inputs (backends/openvino/preprocess.py:215) runs before the PrePostProcessor adds the transposes. If OpenVINO folds them away, the output is the input again, and the CPU plugin has the problem that docstring describes. README.md:159-161 doesn't say what happens in another dim order. Please add both cases on the CPU.

  Fix this →

API Design

• The partitioner and the README disagree on returned copies into another dim order (backends/openvino/partitioner.py:107-108, backends/openvino/README.md:120-122, :146-152).

  • The docstring says a copy of a partition input into another dim order leaves the partition, because OpenVINO returning a transpose of an input "was checked only on the CPU". That rule covers only _COPY_OPS.
  • x.to(memory_format=torch.channels_last) of a partition input lowers to _to_dim_order_copy. It is supported (partitioner.py:156) but not in _COPY_OPS, so it stays and returns that same transpose on every device.
  • The README says the backend handles "only" a clone or a full slice (:121). Then it says a copy into another dim order "is handled" (:149-150), with no device caveat.

  Pick one rule. Either drop the CPU caveat, or make a _to_dim_order_copy of a partition input that keeps its dtype leave like a clone. Then make the docstring and both README paragraphs state it, and test .to(memory_format=torch.channels_last) of a partition input.

  Fix this →

Backward Compatibility

• Nothing records or checks the dim order each port was lowered with (backends/openvino/runtime/OpenvinoBackend.cpp:789-794, :675-680; README.md:113-116). The runtime now passes sizes in memory order for every tensor that is not in the default dim order, whichever ExecuTorch lowered the blob. The README says an older .pte with such an edge fails to bind and logs an error. That isn't always what happens:

  • Programs that are correct on main now fail. When only C is 1, or only H and W are 1, ExecuTorch keeps the channels-last dim order, but the bytes are the same as a contiguous tensor's (test_dim_order.py:104-107). Examples are a (N, C, 1, 1) pooled output and a (N, 1, H, W) grayscale input. Main passes the logical sizes, so these give correct values. If such a program was lowered before this PR, that edge now fails to bind.
  • Dynamic outputs get a wrong shape with no error. They bind to the buffer OpenVINO owns (:564-578), so nothing checks their shape. copy_output then reads the compiled (N, C, H, W) as sizes in memory order and resizes the output to (N, W, C, H). A (N, C, 1, 1) output becomes (N, 1, C, 1). resize_tensor checks only numel for DYNAMIC_BOUND.
  • Static tensors with C == H == W, e.g. (1, 64, 64, 64), have the same sizes in memory order. They still bind and still return wrong values, with no error.
  • Suspected, in new exports: a mutated buffer that is neither contiguous nor 4-D channels-last.
    • tag_constant_data skips mutated buffers (exir/backend/utils.py:383), so the buffer reaches the partition as an input.
    • _partition_inputs takes its dim order from the placeholder's strides, but the emitter stores such a buffer contiguous (exir/emit/_emitter.py:2143-2156).
    • Take register_buffer("s", torch.zeros(4, 3).t()), updated by the program and read by the partition. The lowering expects (4, 3) and the runtime passes (3, 4), so binding fails with no hint. A square buffer binds and is read transposed. Main reads both correctly.
    • This also makes "as ExecuTorch stores it" at preprocess.py:95 untrue.

  Suggested fix:

  • Put a small header in processed_bytes, like backends/xnnpack/runtime/XNNHeader.h, holding a version and each port's dim order. init strips it before core_import_model.
  • Treat a blob without the header as an old one and bind it with the logical sizes, as main does. Existing .pte files then keep working exactly as before.
  • For a blob with the header, execute compares each tensor's dim_order() with the recorded one and fails with a clear error on a mismatch. That catches the buffer case and replaces reexport_hint.

  If the maintainers would rather require re-export, the README has to name the silent cases. The PR description also needs a BC note so the change reaches the release notes.

  Fix this →

Specific Comments

• exir/tensor.py:54: the return annotation is still Tuple[bytes], but the function returns ints. Only the parameter annotations on :53 were fixed.
• PR description, "Copies used only inside a partition can now stay": this isn't a change. Main's docstring already keeps such copies, and test_copy_of_input_used_inside passes on main. What did change is that a returned copy of a constant into another dim order now stays, because is_contiguous() was removed from _remove_copies_of_inputs.
• PR description, "Each new edge case fails without the fix": this isn't true of "input to sin", which is elementwise so the bytes keep their order, of "contiguous", or of the size-one subtests.

Recommendation

Request Changes

• The runtime can't tell an old blob from a new one. Existing .pte files with a non-default dim order at an edge can fail, or return wrong shapes or values with no error, and some of them are correct on main.
• A version header fixes this and also catches the mutated-buffer case.
• Whether OpenVINO .pte files may require re-export is the maintainers' call. If they may, the README must describe the silent cases.
• The docs contradiction and the test assertions are small fixes.

Review tasks

• Gather context
• Independent reviews (3 reviewers)
• Consolidate
• Fact-check
• Check the follow-up commit d691c32 against the earlier review findings
• Post final review

[Copilot]

🟡 Changes recommended

A critical compatibility issue silently corrupts legacy channels-last dynamic output shapes by treating logical dimensions as memory order without versioning or rejecting incompatible layouts.

1 open issue

• Preserve legacy output layout when permuting dynamic shapes · New

_{Review effort: Balanced}

[suryasidd]

Thanks for the review. Added a header to distinguish between old .pte and new and kept the same behaviour for old pte's

[Copilot]

🟢 Approval recommended

No active review issues remain for the changes aligning tensor layouts at OpenVINO partition boundaries.

No open issues 🎉

1 resolved since last review

• Preserve legacy output layout when permuting dynamic shapes

_{Review effort: Balanced}