Repository navigation
OpenVINO: fix bool min/max and masked read lowering, and follow-ups to #24294 - #24396
Open
shoumikhin wants to merge 34 commits into
Open
shoumikhin wants to merge 34 commits into
shoumikhin wants to merge 34 commits into
Conversation
The OpenVINO CI job runs backends/openvino/tests/test_runner.py. When a test failed, the runner printed "completed with failures" but still exited with code 0, so the CI job stayed green and the failure went unnoticed. Now the runner exits with code 1 when any test fails, so CI turns red. Co-authored-by: suryasidd <surya.siddharth.pemmaraju@intel.com>
…d it A position cumsum such as cumsum(ones_like(input_ids)) - 1 reads only the shape of input_ids. Once FoldConstantCumsumPass folds it, nothing in the partition reads input_ids. When that input comes first, OpenVINO's PyTorch converter takes it for forward's self and removes it. The runtime still passes every partition input, so every call failed with "expected 2 args (inputs=1 + outputs=1), got 3". An unused input in any other position is kept by the converter. Not folding is no fix, because masks built from such a cumsum then go wrong on OpenVINO's CPU plugin from the second call on. Instead, when the converted model has exactly one parameter fewer than the partition has inputs, preprocess puts the first one back, so the compiled model takes the same inputs the delegate passes. The test leaves both the first input and a later one unused.
ExecuTorch can plan an output with a dynamic size while OpenVINO compiles a fixed shape for it, for example x[torch.ones(4, dtype=bool)] once OpenVINO folds the mask. The buffer the runtime makes for such an output started empty, and OpenVINO refuses an output tensor whose shape differs from a fixed one, so every call failed with 0x1. Before the owned buffers, these programs ran. The buffer now starts with the shape of the infer request's own output tensor: empty along each dynamic dimension and the full shape where OpenVINO fixed it.
FoldConstantCumsumPass evaluates a full_like from an empty tensor of its
input's shape. That tensor went to the default device, so lowering for
OpenVINO under torch.device("cuda") or set_default_device("cuda") failed
on a CPU-only PyTorch with "Torch not compiled with CUDA enabled", and
on a CUDA build it used the GPU for nothing. The pass evaluates
everything else on the CPU already; this tensor now goes there too.
With inference_precision given twice, for example bf16 and then an empty value, lowering keeps the last entry and so compiles with the float32 default, but the runtime skipped empty entries and applied bf16. On x86, where the runtime decides, the program then computed in bfloat16. The runtime now takes the last entry as well, including an empty one, which means the default.
Lowering passes the precision too, so the comment was wrong to say only the runtime does. What makes the check depend on the runtime is that on x86 OpenVINO applies only the precision it gets when it loads the model.
The cumsum fold lines had landed between the comment and the decoder call it explains, so the comment read as if it were about the fold.
Since each cumsum is folded on its own, _evaluate is only ever asked for one node. Taking and returning that one value drops the list and dict it no longer needs, and a check that could never be false.
Only the attention mask test covered an int32 cumsum, and only through an OpenVINO CPU bug that newer OpenVINO releases fix. A fold limited to int64 now fails a pass-level test too.
Only x86 CPUs with bfloat16 units computed in bfloat16 before, so only they get slower, and only there does asking for bf16 bring the earlier speed back. On other x86 CPUs OpenVINO already computed in float32.
The precision docs said int64 results only go wrong outside the int32 range. Measured through the delegate on a CPU, an int64 sum is computed in float32: 16777216 + 1 gives 16777216, and 123456789 + 987654321 gives 1111111168. cumsum and prod near 2^24 stay exact.
Only an output compiled with a fully fixed shape refuses a buffer of another shape. A partly dynamic output accepts an empty one. Comment only.
OpenVINO 2025.1 has the same CPU plugin bug with integer cumsums over constants as the later releases, so the fold's docstring now starts the range there. Docstring only.
…ioner
OpenVINO converts min, max, amin, amax, minimum, maximum, clamp and sign
to its ReduceMin, ReduceMax, Minimum, Maximum and Sign operations, and
none of those accept bool tensors. The partitioner still gave these ops
to OpenVINO when they read bool tensors, so the whole program failed to
lower:
ReduceMin ... Element type of data input must be numeric. Got: boolean
With PyTorch 2.15, MultivariateNormal exports two such mins when it
checks scale_tril, so a model that builds one could not use OpenVINO.
The partitioner now leaves these ops to the CPU when every tensor they
read is bool, and the portable kernels compute them as PyTorch does. An
op with a bool and a float input, such as maximum(mask, x), is computed
in float, which OpenVINO converts, so it stays with OpenVINO. Other
dtypes are not affected.
create_owned_output reported errors through its return value and its buffer through an out-parameter, and took a whole tensor only to read its dtype. It now returns exr::Result<ov_tensor_t*> and takes the dtype, like create_ov_tensor beside it, so its call site reads like the one for create_ov_tensor again. No behavior change.
…_precision The check that the runtime takes the last inference_precision entry ran only on x86 CPUs with bfloat16 units, so CI could skip it. A new test gives an invalid value and then an empty one. Lowering keeps only the empty one, and the runtime has to as well, or it fails to load the program. On the runtime before that fix it fails with "unsupported inference_precision 'f7'".
shoumikhin
added
the
release notes: openvino
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24396
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 6 Unrelated Failures, 2 Unclassified FailuresAs of commit b7623fe with merge base 05092c1 ( NEW FAILURE - The following job has failed:
UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:
BROKEN TRUNK - The following jobs 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. |
meta-cla
Bot
added
the
CLA Signed
Contributor
Author
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 22m 44s —— View job PR Review: #24396. I found no blockers, only minor points: the CI script now skips the model tests after an op failure, preprocess has no sanity check, two tests can't catch what they target, and it's unclear whether all ten bool ops need to leave OpenVINO. Detailed reviewSummaryThe PR keeps bool Code Quality
Infrastructure
Testing
API Design
Performance
RecommendationApprove. Nothing blocks merging, and none of the findings is a regression. The CI script change, the preprocess check and the two test fixes are small enough to make in this PR. The two partitioning points can be follow-ups. |
The OpenVINO CI job runs the op tests and then the model tests under set -e. Now that the test runner exits with code 1 when a test fails, a failing op test stopped the job before the model tests started, so a red run said nothing about the models. The script now runs both suites and exits with code 1 if either failed.
Preprocess put the first partition input back whenever the converted model had one parameter fewer than the partition has inputs. Now it does so only when that first input is really unused, and lowering fails when the counts still differ. The runtime binds inputs by position, so another mismatch would have made every call fail, or swapped two inputs of the same type and shape. A partition can also have no inputs at all, such as a comparison that reads only a buffer next to a bool min on the CPU, and a test covers that case.
TestBoolMinMax compared only the values of min(dim) and max(dim). Their indices are where the order of ties matters, and the all-true and all-false rows are all ties, so the test now compares the indices with eager too. Its docstring also says more exactly why these ops leave OpenVINO.
x was int64 in both subtests, so the result was int64 whatever dtype the folded cumsum had, and a fold that dropped dtype=torch.int32 still passed. x now has the subtest's dtype, so such a fold fails the int32 subtest.
The docs said only sum reduces integers in float32. Measured through the delegate on a CPU with OpenVINO 2026.4.1 and 2025.1.0, amax, amin, max and min of int32 and int64 tensors do too: amax over a tensor that holds 16,777,217 gives 16,777,216. Inputs and running totals round as well, so the sum of 16,777,217 and -16,777,216 gives 0. cumsum and maximum stay exact, and so does prod where OpenVINO computes it (2026.4.1).
Contributor
Author
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 29m 8s —— View job PR Review: #24396. I found no blockers, only two optional simplifications and one untested assumption behind keeping bool Detailed reviewSummaryThe PR keeps bool Code Quality
Testing
RecommendationApprove. Nothing blocks merging. Both Code Quality points are optional, and the tie test can go with the deferred follow-up. |
The docs did not say which CPUs compute integer ops in float32, and they named the reductions only. Measured with plain OpenVINO 2026.4.1 on the CPU device and the f32 hint: on x86, sum, max and min of int32 and int64 values go through float32 (the max of 16,777,217 and 16,777,219 gives 16,777,220), while element-wise ops stay exact. On an ARM CPU, element-wise add, maximum and clamp go through float32, and so do reductions along any but the last dimension, while reductions along the last one stay exact. The paragraph now gives one sentence per CPU, in plainer English.
"On an x86 CPU with bfloat16 units that is slower" first reads as if "that is slower" describes the CPU. Say that float32 is slower there.
"Meta, unlike CUDA, fails on every build" packed the reason into a riddle, and "Meta" at the start of a sentence reads like the company. Say that without the fix the meta device keeps the cumsum unfolded on every build, because a meta tensor has no data to copy to the CPU, while CUDA would only show it on a build without CUDA. Comment only.
_reads_only_bool now collects its inputs with node.all_input_nodes, like the cumsum fold does, so it also sees a tensor passed by keyword. None of the ops it guards takes one today, so nothing changes for them. The docstring now says that OpenVINO promotes mixed tensor inputs: a scalar bound, as in clamp(mask, min=0.5), is converted to the input's bool type, which is why only tensors are checked.
…odel An owned output buffer took its starting shape from the infer request's output at that moment. On a delegate handle shared by several calls, as in programs from ExecuTorch 0.7 and earlier, an earlier call may have bound its own buffer there, so a small dynamic output could start with a large static output's shape and reserve that much memory again. The shape now comes from the compiled model's output port, empty along each dynamic dimension, which no call can change. It no longer depends on what a plugin's request holds before or between inferences. The shared-handle test also runs a static call before a dynamic one. If dynamic outputs were left unbound instead of owned, OpenVINO would use the earlier call's buffer there, and this test fails.
No test reached the two checks _match_partition_inputs gained: put the first input back only when it is unused, and fail when the counts still differ. A direct test now hands it a converted model without its first input, once for a graph that never reads that input and once for one that does. The comment above the count check also no longer says it catches swapped inputs: a wrong count makes every call fail, and the check reports that at lowering instead.
Contributor
Author
|
Thanks. On your three points:
|
Contributor
Author
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 37m 23s —— View job PR Review: #24396. I found no blockers, only two minor points. First, if OpenVINO ever compiles an output with a dynamic rank, that output now fails on every call where it used to run. Second, the new direct test of Detailed reviewSummaryThe PR keeps Backward Compatibility
Code Quality
RecommendationApprove. Nothing blocks merging. Both fixes are small. The first one matters only if OpenVINO can compile a partition with a dynamic-rank output. |
The direct test of _match_partition_inputs gave both inputs the same shape and dtype and checked only how many parameters came back, so a version that appended the restored input, or built it from the second input, still passed. The runtime binds inputs by position, so either mistake would feed one input to the other's parameter. The test now gives the first input its own shape and dtype and checks both parameters in order. It needs no device, so it moves out of the op test class into its own unittest.TestCase.
An output that OpenVINO compiled with a dynamic rank cannot get a buffer, because an ExecuTorch tensor keeps its rank. No partition tried so far compiles to one, but if one does, the error now says so and names the fix: export with static sizes for the inputs that output depends on.
…ties The CPU precision notes listed where OpenVINO's results differ from eager PyTorch, but not the indices min(dim) and max(dim) return on ties: PyTorch returns the first one, and OpenVINO's CPU plugin can return a later one, for uint8 and other dtypes. Both pages now say so.
"The sum 16,777,216 + 1" read like element-wise add, which x86 computes exactly; the example is about the sum reduction. It now says sum over a tensor that holds both values.
Reading masked values after writing them, as in
x[mask] = x[mask] * 2
return x + x[mask].sum()
failed to lower with "Dynamic rank for aten::index input is not
supported". OpenVINO converts index_put to a placeholder whose rank it
takes from the decoder, and its FX decoder reads ranks only from each
node's tensor_meta. ExecuTorch edge graphs leave tensor_meta empty and
keep the shape in val. Preprocess now fills tensor_meta from val before
conversion, the way other ExecuTorch passes do. OpenVINO releases that
do not delegate index_put, such as 2025.1.0, skip the new test.
shoumikhin
changed the title
Contributor
Author
|
I kept the failure because seven graphs built to get a dynamic-rank output all got a fixed rank, but the error now says to export with static sizes for the inputs that output depends on. |
Contributor
Author
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 25m 31s —— View job PR Review: #24396. I found no blockers and one minor docs point: the new tie note names uint8 but not float32, and the new notes on OpenVINO's CPU behavior don't say which release they describe. Detailed reviewSummaryThe PR keeps bool
API Design
RecommendationApprove. Nothing blocks merging. The only finding is a small docs edit in the two mirrored files. |
The lint job runs mypy without OpenVINO installed, so the op_support import that the new index_put test needs failed with import-not-found. The file now turns that check off, as test_reductions.py does for the same import.
The new notes on the CPU plugin did not say which OpenVINO release they describe, and the tie note named uint8 but not float32, so a reader with a float32 model could take it for an integer-only quirk. Both notes now name OpenVINO 2026.4.1, the release they were measured with, and the tie note names float32 as well as int32 and uint8.
Contributor
Author
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 42m 52s —— View job PR Review: #24396. I found no blockers and two minor points, neither a regression. First, preprocess still fails to lower when OpenVINO drops a first input that only Detailed reviewSummaryThe PR keeps bool
Code Quality
RecommendationApprove. Nothing blocks merging. Each fix is a one-line change plus a test, and neither model ran before this PR. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is wrong today
The OpenVINO partitioner gives
min,max,amin,amax,minimum,maximum,clampandsignto OpenVINO even when every tensor they read is bool. OpenVINO cannot convert these for bool:torch.min(x > 0, dim=1)andMultivariateNormal's argument check (PyTorch 2.15) hit this. Reading masked values after writing them fails too:What this change does
The partitioner leaves these ops to ExecuTorch's CPU kernels when every tensor they read is bool. Ops that also read a float tensor, like
maximum(mask, x), stay with OpenVINO. Casting to uint8 could give wrongmin(dim)andmax(dim)indices on ties, as the docs now note.For the masked read, preprocess fills each node's
tensor_metafromval. OpenVINO reads ranks only fromtensor_meta, which ExecuTorch leaves empty.Follow-ups to #24294:
cumsumread, failing every call. Preprocess puts it back and checks the input count.cumsumfold works under a CUDA default device on a CPU-only PyTorch (exir's ScalarToTensorPass still fails there).inference_precisionentry now also means the default at load.What was tested
Linux x86_64, OpenVINO 2026.4.1 and 2025.1.0, CPU, unless noted:
minmodel (both releases) and the masked read (2026.4.1) failed to lower, and now match eager.tensor_metachange, all other test partitions convert to the same OpenVINO graphs.