-
Notifications
You must be signed in to change notification settings - Fork 295
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
Upload Android test spec to ossci-android #4676
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4676
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit acc6482 with merge base 2654f59 (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@huydhn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@pytorchbot drci |
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!
- name: Only push to S3 when running the workflow manually from main branch | ||
# if: ${{ github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/main' }} |
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.
- Should it be uncommented?
- What are validated in this job on PR if the spec is only uploaded once post-merge?
- Why uploading when running the workflow manually?
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.
Yeah, the testing is done, I will need to uncomment this before landing. The 3 point is just a copy/paste from iOS workflow (where we manually upload iOS built artifacts on release, but it's not needed here)
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.
Aha I see, so we don't need to trigger this on PR. pull_request:
is added for testing purpose only.
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 there a way to validate the test-spec on PR, instead of updating the S3 to a broken one and defer the issue discovering until the runtime. It will be nice if we could run some validation in the PR that modifies the test-spec
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.
Ah, we can keep pull_request
trigger here as it will use aws s3 cp --dry-run
which doesn't copy the file and act as a form of testing on PR
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 there a way to validate the test-spec on PR, instead of updating the S3 to a broken one and defer the issue discovering until the runtime. It will be nice if we could run some validation in the PR that modifies the test-spec
Good point! Let me see if I can trigger the android-perf
job with the new spec for testing as it's the only way to know if the spec works.
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 don't run any validation on PR, I don't see the value of keeping the pull_request:
other than validating the job itself runnable.
Ideally some static analysis for the test-spec on PR would be good, just to ensure no syntax issue, mandatory things are provided in valid format, etc.
It may not be a good idea to add dependency to the android-perf
, for the short-term to validate the test-spec, I'm okay with it, if we can make sure we only work on a tiny model with one backend when triggering the job (defaults for on-demand workflow would be good). FYI even today we coded periodic workflow and on-demand workflow with the same defaults, later the periodic defaults will be expanded.
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's ok to call android-perf
with the small story model and the default set of parameters. To achieve this, I make the test spec configurable as an input and set its default value to the S3 source of truth at https://ossci-android.s3.amazonaws.com/executorch/android-llm-device-farm-test-spec.yml
@huydhn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@huydhn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
description: The test spec to drive the test on AWS devices | ||
required: false | ||
type: string | ||
default: https://ossci-android.s3.amazonaws.com/executorch/android-llm-device-farm-test-spec.yml |
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 the prefix is common for all, let's only put the file name android-llm-device-farm-test-spec.yml
here
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.
Oh, this is not a common prefix here though. For PR testing, it needs to stay as an artifact available only to the PR, i.e. https://gha-artifacts.s3.amazonaws.com/pytorch/executorch/10361242176/artifact/android-llm-device-farm-test-spec.yml. On the other hand, once validation is done and the spec file is committed, it will be uploaded to the source of truth at https://ossci-android.s3.amazonaws.com/executorch/android-llm-device-farm-test-spec.yml
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.
@huydhn, fyi, we start seeing the failures due to empty test spec https://hud.pytorch.org/hud/pytorch/executorch/main/1?per_page=50&name_filter=android-perf%20%2F%20benchmark
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.
Let me take a look
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.
@huydhn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@pytorchbot drci |
@huydhn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
A follow-up PR of #4657 to automatically update the Android spec file on S3 if the file
examples/demo-apps/android/LlamaDemo/android-llm-device-farm-test-spec.yml
on GitHub is updated. This requires a role to upload toossci-android
S3 bucket from https://github.com/pytorch-labs/pytorch-gha-infra/pull/455Testing
https://github.com/pytorch/executorch/actions/runs/10361242176 nows validate the test spec first by calling
android-perf
with the smallstories110M
model before trying to upload it toossci-android
Another test to make sure that
android-perf
can still be trigger manually via workflow_dispatch https://github.com/pytorch/executorch/actions/runs/10361255713