Stop reading a backend config field that no longer exists - #22298
Open
shoumikhin wants to merge 3 commits into
Open
Stop reading a backend config field that no longer exists#22298shoumikhin wants to merge 3 commits into
shoumikhin wants to merge 3 commits into
Conversation
Exporting llama for Core ML fails before it starts:
AttributeError: 'BackendConfig' object has no attribute 'mps'
`BackendConfig` lost its `mps` field when the MPS backend was removed, but the Core ML branch in
`export_llama_lib` still reads `llm_config.backend.mps.enabled`. That read sits inside the
condition guarding the branch, so it is evaluated on every Core ML export rather than only when
MPS was requested, which is why the whole path is dead rather than just the MPS part of it.
This is what fails `test-llama-runner-mac (fp32, coreml)` and
`test-huggingface-transformers-macos (llama3.2-1b|coreml_fp32_gpu)`.
Test plan:
The removed term could only ever be False, so no outcome changes. Enumerated all eight
combinations of the three remaining flags against the old expression with the field forced False:
identical results, zero mismatches.
Against an installed wheel, the old expression raises the AttributeError above and the new one
returns True for a Core ML export, which is the branch it should take.
Confirmed no other source file reads `backend.mps` or `MpsConfig`.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22298
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 3 PendingAs of commit 7d3ee15 with merge base 9b558d9 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
shoumikhin
added
the
release notes: examples
Review pointed out the fix had no test, and that the macOS Core ML llama job which would catch this
does not run on a pull request touching only this file: it lives in the trunk workflow, whose
pull_request paths cover the CI scripts, the pytorch pin and the zephyr tree, none of which this
change touches. So nothing here would notice the line returning.
The test enables Core ML, patches the Core ML lowering to raise a marker, and asserts control
reaches it. No macOS and no coremltools: what matters is that the guard no longer stops the branch
before it starts. `enable_dynamic_shape` is False because the Core ML recipe rejects dynamic shapes
and that validation runs first.
Test plan:
Against the installed wheel, swapping only `export_llama_lib.py`:
base FAILED, AttributeError: 'BackendConfig' object has no attribute 'mps', line 1809
head 1 passed in 6.66s
It runs in the `unittest` jobs, which do run on this pull request.
This was referenced Aug 29, 2026
The first test pinned only one of the two cases the fix repaired, and it stayed green when the whole
exclusion clause was deleted, which silently drops the Vulkan and QNN partitioners. So it guarded
the removed field read and not the routing the clause exists for.
Core ML with QNN is now covered too. It must fall through to the combined lowering, which still
lowers Core ML but keeps the QNN partitioner. That case raised before the fix and it fails on the
mutant, so between them the two tests pin both halves.
Both now use `side_effect` with `assert_called_once`, which is how the rest of the tree writes this,
instead of a local marker exception and a stub.
Also removed the paragraph claiming the macOS Core ML job does not run for a change to this file.
A trunk tag ran it on this branch and it passed, so the claim was wrong, and a comment inside a test
that describes when a workflow starts cannot be checked from the test and goes stale on its own.
Test plan:
both tests, fixed pass
stale field restored the Core ML case fails with the AttributeError
exclusion clause deleted the QNN case fails
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Exporting llama for Core ML fails before it starts:
BackendConfiglost itsmpsfield when the MPS backend was removed, but the Core ML branch inexport_llama_libstill readsllm_config.backend.mps.enabled:Scope, stated precisely:
orshort-circuits, so this raises only when Vulkan is off. Core ML aloneand Core ML with QNN raise. Core ML with Vulkan does not, because the first term is already true.
This is what fails
test-llama-runner-mac (fp32, coreml), which is red onmain.How it got in
The MPS removal cleaned this file completely: four
backend.mpsreads before, zero after. The readcame back in the change that moved Core ML to
to_edge_transform_and_lowerand added the dedicatedCore ML lowering helper. That change was written against an older
mainand merged after theremoval, so this is stale-branch merge skew rather than a missed spot.
That also means the dedicated Core ML lowering has never had a green run, because the helper and
the broken guard arrived together. This change is the first time it can execute. It lowers through
to_edge_transform_and_lowerwhere the previous combined path usedexport_to_edgeplusto_backend, which keeps whole the ops Core ML implements itself, so the delegated graph changestoo. Expect the first real run of that job to find more than this.
What changes behaviour
The removed term never evaluated to
False. It raised, which is the bug. So one outcome doeschange: Core ML with QNN used to raise and now runs the combined lowering, which still lowers Core
ML while keeping the QNN partitioner.
Test plan
Two tests, one per repaired route:
AttributeErrorAttributeErrorThe QNN case matters on its own: without it, deleting the whole exclusion clause leaves the suite
green while silently dropping the Vulkan and QNN partitioners. I checked that mutation, and the QNN
test is what catches it.
The full macOS Core ML jobs ran on this branch through a
ciflow/trunktag and both passed,including
test-llama-runner-mac (fp32, coreml), which fails at the merge base. Worth knowing thatthis job does not start from the pull request path filter alone; the tag route is what runs it.
Follow-ups, not fixed here
.lintrunner.tomlhasexamples/**/*.pycommented out of the mypy scope. The pinned mypy flagsexactly this bug, so putting the directory in scope would have caught it in seconds, but that file
alone reports hundreds of pre-existing errors, so it is a much larger change.