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.

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

Implement CustomDiffusionAttnProcessor2_0. #4604

Merged
merged 9 commits into from
Sep 18, 2023

Conversation

eliphatfs
Copy link
Contributor

@eliphatfs eliphatfs commented Aug 15, 2023

What does this PR do?

Fixes #4588

Before submitting

Who can review?

@sayakpaul @nupurkmr9

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@sayakpaul
Copy link
Member

@eliphatfs let me know if this PR is ready for review.

@eliphatfs
Copy link
Contributor Author

eliphatfs commented Aug 16, 2023

Hi, how do I do the ko-documentation? I don't know any Korean.
Edit: guessing from the file structure it seems there is nothing to do with the attention processors. I will just skip then.

@eliphatfs
Copy link
Contributor Author

I don't find existing tests for CustomDiffusionAttnProcessor, so I will skip adding that for 2_0.

@eliphatfs eliphatfs marked this pull request as ready for review August 16, 2023 01:18
Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Looking good, thank you!

@yiyixuxu could you also give this a look?

@sayakpaul sayakpaul requested a review from yiyixuxu August 16, 2023 04:08
@eliphatfs
Copy link
Contributor Author

The CI complains about my code style. How do I spot the problem?

@sayakpaul
Copy link
Member

sayakpaul commented Aug 16, 2023

You can run make style && make quality from you local fork of diffusers.

Guidance: https://github.com/huggingface/diffusers/blob/main/CONTRIBUTING.md

@eliphatfs
Copy link
Contributor Author

Sorry I didn't read the guide carefully; I have updated the code formatting, but there are extra errors when I wrong make quality that seems nothing to do with me:

src/diffusers/pipelines/audio_diffusion/pipeline_audio_diffusion.py:181:12: E721 Do not compare types, use `isinstance()`
src/diffusers/pipelines/stable_diffusion_xl/pipeline_stable_diffusion_xl.py:793:42: E721 Do not compare types, use `isinstance()`
src/diffusers/pipelines/stable_diffusion_xl/pipeline_stable_diffusion_xl_img2img.py:878:20: E721 Do not compare types, use `isinstance()`
src/diffusers/pipelines/stable_diffusion_xl/pipeline_stable_diffusion_xl_inpaint.py:1083:20: E721 Do not compare types, use `isinstance()`
tests/pipelines/consistency_models/test_consistency_models.py:190:12: E721 Do not compare types, use `isinstance()`
tests/pipelines/unidiffuser/test_unidiffuser.py:112:12: E721 Do not compare types, use `isinstance()`
tests/pipelines/unidiffuser/test_unidiffuser.py:548:12: E721 Do not compare types, use `isinstance()`
tests/pipelines/unidiffuser/test_unidiffuser.py:651:12: E721 Do not compare types, use `isinstance()`

@sayakpaul
Copy link
Member

Cc @DN6 could you help here?

@DN6
Copy link
Collaborator

DN6 commented Aug 16, 2023

@eliphatfs What version of ruff do you have installed? You can check by running ruff -V.
Could you share the output of that command please?

@eliphatfs
Copy link
Contributor Author

Oh it was 0.0.284; after installing 0.0.280 as specified the errors vanished. 0.0.280 was not installed on my machine due to some version clash in flax which refers to orbax package that is no longer installable: google/orbax#436
I have fixed all style problems. Thanks for your help!!

@eliphatfs
Copy link
Contributor Author

eliphatfs commented Aug 17, 2023

Done!

@eliphatfs
Copy link
Contributor Author

Forgot to update loaders. Now test_examples pass on my local clone.

@sayakpaul
Copy link
Member

@DN6 could you give this a look?

@eliphatfs
Copy link
Contributor Author

Bump?

@sayakpaul
Copy link
Member

We are doing two major refactors just as an FYI which might influence this PR:

#4473
#4765

Let's maybe revisit this after that? WDYT?

@eliphatfs
Copy link
Contributor Author

I see the PRs you mentioned have been merged. Shall we continue working on this thing?

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Nice this works for me. Thank you so much for implementing this!

@sayakpaul
Copy link
Member

Ah, we have some failures on the CI and merge conflicts. Let's make sure to resolve them.

@eliphatfs
Copy link
Contributor Author

I have resolved the conflict; looking into the CI failures.

@eliphatfs
Copy link
Contributor Author

Sorry but I request a re-run of the tests. It seemed to me that the errors are not relevant to this PR. CI is complaining about a field missing in a huggingface API result. I guess it may be due to API changes and since I have merged the changes in diffusers main branch maybe we can retry the tests.

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.

Ok for me. @sayakpaul @DN6 @yiyixuxu wdyt?

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Okay for me to merge once the CI is green (except the documentation workflow).

@patrickvonplaten patrickvonplaten merged commit 16b9a57 into huggingface:main Sep 18, 2023
8 of 9 checks passed
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* Implement `CustomDiffusionAttnProcessor2_0`

* Doc-strings and type annotations for `CustomDiffusionAttnProcessor2_0`. (huggingface#1)

* Update attnprocessor.md

* Update attention_processor.py

* Interops for `CustomDiffusionAttnProcessor2_0`.

* Formatted `attention_processor.py`.

* Formatted doc-string in `attention_processor.py`

* Conditional CustomDiffusion2_0 for training example.

* Remove unnecessary reference impl in comments.

* Fix `save_attn_procs`.
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* Implement `CustomDiffusionAttnProcessor2_0`

* Doc-strings and type annotations for `CustomDiffusionAttnProcessor2_0`. (huggingface#1)

* Update attnprocessor.md

* Update attention_processor.py

* Interops for `CustomDiffusionAttnProcessor2_0`.

* Formatted `attention_processor.py`.

* Formatted doc-string in `attention_processor.py`

* Conditional CustomDiffusion2_0 for training example.

* Remove unnecessary reference impl in comments.

* Fix `save_attn_procs`.
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.

Implement CustomDiffusionAttnProcessor2_0
5 participants