-
Notifications
You must be signed in to change notification settings - Fork 736
Move QNN SDK into backends/qualcomm/CMakeLists.txt #15546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15546
Note: Links to docs will display an error until the docs builds have been completed. ❌ 8 New Failures, 3 Unrelated FailuresAs of commit 1acb151 with merge base 3826f44 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but was 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. |
This PR needs a
|
|
Thanks! I added the binary ci job to make sure the wheel package publishing path is still working |
|
Hi @haowhsu-quic, can you review the changes inside |
backends/qualcomm/CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| if(NOT DEFINED QNN_SDK_ROOT) | ||
| if(_qnn_host_is_linux_x86) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure I understand the reason for this condition, as both Android and x86 builds require the QNN SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for pip wheels only. Like Chen said, maybe I should condition on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shewu-quic can you give some advice on how to move forward? The purpose of this PR is quite simple: to move the qnn build logic from setup.py to backends/qualcomm/CMakeLists.txt. Notice that the logic in setup.py is unique to wheel building and I'm trying to wrap my head on why building the runtime doesn't require downloading the sdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding, you want to include libqnn_executorch_backend.so and the pybind files in the wheel build so users can install via pip without needing to set QNN_SDK_ROOT or download qnn sdk.
We could add a step to download the QNN SDK if QNN_SDK_ROOT is not set, similar to your approach, and use "if is_linux_x86():" in setup.py to avoid installation on unsupported systems for now. Does this approach work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the reply, I think that should work. Let me sync with @cccclai to have more understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to gate this change by EXECUTORCH_BUILD_PYBIND. After talking to @cccclai I think we agree that this code path should not be used by cases other than building ET wheel.
backends/qualcomm/CMakeLists.txt
Outdated
| set(_qnn_host_is_linux_x86 TRUE) | ||
| endif() | ||
|
|
||
| if(NOT DEFINED QNN_SDK_ROOT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic below is for wheel only, so probably better to have a separate env variable to control
209f881 to
eec27d3
Compare
b390043 to
5bb8f91
Compare
shewu-quic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
cccclai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the changes
5bb8f91 to
1acb151
Compare
Summary
[PLEASE REMOVE] See CONTRIBUTING.md's Pull Requests for ExecuTorch PR guidelines.
[PLEASE REMOVE] If this PR closes an issue, please add a
Fixes #<issue-id>line.[PLEASE REMOVE] If this PR introduces a fix or feature that should be the upcoming release notes, please add a "Release notes: " label. For a list of available release notes labels, check out CONTRIBUTING.md's Pull Requests.
Test plan
[PLEASE REMOVE] How did you test this PR? Please write down any manual commands you used and note down tests that you have written if applicable.