Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23027
Note: Links to docs will display an error until the docs builds have been completed. ❌ 130 New Failures, 2 Unrelated Failures, 7 Unclassified FailuresAs of commit 4033b26 with merge base ed72896 ( NEW FAILURES - The following jobs have failed:
UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:
FLAKY - The following jobs failed but were 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
|
|
This PR needs a
|
psiddh
added
the
module: qnn
psiddh
marked this pull request as ready for review
September 29, 2026 18:35
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The centralized cleanup correctly preserves aggregate outputs while preventing unsupported observers, with focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Centralizes QNN quantization cleanup so integer and boolean activations never receive observers.
Changes:
- Removes non-floating-point input and output quantization specs after annotation.
- Adds an end-to-end
slice_scatterregression test.
| File | Description |
|---|---|
backends/qualcomm/quantizer/rules.py |
Adds non-float annotation cleanup. |
backends/qualcomm/quantizer/quantizer.py |
Runs cleanup after annotation. |
backends/qualcomm/tests/test_passes.py |
Tests annotation and conversion behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Observers only work with float tensors, so a quantization spec on an integer or boolean activation makes convert_pt2e emit a quantize_per_tensor whose dtype assert fires on the following export. Annotators guard the operands they look at - Cat, Stack, Embedding, Where and IndexPut each carry their own check - but annotate_single_in, Chunk, IndexCopy and SliceScatter annotate whatever they are given, so an int64 activation still reaches an observer. slice_scatter over an int64 cache index, the shape HuggingFace's cache_position update takes, is the case that surfaced this. Enforce the invariant once after annotation instead of adding a fifth per-op guard. Note _is_float_tensor cannot simply be negated for this: it also returns False for the list and tuple outputs of native_layer_norm and max.dim, which are legitimately annotated, so the sweep tests the dtype of single tensor values only and leaves every float annotation untouched. Covered by a unittest and by a test in the rework framework; both fail before the change with an output_qspec on the int64 slice_scatter.
psiddh
force-pushed
the
qnn-nonfloat-activation-guard
branch
from
September 29, 2026 22:08
7000c6d to
4033b26
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The centralized cleanup preserves structured outputs while removing unsupported non-floating tensor qspecs, with regression coverage across both quantizer test suites.
Review effort: Balanced
Findings: None
This branch has not been deployed
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.
Observers only work with float tensors, so a quantization spec on an integer or boolean activation makes convert_pt2e emit a quantize_per_tensor whose dtype assert fires on the following export. Annotators guard the operands they look at - Cat, Stack, Embedding, Where and IndexPut each carry their own check - but annotate_single_in, Chunk, IndexCopy and SliceScatter annotate whatever they are given, so an int64 activation still reaches an observer. slice_scatter over an int64 cache index, the shape HuggingFace's cache_position update takes, is the case that surfaced this.
Enforce the invariant once after annotation instead of adding a fifth per-op guard. Note _is_float_tensor cannot simply be negated for this: it also returns False for the list and tuple outputs of native_layer_norm and max.dim, which are legitimately annotated, so the sweep tests the dtype of single tensor values only and leaves every float annotation untouched.
cc @cbilgin