Repository navigation
Refuse ops coremltools cannot convert in the Core ML partitioner, and say why - #24425
shoumikhin wants to merge 4 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24425
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 7 PendingAs of commit 98b4f25 with merge base 822cb60 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude /pr-review |
meta-cla
Bot
added
the
CLA Signed
|
Claude finished @shoumikhin's task in 29m 0s —— View job Requesting changes: the new Detailed reviewPR Review: #24425SummaryThe PR keeps bool min/max and the Code Quality
Testing
Backward Compatibility
RecommendationRequest Changes None of this breaks correctness. But saying why is half of this PR, and the
|
coremltools 9.0 cannot compute min, max, amin, amax, minimum or maximum of bool tensors. It raises a ValueError instead of reporting the op as unsupported, so lowering fails, for example on the all-true check that torch.distributions.MultivariateNormal exports. A bool next to a float or int tensor converts, since the result is then float or int. The partitioner now leaves the calls with a bool result to the portable CPU kernels and still delegates the rest. The new test checks which ops stay on the CPU, and that an amax of a float tensor and a maximum of a bool and a float tensor stay in Core ML, and on macOS compares the output with eager.
coremltools converts y[:, idx] = v only when idx has a single entry. With idx = torch.tensor([0, 2]), lowering fails with "Input squeeze_0 has rank 1 != other inputs rank 0". accumulate=True, a bool tensor next to a slice below iOS 18, a bool index next to a slice or next to other indices, index tensors that broadcast, and a size known only at run time next to the slice fail too. With a 0-d index or several indices next to a slice, coremltools lays out the written part differently from torch, so values land in the wrong place unless they vary only in the dims after the last index. The partitioner now sends these forms to the portable CPU kernels, and logs which form it found. Forms coremltools handles, such as a single-entry index for a cache update, or a bool mask on its own, stay in Core ML. The new test counts the index_put calls left on the CPU for each form, with a static and a dynamic batch size, and on macOS 15 compares the output with eager.
With lower_full_graph=True, an op that the partitioner refuses failed with a message saying that Core ML does not support the op, even when coremltools converts the op and the partitioner refuses only one form of it. The error now gives the reason, such as "aten.index_put.default on a bool tensor with a minimum_deployment_target below iOS 18 is not supported by CoreML. Set lower_full_graph=False in the CoreMLPartitioner to run it outside of CoreML." The index_put test checks the reason for some of its forms, including a bool write that a later deployment target would not fix, which gives its own reason.
test_int4_weight_only_per_group used random weights and a random input, and it failed for some of them on macOS, on main too. - Core ML runs this recipe in float16, so its output differs from the eager quantized model by up to about 2e-3, more than the test's 1e-3 tolerance. The other int4 and int8 recipe tests use 1e-2. - The SQNR against the float model came from a single input row, which gives only eight outputs. For some weights it fell to 12 dB. The eager quantized model gives the same SQNR as Core ML there, so this is the int4 rounding of those weights seen through eight outputs, not a Core ML error. The test now feeds 16 rows, uses the 1e-2 tolerance, and fixes the seed. Over seeds 0 to 99 every case passes, with the SQNR at 17 dB or more.
32df0cb to
98b4f25
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 29m 49s —— View job Requesting changes: the new bool min/max test will probably fail about 1 run in 20 on macOS because Core ML compares Detailed reviewPR Review: #24425SummaryThe PR keeps bool min/max and the Code Quality
Testing
RecommendationRequest Changes This PR fixes one random macOS failure in the recipe tests but adds another in the bool min/max test. Inputs that float16 represents exactly fix it. The other findings are small fixes to the error message and the tests.
|
What is wrong today
Some ops run in PyTorch but not in coremltools. The Core ML partitioner still hands them to Core ML, so lowering fails with an error from deep inside coremltools, or the op writes to the wrong place:
min,max,amin,amax,minimumandmaximumof bool tensors raise aValueError.torch.distributions.MultivariateNormalexports one in its all-true check. A bool next to a float or int tensor works.index_putnext to a slice, such asy[:, idx] = v, converts only with single-entry int indices,accumulate=False, static sizes in the other dims, and a non-boolybelow iOS 18. For example,idx = torch.tensor([0, 2])fails with "Input squeeze_0 has rank 1 != other inputs rank 0". With a 0-d index or several indices, coremltools lays out the written part differently from PyTorch, so values land in the wrong place unless they vary only after the last index. Without a slice, several index tensors must have the same shape and no bool mask.Also, with
lower_full_graph=True, the error says Core ML does not support the op even when the partitioner refused only one form of it.What this change does
lower_full_graph=Trueerror now gives the reason, for example "aten.index_put.default on a bool tensor with a minimum_deployment_target below iOS 18 is not supported by CoreML."test_int4_weight_only_per_group, which failed on macOS for some random weights on main too, now feeds 16 rows, uses the 1e-2 tolerance of the other int4 and int8 recipe tests, and fixes the seed.Testing
test_coreml_partitioner.py,test_torch_ops.pyandtest_coreml_recipes.pyunderbackends/apple/coreml/testpass on Linux and on macOS with the Core ML runtime.index_putalso with a dynamic batch size, check the error reasons, and on macOS 15 compare the static cases with eager PyTorch.