Skip to content

[MPS] Handle MPS failures of test_modules.py in common_modules.py #95334

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
razarmehr wants to merge 5 commits into from
Closed

[MPS] Handle MPS failures of test_modules.py in common_modules.py #95334

razarmehr wants to merge 5 commits into from

Conversation

@razarmehr
Copy link
Collaborator

@razarmehr razarmehr commented Feb 22, 2023

  • Also cleaned up test_modules.py from skipMPS code.
  • Added skipMPS for unsupported or failing tests on MPS backend in common_modules.py.
    (We'll remove skipMPS from those tests once a fix is available for them.)

@razarmehr razarmehr added ciflow/trunk Trigger trunk jobs on your pull request ciflow/mps Run MPS tests (subset of trunk) labels Feb 22, 2023
@razarmehr razarmehr requested review from a team, mruberry and ngimel as code owners February 22, 2023 23:07
@pytorch-bot
Copy link

pytorch-bot bot commented Feb 22, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 604011b:
💚 Looks good so far! There are no failures yet. 💚

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

@razarmehr razarmehr added release notes: mps Release notes category open source labels Feb 22, 2023
@razarmehr razarmehr force-pushed the MPS_test_modules branch 2 times, most recently from 3c8cda5 to d5aabd3 Compare February 23, 2023 17:54
@ezyang ezyang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 27, 2023
@kulinseth
Copy link
Collaborator

kulinseth commented Mar 9, 2023

Overall looks good to me. @albanD can you please take a look ? This will be helpful to increase our testing coverage.

cc @malfet

@kulinseth kulinseth mentioned this pull request Mar 9, 2023
Closed
albanD
albanD reviewed Mar 9, 2023
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Looks great!
Could you remind me what is running on the regular m1 jobs and on the mps jobs? Is test_modules running on regular m1 test jobs and so this is covered?

@razarmehr razarmehr force-pushed the MPS_test_modules branch 2 times, most recently from 0a9462c to a8b63c5 Compare March 17, 2023 15:52
@razarmehr razarmehr requested a review from albanD March 17, 2023 16:01
@albanD
Copy link
Collaborator

albanD commented Mar 17, 2023

I'm still curious whether or not this is running in CI today?

@razarmehr
Copy link
Collaborator Author

razarmehr commented Mar 17, 2023

I'm still curious whether or not this is running in CI today?

Is enabling test_modules.py for CIs something that I should do in this PR? If you or @malfet can point out where I should make that change, then I would.

@albanD
Copy link
Collaborator

albanD commented Mar 17, 2023

Yes we should not land code that is not tested for sure ;)
@kulinseth are you familiar with what currently runs? I don't know from the top of my head, we would need to check the workflows and what they run since they don't go through the general run_test (IIRC).

albanD
albanD approved these changes Mar 17, 2023
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Ho nice! Thanks!
Let's see how happy CI is about this!

@razarmehr
Copy link
Collaborator Author

razarmehr commented Mar 17, 2023

Yes we should not land code that is not tested for sure ;) @kulinseth are you familiar with what currently runs? I don't know from the top of my head, we would need to check the workflows and what they run since they don't go through the general run_test (IIRC).

I believe this commit should enable running the test_modules for MPS CIs. I'll monitor the CIs in this PR to confirm it.

@kulinseth
Copy link
Collaborator

kulinseth commented Mar 17, 2023

Ho nice! Thanks!
Let's see how happy CI is about this!

@albanD , I think we should separate out the CI update in a separate PR. I am not sure we have enough HW yet on it. @razarmehr , can you please not combine the testing enablement in the same PR ?

@kulinseth
Copy link
Collaborator

kulinseth commented Apr 2, 2023

@razarmehr can you take a look what's failing now ?

We have enabled more HW, and now we are ready to merge the PR.

@razarmehr
Copy link
Collaborator Author

razarmehr commented Apr 7, 2023

@razarmehr can you take a look what's failing now ?

We have enabled more HW, and now we are ready to merge the PR.

There are multiple MPS failures with the newly added tests that need to be fixed before merging this PR.

malfet
malfet reviewed Apr 7, 2023
View reviewed changes
@razarmehr razarmehr mentioned this pull request May 8, 2023
Open
@razarmehr
Disable several test_modules.py failures on MPS backend.
7de37a2 
Fix a crash in LayerNorm test os test_modules.py
@razarmehr razarmehr force-pushed the MPS_test_modules branch from 8f90237 to 7de37a2 Compare May 8, 2023 22:25
@razarmehr
Copy link
Collaborator Author

razarmehr commented May 9, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented May 9, 2023

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malfet malfet malfet left review comments

@kulinseth kulinseth kulinseth approved these changes

@albanD albanD albanD approved these changes

@DenisVieriu97 DenisVieriu97 Awaiting requested review from DenisVieriu97

@mruberry mruberry Awaiting requested review from mruberry mruberry is a code owner

@ngimel ngimel Awaiting requested review from ngimel

Assignees

No one assigned

Labels

ciflow/mps Run MPS tests (subset of trunk) ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: mps Release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@razarmehr @kulinseth @albanD @pytorchmergebot @malfet @ezyang