Uh oh!
There was an error while loading. Please reload this page.
Stop reading a backend config field that no longer exists - #22298
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. ❌ 3 New Failures, 21 Pending, 1 Unrelated FailureAs of commit a589a92 with merge base beb5b16 ( NEW FAILURES - The following jobs have failed:
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. |
shoumikhin
commented
Aug 31, 2026
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 |
067796f to
a766497Compareshoumikhin
commented
Aug 31, 2026
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. |
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.
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.
8cb4cf5 to
a589a92CompareUh oh!
There was an error while loading. Please reload this page.
Summary
The code fix this pull request opened with has since landed on main separately, so what is left here is
the test that keeps it from coming back.
The bug: exporting llama for Core ML failed before it started, because the branch selecting the Core ML
lowering read a backend config field that no longer existed.
orshort-circuits, so this raised only when Vulkan was off. Core ML alone and Core ML with QNNraised; Core ML with Vulkan did not, because the first term was already true.
What is in this change now
Three tests, one per route through that branch, with the lowering patched so they need no macOS and no
coremltools:
The second and third cases matter on their own. The exclusion clause exists to keep those two
partitioners, and the Core ML lowering takes neither as an argument, so routing there would drop them
in silence. Both cases assert the backend flags actually reach the shared lowering, since a call count
cannot see them.
The mutation matrix is complete:
The model preparation is stubbed, because routing is decided before the model is touched. That takes
the three cases from about 4.3 GB of peak memory to 420 MB. The KV cache is pinned off with a comment
saying why: with it on, the source transforms import the Qualcomm SDK, which routing does not need.
Test plan
All three pass against main as it stands now, and each fails on the mutation named above.