Raise NotImplementedError at conversion time for reflect/replicate pad >2D (fixes #2576, #2571)#2701
Conversation
…d >2D
`mb.pad` only supports `reflect` / `replicate` modes when at most the final
two dimensions carry non-zero padding. The generic `pad` converter
forwarded the mode through unconditionally, which produced a confusing
model-compile-time failure inside `predict()`:
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
This patch checks the (static) padding profile inside `_translate_torch_args`
and raises a clear `NotImplementedError` at conversion time instead. The
check fires for the torch.export-decomposed shape of `ReflectionPad3d` /
`ReplicationPad3d` with non-trivial padding (`aten.pad` with mode=reflect
or replicate and >2 dims of non-zero pad).
A regression test under `TestPad` covers both torch modules.
Fixes apple#2576
Fixes apple#2571
| ) | ||
| 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 |
2 participants
Summary
mb.padonly supportsreflect/replicatemodes when at most the final two dimensions carry non-zero padding. The genericpadtorch converter forwards the mode through unconditionally, so torch.export-decomposedtorch.nn.ReflectionPad3d/torch.nn.ReplicationPad3d(which decompose toaten.padwithmode="reflect"/"replicate"and 3 padded dims) currently convert successfully and then explode atpredict()time with:The two original issues (#2576 ReflectionPad3D, #2571 ReplicationPad3d) reported this exact failure mode and asked for an ahead-of-time error instead.
Fix
Added a static check inside
_translate_torch_argsin thepadop converter (coremltools/converters/mil/frontend/torch/ops.py). Whenmode in ("reflect", "replicate"), thepadlist is fully static, and more than two dimensions have non-zero pad, raiseNotImplementedErrorwith a message that names the mode, the padded-dim count, and the typical culprit modules.The check is scoped tightly:
pad(dynamic-padding path was already raising for other reasons);reflect/replicatemodes (constant and circular unaffected);padded_dims > 2— valid 2D cases (ReflectionPad2d, etc.) still convert unchanged.Tests
Added
TestPad::test_pad_reflect_replicate_3d_raisesparameterized over bothtorch.nn.ReflectionPad3dandtorch.nn.ReplicationPad3d:I also manually verified
torch.nn.ReflectionPad2d(padding=2)on a rank-4 input still passes the new guard and proceeds to the rest of the converter.Issues
Fixes #2576
Fixes #2571