Repository navigation
Fix wrong dtypes and values in Core ML conversion - #24424
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 Cancelled Jobs, 63 Pending, 2 Unrelated FailuresAs of commit ab6ba2b with merge base ee4b85b ( CANCELLED JOBS - The following jobs were cancelled. Please retry:
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
|
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 |
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
|
d5e114a to
c812e63
Compare
|
Thanks for the review. Fixed in the latest push
Not changed
Testing: the Core ML tests pass on Linux. They have not run on macOS for this push yet. |
|
@claude /pr-review |
PR Review: #24424
|
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 53m 56s —— View job Needs Discussion: int fills are now int32, so they probably go through coremltools' Detailed reviewPR Review: #24424SummaryThe PR makes the Core ML converter match torch's dtypes and values for int inputs of float-only, rounding and clipping ops, for Code Quality
Testing
Backward Compatibility
RecommendationNeeds Discussion None of the findings is a confirmed blocker. If the
|
c812e63 to
8935822
Compare
|
Thanks for the review. Fixed
Not changed
Testing: the Core ML tests pass on Linux and on macOS at the latest push. |
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 58m 23s —— View job Approve: no blockers, with two minor follow-ups (a float Detailed reviewPR Review: #24424SummaryThe PR overrides coremltools converters so that fills, empties, Code Quality
Testing
RecommendationApprove: nothing blocks; the items above are minor follow-ups. |
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, checks that everything goes to Core ML, 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. It also fails on torch.arange(4, dtype=torch.float32), which coremltools makes int32. 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. hardtanh clips in the dtype torch gives its input, not in the one coremltools gives it, so hardtanh(torch.arange(4, dtype=torch.float32), 0.5, 2.5) clips at 0.5 and 2.5. A TorchScript node carries no dtype, so it keeps the coremltools converter. A narrower int, which Core ML has from iOS 17, still fails for relu and relu6, since Core ML's maximum takes int32 but no narrower int. hardtanh works there, since it casts such an input to int32 first. The new tests lower each op on an int input, hardtanh with int bounds on a float16 input, and hardtanh of a float arange, check that everything goes to Core ML, and on macOS compare the output with eager. The arange test also checks the bounds of the clip in the converted program, and converts a TorchScript hardtanh of a float input.
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 torch.arange(4, dtype=torch.float32), 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.
Torch promotes the tensor and the bound of clamp_min and clamp_max to one dtype, while coremltools requires the two to have the same dtype. So x.clamp_min(0) of a float x fails to lower with an AssertionError, and so does a float bound on an int tensor, such as x.long().clamp_max(2.5). The backend now casts the tensor and the bound to the dtype torch gives the result, then takes their maximum or minimum as coremltools does. A TorchScript node, which carries no dtype, keeps the coremltools converter. The new test lowers both ops with a bound of the other dtype, as a number and as a tensor, on a float and on an int tensor, 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(), a float bound of clamp_min or clamp_max, such as torch.full((1, 4), 0).clamp_min(0.5), or a copy between it and a float tensor, such as y[:, 0] = torch.full((2,), 7) on a float y. The four 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 tests of the four commits before now also lower int fills read by sqrt, floor and clamp_min, and copied to and from a float tensor. Another lowers empty, new_empty and empty_like with dim order ops skipped and checks their dtypes.
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 replaces such a tile with a copy of its input, and returns an input or a value that is already an output through a copy too. Each copy carries the debug info of the output it replaces, such as the expand. The pass 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, or whose shape is known only at run time, stays a tile. The tests check the outputs of the converted program and the debug handle each one carries, for an expand before and after the value it expands, 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 wrong 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.
|
Thanks for the review. Duplicate comment Not changed here
|
8935822 to
ab6ba2b
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 53m 4s —— View job Approve: no confirmed bugs. Two low-confidence findings, since coremltools couldn't be run or read in this job: the Detailed reviewPR Review: #24424SummaryThe PR overrides coremltools converters so that these convert the way torch computes them:
It also adds a MIL pass that gives each Core ML output a value of its own. There are no blockers. The two low-confidence findings depend on coremltools: the Code Quality
Backward Compatibility
Performance
RecommendationApprove: nothing confirmed blocks. Please check the
|
metascroy
left a comment
metascroy
left a comment
There was a problem hiding this comment.
Thanks, the fixes look good: the separate fill override, override=True on the pass, removing the dead tile, and lowering Fills and checking dtypes in the delegate's own converted program. Approving.
Before landing, could you run the Core ML tests on macOS at this head (ab6ba2b0)? As you mentioned, the newer fixes (int and bool inputs of float-only ops, rounding and clipping ops, copies, clamp_min/clamp_max and hardtanh) have only run on Linux, and the runtime comparisons only run on macOS. A pass of test_torch_ops.py, test_coreml_partitioner.py and test_coreml_recipes.py with the Core ML runtime would be enough. Please post the result here.
pytorch#24424 gives ones_like, zeros_like, full_like and fill the dtype torch gives them by making each a fill of the shape of its input. When that shape is known at conversion, coremltools turns such a fill into a constant as big as the input, so a fill like a 1x256x512 activation adds 256 KB of float16 numbers to the model. From iOS 16, coremltools converted ones_like and full_like to fill_like, which makes the tensor at run time instead. All four now use fill_like from iOS 16, still with the dtype torch gives them. Below iOS 16, where Core ML has no fill_like, they stay fills of the input's shape. The new test checks that these fills become fill_like at iOS 16, and on macOS compares the output with eager.
What is wrong today
coremltools converts some ops differently from PyTorch, so lowering fails or outputs are wrong:
sqrt,exp,sinand similar ops fail to lower on an int or bool tensor, which PyTorch computes in float. So dofloor,ceil,round,relu,relu6andhardtanhon an int tensor, though PyTorch keeps their result int.y[:, i] = vdoes, keeps its dtype, so a later float-only op fails to lower.clamp_minandclamp_maxfail to lower when the tensor and the bound differ in dtype, as inx.clamp_min(0)on a floatx.fullandzeros_likeget the wrong dtype: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: coremltools ignores the keywordtorch.exportpasses.nn.RMSNorm(eps=None)fails to lower: coremltools requireseps.expandof another output can silently return one of them wrong.y[:, i] = vwithi = -1writes to an empty slice: lowering fails for a constanti, and for an inputiCore ML fails at run time or, from iOS 18, silently skips the write.What this change does
compiler/torch_ops.pyoverrides the converters of these ops; 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, the dtype ofyfor a value copied into it, and the result's dtype forclamp_minandclamp_max. 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, since the fill fix sends int fills to them.compiler/coreml_preprocess.pyturns each same-shapetileoutput into a copy of its input, and copies an output that repeats an input or another output.Testing
test_torch_ops.py,test_coreml_partitioner.py,test_coreml_recipes.py) pass on Linux and on macOS, where the new tests also compare the Core ML output with eager PyTorch.