Skip to content

Conversation

@pcuenca
Copy link
Member

@pcuenca pcuenca commented Oct 20, 2022

No description provided.

@pcuenca pcuenca marked this pull request as draft October 20, 2022 13:16
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 20, 2022

The documentation is not available anymore as the PR was closed or merged.

@pcuenca pcuenca marked this pull request as ready for review October 24, 2022 11:06
Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

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

Thank you for tracking down all the issues @pcuenca, looks great!

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Cool good job!

(Question) would it already make sense to add some tests for MPS?

Co-authored-by: Anton Lozhkov <[email protected]>
@pcuenca
Copy link
Member Author

pcuenca commented Oct 24, 2022

(Question) would it already make sense to add some tests for MPS?

Yes, we already have some, maybe we can add a few more :)

@pcuenca
Copy link
Member Author

pcuenca commented Oct 24, 2022

@anton-l tests pass locally in my computer, any idea why that test might be failing?

@anton-l
Copy link
Member

anton-l commented Oct 25, 2022

@pcuenca looks like first time they've failed was on the pipeline tests PR: https://github.com/huggingface/diffusers/actions/runs/3312968864

But the test code is identical:

So I'm very puzzled... Unless the test order mattered and we've had mps warmup steps from other tests affecting this one?

Copy link
Contributor

@patil-suraj patil-suraj left a comment

Choose a reason for hiding this comment

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

Looks good, thanks a lot!

@anton-l
Copy link
Member

anton-l commented Oct 25, 2022

An identical test_inference_2 passes after test_inference fails, looks like we need even more warmup for DDIM?
#976

@pcuenca pcuenca merged commit 3d02c92 into main Oct 25, 2022
@pcuenca pcuenca deleted the m1-torch-1.13 branch October 25, 2022 14:41
PhaneeshB pushed a commit to nod-ai/diffusers that referenced this pull request Mar 1, 2023
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* Docs: refer to pre-RC version of PyTorch 1.13.0.

* Remove temporary workaround for unavailable op.

* Update comment to make it less ambiguous.

* Remove use of contiguous in mps.

It appears to not longer be necessary.

* Special case: use einsum for much better performance in mps

* Update mps docs.

* Minor doc update.

* Accept suggestion

Co-authored-by: Anton Lozhkov <[email protected]>

Co-authored-by: Anton Lozhkov <[email protected]>
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.

6 participants