Stop reading a backend config field that no longer exists - #22298
Stop reading a backend config field that no longer exists#22298shoumikhin wants to merge 5 commits into
Conversation
🔗 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. ⏳ 7 Pending, 1 Unrelated FailureAs of commit 8cb4cf5 with merge base 9b558d9 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
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
|
Good catch on the coverage gap. Fixed and pushed. The tests pinned only the QNN half of the clause. I confirmed your mutation: dropping just the Vulkan You were also right that the helper hid the Core ML flag. Deleting that line left the test named for The bare Both combined cases now assert the backend flags reach the shared lowering, since a call count cannot On the commit messages: agreed and they are rewritten. The transformers macOS job claim was wrong, Two I am leaving: the per-test export cost, and the note that the Core ML |
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 still
reads `llm_config.backend.mps.enabled` while deciding whether Core ML can be lowered on its own. The
read happens whether or not 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)`.
Test plan:
The removed term never evaluated to False. Reading a field that is not there raises, so two routes
change from crashing to running: Core ML alone, and Core ML with QNN. Core ML with Vulkan is
unchanged, because the first term short-circuits before the bad read.
Enumerated all eight combinations of the three remaining flags with the field actually raising:
exactly those two differ, and no combination that worked before changes.
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.
The fix had no test, so nothing stopped the field read returning.
Patching the lowering to raise a marker keeps this off macOS and away from coremltools: what is
asserted is that control reaches the lowering at all, which is exactly what the stale read prevented.
Test plan:
with the fix reaches the Core ML lowering
without it raises AttributeError before any lowering
The first test pinned only one of the two routes 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 a patched lowering with an assertion on the call, which is how the rest of the tree
writes this, instead of a local marker exception and a stub.
Also removed a paragraph from the test docstring about when the macOS Core ML job runs. A comment
inside a test cannot be checked from the test and goes stale on its own, and the workflow's own path
filter does not cover this directory, so the sentence was misleading either way.
Test plan:
both tests, fixed pass
stale field restored the Core ML case fails with the AttributeError
exclusion clause deleted the QNN case fails
Review found the tests pinned only the QNN half. Dropping just the Vulkan term left both green, and
that mutation sends a Core ML plus Vulkan export to the Core ML lowering, which takes no Vulkan
argument, so the partitioner is dropped in silence. A third case covers it, and the mutation matrix
is now complete:
drop vulkan term the Vulkan case fails
drop qnn term the QNN case fails
whole clause deleted both combined cases fail
stale field restored both Core ML cases fail
The helper also turned Core ML on inside itself, so neither call site showed which backends its case
used, and deleting that hidden line left the test named for Core ML with QNN passing. Backends are
now named at each call.
Three smaller corrections that came with it. The helper resolved the backend field from a string
through getattr, which is the same unchecked lookup this change removes and is against the house
rule on dynamic attribute access. The patch target was built from an f-string, the only one in the
tree, now patch.object. And a bare RuntimeError was the marker, which swallowed real RuntimeErrors
from inside the export and reported only that the lowering was never called; a private marker class
shows the real cause instead.
Both combined cases now also assert the backend flags reach the shared lowering, since a call count
alone cannot see them and the exclusion clause exists to preserve exactly those partitioners.
067796f to
a766497
Compare
Each routing case traced and lowered the whole model to check which branch it takes. Stubbing the
model preparation, which the routing decision does not depend on, takes peak memory from about
4.3 GB to 420 MB and the three cases from 7.1 to 3.5 seconds of processor time.
The mutation matrix is unchanged, so the cases still pin what they did before:
drop vulkan term the Vulkan case fails
drop qnn term the QNN case fails
whole clause deleted both combined cases fail
stale field restored both Core ML cases fail
Also pinned the KV cache off. With it on, the source transforms import the Qualcomm SDK, which
routing does not need and which is absent on most machines, so the Core ML with QNN case only avoided
it through a default nothing stated.
|
Second pass, all four addressed. The commit messages are actually rewritten now. I said last time they were, and they were not, so The tests no longer export a model to check a branch. Routing is decided before the model is The KV cache is pinned off, with a line saying why. Your finding was right: with it on the source The two unread Core ML options are a real bug and I filed it separately. |
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.