Skip to content

Add regression test for unlifted get_attr constants - #22022

Open
psiddh wants to merge 1 commit into
pytorch:mainfrom
psiddh:export-D116963247
Open

Add regression test for unlifted get_attr constants#22022
psiddh wants to merge 1 commit into
pytorch:mainfrom
psiddh:export-D116963247

Conversation

@psiddh

@psiddh psiddh commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary:
is_parameter() / get_parameter() in backends/qualcomm/builders/utils.py only
recognized lifted constants (is_param / is_buffer / is_lifted_tensor_constant).
An UNLIFTED get_attr node -- a tensor a pass attached with setattr and referenced
via a plain get_attr, present in neither state_dict nor constants nor the graph
signature -- fell through, so get_parameter() tripped its assert param is not None
and raised Expect <name> to be parameter, buffer, or lifted tensor constant.

#22020 fixed that by adding a node.op == "get_attr" branch to both functions.
OSS CI never caught the original break because every failing path was internal-only
(the LPAI LpaiPartitionFallbackSupport pass and the internal
examples/models/fb/llama4/... exports); the OSS [htp] path lifts its constants, so
it never produces an unlifted get_attr.

This adds backends/qualcomm/tests/test_builders_utils.py, a backend-agnostic,
pure-fx-graph unit test that closes that coverage gap. It builds an ExportedProgram
whose graph carries an unlifted get_attr tensor constant and asserts is_parameter()
returns True, get_parameter() returns the tensor cast to node.meta["val"].dtype, and
that the lookup falls back to edge_program.graph_module when
node.graph.owning_module cannot resolve the target. No QNN SDK, no device, no LPAI
backend, no internal models -- it runs anywhere torch.export runs, including OSS CI.

Note on ordering: this diff contains only the new test, no utils.py change. It depends
on #22020 and will fail until that lands.

Differential Revision: D116963247

Summary:
`is_parameter()` / `get_parameter()` in `backends/qualcomm/builders/utils.py` only
recognized lifted constants (`is_param` / `is_buffer` / `is_lifted_tensor_constant`).
An UNLIFTED `get_attr` node -- a tensor a pass attached with `setattr` and referenced
via a plain `get_attr`, present in neither `state_dict` nor `constants` nor the graph
signature -- fell through, so `get_parameter()` tripped its `assert param is not None`
and raised `Expect <name> to be parameter, buffer, or lifted tensor constant`.

D116957343 fixed that by adding a `node.op == "get_attr"` branch to both functions.
OSS CI never caught the original break because every failing path was internal-only
(the LPAI `LpaiPartitionFallbackSupport` pass and the internal
`examples/models/fb/llama4/...` exports); the OSS `[htp]` path lifts its constants, so
it never produces an unlifted `get_attr`.

This adds `backends/qualcomm/tests/test_builders_utils.py`, a backend-agnostic,
pure-fx-graph unit test that closes that coverage gap. It builds an `ExportedProgram`
whose graph carries an unlifted `get_attr` tensor constant and asserts `is_parameter()`
returns True, `get_parameter()` returns the tensor cast to `node.meta["val"].dtype`, and
that the lookup falls back to `edge_program.graph_module` when
`node.graph.owning_module` cannot resolve the target. No QNN SDK, no device, no LPAI
backend, no internal models -- it runs anywhere `torch.export` runs, including OSS CI.

Note on ordering: this diff contains only the new test, no `utils.py` change. It depends
on D116957343 (land enqueued at the time of writing) and will fail until that lands.

Differential Revision: D116963247
Copilot AI lite review requested due to automatic review settings August 21, 2026 17:58
@pytorch-bot

pytorch-bot Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22022

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures, 6 Unrelated Failures

As of commit 2cf17e1 with merge base 89e6f90 (image):

NEW FAILURES - The following jobs have failed:

  • Build Presets / apple (ios) / build (gh)
    ##[error]fatal: unable to access 'https://github.com/pybind/pybind11.git/': Failed to connect to github.com port 443 after 7979 ms: Couldn't connect to server
  • Cadence Build & Test / hifi-build / hifi4 (gh)
    ##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
  • Cadence Build & Test / vision-build / vision (gh)
    ##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 21, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@meta-codesync

meta-codesync Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@psiddh has exported this pull request. If you are a Meta employee, you can view the originating Diff in D116963247.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@qti-horodnic qti-horodnic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Sid. Should we add test_builders_utils.py to the CI? I see only test_passes.py is currently there.

On a similar note, we should ideally have testing specifically for these util methods, I'll discuss that internally with the team.

@psiddh

psiddh commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

LGTM, thanks Sid. Should we add test_builders_utils.py to the CI? I see only test_passes.py is currently there.

On a similar note, we should ideally have testing specifically for these util methods, I'll discuss that internally with the team.

Thanks for bringing this up. I was thinking on the similar lines. Please discuss and let us know how we can increase the OSS CI coverage further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants