-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Make IPAdapter compatible to torch.compile
#7988
Conversation
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.
should we also add a test?
comps = [] | ||
for i in l: | ||
comps.append(i) | ||
[ln0, ln1, attn, ff] = comps |
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.
ok but do we know why it has to be done this way? any documentation etc?
e.g. would this work?
for layer in self.layers:
[ln0, ln1, attn, ff] = layers
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.
If you try to unpack a ModuleList
like above or just like in the original implementation. Following error occurs:
torch._dynamo.exc.Unsupported: UNPACK_SEQUENCE NNModuleVariable()
So I assume that any unpacking with ModuleList
is not supported in torch.compile
and only for loop through it is supported. Hence, I for loop the ModuleList
to not breaking the graph and upack using python list
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.
I also try moving ln0, ln1, attn, ff
into a class, but it will affect the weight loading
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.
I also try moving
ln0, ln1, attn, ff
into a class, but it will affect the weight loading
hi @rootonchair, you can use the IPAdapterPlusImageProjectionBlock
class, but you have to update the loading function too.
I think the final result would be really good, as the code consistency in the library would improve
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. |
cc @fabiorigano too |
hi @yiyixuxu thanks for adding me here I think it would be nice to have a forward function that uses the diffusers/src/diffusers/models/embeddings.py Line 892 in d6ca120
that we introduced with the Face ID PR #7186 |
thanks @fabiorigano! |
@yiyixuxu yes it does. As @fabiorigano suggested, we could utilize |
This works for me. Thank you, @rootonchair! Perhaps the use of |
I can open a new PR to use |
That would be great, thank you! |
Sure @fabiorigano, thank you |
@rootonchair does #7994 work for you? |
@sayakpaul yes it does. Fantastic works |
Fantastic. Would you mind closing the PR then? I am sorry about the inconvenience here. |
Ah, no problem. I assume closing #7985 too? |
What does this PR do?
Fixes #7985
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.