Skip to content
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

[CI] Collect inductor max-autotune performance every Sunday #99387

Closed
wants to merge 3 commits into from

Conversation

desertfire
Copy link
Contributor

@desertfire desertfire commented Apr 18, 2023

@desertfire desertfire requested a review from a team as a code owner April 18, 2023 00:56
@pytorch-bot
Copy link

pytorch-bot bot commented Apr 18, 2023

🔗 Helpful Links

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

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

❌ 2 Failures

As of commit 1affb96:

BROKEN TRUNK - The following jobs failed but were present on the merge base 7ff1f3f:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

@pytorch-bot pytorch-bot bot added release notes: releng release notes category labels Apr 18, 2023
desertfire added a commit that referenced this pull request Apr 18, 2023
ghstack-source-id: f069e99ecc14ec4826dd34cc8d695c9bee9ada13
Pull Request resolved: #99387
cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx

[ghstack-poisoned]
desertfire added a commit that referenced this pull request Apr 18, 2023
ghstack-source-id: 2833907826b7d803988531ccbcab6c99563196a3
Pull Request resolved: #99387
cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx

[ghstack-poisoned]
desertfire added a commit that referenced this pull request Apr 18, 2023
ghstack-source-id: 4e80b0f3ff2702a9b5f64622eeb573b8a0f5ba77
Pull Request resolved: #99387
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Instead of creating a whole new workflow, wouldn't it be better to just condition one of the jobs to run only on Sunday, something like if: github.event.schedule == '0 0 * * 0'

@@ -0,0 +1,45 @@
name: inductor-A100-max-autotune-weekly
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also need to add inductor-A100-max-autotune-weekly to https://github.com/pytorch/pytorch/blob/main/.github/workflows/upload-torch-dynamo-perf-stats.yml#L5 to get the performance results sent to Rockset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we will need that, but it might need some twist on the UI. Weekly dots vs. daily lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@huydhn , I will leave the upload script as is for now and let's work out the details on how to distinguish the data from nightly and weekly.

@huydhn
Copy link
Contributor

huydhn commented Apr 18, 2023

Instead of creating a whole new workflow, wouldn't it be better to just condition one of the jobs to run only on Sunday, something like if: github.event.schedule == '0 0 * * 0'

+1. This is a less-known fact that there can be more than one schedule on the workflow, i.e. https://github.com/pytorch/pytorch/blob/main/.github/workflows/periodic.yml#L5-L6, and the schedule can be used as a condition https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule.

Although this is fine too as I have a feeling that combine everything in the same test config matrix wouldn't be that easy

@desertfire
Copy link
Contributor Author

Instead of creating a whole new workflow, wouldn't it be better to just condition one of the jobs to run only on Sunday, something like if: github.event.schedule == '0 0 * * 0'

We may need a separate workflow name to make sure dashboard can treat the data differently? cc @huydhn
Another reason is user will be able to manually start a max-autotune run with this separate workflow.

@huydhn
Copy link
Contributor

huydhn commented Apr 18, 2023

We may need a separate workflow name to make sure dashboard can treat the data differently? cc @huydhn Another reason is user will be able to manually start a max-autotune run with this separate workflow.

The second part about manual dispatch makes sense. About the first part, this sounds like something to be fixed on my part as I don't use the workflow name at the moment anywhere in the dashboard. It means that inductor_with_cudagraphs running with Torchbench for example wouldn't know if the records come with or without max autotune enabled. So I agree that having a different name would be needed if the rest of the configs is the same (same set of compilers, same suites, same tests)

@desertfire
Copy link
Contributor Author

@pytorchbot merge -f "broken trunk; this PR only affects perf test"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@facebook-github-bot facebook-github-bot deleted the gh/desertfire/139/head branch June 8, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants