Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix the bug that joint_attention_kwargs is not passed to the FLUX's transformer attention processors #9517

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Fix the bug that joint_attention_kwargs is not passed to the FLUX's transformer attention processors #9517

merged 9 commits into from
Oct 8, 2024

Conversation

HorizonWind2004
Copy link
Contributor

Issue link is below:

#9516

@HorizonWind2004 HorizonWind2004 mentioned this pull request Sep 24, 2024
Closed
yiyixuxu
yiyixuxu reviewed Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

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

thanks for the PR! I left a question

src/diffusers/models/transformers/transformer_flux.py Outdated
@@ -161,6 +162,7 @@ def forward(
encoder_hidden_states: torch.FloatTensor,
temb: torch.FloatTensor,
image_rotary_emb=None,
joint_attention_kwargs={},
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain what additional argument you need to pass down to flux attention processor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your recognition!

In our work, I am trying to integrate box and mask into the FLUX model and implement layout control (similar to what has been done in many works on SD1.4). This requires modifying the attention processor. I believe that the architecture of FLUX and other transformers can also be used to develop better layout control algorithms, so I believe these modifications will contribute to future training-free experiments on FLUX.

yiyixuxu
yiyixuxu reviewed Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

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

thanks! let's support this :)

src/diffusers/models/transformers/transformer_flux.py Outdated
@@ -161,6 +162,7 @@ def forward(
encoder_hidden_states: torch.FloatTensor,
temb: torch.FloatTensor,
image_rotary_emb=None,
joint_attention_kwargs={},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
joint_attention_kwargs={},
joint_attention_kwargs=None,

src/diffusers/models/transformers/transformer_flux.py Outdated
):
residual = hidden_states
norm_hidden_states, gate = self.norm(hidden_states, emb=temb)
mlp_hidden_states = self.act_mlp(self.proj_mlp(norm_hidden_states))

joint_attention_kwargs = joint_attention_kwargs if joint_attention_kwargs is not None else {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we pass this to attn too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I think it will be useful for other trial!

@yiyixuxu yiyixuxu requested a review from a-r-r-o-w September 25, 2024 23:41
@HorizonWind2004
Update transformer_flux.py
5104098 
fix a little bug
@HorizonWind2004
Copy link
Contributor Author

I fixed a bug and now it is okay XD.

@HuggingFaceDocBuilderDev
Copy link

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.

a-r-r-o-w
a-r-r-o-w reviewed Sep 27, 2024
View reviewed changes
Copy link
Member

@a-r-r-o-w a-r-r-o-w 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, looks good! could you run make style to fix the failing quality tests.

i have a question though: in this comment, you mention that your work uses this feature to modify something in, or pass additional arguments to, the attention processor. it is understandable why this would be useful, but we generally do not add features that are not testable or usable without a public implementation. is your work/method utilizing this change available for testing this? if it will be available in the near future, i think it might be best to postpone merging this PR after that

@a-r-r-o-w
Copy link
Member

thanks! let's support this :)

oh, i just saw yiyi's comment about being okay with supporting this. in that case, please disregard my question above. let's fix the quality tests and we should be okay to merge

@yiyixuxu
Copy link
Collaborator

@a-r-r-o-w
I think, based on my undersanding, they just want to use a custom attention processor so they can do pipe.transformer.set_attn_processor(); they do not intend to add that to diffusers;
but I think if we do not allow passing that kwargs, it won't work with a custom attention processor taking additional arguments. o

@HorizonWind2004
Copy link
Contributor Author

@a-r-r-o-w I think, based on my undersanding, they just want to use a custom attention processor so they can do pipe.transformer.set_attn_processor(); they do not intend to add that to diffusers; but I think if we do not allow passing that kwargs, it won't work with a custom attention processor taking additional arguments. o

Yes, that's it! QWQ

@HorizonWind2004
Copy link
Contributor Author

@a-r-r-o-w
I sincerely apologize for my late response. I have been busy with another project and forgot to check GitHub. I will proceed with fixing the issues that caused the quality tests to fail right away. TAT

@a-r-r-o-w
Copy link
Member

Hi @HorizonWind2004, looks good to me! Could you run make style in the diffusers root folder and push the changes? Happy to merge after that once @yiyixuxu approves too

@HorizonWind2004
Copy link
Contributor Author

Hi @HorizonWind2004, looks good to me! Could you run make style in the diffusers root folder and push the changes? Happy to merge after that once @yiyixuxu approves too

Yes! Now it is okay.

@HorizonWind2004
Copy link
Contributor Author

@a-r-r-o-w @yiyixuxu
Now I've run make style && make quality and push it! OVO

a-r-r-o-w
a-r-r-o-w approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@a-r-r-o-w a-r-r-o-w left a comment

Choose a reason for hiding this comment

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

thanks!

yiyixuxu
yiyixuxu approved these changes Oct 8, 2024
View reviewed changes
Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

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

thanks!

@yiyixuxu yiyixuxu merged commit acd6d2c into huggingface:main Oct 8, 2024
15 checks passed
leisuzz pushed a commit to leisuzz/diffusers that referenced this pull request Oct 11, 2024
@HorizonWind2004
Fix the bug that joint_attention_kwargs is not passed to the FLUX's…
900a1b5 
… transformer attention processors (huggingface#9517)

* Update transformer_flux.py
@Matrix53 Matrix53 mentioned this pull request Oct 31, 2024
Merged
6 tasks
sayakpaul pushed a commit that referenced this pull request Dec 23, 2024
@HorizonWind2004 @sayakpaul
Fix the bug that joint_attention_kwargs is not passed to the FLUX's…
752f5c2 
… transformer attention processors (#9517)

* Update transformer_flux.py
@huanngzh huanngzh mentioned this pull request Jan 5, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yiyixuxu yiyixuxu yiyixuxu approved these changes

@a-r-r-o-w a-r-r-o-w a-r-r-o-w approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HorizonWind2004 @HuggingFaceDocBuilderDev @a-r-r-o-w @yiyixuxu