Skip to content

Apple MPS Compatibility#17

Closed
FahimF wants to merge 5 commits intofastai:masterfrom
FahimF:macos-support
Closed

Apple MPS Compatibility#17
FahimF wants to merge 5 commits intofastai:masterfrom
FahimF:macos-support

Conversation

@FahimF
Copy link
Copy Markdown

@FahimF FahimF commented Oct 22, 2022

Updated the "Stable Diffusion Deep Dive" and stable_diffusion notebooks to be Apple MPS compatible as much as possible. Added performance enhancements for MPS and added comments where necessary when the code diverged from the original code.

Should be fully MPS compatible and run on an MPS device except for one main issue. I have opened a ticket with Hugging Face to see if the issue can be resolved from their side ...

* Add macOS support for the Deep Dive notebook without (hopefully) breaking it for other platforms
* Added a few tweaks and fixes to the Deep Dive notebook to work with the latest PyTorch and diffusers changes
* Updated the stable_diffusion notebook to work under Apple Silicon and tested all the cells
@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@pcuenca
Copy link
Copy Markdown
Contributor

pcuenca commented Oct 22, 2022

Hi @FahimF!

First a note: the issue you mention will be resolved when this PR is merged: huggingface/diffusers#942. Thanks for your report!

Regarding the actual support of the mps device, I think it's a bit tangential to the main purpose of the notebooks, which is to illustrate how diffusion models work, and might be a distraction. Perhaps it would be more useful to create a copy of the notebooks instead, so students willing to try them out on their Macs can refer to them. How do you feel about this topic @johnowhitaker and @jph00?

@FahimF
Copy link
Copy Markdown
Author

FahimF commented Oct 22, 2022

Hey @pcuenca 🙂 If the above issue and the other one where you can't pass an array of prompts to the pipeline on MPS (I believe that one's something which needs to be fixed from the PyTorch end) are fixed, then the only thing that the notebooks would require is to add a device based on GPU and use that device name instead of hardcoding for CUDA.

But you are right, as it stands now, it might be too much of a distraction and something which requires too many explanations. (And also a possible source of confusion). My original idea was to just have a separate branch on my forked repo with the changes and put up a link to that on the forums. But was wondering if perhaps somebody might go, "How come you didn't submit a PR?" 🙂

But I think that might be the least trouble overall till those issues are resolved. At that point, it shouldn't matter much anyway. Will wait in case there's another opinion, but if not, will close the PR and proceed with adding a link on the forums?

* Fix missed cell in Deep Dive notebook that had issues under MPS
@jph00
Copy link
Copy Markdown
Member

jph00 commented Oct 22, 2022

Thanks for the interesting PR! I agree with your conclusion. Hopefully some or all of these ideas will make their way into Diffusers given time - if supporting Mac was as simple as adding a device name at the top or there was an automatic device selection thingie, that would be awesome. Then we'd be able to support Mac inference without adding lots of code to the notebook!

@jph00 jph00 closed this Oct 22, 2022
@drscotthawley
Copy link
Copy Markdown
Contributor

(Dropping an update rather than starting a brand-new Issue)

Those of interested in Mac support will do well to keep an eye on relevant active PyTorch Issues and Pull requests:

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.

4 participants