Skip to content

Implement CustomDiffusionAttnProcessor2_0.#4604

Merged
patrickvonplaten merged 9 commits into
huggingface:mainfrom
eliphatfs:main
Sep 18, 2023
Merged

Implement CustomDiffusionAttnProcessor2_0.#4604
patrickvonplaten merged 9 commits into
huggingface:mainfrom
eliphatfs:main

Conversation

@eliphatfs

@eliphatfs eliphatfs commented Aug 15, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #4588

Before submitting

Who can review?

@sayakpaul @nupurkmr9

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

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

@sayakpaul

Copy link
Copy Markdown
Member

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

@eliphatfs

eliphatfs commented Aug 16, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Comment thread src/diffusers/models/attention_processor.py Outdated

@sayakpaul sayakpaul left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@sayakpaul

sayakpaul commented Aug 16, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

Cc @DN6 could you help here?

@DN6

DN6 commented Aug 16, 2023

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
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

eliphatfs commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

Done!

@eliphatfs

Copy link
Copy Markdown
Contributor Author

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

@sayakpaul

Copy link
Copy Markdown
Member

@DN6 could you give this a look?

@eliphatfs

Copy link
Copy Markdown
Contributor Author

Bump?

@sayakpaul

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@sayakpaul sayakpaul left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@eliphatfs

Copy link
Copy Markdown
Contributor Author

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

@eliphatfs

Copy link
Copy Markdown
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.

@patrickvonplaten patrickvonplaten left a comment

Copy link
Copy Markdown
Contributor

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?

@sayakpaul sayakpaul left a comment

Copy link
Copy Markdown
Member

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
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