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. ❌ 1 New Failure, 1 Unrelated FailureAs of commit 98b4f25 with merge base 822cb60 ( NEW FAILURE - The following job has 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 |
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.
|
metascroy
left a comment
metascroy
left a comment
There was a problem hiding this comment.
Thanks, the refusals look right to me. I checked the main claims against stock coremltools 9.0 (without this PR):
- the single-entry KV-cache write
cache[:, :, pos] = kconverts, and this PR keeps it in Core ML; - the same write with a dynamic batch fails with "Cannot add const";
accumulate=Truenext to a slice fails withNotImplementedError;- a 0-d index whose values vary only after it converts, and this PR keeps it in Core ML;
- a bool
amaxfails withValueError.
The portable kernels for min/max/amin/amax/minimum/maximum and index_put all accept bool, so the refused ops run correctly on the CPU. Judging bool min/max by the output dtype also keeps maximum(bool, float) in Core ML. Nice.
Did you run the new tests locally on a Mac? The runtime comparisons only run on macOS (macOS 15 for the iOS 18 case), and the test-coreml (coreml, models) job times out at 3h, on main too, so CI doesn't tell us much here. If you did, could you say which macOS version? I ask partly because of the flakiness in the inline comments: one run passing doesn't rule it out.
Two smaller notes:
- The
test_int4_weight_only_per_groupchange (seed, 16 rows, tolerance from 1e-3 to 1e-2) isn't related to the refusals. It's fine, but it would be easier to track as its own PR or at least its own note, since it loosens a tolerance. - When this lands, #24317 will need a rebase: it has its own
_is_index_put_coremltools_cannot_convert,_is_min_max_of_boolandios18flag, which should give way tooverride_reasonandminimum_deployment_targetfrom here.
| ok = (x.tril() == x).view(1, -1) | ||
| flags = ok.min(-1)[0].to(x.dtype) + ok.amax(-1).to(x.dtype) | ||
| both = torch.maximum(ok, ok.flip(-1)).view(x.shape).to(x.dtype) | ||
| floats = x.amax(-1, keepdim=True) + torch.maximum(x > 0.5, x) |
There was a problem hiding this comment.
I think this will flake on macOS. x > 0.5 runs in Core ML at the default float16 precision, so a float32 value just above 0.5 rounds to 0.5 and the comparison flips. torch.maximum then gives 1.0 vs x, about 0.5 off, well beyond atol=1e-2. Simulating it, a 14×14 torch.rand has at least one such element in about 4.7% of runs (roughly 1 in 20), and the test doesn't fix a seed. Fixing the seed, using compute_precision=ct.precision.FLOAT32, or using inputs away from the 0.5 threshold would make it deterministic.
There was a problem hiding this comment.
You're right, and thanks for simulating it. The next push feeds values that float16 holds exactly, torch.rand(14, 14).half().float(), so x > 0.5 gives the same answer in Core ML and in eager.
| y[self.rows_mask, self.trio] = c | ||
| torch.ops.aten.index_put_(y, [None, self.col], a, True) | ||
| torch.ops.aten.index_put_(y, [None, self.last], c) | ||
| mask = y > 0.5 |
There was a problem hiding this comment.
Same float16 threshold issue as in the min/max test, at a lower rate: y > 0.5 (and a > 0.5 below) run in Core ML, and the mask is added to the output, so a flip changes it by 1. There are fewer elements, so it's rarer, but the same fix applies (seed, float32 precision, or values away from the threshold).
There was a problem hiding this comment.
Fixed the same way in the next push. The inputs are values float16 holds exactly, and every value the test compares with 0.5 is one of them or twice one. The accumulate=True write runs on the CPU, so the sum it makes is exact too.
| indices = [i if i is None else i.meta.get("val") for i in node.args[1]] | ||
| given = {dim: index for dim, index in enumerate(indices) if index is not None} | ||
| if not given or not all( | ||
| isinstance(t, torch.Tensor) for t in (y, values, *given.values()) |
There was a problem hiding this comment.
Nit: if any meta["val"] is missing, this returns None, so the op stays in Core ML. That seems like the right choice since export always sets it, but a short comment saying so would make it clear it's intentional.
There was a problem hiding this comment.
Yes, that is on purpose. The next push adds a comment: export always sets val, so an index_put without it is left to coremltools, as before this change.
|
|
||
| self._compare_eager_quantized_model_outputs( | ||
| session, example_inputs, atol=1e-3 | ||
| session, example_inputs, atol=1e-2 |
There was a problem hiding this comment.
Is the tolerance increase still needed now that the seed is fixed and there are 16 rows? If 1e-3 passes with a fixed seed, keeping it would keep this test as strict as before. If not, 1e-2 matching the other int4/int8 recipe tests is reasonable.
There was a problem hiding this comment.
Not strictly. With seed 0 and 16 rows, 1e-3 passes here, but only just: the largest difference from the eager quantized model is 0.92e-3. Over seeds 0 to 99, 67 of them go over 1e-3, up to 1.82e-3, so 1e-3 would pass only because of the seed we picked. The difference comes from Core ML computing in float16, and main has it too. 1e-2 matches the other int4 and int8 recipe tests, so I would keep it.
|
Thanks for checking these against stock coremltools. Mac runs. Yes, on macOS 27.0.1 on an M3 Max, with the Core ML runtime. At the head you reviewed, run together with #24424, The int4 test change is its own commit. Its message says why the tolerance moves to 1e-2, and the description names it. #24317. Yes. Its next push drops its own copies of these refusals, since they moved here, and it will use the deployment target this PR adds once this lands. That push waits for #24404 as well. |
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.