-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[QNN EP] Enable v81 devices #26341
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
base: main
Are you sure you want to change the base?
[QNN EP] Enable v81 devices #26341
Conversation
* Include v81 stub/skel/cat files in build artifacts. * Disable tests that are broken on v79 and v81.
copy $(Build.BinariesDirectory)\${{parameters.buildConfig}}\${{parameters.buildConfig}}\QnnHtpPrepare.dll $(Build.BinariesDirectory)\${{parameters.artifactName}}\lib | ||
copy $(Build.BinariesDirectory)\${{parameters.buildConfig}}\${{parameters.buildConfig}}\QnnHtpV68Stub.dll $(Build.BinariesDirectory)\${{parameters.artifactName}}\lib | ||
copy $(Build.BinariesDirectory)\${{parameters.buildConfig}}\${{parameters.buildConfig}}\QnnHtpV73Stub.dll $(Build.BinariesDirectory)\${{parameters.artifactName}}\lib | ||
copy $(Build.BinariesDirectory)\${{parameters.buildConfig}}\${{parameters.buildConfig}}\QnnHtpV81Stub.dll $(Build.BinariesDirectory)\${{parameters.artifactName}}\lib |
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.
From here to the end, I'm unable to test this change locally so please look closely. I found these files via vigorous git grep
and hunting through history. Unfortunately, it was a bit murky due to refactors and no analogous change adding a new architecture version (v68 and v73 were added together).
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.
how should we test these changes?
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 am not familiar enough with your build to answer this question authoritatively. Code inspection suggests that this logic is hit when tools/ci_build/github/azure-pipelines/custom-nuget-packaging-pipeline.yml
is run, though I cannot find any references to that file (git grep custom-nuget-packaging-pipeline.yml
returns nothing).
At a high level though, the hope is that all Windows packages you create should now include the v81 files. I've confirmed this is the case for the Python wheel, but it would also be good to build nuget packages and make sure they're there. I'm not aware of other places this is needed, but it's possible that I just don't know all the binary distribution formats.
ExpectedEPNodeAssignment::All); | ||
} | ||
|
||
// Broken on v79 and v81 devices: |
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.
where should the fix be? is the test invalid, or is it a known issue for v79/v81 that will be fixed in future versions?
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.
These are known issues that are being tracked internally at Qualcomm.
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.
is the plan to re-enable these tests at some point?
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.
Yes, of course
/azp run Windows ARM64 QNN CI Pipeline |
Azure Pipelines successfully started running 1 pipeline(s). |
was v81 support only added in a recent version of the QNN SDK? if so, should we attempt to keep supporting building with older SDK versions or just increase the minimum required SDK version? |
Based on what I know, I think bumping the minimum required version is the way to go. A hard build failure is preferable to accidentally incomplete packages. Given that there isn't any testing of this repo or its build products on real Qualcomm devices, such an error would likely go undetected during the release process (similar to the 1.22.2 wheels reverting to x64 from ARM64x). Of course we test on real hardware on our internal fork, but the packaging parts of the build are not shared. An alternative is to make more parts of the build aware of what's in the various QNN versions, but this further complicates an already complicated system. This is especially challenging/risky because I can't test any of the affected code. What I do not know are the consequences for you of raising the minimum version other than having to revert two changes instead of one if the latest QNN version ends up needing to be yanked. If they're dire, I would be happy to investigate a more complex change. |
Description
This change adds support for the latest Qualcomm NPUs with the v81 architecture.
Specifically: