Skip to content

Core ML: stop the gather opt-out from failing tied embeddings - #21969

Merged
metascroy merged 3 commits into
pytorch:mainfrom
john-rocky:coreml-tied-embedding-quantize
Aug 28, 2026
Merged

Core ML: stop the gather opt-out from failing tied embeddings#21969
metascroy merged 3 commits into
pytorch:mainfrom
john-rocky:coreml-tied-embedding-quantize

Conversation

@john-rocky

Copy link
Copy Markdown
Contributor

Fixes#21856.

What happens today

op_linear_quantizer_config is applied with op_type_configs={"gather": None}, so that
embedding tables are not compressed by the linear quantizer. When a model ties its embedding
table to its output projection — one weight, two consumers — that constant reaches the
compressor configured two ways at once: quantized as a linear weight, exempt as a gather
table. coremltools refuses the conflict rather than resolving it:

ValueError: compression config conflict detected between ops
%aten_embedding_default_...: gather(x=%p_out_weight_to_fp16, ...)
and
%aten_mm_default...: linear(x=..., weight=%p_out_weight_to_fp16, ...)
... has config None while ... has OpLinearQuantizerConfig(mode='LINEAR_SYMMETRIC', ...)

So the opt-out does not skip the table. It fails the whole lowering, and a very common LLM
shape cannot be lowered at all with coreml_quantize set.

The change

Gathers whose constant is also consumed by an op of another type are named in
op_name_configs with the same config as the op they share it with, so the constant is seen
one way. Every other gather keeps the opt-out, and a model whose MIL program is not available
returns no names, which leaves the behaviour exactly as it was.

Checking it

The new test in test_coreml_partitioner.py lowers a two-layer model twice, tied and untied.
Against main the tied case raises the error above; with this change both lower. The untied
case passes either way, which is the point — the opt-out still applies where it always did.

Measured on macOS arm64, coremltools 9.0.

The linear quantizer config opts gathers out so embedding tables are not
compressed by it. When a model ties its embedding table to its output
projection, that one constant reaches the compressor configured two ways at
once — quantized as a linear weight, exempt as a gather table — and coremltools
refuses the conflict rather than resolving it. The opt-out then does not skip
the table, it fails the whole lowering, which is how a common LLM shape becomes
unlowerable with coreml_quantize set.
Gathers that share their constant with an op of another type are now configured
the same way as the op they share it with; every other gather keeps the
opt-out. A model whose program is unavailable is left exactly as before.
Fixespytorch#21856
@pytorch-bot

pytorch-botBot commented Aug 20, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21969

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 1 Cancelled Job, 2 Unclassified Failures

As of commit 02f2e1e with merge base 7c3cdbb (image):

NEW FAILURE - The following job has failed:

UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:

CANCELLED JOB - The following job was cancelled. Please retry:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2026
@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@metascroy

Copy link
Copy Markdown
Contributor

@claude review this code

@claude

claudeBot commented Aug 25, 2026

Copy link
Copy Markdown

Claude finished @metascroy's task in 1m 53s —— View job


Code review

  • Read coreml_preprocess.py changes
  • Read test_coreml_partitioner.py changes
  • Analyze correctness & edge cases
  • Post review

The change is small, well-scoped, and the reasoning in the comments is excellent — it explains why coremltools rejects the tied constant rather than just what the code does. The graceful degradation (empty list when _mil_program is unavailable → prior behavior) is the right instinct. A few observations, none blocking.


Correctness

The detection logic is sound.gathers_sharing_a_weight builds const→consumers and flags a gather only when gathers and len(gathers) < len(ops) — i.e. a non-gather also consumes the same constant. Two gathers sharing one table (len(gathers) == len(ops)) correctly stay opted out, and a gather with no co-consumer stays opted out. That matches the stated intent.

Behavioral trade-off worth calling out (not a bug). The resolution quantizes the tied table (names the gather with op_linear_quantizer_config) rather than leaving the whole constant uncompressed. The original {"gather": None} opt-out exists to keep embedding tables at full precision. For a tied weight that intent now flips: the table gets compressed to int4 along with the projection. This is almost certainly the right call for LLMs (the alternative is a hard failure, and the shared projection benefits from quantization), but it's a silent accuracy change for anyone who tied weights specifically expecting the table to stay uncompressed. Worth a one-line note in the PR description or a logger.info naming the tied gathers.

Minor points

  • op_name_configs assumes the co-consumer carries global_config. At line 588–592, tied gathers are assigned op_linear_quantizer_config (the global config). That is correct only because the current OptimizationConfig overrides nothing except gather. If a future change adds another op_type_configs entry (or a per-op override) for the type that shares the table, the gather would be matched to the global config while its co-consumer uses a different one — reintroducing the conflict. A short comment tying the config choice to "same config as the sharing op, given gather is the only type override" would harden this against future edits.

  • Nested blocks aren't traversed.for op in function.operations walks the top-level ops of each function; ops inside cond/while sub-blocks aren't visited. Not reachable for the embedding+linear shape here, so fine — just flagging it if this helper ever gets reused more broadly.

  • Double-counting is harmless but subtle.consumers[...].append(op) runs per matching input value, so an op referencing the same const in two inputs is counted twice. It can't produce a false positive here (gather's constant is only its x input) and duplicate names collapse in the {name: config ...} dict comprehension, but the len(gathers) < len(ops) comparison is technically counting input-edges, not distinct ops.

Test

  • to_executorch() exercises the CoreML preprocess (convert + linear_quantize_weights) on any platform, so this genuinely covers the fix on Linux CI, not just macOS — good. Looping tied/untied and asserting both reach a program is a clean regression guard.
  • The name is ..._is_quantized_with_its_linear but the assertion only checks that lowering succeeds, not that the table was actually compressed. Consider asserting on the resulting weight dtype (or at least renaming to ..._lowers) so the test name matches what it verifies.
  • ct.precision(ct.precision.FLOAT16.value) (line ~460) is a redundant round-trip; ct.precision.FLOAT16 is equivalent.

Overall this is a clean, minimal fix for a real and common failure. The two things I'd most want addressed are surfacing the "tied tables now get quantized" behavior change and tightening the test to match its name; everything else is optional polish.
· coreml-tied-embedding-quantize

return mlmodel

@staticmethod
def gathers_sharing_a_weight(mlmodel: ct.models.MLModel) -> List[str]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a public API on CoreMLBackend?

Or just a private helper in preproces?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — it has exactly one caller, _convert_to_mlmodel in the same file, so nothing needs it public. Renamed to _gathers_sharing_a_weight, matching the class's underscore convention.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make it a private free function in the file, not a static function on the class?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 02f2e1e — it's now a module-level private function. The scan never read anything from the class, so the body is unchanged; _convert_to_mlmodel calls it directly.

It has exactly one caller, _convert_to_mlmodel in the same file, so it
follows the class's underscore convention instead of reading as public
API on CoreMLBackend.
Review asked for a private free function in the file rather than a
static method on CoreMLBackend. The scan reads nothing from the class,
so it moves to module level unchanged.
@john-rocky

Copy link
Copy Markdown
ContributorAuthor

On the attempt-2 CI failures — none of them reaches this PR's code. android / build-android failed downloading the golden-artifacts snapshot: golden_artifacts_26052718.zip has aged out of S3, so the old curl -sL saved the 404 page and unzip failed; main already pins a fresh snapshot and uses curl -f. The three ARM jobs spent their full 120 minutes waiting for a docker image (executorch-ubuntu-24.04-arm-sdk-1d92118e…) that fork PRs cannot build or push, and were cancelled at the timeout. All four jobs are green on current main, and the free-function push re-runs everything against a fresh merge with main.

@metascroy

Copy link
Copy Markdown
Contributor

Looks good, thanks for the contribution @john-rocky!

@metascroy
metascroy merged commit 0447246 into pytorch:mainAug 28, 2026
197 of 201 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core ML coreml_quantize fails on tied embeddings: "compression config conflict detected between ops"

3 participants

@john-rocky@metascroy@nil-is-all