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

Simplify MPS support #1726

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Simplify MPS support #1726

merged 5 commits into from
Sep 13, 2024

Conversation

rasbt
Copy link
Collaborator

@rasbt rasbt commented Sep 13, 2024

Simplified the MPS support based on your suggestion @t-vi (CC @Andrei-Aksionov).

It's a bit trickier to have unit tests for this behavior now (because we can't force to run the MPS code on non-Mac machines now), but it's fine because we know the previous unit tests for equivalency passed. And sure, we can have macOS tests for this alternative path, but the problem is that MPS has subtle numerical differences, so I don't expect the results of MPS and CPU to be equivalent. But we can address this is in #1725

Note that I also changed the default type for MPS to bf16 because it seems supported now. If we use regular float 16, we can get weird inference results for many models, which is why I changed it.

@Andrei-Aksionov
Copy link
Collaborator

Perhaps it's possible to mock tensor's device output, so you can test both code execution paths.

@t-vi
Copy link
Contributor

t-vi commented Sep 13, 2024

It's a bit trickier to have unit tests for this behavior now (because we can't force to run the MPS code on non-Mac machines now),

what you can do is

  • make the mps variant a separate function with the same signature (called with if on the device type),
  • run the model with / without mocking the batched_index_copy to the mps variant.

@rasbt rasbt merged commit eda1aaa into main Sep 13, 2024
8 of 9 checks passed
@rasbt rasbt deleted the new-mps-support branch September 13, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants