Repository navigation
Qualcomm AI Engine Direct - Keep node names unique across the QNN and Samsung passes - #24316
Conversation
shoumikhin
added
the
release notes: qualcomm
shoumikhin
added
the
release notes: qualcomm
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24316
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 3218fe7 with merge base 05092c1 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 19m 11s —— View job The reservation fix is correct for QNN's export pipeline, but it patches an upstream torch.export bug by writing private torch.fx state with no TODO, and both changed tests check less than they claim. Detailed reviewPR Review: #24316Summary
This review did not run the QNN tests (there is no QNN SDK on the runner). Claims about torch internals come from reading code, not from running it. Code Quality
Testing
RecommendationApprove Nothing blocks. The reservation fixes the reported export-pipeline failures. Add the upstream TODO before merging. The shared-helper move and the test hardening can come later. |
|
@haowhsu-quic @shewu-quic @winskuo-quic could you take a look? This changes the Qualcomm backend. |
3e91127 to
b84aa23
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 20m 42s —— View job The node name reservation fixes the reported Detailed reviewPR Review: #24316Summary
All findings are minor:
This review could not run Python or the QNN tests on this runner. Claims about torch internals come from reading code, not from running it. Code Quality
Testing
RecommendationApprove Nothing blocks: the reservation fixes the reported export failures. Link the upstream issue in the TODO before merging. The other changes can come later. |
b84aa23 to
e5f40e2
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 24m 36s —— View job The node name reservation fixes the reported Detailed reviewPR Review: #24316Summary
This review ran nothing. Python can't run on this runner and the torch source isn't available, so claims about torch behavior come from reading ExecuTorch code. Code Quality
Testing
RecommendationApprove Nothing blocks: the reservation fixes the reported failures, and CI is green. Link the upstream issue in the TODO before merging. The other changes can follow. |
psiddh
added
the
module: qnn
| @@ -100,6 +100,12 @@ | |||
| class QnnPassManager(PassManager): | |||
|
|
|||
| def _transform(self, graph_module: GraphModule): | |||
There was a problem hiding this comment.
transform_for_to_edge_pipeline doesn't call this, so the name reservation doesn't cover it. Should we call the same reservation there as well ? (in a follow up PR)
There was a problem hiding this comment.
Thanks for checking. I looked at this, and I think that pipeline is already safe. It always runs on an edge program, and to_edge rebuilds the graph with its own passes, so every node name is recorded again. I tested it with a model whose input is named "mul". Right after torch.export, the model's own mul node gets the name mul_1, and that name is not recorded. After to_edge, all names are recorded, and a new node named "mul" correctly becomes "mul_1". So I left it out, to keep the reservation only where it is needed.
shoumikhin
changed the title
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 30m 34s —— View job The node name changes look ready. But on HTP, the new constant-only exemption also covers the group-wise quantize/dequantize ops that QNN has to keep whole, so pre-quantized group-wise weights would likely be decomposed. That should be fixed before merging. Detailed reviewPR Review: #24316SummaryThe PR reserves every existing node name before the QNN and Samsung passes run, using a shared
This review ran no code. Python and the QNN SDK aren't available on this runner, and torch isn't installed. What it says about torch's decomposition table comes from ExecuTorch code, not torch source. Backward Compatibility
Code Quality
Testing
RecommendationRequest Changes On HTP, the constant-only exemption likely breaks models with pre-quantized group-wise weights. Fixing it takes a small change to the lambda and a test that runs in CI. |
|
Thanks, all three are done.
I could not find an upstream PyTorch issue for the export naming bug yet. |
|
Thanks, all three are addressed.
|
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 38m 37s —— View job Needs discussion: on HTP this PR decomposes constant-only kept-whole ops even when their partition stays delegated, and in quantized models such as the in-repo Swin-v2-T example the ops that replace a constant-only Detailed reviewPR Review: #24316SummaryThis PR records existing node names before the QNN and Samsung pass managers run, so passes no longer raise This review ran no code. Python, torch and the QNN SDK are not available on this runner, so what it says about HTP and torch behavior comes from reading the code. Claims that reading could not confirm are marked as suspected. Backward Compatibility
API Design
Testing
Code Quality
RecommendationNeeds Discussion The Backward Compatibility finding should be settled before merging. This PR decomposes constant-only ops in partitions that stay delegated, on purpose, and on quantized HTP models that probably moves them to CPU. Please either confirm the behavior on a quantized model with a constant-only |
torch.export does not record some of the names it assigns: the names of inputs, and the names of ops it renamed to make room for an input. With an input called mul, the model's own input * 2 becomes mul_1, and nothing marks mul_1 as taken. A later pass that creates a node can then pick one of those names again, and the graph fails with "Node redefined name t_1!". QNN models with autocast regions or attention hit it, and so does the Samsung DecomposeRoll pass on a model with an input named cat. reserve_node_names in backends/transforms/utils.py records the name of every node of the top-level graph before the passes run. The QNN and Samsung pass managers call it at the start of _transform. torch.fx has no public way to reserve a name: copying the graph would register the names, but it also renames inputs named after Python builtins (input becomes input_1) while the graph signature keeps the old names. So the helper adds the names to the graph's name set directly, with a TODO to drop it once torch.export records the names (pytorch/pytorch#202927). The node name test runs the QNN export passes on an autocast model and on an attention model whose inputs are named input and mul. It checks the graph lints, keeps the input names of the graph signature, and matches eager, and that the collision is real without the helper.
9b745c0 to
3218fe7
Compare
shoumikhin
changed the title
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 20m 58s —— View job Nothing blocks merging. There are two minor findings: Detailed reviewPR Review: #24316SummaryThe PR adds This review ran no code. Python can't run on this runner and torch isn't installed, so what it says about torch.fx comes from reading ExecuTorch code and the PR description. Code Quality
Testing
RecommendationApprove Nothing blocks. Both findings are minor and can be handled in this PR or a follow-up. |
… Samsung passes torch.export does not record some of the names it assigns: the names of inputs, and the names of ops it renamed to make room for an input. With an input called mul, the model's own input * 2 becomes mul_1, and nothing marks mul_1 as taken. A later pass that creates a node can then pick one of those names again, and the graph fails with "Node redefined name t_1!". QNN models with autocast regions or attention hit it, and so does the Samsung DecomposeRoll pass on a model with an input named cat. reserve_node_names in backends/transforms/utils.py records the name of every node of the top-level graph before the passes run. The QNN and Samsung pass managers call it at the start of _transform. torch.fx has no public way to reserve a name: copying the graph would register the names, but it also renames inputs named after Python builtins (input becomes input_1) while the graph signature keeps the old names. So the helper adds the names to the graph's name set directly, with a TODO to drop it once torch.export records the names (pytorch/pytorch#202927). The node name test runs the QNN export passes on an autocast model and on an attention model whose inputs are named input and mul. It checks the graph lints, keeps the input names of the graph signature, and matches eager, and that the collision is real without the helper. Same change as #24316 on main.
qti-horodnic
left a comment
qti-horodnic
left a comment
There was a problem hiding this comment.
Hi @shoumikhin, I see this pr has been merged already but it I reviewed it and lgtm. I left a few minor comments, feel free to take them up as part of the other prs you have open
| no_collision.append(type(model).__name__) | ||
| reserve.assert_called_once() | ||
| # A skip, not a failure: a torch update that removes the collision is fine. | ||
| if no_collision: |
There was a problem hiding this comment.
If only one case stops colliding, the whole test is skipped, but the other case still tests the helper. We can skip only when len(no_collision) == len(cases), or call skipTest inside each subTest.
| return FakeTensorConverter().from_real_tensor(example_fake_tensor.fake_mode, t=data) | ||
|
|
||
|
|
||
| def reserve_node_names(graph_module: torch.fx.GraphModule) -> None: |
There was a problem hiding this comment.
Can we add a torch-only unit test in backends/transforms/test/? E.g., export forward(self, x, mul): return x * 2 + mul, call reserve_node_names, add a node with create_node(..., name="mul_1"), and call graph.lint(). This will also cover the Samsung path, which has no test
| get_qnn_pass_manager_cls()().transform_for_export_pipeline( | ||
| ep | ||
| ).graph.lint() | ||
| except Exception as e: |
There was a problem hiding this comment.
If a pass fails for a different reason, this shows as an assertIn failure, and we lose the original traceback. I suggest doing if "Node redefined name" not in str(e.__cause__ or e): raise instead
|
Thanks for the review! All 3 are addressed in #24429:
|
What is wrong today
torch.export does not record some of the node names it assigns: the names of inputs, and the names of ops it renamed to make room for an input. With an input called
mul, the model's owninput * 2becomesmul_1, and nothing marksmul_1as taken. A later pass that adds a node can pick one of those names again, and the graph breaks:QNN models with autocast regions or attention hit this in the QNN export passes. The Samsung passes hit it too: with an input named
cat,torch.catfollowed bytorch.rollfails inDecomposeRoll.What this change does
reserve_node_namesinbackends/transforms/utils.pyrecords the name of every node in the top-level graph, so nodes added later get names that are not taken. The QNN and Samsung pass managers call it at the start of_transform.torch.fx has no public way to reserve a name, so the helper adds the names to the graph's name set directly. Copying the graph would register the names through public API, but it also renames inputs named after Python builtins (
inputbecomesinput_1) while the graph signature keeps the old names. The real fix belongs in torch.export (pytorch/pytorch#202927), and a TODO says to drop the helper once that lands.Tests
test_export_pipeline_keeps_node_names_uniqueruns the QNN export passes on an autocast model and on an attention model whose inputs are namedinputandmul. It checks that the graph lints, keeps the input names the graph signature uses, and matches eager, and that the collision really happens without the helper. If a torch update removes the collision, the test skips with a pointer to the torch issue instead of failing.On Linux x86_64 with torch 2.14 and a 2.16 nightly: the test fails with
Node redefined name t_1!andNode redefined name mul_1!when the call is removed, and it fails on theinputcase when the helper copies the graph instead. The Samsung case above fails without the helper and passes with it (torch only).test_passes.pypasses, except two tests that need torchtune or the quantized CPU kernels.Earlier versions of this PR also carried an HTP change that decomposes constant-only ops left on the CPU, and an RMSNorm dtype fix. Both now have their own PRs:
RMSNorm dtype fix: #24428
HTP constant-only decomposition: #24429
cc @cbilgin @psiddh