Conversation
| ) | ||
| if padded_dims > 2: | ||
| raise NotImplementedError( | ||
| f"CoreML's pad operation only supports `{mode}` mode when " |
There was a problem hiding this comment.
Let's just stick with the error message from the Core ML Framework, I think it clearer: Padding for more than two dimensions only supports constant mode".
| ], | ||
| ) | ||
| def test_pad_reflect_replicate_3d_raises(self, mode, torch_module): | ||
| # Regression test for issues #2576 and #2571: MIL `mb.pad` only supports |
There was a problem hiding this comment.
Let's update this comment using the language mentioned in my other comment.
Per @TobyRoseman's review on apple#2701: - ops.py: replace the verbose multi-line error with the existing Core ML Framework wording ('Padding for more than two dimensions only supports constant mode') so the conversion-time and runtime errors match. - test_torch_ops.py: update the docstring to the same language and match the new error string in pytest.raises (the previous regex matched the mode name, which is no longer in the message).
|
Thanks for the review @TobyRoseman! Updated in abb9faa — now raising the Core ML Framework's own wording verbatim ( |
|
This may be a better fix: #2697 |
|
Agreed @TobyRoseman — #2697's approach is better, since it actually makes >2D reflect/replicate pads work via the new |
Per @TobyRoseman's review on apple#2701: - ops.py: replace the verbose multi-line error with the existing Core ML Framework wording ('Padding for more than two dimensions only supports constant mode') so the conversion-time and runtime errors match. - test_torch_ops.py: update the docstring to the same language and match the new error string in pytest.raises (the previous regex matched the mode name, which is no longer in the message).
6140a85 to
52ac1ed
Compare
Core ML only accepts `reflect` / `replicate` padding when every non-zero pad
lies in the last two dimensions of the input, while torch has no such
restriction. `torch.nn.ReflectionPad3d` / `torch.nn.ReplicationPad3d` pad three
dimensions, and `torch.nn.functional.pad` can pad any subset of dimensions, so
conversion succeeded but the resulting model failed to load with
Error compiling model: "Failed to parse the model specification.
Error: Unable to parse ML Program: in operation pad_cast_fp16:
Padding for more than two dimensions only supports `constant` mode".
Both modes map every output index to an input index one axis at a time, so a
multi-axis pad is equivalent to a sequence of single-axis pads. Lower an
unsupported pad into a chain that pads the last two dimensions first and then
each remaining axis on its own, transposed to the end so Core ML only ever sees
padding on the final dimension.
Padding that is already confined to the last two dimensions, `constant` mode,
and dynamic pad values are all emitted exactly as before.
Fixes apple#2571
Fixes apple#2576
52ac1ed to
c72b1f7
Compare
LeSingh1
changed the title
|
@TobyRoseman — following up on your ask to review #2697, and on the question you left there about doing this at op conversion time. I have a full local build now (native libs + I have to correct my own earlier review of #2697 — it does not fix the issue. I read the logic on paper and said it looked right; it does not survive a real run. Applied to current Why: the restriction is not "at most two padded dimensions", it is "all non-zero padding must be in the last two dimensions". Enumerated against a rank-5 input, identical for
Splitting a 3-axis pad into chunks of two arbitrary axes still produces a chunk on axes (2, 3) — that is exactly the To your question: yes, this belongs at op conversion time, and it is straightforward there. I've replaced this PR's contents with that and updated the title and description to match (the previous "raise early" version is at 6140a85 if you'd rather have it back). Verification — same tests against Coverage added: Credit to @tritolol — #2697 is where the "a multi-axis non-constant pad is separable" idea comes from; this puts it in the converter and adds the transpose the runtime restriction requires. Happy to close this in favour of an updated #2697 if @tritolol wants to take it from here. |
2 participants
Problem
reflectandreplicatepadding fail to convert when padding is applied outside the last two dimensions. The Core ML runtime rejects the resulting op:The restriction is often described as "at most two padded dimensions", but that is not what the runtime enforces. Enumerated against a rank-5 input (identical for
reflectandreplicate):[0,0, 0,0, 0,0, 0,0, 1,1][0,0, 0,0, 0,0, 1,1, 1,1][0,0, 0,0, 1,1, 0,0, 0,0][0,0, 0,0, 1,1, 1,1, 0,0][0,0, 1,1, 0,0, 0,0, 0,0]The actual rule is all non-zero non-constant padding must be in the last two dimensions. So a rank-4
F.pad(x, (0,0,1,2,2,1), mode="replicate")pads only two axes and is still rejected.Fix
Lowered at op conversion time in the torch
padconverter, per @TobyRoseman's question on #2697.reflectandreplicatemap each output index to an input index one axis at a time, so a multi-axis pad is exactly a sequence of single-axis pads. The converter pads the last two dimensions first, then each remaining axis individually with that axis transposed to the end — so Core ML only ever sees non-constant padding on the final dimension.No new pass and no pipeline ordering to reason about, and nothing merges the chain back because the emitted pads are never adjacent. Constant mode, dynamic pad values, and already-legal pads emit byte-identical output.
Testing
Same tests against
mainand against this branch, both backends, both frontends:Coverage added:
test_pad_reflect_replicateextended to rank 5 (noneuralnetworkskip needed),test_pad_reflect_replicate_3dforReflectionPad3d/ReplicationPad3dsymmetric and asymmetric, andtest_pad_reflect_replicate_leading_dimsfor the two-leading-axes case.Numerics verified against eager PyTorch on
mlprogramfp32 andneuralnetwork.