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

MPS Load capabilities #623

Merged
merged 5 commits into from
Feb 18, 2023
Merged

Conversation

guillaume-be
Copy link
Contributor

I do not have a MPS device to test the behaviour. It would be great if one of the contributors who identified the issue and the workaround could validate it solves the loading issues before merging.

@bakkot
Copy link

bakkot commented Feb 18, 2023

I am not one of the original contributors, but I do have an MPS device, and I confirm this fixed the issue.

Specifically, I built https://github.com/LaurentMazare/diffusers-rs with the device manually set to MPS by changing this line to Device::Mps. When specifying this crate as a dependency at main, that fails with the error mentioned in #609. When I use this crate at this PR, it works, and runs several times faster than the non-MPS version.

While I'm here, I note that this PR doesn't restore the original device in the two error cases (the ? and the explicit return Err). That seems like it might be confusing.

@LaurentMazare
Copy link
Owner

Thanks for confirming, agreed that it would be nicer to set the device back to Mps if an error causes an early exit, @guillaume-be is it something that you could look at and then I think this is good to be merged.

@guillaume-be
Copy link
Contributor Author

Thank you very much for confirming @bakkot - @LaurentMazare yes I should be able to implement a more robust setup later today

@guillaume-be
Copy link
Contributor Author

@bakkot , @LaurentMazare I have wrapped the tensor loading in a closure to ensure the device reset code gets executed before the closure Result gets passed on to the caller

@LaurentMazare
Copy link
Owner

Looks good, thanks for the PR!

@LaurentMazare LaurentMazare merged commit da78395 into LaurentMazare:main Feb 18, 2023
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