Update _get_prev_sample function in PNDMScheduler to be better supported by fx

[aviator19941]

What does this PR do?

Updates the alpha_prod_t and alpha_prod_t_prev variables in the _get_prev_sample function in PNDMScheduler, so that it can be traced through torch fx. Torch fx does not support Python control flow over tensors as stated in this Pytorch issue (pytorch/pytorch#116690 (comment)) as well as indexing a tensor, such as, x[i], causes a guard on i directly, and using index_select it won't be subject to this as stated here (pytorch/pytorch#116690 (comment)). The results of alpha_prod_t and alpha_prod_t_prev are the same, this change just makes it so it can be traced through torch fx.

Fixes # (issue)

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline?
• Did you read our philosophy doc (important for complex PRs)?
• Was this discussed/approved via a GitHub issue or the forum? Please add a link to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[aviator19941]

@patrickvonplaten Can you please take a look at this? Thank you very much!

[yiyixuxu]

I think if this is something we want to support, we probably want to update everywhere, not just this scheduler. So not too sure about this

cc @sayakpaul here
let me know what you think -

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[sayakpaul]

Agree with @yiyixuxu.

What's the speed implication of this change? I am fine with the change as it seems relatively simple and doesn't affect the readability of the code.

[aviator19941]

Agree with @yiyixuxu.

What's the speed implication of this change? I am fine with the change as it seems relatively simple and doesn't affect the readability of the code.

The speed implication of this change is ASAP, but I will update the rest of the schedulers in this PR or another PR. Whichever makes more sense for you.

[sayakpaul]

The speed implication of this change is ASAP

I don't understand it. I meant do we get faster performance with this change? How does it affect latency, if relevant?

[sayakpaul]

but I will update the rest of the schedulers in this PR or another PR. Whichever makes more sense for you.

I think it makes sense to change the schedulers here in this API for the consistency. @yiyixuxu WDYT?

[aviator19941]

The speed implication of this change is ASAP

I don't understand it. I meant do we get faster performance with this change? How does it affect latency, if relevant?

Oh sorry, I thought you were talking about the PR itself. Is there an existing command you use to check latency? Running the Stable Diffusion 1.4 model on CPU takes about the same time per iteration (~2.55 s/it for 50 inference steps) with the change vs. without the change.

[yiyixuxu]

I'm not sure if we want this.
cc @pcuenca let me know what you think!

[pcuenca]

Thanks for the ping @yiyixuxu!

I'm personally not in favor of going forward with these changes. Even though they look easy, they impose a maintenance and readability burden in the project. First, it can be a little bit puzzling for users to read the schedulers' code and try to understand why we are using index_select or torch.tensor(). Second, once we support torch fx we'll be committed to it, and it will be relatively easy to break it during code updates unless we are careful to introduce a new set of comprehensive tests.

Unless usage justifies it, I'd suggest these optimizations are kept in a separate fork. We can maybe refer to it somewhere in the docs to avoid duplicating effort.

Happy to listen to other opinions!

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

[sayakpaul]

@aviator19941 let us know what you think of #6878 (review).

[yiyixuxu]

@pcuenca
thanks for the feedback!! make sense!

[yiyixuxu]

@aviator19941
I think we will not be adding this change to diffusers for now - is it ok if we close this PR?

[aviator19941]

@aviator19941 I think we will not be adding this change to diffusers for now - is it ok if we close this PR?

Yes it's ok if we close this PR, I will use a separate fork for my changes instead, thanks!