-
Notifications
You must be signed in to change notification settings - Fork 744
Fix build_android_library.sh #13695
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
Fix build_android_library.sh #13695
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13695
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit d9b55d4 with merge base cac1a71 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
|
@kirklandsign @kimishpatel @cccclai why we didn't our CI catch this error? Is it something we can add regression test in our CI? |
|
actually this one is probably not needed. I erroneously thought this lib is needed. Issue is the qnn delegate tries to load this library in "some" cases. Those cases are not valid for us, so I am not sure why that loading path exist to begin with. Maybe @haowhsu-quic knows |
actually this one, https://github.com/pytorch/executorch/blob/main/examples/qualcomm/utils.py#L146, is doing exactly this thing |
|
I'm still not sure why it's needed...when I just compile the binary and use adb, it seems fine without this .so. It's good to have for jit use cases anyway (most users don't use this flow) |
2b21b80 to
97ce90d
Compare
|
seems like the branch history is super messed up |
wow ok will fix |
Qnn backend requires libQnnDlc. Package that in the aar file
97ce90d to
d9b55d4
Compare
|
@kirklandsign so now this pr adds the lines that you removed earlier. I think for aar build for maven we dont need to include these in executorch.aar, but anyone relying on building aar locally needs these. what do you wanna do? |
Thank you. I have https://github.com/pytorch/executorch/pull/13912/files to update the tutorial. |
We are building and testing the LlamaDemo app with XNNPACK AAR package only. We didn't build QNN AAR CI. Currently we are using emulator to run CI and QNN won't run. |
kirklandsign
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.
If we copy these files to AAR, and the user continues to use com.qualcomm.qti:qnn-runtime, it will cause duplicated .so right?
kirklandsign
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.
So for local build, we need to add com.qualcomm.qti:qnn-runtime manually, per use case. See https://github.com/pytorch/executorch/pull/13912/files
yeah thats better. abandoning this one |
Summary
Qnn backend requires libQnnDlc. Package that in the aar file
Test plan
CI
Plus local testing for llamademo app revealed that without this change it throws error