Skip to content

[Fix bug] always run prepare-test-spec #8974

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

Closed
wants to merge 2 commits into from
Closed

[Fix bug] always run prepare-test-spec #8974

wants to merge 2 commits into from

Conversation

yangw-dev
Copy link
Contributor

@yangw-dev yangw-dev commented Mar 5, 2025

Follow up with #8125

Example of failed benchmark run: https://github.com./pytorch/executorch/actions/runs/13666476394

We made prepare-test-spec have dependency on export-model, it seems like when there is any failure happens in the export model step, it will skip the whole prepare-test-spec. We want to always run prepare-test-spec.

in step prepare-test-spec, we will check wether the export-model successfully build the model, if not, no test-spec will be uploaded. This makes the final step mobile-on-device step fails early at verify-test-spec part

Copy link

pytorch-bot bot commented Mar 5, 2025

🔗 Helpful Links

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

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

⏳ No Failures, 3 Pending

As of commit 71e0775 with merge base 5ac4a3c (image):
💚 Looks good so far! There are no failures yet. 💚

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 Mar 5, 2025
@yangw-dev yangw-dev marked this pull request as ready for review March 5, 2025 21:14
@yangw-dev yangw-dev requested a review from guangy10 March 5, 2025 21:15
@yangw-dev yangw-dev requested a review from ZainRizvi March 5, 2025 21:26
@yangw-dev yangw-dev temporarily deployed to upload-benchmark-results March 5, 2025 22:04 — with GitHub Actions Inactive
@yangw-dev yangw-dev had a problem deploying to upload-benchmark-results March 5, 2025 22:15 — with GitHub Actions Failure
@yangw-dev yangw-dev requested a review from huydhn March 5, 2025 23:36
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.

Oops, I think we should revert #8482 instead because having always() here will negate #8482. In hindsight, we shouldn't make prepare-test-spec depend on export-models. I forgot to tell you that the issue #8125 has been fixed by #8786 (I did it last minute before signing off on Thursday, my bad)

@yangw-dev yangw-dev temporarily deployed to upload-benchmark-results March 6, 2025 00:24 — with GitHub Actions Inactive
@yangw-dev yangw-dev had a problem deploying to upload-benchmark-results March 6, 2025 00:38 — with GitHub Actions Failure
@yangw-dev
Copy link
Contributor Author

got it!

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. topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants