Skip to content

Conversation

@huydhn
Copy link
Contributor

@huydhn huydhn commented Dec 4, 2024

After #135386 and #141999, MPS benchmark has been running for a while and the data has been uploaded correctly. However, the dashboard is still using the old schema that requires the output CSV files to be named in a certain way for its query to work https://github.com/pytorch/test-infra/blob/main/torchci/clickhouse_queries/compilers_benchmark_performance/query.sql#L32-L40. Specifically, the filename needs to be in the following format inductor_${backend}_${suite}_${dtype}_${mode}_${device}_${target}.csv.

The new schema gets away with all this hacky setup, but the dashboard hasn't been migrated to the new schema yet. So, this is a quick way to just get the data to show up first.

Testing

https://github.com/pytorch/pytorch/actions/runs/12153886764

@huydhn huydhn requested review from malfet and skotapati December 4, 2024 05:54
@huydhn huydhn requested a review from a team as a code owner December 4, 2024 05:54
@pytorch-bot
Copy link

pytorch-bot bot commented Dec 4, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 4a85860 with merge base 86f306b (image):
💚 Looks good so far! There are no failures yet. 💚

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

@huydhn
Copy link
Contributor Author

huydhn commented Dec 4, 2024

pytorch/test-infra#6006 is needed to allow benchmark in eager mode to show up on the dashboard. It was blocked originally because it is just used as the baseline for TorchInductor.

@skotapati I'm not quite sure what we are looking at here because the dashboard uses eager as the baseline. So, most of the metrics don't provide much information, for example speedup will always be around 1. The only thing that might be useful is the absolute execution time.

@skotapati
Copy link
Contributor

Hi @huydhn, thanks for putting this up. Yeah the only useful stat here will be the absolute runtime, since MacOS currently only supports this benchmark in eager mode. The idea is that we'll use this to track performance regressions over time, but individual runs on their own won't tell us much.

I'll discuss with Nikita & Kulin, but we may want to format this dashboard a little differently than the inductor data, so that there's no confusion. Either way it's great to see the data showing up on the dashboard, from my end I'll follow up with a PR to enable the remaining workloads

@malfet
Copy link
Contributor

malfet commented Dec 5, 2024

@pytorchbot merge -f "Lint is green"

@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). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

huydhn added a commit to pytorch/test-infra that referenced this pull request Dec 6, 2024
This goes together with pytorch/pytorch#142034.
MPS benchmark is running in eager mode at the moment.
AmdSampsa pushed a commit to AmdSampsa/pytorch that referenced this pull request Dec 9, 2024
After pytorch#135386 and pytorch#141999, MPS benchmark has been running for a while and the data has been uploaded correctly.  However, the dashboard is still using the old schema that requires the output CSV files to be named in a certain way for its query to work https://github.com/pytorch/test-infra/blob/main/torchci/clickhouse_queries/compilers_benchmark_performance/query.sql#L32-L40.  Specifically, the filename needs to be in the following format `inductor_${backend}_${suite}_${dtype}_${mode}_${device}_${target}.csv`.

The new schema gets away with all this hacky setup, but the dashboard hasn't been migrated to the new schema yet. So, this is a quick way to just get the data to show up first.

### Testing

https://github.com/pytorch/pytorch/actions/runs/12153886764
Pull Request resolved: pytorch#142034
Approved by: https://github.com/skotapati, https://github.com/malfet
@github-actions github-actions bot deleted the mps-benchmark-filename branch January 5, 2025 02:10
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.

5 participants