-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Update _get_prev_sample function in PNDMScheduler to be better supported by fx #6878
Conversation
@patrickvonplaten Can you please take a look at this? Thank you very much! |
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 |
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. |
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. |
I don't understand it. I meant do we get faster performance with this change? How does it affect latency, if relevant? |
I think it makes sense to change the schedulers here in this API for the consistency. @yiyixuxu WDYT? |
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. |
I'm not sure if we want this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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. |
@aviator19941 let us know what you think of #6878 (review). |
@pcuenca |
@aviator19941 |
Yes it's ok if we close this PR, I will use a separate fork for my changes instead, thanks! |
What does this PR do?
Updates the
alpha_prod_t
andalpha_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 oni
directly, and usingindex_select
it won't be subject to this as stated here (pytorch/pytorch#116690 (comment)). The results ofalpha_prod_t
andalpha_prod_t_prev
are the same, this change just makes it so it can be traced through torch fx.Fixes # (issue)
Before submitting
documentation guidelines, and
here are tips on formatting docstrings.
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.