Skip to content

Fix reporting backends and dtyep to benchmark results #6023

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix reporting backends and dtyep to benchmark results #6023

wants to merge 1 commit into from

Conversation

@guangy10
Copy link
Contributor

@guangy10 guangy10 commented Oct 8, 2024 •
edited
Loading

Couple minor fixes for reporting the benchmarking results:

  • qnn models are not reporting "backend" and "dtype" info in the benchmark_results.json (Android)
  • tinyllama mdoel is not reporting "backend" and "dtype" info in the benchmark_results.json (Android)
  • include compute precision to the exported coreml model name
  • rename "llama2" to "tinyllama" to eliminate confusion (many people thought it was llama2-7b)

@guangy10 guangy10 requested a review from huydhn October 8, 2024 22:38
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 8, 2024 •
edited
Loading

🔗 Helpful Links

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

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

❌ 2 New Failures

As of commit 22b823e with merge base b118d8e (image):

NEW FAILURES - The following jobs have failed:

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

@guangy10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@guangy10 guangy10 mentioned this pull request Oct 8, 2024
Closed
@guangy10 guangy10 force-pushed the fix_report_benchmark_results branch from c462b4e to 9e0d88f Compare October 8, 2024 22:52
@facebook-github-bot
Copy link
Contributor

@guangy10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@guangy10 guangy10 had a problem deploying to upload-benchmark-results October 8, 2024 23:29 — with GitHub Actions Failure
@guangy10 guangy10 force-pushed the fix_report_benchmark_results branch from 9e0d88f to 698e137 Compare October 9, 2024 00:07
@guangy10 guangy10 requested a review from haowhsu-quic October 9, 2024 00:09
@facebook-github-bot
Copy link
Contributor

@guangy10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@guangy10 guangy10 temporarily deployed to upload-benchmark-results October 9, 2024 00:39 — with GitHub Actions Inactive
haowhsu-quic
haowhsu-quic reviewed Oct 9, 2024
View reviewed changes
@guangy10 guangy10 force-pushed the fix_report_benchmark_results branch 2 times, most recently from e0f7b35 to b370ae7 Compare October 9, 2024 01:35
@facebook-github-bot
Copy link
Contributor

@guangy10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@guangy10 guangy10 force-pushed the fix_report_benchmark_results branch 2 times, most recently from ef92cb7 to 29aea6e Compare October 9, 2024 01:54
@guangy10 guangy10 had a problem deploying to upload-benchmark-results October 9, 2024 02:17 — with GitHub Actions Failure
@guangy10 guangy10 force-pushed the fix_report_benchmark_results branch from 29aea6e to d515415 Compare October 9, 2024 04:39
@guangy10 guangy10 had a problem deploying to upload-benchmark-results October 9, 2024 05:17 — with GitHub Actions Failure
@huydhn
Copy link
Contributor

huydhn commented Oct 9, 2024

I test this out in #5982, so its regex takes the first word as the model name, the last one as the dtype. And everything in between will be the backend. It's as as good as having proper JSON output from device, but I guess this will do for now

@huydhn huydhn added the ciflow/android Trigger Android CI label Oct 9, 2024
@huydhn
Copy link
Contributor

huydhn commented Oct 9, 2024

Also, I learn from https://github.com/pytorch/executorch/pull/5710/files#r1788458509 that changing the export name might cause unexpected failures because some names are hardcoded in the repo. It's a good idea to double check them.

@guangy10 guangy10 temporarily deployed to upload-benchmark-results October 9, 2024 06:15 — with GitHub Actions Inactive
@guangy10
Copy link
Contributor Author

guangy10 commented Oct 9, 2024

Also, I learn from https://github.com/pytorch/executorch/pull/5710/files#r1788458509 that changing the export name might cause unexpected failures because some names are hardcoded in the repo. It's a good idea to double check them.

Yeah, I reverted the changes that renames the exported artifacts directly but append the dtype suffix in the test script instead. It works for now, we can clean it up later

@facebook-github-bot
Copy link
Contributor

@guangy10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@guangy10 guangy10 force-pushed the fix_report_benchmark_results branch from d515415 to c691225 Compare October 9, 2024 19:29
@facebook-github-bot
Copy link
Contributor

@guangy10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@guangy10 guangy10 force-pushed the fix_report_benchmark_results branch from c691225 to 22b823e Compare October 9, 2024 20:43
@facebook-github-bot
Copy link
Contributor

@guangy10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@guangy10 guangy10 had a problem deploying to upload-benchmark-results October 9, 2024 21:22 — with GitHub Actions Failure
@guangy10 guangy10 had a problem deploying to upload-benchmark-results October 9, 2024 21:36 — with GitHub Actions Failure
@facebook-github-bot
Copy link
Contributor

@guangy10 merged this pull request in 012cba9.

@guangy10
Copy link
Contributor Author

guangy10 commented Oct 9, 2024

@pytorchbot cherry-pick --onto release/0.4 -c fixnewfeature

@guangy10 guangy10 deleted the fix_report_benchmark_results branch October 9, 2024 22:15
pytorchbot pushed a commit that referenced this pull request Oct 9, 2024
@pytorchbot
Fix reporting backends and dtyep to benchmark results (#6023)
1395020 
Summary:
Couple minor fixes for reporting the benchmarking results:
 - qnn models are not reporting "backend" and "dtype" info in the benchmark_results.json (Android)
 - tinyllama mdoel is not reporting "backend" and "dtype" info in the benchmark_results.json (Android)
 - include compute precision to the exported coreml model name
 - rename "llama2" to "tinyllama" to eliminate confusion (many people thought it was llama2-7b)

Pull Request resolved: #6023

Reviewed By: huydhn

Differential Revision: D64074262

Pulled By: guangy10

fbshipit-source-id: c6c53d004c4fb3ad410a792639af2c22a6978b67
(cherry picked from commit 012cba9)
@pytorchbot
Copy link
Collaborator

Cherry picking #6023

The cherry pick PR is at #6073 and it is recommended to link a fixnewfeature cherry pick PR with an issue. The following tracker issues are updated:

Details for Dev Infra team Raised by workflow job

@pytorchbot pytorchbot mentioned this pull request Oct 9, 2024
Closed
jackzhxng pushed a commit that referenced this pull request Oct 10, 2024
@pytorchbot
Fix reporting backends and dtyep to benchmark results (#6073)
3c0c994 
Fix reporting backends and dtyep to benchmark results (#6023)

Summary:
Couple minor fixes for reporting the benchmarking results:
 - qnn models are not reporting "backend" and "dtype" info in the benchmark_results.json (Android)
 - tinyllama mdoel is not reporting "backend" and "dtype" info in the benchmark_results.json (Android)
 - include compute precision to the exported coreml model name
 - rename "llama2" to "tinyllama" to eliminate confusion (many people thought it was llama2-7b)

Pull Request resolved: #6023

Reviewed By: huydhn

Differential Revision: D64074262

Pulled By: guangy10

fbshipit-source-id: c6c53d004c4fb3ad410a792639af2c22a6978b67
(cherry picked from commit 012cba9)

Co-authored-by: Guang Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@huydhn huydhn huydhn approved these changes

+1 more reviewer

@haowhsu-quic haowhsu-quic haowhsu-quic left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ciflow/android Trigger Android CI CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@guangy10 @facebook-github-bot @huydhn @pytorchbot @haowhsu-quic