Qualcomm AI Engine Direct - Add QNN HadamardTransform op support - #21729
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21729
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
|
|
||
| import numpy as np | ||
|
|
||
| import scipy.linalg |
There was a problem hiding this comment.
make it lazy , move the import inside _match_hadamard_weight) so merely importing the pass package doesn't require it ?
Could we drop scipy here and reuse the _hadamard_matrix from
builders/custom_ops.py? It's not in setup.py/requirements and _passes/init.py imports this eagerly, so every QNN export would need scipy or hit ModuleNotFoundError. am I reading that right?
|
I have one comment / clarification, but o/w lgtm |
The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. pytorch#21729 failed exactly that way, on eight links pytorch#21694 had already fixed or ignored. The workflow now resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way, and the checkouts fetch real history, without which no merge base exists to compute. Authored with Claude Code (Claude Opus 5).
Thanks for review, will update. |
8fe3323 to
87b5243
Compare
|
@chenweng-quic Linters fixes are needed now |
The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. pytorch#21729 failed exactly that way, on nine links pytorch#21694 had already fixed or ignored. The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way. Computing a merge base needs history, so the pull request path checks out in full, restoring what pytorch#17682 traded away for speed at a measured 25s of wall clock. Pushes and the nightly whole tree scan cannot use a merge base and stay shallow. A missing merge base leaves the range unset for a whole tree scan rather than substituting the base tip, which two of the three scripts accept and then quietly pass. Authored with Claude Code (Claude Opus 5).
The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. pytorch#21729 failed exactly that way, on nine links pytorch#21694 had already fixed or ignored. The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way. Computing a merge base needs history, so the pull request path checks out in full, restoring what pytorch#17682 traded away for speed at a measured 25s of wall clock. Pushes and the nightly whole tree scan cannot use a merge base and stay shallow. A missing merge base leaves the range unset for a whole tree scan rather than substituting the base tip, which two of the three scripts accept and then quietly pass. Authored with Claude Code (Claude Opus 5).
…21765) ### Summary The URL, xref, and file-size linters diffed `base..head`, where `base` is the **tip** of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions, and the branch gets blamed for links someone else already repaired. #21729 failed exactly this way. It touches 18 files and adds no URLs at all, but the two-dot diff scoped the lint to 143 files and flagged nine links that #21694 had already fixed or `@lint-ignore`d. #21707 is starker: one Python file with no URLs in it, 199 files linted, the same nine failures. ``` base..head 143 files <- what CI linted base...head 18 files <- what the PR actually changes ``` The stray `jq: parse error` lines in #21729's log are the same symptom from the other direction: the job runs the branch's own pre-#21694 copy of `lint_urls.sh`. ### Fix The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a `pull_request` the reusable workflow resolves from the merge commit, so it carries this fix even though `scripts/` still comes from the branch itself. The scripts also switch to three-dot ranges so `./scripts/lint_urls.sh main HEAD` by hand behaves the same. `lint_xrefs.sh` and `lint_file_size.sh` had the identical bug and get the identical change. **Cost, stated plainly.** A merge base needs real history, so this restores `fetch-depth: 0` on the `pull_request` path — line for line what #17682 removed in February for speed. Measured on this branch, that is ~25s of added wall clock and ~79s of runner time across the three concurrent jobs. #17682's 6min → 10s was mostly the runner and Docker change rather than the fetch depth, though the commit changes both at once and I can't fully separate them. Pushes and the nightly whole-tree scan cannot use a merge base and stay shallow: ```yaml fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }} ``` The quotes matter — bare `0` is falsy in GitHub expressions, so `&& 0 || 1` always yields `1` and would silently disable the fix. **No silent fallback.** When there is no merge base there is no usable range, so the range is left unset and the linters scan the whole tree. Substituting the base tip instead produces a range the scripts fail on quietly: verified against unrelated histories, `lint_urls.sh` and `lint_xrefs.sh` both exit **0** having checked nothing, while `lint_file_size.sh` exits 128 and the wrapper reports "some files exceed the 1 MB limit", which is not what happened. Unset args are loud instead — verified rc=1 on a tree containing a dead link. **`--no-color`.** Both `git diff` calls now pass it, matching the `git grep --no-color` two lines below. With `color.ui = always` in a developer's config, added lines arrive wrapped in escape sequences, `grep -E '^\+'` matches nothing, and the check passes having found nothing (measured: 2 matches → 0). ### Test plan `.ci/scripts/tests/test_link_check_diff_selection.py` builds a diverged history where `main` repairs bad links and shrinks an oversized file while the feature branch simply predates all of it. Four fixture files each pin a different part, and `curl` is stubbed so there is no network: | Fixture | Pins | |---|---| | `both_sides.md` — edited on both branches | the per-file diff range | | `big.bin` — oversized at the branch point, shrunk on main | `lint_file_size.sh`'s range | | `colorful.gitconfig` — `color.ui = always` | `--no-color` | | `base_only.md` / `feature_only.md` | that main-only changes stay invisible and the branch's own additions are still checked | ``` pytest .ci/scripts/tests/test_link_check_diff_selection.py # 4 passed ``` Mutating each changed line individually: ``` inner per-file range -> .. 3 failed caught lint_file_size range -> .. 1 failed caught drop --no-color 1 failed caught file-selection range -> .. 4 passed equivalent mutant, see below ``` The file-selection range is not pinned because it cannot be: with the per-file diff at three dots, the extra files it selects produce empty diffs. Replayed against #21707's real 199-file range, both variants emit byte-identical output. It is changed for consistency, not behaviour. **Narrowing the scope must not blunt the check**, so each linter also has a positive control where the branch itself adds the bad thing: ``` lint_urls adds a dead URL -> rc=1, reports example.invalid/dead lint_xrefs adds a broken reference -> rc=1, reports sub/missing.md lint_file_size adds a 1MB+ file -> rc=1, reports feature_big.bin ``` End to end on real content: a synthetic commit on top of `main` that puts #21694's nine repaired links back, as if a PR had added them, gives `rc=1` with 9 FAIL and 5 OK against the live network. Incidentally `musl.cc` answered this time where CI saw `000`, and `pybind/cmake_example` now 404s where CI saw `301` — which is the retry-then-WARN path earning its keep. Also replayed #21729's and #21707's exact CI refs through the fixed scripts (exit 0 each), and ran all three linters plus `lintrunner` against this PR's own diff. ### Known limitations - The checkout is `head.sha`, so references still resolve against the branch tree rather than the merge result — the mirror image of the bug fixed here. Pre-existing and not worsened by this PR. - The whole-tree branch swallows a `git grep` failure and exits 0, so on a git built without PCRE the scan silently checks nothing. Reachable locally, not on the runners, which is why the nightly scan works. Pre-existing; worth a follow-up rather than widening this PR. ### Rollout This does **not** repair a currently red run. A rerun keeps the original `GITHUB_SHA`, and advancing the base alone does not fire `synchronize`, so an already-open PR picks the fix up only on its next newly triggered pull-request run — any push, or close/reopen. That is still cheaper than a content rebase: no history rewrite, no conflicts, nothing to re-review. Supersedes #21762, which fixes the same bug but leaves the checkout shallow and misses `lint_urls.sh`. Its reviewer's shallow-checkout point is exactly right: a `--depth=1` fetch writes a shallow graft even into an otherwise complete clone, after which `git merge-base` fails and `A...B` is fatal, so the two halves of this change are a pair. The regression test here is adapted from that PR. Authored with Claude Code (Claude Opus 5).
|
@chenweng-quic can you resolve the conflict ? |
Lower a bias-less linear / matmul / pointwise 1x1 conv whose constant weight is a scaled Hadamard matrix into QNN's native HadamardTransform op. A torch.ops.qnn_custom.hadamard_transform op is registered, and a pre-quantization RecomposeHadamard pass detects the matching nodes (via the shared match_hadamard_weight matcher in builders/utils.py) and rewrites them into the custom op. Since hadamard_transform operates on the last dim, linear and matmul rewrite directly, while conv is wrapped in permutes that move the channel dim to the last dim and back. A dedicated htp_rules.py annotator and op_hadamard.py builder handle quantization and lowering. The rewrite is gated on QNN SDK >= 2.47 for backward compatibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
_passes/__init__.py imports RecomposeHadamard eagerly, so the module-level `import scipy.linalg` would raise ModuleNotFoundError for every QNN export on hosts without scipy (it isn't a project dependency). Reuse the existing torch-based `_hadamard_matrix` Sylvester construction from builders/custom_ops.py instead, and do the same in the test files that had duplicated the scipy-based construction to build reference Hadamard weights. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
032ec43 to
a356d89
Compare
Summary: PR pytorch#21729 added HadamardTransform op support, gated on a qnn sdk version. The version check assumed QNN_SDK_ROOT was available, resulting in errors when it is not. This PR matches other usage like op_embedding.py to fallback when SDK is absent. Reviewed By: rascani Differential Revision: D115881389
Summary
Add support for the QNN
HadamardTransformop for the HTP backend.When a constant weight equals a scaled Hadamard matrix (
scipy.linalg.hadamard(dim) * s), the op is lowered to a single QNNHadamardTransform(optionally scaled) instead of the generic op.A first-class custom op
torch.ops.qnn_custom.hadamard_transformis registered, and a pre-quantizationRecomposeHadamardpass detects the matching nodes and rewrites them into the custom op, so it is annotated and lowered as aHadamardTransformfrom the start rather than being detected late in the builder and validated asFullyConnected/MatMul/Conv. A shared matcher (match_hadamard_weightinbuilders/utils.py) inspects the real constant weight during the annotation pass. Three patterns are covered:aten.linear.x @ W, matching the constant right-hand operand (last-dim transform); this is the R3-rotation pattern used by static LLaMA.groups=1, which mixes only the channel dim.Since
hadamard_transformoperates on the last dim, linear and matmul rewrite directly, while conv is wrapped in permutes that move the channel dim to the last dim and back. A dedicatedhtp_rules.pyannotator andop_hadamard.pybuilder handle quantization and lowering. The rewrite is gated on QNN SDK >= 2.47 for backward compatibility.Test plan
The tests assert that the graph is rewritten to
qnn_custom.hadamard_transformbefore quantization, and inspect the optrace QHAS op types to assertHadamardTransformappears after lowering.Performance
CTX_LEN = 1024
AR_LEN = 128