Skip to content

Conversation

kirklandsign
Copy link
Contributor

No description provided.

Copy link

pytorch-bot bot commented Sep 5, 2024

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit c9bf893 with merge base 83d92ff (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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

@facebook-github-bot facebook-github-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 Sep 5, 2024
Copy link
Contributor

@huydhn huydhn left a comment

Choose a reason for hiding this comment

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

LGTM!

PYTHON_EXECUTABLE=python bash .ci/scripts/setup-linux.sh cmake
export ARTIFACTS_DIR_NAME=artifacts-to-be-uploaded
if [[ ${{ matrix.delegate }} == "qnn" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I just notice that the job fail https://github.com/pytorch/executorch/actions/runs/10713455099/job/29705837269#step:12:6342, you will need to define the matrix strategy for this job with

strategy:
  matrix:
    delegate: ${{ fromJson(needs.set-parameters.outputs.delegates) }}
fail-fast: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that we're building backend-specific apps in the android benchmark workflow and it seems like the changes are introduced here. @kirklandsign @huydhn I think we should not build different apps for each backend. There are several reasons for it:

  1. It will complicate the benchmarking workflow as you will have to add additional logics to match model with the app. Because we are benchmarking the perf of a model instead of the app, the app size is not a concern in this case.
  2. Delegates to hybrid backends will be supported for better perf at some point though today nobody is proactively working on it. Ideally QNN unsupported ops should fallback to XNNPACK instead of Portable, otherwise we may end up with QNN delegated model run slower than a pure CPU model w/ XNNPACK. Our stack should have supported building an app with multiple backends/delegates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now we build this XNNPACK+QNN when matrix has qnn. https://github.com/pytorch/executorch/blob/main/.github/workflows/android-perf.yml#L147-L150

We can build a single flavor (XNNPACK+QNN+otherbackends) regardless of matrix as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The requirement to do always build QNN is we need to run

PYTHON_EXECUTABLE=python bash .ci/scripts/setup-qnn-deps.sh
PYTHON_EXECUTABLE=python bash .ci/scripts/build-qnn-sdk.sh

all the time. Probably takes some time and stability. Ideally we have a docker image.

@kirklandsign
Copy link
Contributor Author

Error: File /tmp/qnn/include/QNN/QnnTypes.h does not exist.

Should not export separately!
.ci/scripts/build-qnn-sdk.sh:  export QNN_SDK_ROOT=/tmp/qnn/2.23.0.240531
@kirklandsign
Copy link
Contributor Author

I'm not sure whether the error is from this PR or already there.

https://github.com/pytorch/executorch/actions/runs/10715134533/job/29711462394

@kirklandsign kirklandsign merged commit e854967 into main Sep 5, 2024
50 of 52 checks passed
@kirklandsign kirklandsign deleted the qnn-perf branch September 5, 2024 16:28
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants