refactor: expand CoreMLExportError to error on non-compatible configs for CoreML and remove more CoreML export tests#42
Conversation
24813ce to
dfc5a5f
Compare
| ExportBackend.CoreML, | ||
| unsupported_configs={"backend": ExportBackend.CoreML, "act_dtype": torch.int8}, | ||
| ) | ||
| # CoreML rejects per-channel activation quantization outright. |
There was a problem hiding this comment.
Currently, in the overall export test where we test configs, the quick fix is to exclude the configs that have per-channel activation quantization.
The reason we do this is because the configs that we use for testing CoreML export and CoreAI export are the same even though we know CoreML and CoreAI don't support the same things. Ideally, we'll have another PR that goes through these export configs and cleans it up
There was a problem hiding this comment.
I didn't want to do it in this PR itself as it would make the PR more complicated than it should be and go beyond the "remove bad CoreML export tests + refactor CoreMLExportError" scope
| ), | ||
| marks=pytest.mark.skip(reason="Crashes with segfault"), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
This is the test we removed because we know that CoreML doesn't support per-channel activations.
When I looked at the CoreML model, it did actually have per-channel activations, but there is a bug in coremltools where it always convert dequantize(scale_A) → reshape → quantize(scale_B) to reshape. This works for per-tensor but not always for per-channel. To avoid this error, a user needs to specify pass_pipeline=ct.PassPipeline.EMPTY, but since that's not always guaranteed and there's no per-channel activation tests in coremltools, the easy solution is to reject the config.
guru-desh
changed the title
CoreMLExportError to error on non-compatible configs for CoreML and remove more CoreML export tests
Weight/activation/LUT dtype checks and the activation-granularity check were duplicated across the eager, graph, and palettization export paths. Consolidate them into validate_coreml_compatibility() so CoreML's actual export constraints live in one place, including the per-channel/per-block activation restriction: an upstream coremltools MIL optimizer pass can silently corrupt per-channel-quantized activations across reshape boundaries, so CoreML export now rejects that configuration outright instead of letting it through to a numerics-level failure.
…eML export CoreML export now rejects these configs outright instead of letting them through to a numerics-level SNR failure. Replace the xfail-based coverage with direct CoreMLExportError assertions, and collapse the now-redundant per-axis/per-dtype combinatorial sweeps since the rejection doesn't depend on any of those variables.
…clarity assert_coreml_finalize_rejects_unsupported_dtype and COREML_DTYPE_REJECTION_MATCH are now used for granularity rejections too, not just dtype ones, so their names no longer matched what they check.
dfc5a5f to
1663dde
Compare
3 participants
Summary
There's some CoreML export error tests that are xfailed with
SNRThresholdError. During test triaging, we noticed that they still failed.Upon deeper investigation, we noticed that the tests were checking per channel activations, which CoreML doesn't support. The main changes in the PR are the following:
validate_coreml_compatibilitywhich usesCoreMLExportErrorto raise errors on both incompatible configs and incompatible dtypes (before it was just dtypes). This function centralizes the export raising logic for CoreMLCoreMLExportErrorwas being raisedTesting
I verified that CoreML doesn't support per-channel activations, and CI is passing too