Skip to content

Conversation

@hlky
Copy link
Contributor

@hlky hlky commented Dec 2, 2024

What does this PR do?

#10085 (comment) for some further details.

Fixes #10085

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.

cc @sayakpaul @japppie

@hlky
Copy link
Contributor Author

hlky commented Dec 2, 2024

Note there is some small visual difference between num_images_per_prompt=1 and num_images_per_prompt=2:

num_images_per_prompt=1
minimal_1
num_images_per_prompt=2
minimal

This also occurs without skip_guidance_layers:

num_images_per_prompt=1
minimal_no
num_images_per_prompt=2
minimal_no_2

@HuggingFaceDocBuilderDev

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.

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 a lot for the fix! @hlky
so the implementation was wrong before and just happen to got broadcasted inside one of the AdaLayerNorm

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Dec 2, 2024

can we add more tests here

def test_stable_diffusion_3_different_prompts(self):

to test num_images_per_prompt =2 for SLG with and without CFG

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Dec 2, 2024

feel free to merge once tested is add (or you can add test in a separate PR!)

@hlky
Copy link
Contributor Author

hlky commented Dec 2, 2024

Yes it's easily missed, important to check batch support for new models etc. going forward.

Looks like tests for num_images_per_prompt are missing from some other pipelines like Flux as well so I'll add them all in another PR.

@hlky hlky merged commit beb8566 into huggingface:main Dec 2, 2024
15 checks passed
@sayakpaul
Copy link
Member

Looks like tests for num_images_per_prompt are missing from some other pipelines like Flux as well so I'll add them all in another PR.

@hlky

I think it would make sense to introduce/revisit the pipeline tester mixin to see how we can make it more general so that this is tested by default without us having to write it for every pipeline/model test suite. WDYT?

Feel free to ping me and @DN6 if you need further ideas here.

lawrence-cj pushed a commit to lawrence-cj/diffusers that referenced this pull request Dec 4, 2024
sayakpaul pushed a commit that referenced this pull request Dec 23, 2024
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.

RuntimeError with LyCORIS, Batch Inference and skip_guidance_layers

4 participants