Repository navigation
Fix wrong dtypes and values in Core ML conversion - #24424
shoumikhin wants to merge 8 commits into
Conversation
|
@claude /pr-review |
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24424
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 2 Pending, 1 Unrelated FailureAs of commit d5e114a with merge base ec56cfa ( 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. |
meta-cla
Bot
added
the
CLA Signed
|
Claude finished @shoumikhin's task in 34m 17s —— View job Review summary: No confirmed blockers, but Detailed reviewPR Review: #24424SummaryThe PR overrides coremltools' torch converters for fills, Code Quality
Testing
Backward Compatibility
RecommendationRequest Changes Two items block approval:
The rest are minor or need a Mac to confirm. |
d198c7d to
dae80e0
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 57m 36s —— View job Review summary: I found no confirmed bugs, but the new output-copy pass only looks through Detailed reviewPR Review: #24424SummaryThe PR overrides coremltools' torch converters for fills,
Python, pip and Code Quality
Testing
Backward Compatibility
RecommendationApprove Nothing here is a confirmed bug. Before merging, check 1 and 6 on a Mac:
|
metascroy
left a comment
metascroy
left a comment
There was a problem hiding this comment.
Thanks, this looks good. Each override falls back to the coremltools converter when it doesn't have what it needs, and each fix has a test. I didn't find regressions in the cases I tried against coremltools 9.0: 0-d and out-of-int32-range fills, negative indices on 3-D tensors, and a bool mask next to a slice, which fails the same way with or without this PR. Small comments are inline.
Did you run these tests locally on a MacBook? The end-to-end comparisons in _compare_outputs only run when IS_VALID_TEST_RUNTIME is true, so on Linux the new tests mostly check the converted program. The test-coreml (coreml, models) job is timing out at 3h (it does on main too), so CI doesn't give much signal here. If you ran them on macOS, could you say which macOS version and compute units? That matters most for test_same_shape_expand_output, which depends on CPU_ONLY vs ALL behavior.
Nit: the tests call private APIs (CoreMLBackend._convert_to_mlmodel, mlmodel._mil_program), and the pass uses function._copy_scope_info. They all exist in coremltools 8.3 (fbcode) and 9.0, so this is fine for now, but they could break when coremltools is upgraded.
Some things I checked that look right: torch's decomposition uses the float32 eps for a float16 rms_norm (torch/_decomp/decompositions.py), and the identity copies survive quantization. op_linear_quantizer_config reruns the default pipeline, but noop_elimination skips ops that produce a program output.
| _add_fill(context, node, inputs[1], inputs[2].val) | ||
|
|
||
|
|
||
| @register_torch_op(torch_alias=["fill"], override=True) |
There was a problem hiding this comment.
With the fill alias, a TorchScript fill node (no dtype) now falls back to _ct_ops.full_like instead of coremltools' own fill converter. The result is equivalent, and a TorchScript fill_ model I tried fails the same way with or without this PR. Still, a separate fill override that falls back to _ct_ops.fill would leave the TorchScript path untouched.
There was a problem hiding this comment.
Good catch. In the next push, fill has its own override. It falls back to coremltools' own fill converter when the node has no dtype or the fill value is known only at run time, so the TorchScript path is the same as before.
| return shared | ||
|
|
||
|
|
||
| @register_pass(namespace="executorch", name="copy_reused_outputs") |
There was a problem hiding this comment.
Nit: without override=True, coremltools raises KeyError: Pass executorch::copy_reused_outputs already registered if this module is imported twice (e.g. a reload in a notebook). The torch op overrides in torch_ops.py already pass override=True.
There was a problem hiding this comment.
Done in the next push: the pass now registers with override=True, like the op overrides.
| and output.op.op_type == "tile" | ||
| and output.op.x.sym_type == output.sym_type | ||
| ): | ||
| output = output.op.x |
There was a problem hiding this comment.
When the tile's input isn't otherwise reused, the output becomes output.op.x and the tile op is left with no users. Since this pass runs last, nothing removes it, so it ends up in the serialized program. Harmless, but you could remove it here (or run dead_code_elimination after this pass).
There was a problem hiding this comment.
Thanks. With the default pass list, coremltools already removes it: after this pass it runs its backend_mlprogram pipeline, which includes dead_code_elimination. But a custom pass list without dead_code_elimination skips that pass in the backend pipeline too, and then the tile stays. So the next push removes the replaced tile in this pass, and test_same_shape_expand_output checks that with such a list.
| Fills(), (torch.arange(6).reshape(2, 3), torch.rand(2, 3).half()) | ||
| ) | ||
| program = ct.convert( | ||
| fills.run_decompositions({}), |
There was a problem hiding this comment.
Fills is only checked through ct.convert(..., convert_to="milinternal"); only the MultiheadAttention model is lowered and run. Could you also lower Fills through to_edge_transform_and_lower and compare it with _compare_outputs? That covers the ExecuTorch path, which is where node.meta["val"] comes from, and checks the output dtypes and values on the Core ML runtime.
There was a problem hiding this comment.
Done in the next push. The test now also lowers Fills with the partitioner, checks that every op goes to Core ML, and on macOS compares the outputs with eager through _compare_outputs.
There was a problem hiding this comment.
The latest push also changes how the test checks the dtypes. _compare_outputs cannot catch a wrong dtype here. The runtime casts each Core ML output to the dtype ExecuTorch expects (copy_value in multiarray.mm), and float32 stores every value Fills returns exactly. So instead of converting the exported program directly, the test now converts lowered_module_0.original_module, the program the delegate itself converts, with convert_to="milinternal", and checks that the outputs are int32, bool and float16. I also replaced one zeros_like with full_like, since no test covered the full_like override.
|
Thanks for the review. Mac runs. Yes. I ran the Core ML test files on macOS 27.0.1 on an M3 Max, with the Core ML runtime, so the comparisons with eager ran too. At the head you reviewed, run together with #24425, Private APIs. Agreed. The tests use them to look at the converted program. The pass needs |
Torch computes sqrt, exp, sigmoid, sin and similar ops in float when their input is an int or bool tensor. coremltools passes such an input on to the Core ML op, which takes only float, so lowering fails. For example, x.long().sqrt() fails with "expects tensor or scalar of dtype from type domain ['fp16', 'fp32'] but got tensor[2,3,int32]". coremltools casts an int input for log and log1p, but not a bool one. The backend now wraps the coremltools converters of these ops, log and log1p included, so that an int or bool input is first cast to fp32, as torch does. The new test lowers each op on an int or bool input and on macOS compares the output with eager.
For an int input, torch returns floor, ceil and round of it unchanged and computes relu, relu6 and hardtanh in int. torch.nn.ReLU6 and torch.nn.Hardtanh export as hardtanh. coremltools passes the input on to the Core ML op, which takes only float, so lowering fails. For example, x.long().floor() fails with "expects tensor or scalar of dtype from type domain ['fp16', 'fp32'] but got tensor[2,3,int32]". hardtanh also fails on a float input with int bounds, which F.relu6(x, inplace=True) passes, or on a float16 input, since coremltools hands the bounds to Core ML's clip as they are, and clip needs them in the dtype of the input. The backend now converts these ops for an int input as torch does: the input itself for floor, ceil and round, and the input clipped in int for relu, relu6 and hardtanh. Torch drops the fraction of each hardtanh bound, so hardtanh(x, -1.5, 2.7) clips an int x at -1 and 2. For a float input, hardtanh passes its bounds to clip in the dtype of the input, as apple/coremltools#2894 does upstream, and the other ops keep the coremltools conversion. A narrower int, which Core ML has from iOS 17, still fails for relu, relu6 and hardtanh, since Core ML's maximum takes int32 but no narrower int. The new test lowers each op on an int input, and hardtanh with int bounds on a float16 input, checks that everything goes to Core ML, and on macOS compares the output with eager.
Torch's copy writes src in the dtype of self, so copying an int or bool tensor into a float one gives float values, and copying a float into an int one drops the fraction. coremltools' copy keeps the dtype of src. Lowering turns y[:, i] = v into a copy as well. So an int or bool v written into a float y stays int or bool in Core ML, and the next op that takes only float fails to lower. For example, F.gelu(x.clone().copy_(x.long())) fails with "expects tensor or scalar of dtype from type domain ['fp16', 'fp32'] but got tensor[2,3,int32]", and y[:, 0] = x[:, 1].long() on a float y fails in the select_scatter that follows. A float v written into an int y fails there the same way. The backend now casts src to the dtype torch gives self before coremltools converts the copy. It reads that dtype from the node, not from self in Core ML, since coremltools gives some tensors another dtype, such as int32 to a float arange, and casting to it would drop the fraction of the copied values. The new test lowers int and bool values written into a float tensor, an int tensor copied into a float one and read by gelu, a float written into an int tensor, and a float copied into a float arange and read by softmax, checks that everything goes to Core ML, and on macOS compares the output with eager.
coremltools makes full without a dtype, zeros and new_zeros as float, ones_like as float from iOS 16, zeros_like with the dtype of its input, and full_like, new_full, fill and scalar_tensor with the type of their fill value. So torch.full((1, 16), 0), which is int64 in torch, comes out as float. Index math on it then fails to lower, or loses integers above 2048 at float16 precision. nn.MultiheadAttention turns a bool mask into a float one with zeros_like(mask, dtype=...), which coremltools kept bool, so attention came out wrong. The backend now converts these ops, and empty, new_empty and empty_like, with the dtype torch gives them. A fill value known only at run time, or a node without a dtype, keeps the coremltools conversion. torch.ones needs nothing, since coremltools casts it to its dtype, and new_ones has no coremltools converter, so lowering decomposes it into full. An int fill can now reach a Core ML op that takes only float, such as torch.full((1, 4), 4).sqrt() or torch.full((1, 4), 4).floor(), or a copy between it and a float tensor, such as y[:, 0] = torch.full((2,), 7) on a float y. The three commits before keep these models lowering. The new test lowers the fills and the attention with every op in Core ML, checks the dtypes Core ML gives the fills and the attention mask, and on macOS compares both with eager. The int input tests now also lower int fills read by sqrt and by floor, and copied to and from a float tensor.
torch.export passes approximate="tanh" to gelu as a keyword, and coremltools reads it only from the positional arguments, so gelu(approximate="tanh") ran as the exact GELU. The two differ by up to 4.7e-4. The backend now reads the keyword. The new test checks the mode in the converted program, and on macOS compares the output with eager.
torch.nn.RMSNorm leaves eps out by default, and coremltools 9.0 requires it: "got 3 input(s), expected [4]". Torch then uses the float32 machine epsilon, also for float16 inputs, so the backend passes that. The new test checks the eps in the converted program, and on macOS compares the output with eager.
coremltools turns ops such as clone into nothing, so a partition can return its own input, as in return x.clone(), x * 2, or one value twice. It also keeps a same-shape expand as a tile that only passes its input through, as for the scale_tril that torch.distributions.MultivariateNormal returns next to its expand. Core ML then returns one of those outputs as zeros or as the bytes of another output. For the expand, this happens in float32 on the CPU and GPU. A pass at the end of the conversion now treats such a tile as its input, and returns an input or a value that is already an output through a copy. It removes the tile it replaces, since a custom pass list may leave out the dead code elimination that removes it otherwise. An expand that changes the shape stays a tile. The tests check the outputs of the converted program, also that no replaced tile is left with a pass list that has no dead code elimination, and on macOS run it: the expand on the CPU in float32, where the value came back as zeros without this change.
coremltools writes y[:, i] = v to y[:, i:i + 1] without first wrapping a negative i, so for i = -1 it writes to an empty slice. With a constant i, lowering fails with "The update tensor should have shape [3, 0]. Got (3, 1)". With i as an input, Core ML raises at run time below iOS 18, and from iOS 18 it skips the write, so the output is silently wrong. The Core ML converter for index_put now adds the size of the dim to each negative int index next to a slice, then hands the op to the coremltools converter. A constant index is wrapped while converting, so it adds nothing at run time. A write without a slice goes to coremltools unchanged, and so does a TorchScript node. The new test checks that only an input index next to a slice gets a select at run time, that a constant -1 and an input -1 lower into Core ML with a static and a dynamic width, and on macOS compares the output with eager, also at a second width for the dynamic one. It also converts a TorchScript index_put.
dae80e0 to
d5e114a
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 58m 13s —— View job Request Changes: the new Detailed reviewPR Review: #24424SummaryThis PR makes the Core ML converter give fills, copies, and the int or bool inputs of elementwise ops the dtypes torch gives them. It also reads gelu's
The rest are minor or low confidence. Python and the coremltools source weren't available in this job, so I couldn't run anything. Findings that depend on coremltools internals are marked unverified. Code Quality
Testing
Backward Compatibility
RecommendationRequest Changes: fix the repeated tile in
|
What is wrong today
coremltools converts some ops differently from PyTorch, so lowering fails or outputs are wrong:
sqrt,exp,sigmoid,sinand similar ops fail to lower on an int or bool tensor, which PyTorch computes in float.floor,ceil,round,relu,relu6andhardtanhfail the same way on an int tensor, though PyTorch keeps their result int.y[:, i] = vdoes, keeps it int or bool, so a later float-only op fails to lower.full,zeros_likeandscalar_tensorget the wrong dtype. For example,torch.full((1, 16), 0)is int64 in PyTorch and float in Core ML, so later ops fail or lose precision.nn.MultiheadAttentionhits this through its bool mask.gelu(approximate="tanh")runs as the exact GELU, since coremltools ignores the keyword thattorch.exportpasses.nn.RMSNorm(eps=None)fails to lower: coremltools requireseps.expandof another output can return one of them as zeros or as another output's bytes.y[:, i] = vwithi = -1writes to an empty slice: lowering fails for a constanti, and for an inputiCore ML raises at run time below iOS 18 and silently skips the write from iOS 18.What this change does
compiler/torch_ops.pyoverrides the coremltools converters of these ops, so each override can go once coremltools fixes its bug. Inputs get the dtype PyTorch computes in: float for the ops that need it, int kept for the rounding and clipping ops, and the dtype ofyfor a value copied into it. Fills get PyTorch's dtype,gelureads its keyword,rms_normgets PyTorch's default eps, and a negative index next to a slice gets the dim size added. The input fixes come first, because the fill fix lets an int fill reach those ops.compiler/coreml_preprocess.pycopies an output of the converted Core ML program that repeats an input or another output, also through a same-shapetile.Testing
test_torch_ops.py,test_coreml_partitioner.py,test_coreml_recipes.py) pass on Linux. On macOS with the Core ML runtime, where the new tests also compare with eager PyTorch, they passed at an earlier revision.