Skip to content

fix loop bug in SlicedAttnProcessor - #8836

Merged
yiyixuxu merged 13 commits into
huggingface:mainfrom
shinetzh:fix_bug_of_SlicedAttnProcessor
Jul 20, 2024
Merged

fix loop bug in SlicedAttnProcessor#8836
yiyixuxu merged 13 commits into
huggingface:mainfrom
shinetzh:fix_bug_of_SlicedAttnProcessor

Conversation

@shinetzh

@shinetzhshinetzh commented Jul 11, 2024

Copy link
Copy Markdown
Contributor

Fixes # (loop bug in SlicedAttnProcessor)

@sayakpaul@yiyixuxu@DN6

@sayakpaul

sayakpaul commented Jul 11, 2024

Copy link
Copy Markdown
Member

Thank you! Do you think our tests need to be updated to catch bugs like this?

diffusers/tests/pipelines/test_pipelines_common.py

Line 1331 in 673eb60

deftest_attention_slicing_forward_pass(self, expected_max_diff=1e-3):

@sayakpaul
sayakpaul requested a review from yiyixuxuJuly 11, 2024 05:33
@shinetzhshinetzh mentioned this pull request Jul 11, 2024
Closed
@shinetzh

shinetzh commented Jul 11, 2024

Copy link
Copy Markdown
ContributorAuthor

Bugs like this in test_attention_slicing_forward_pass have been fixed, include "SlicedAttnProcessor" and "SlicedAttnAddedKVProcessor". May be there is no need to catch bugs like this in test_attention_slicing_forward_pass. But maybe tests on other function or module are needed.

@shinetzh

shinetzh commented Jul 11, 2024

Copy link
Copy Markdown
ContributorAuthor

Or, if it's needed to update the test "test_attention_slicing_forward_pass" to catch bugs like this, I am happy to do that

@shinetzh

shinetzh commented Jul 11, 2024

Copy link
Copy Markdown
ContributorAuthor

I have update the test_attention_slicing_forward_pass to catch bugs like this.

sayakpaul
sayakpaul approved these changes Jul 11, 2024
View reviewed changes

@sayakpaulsayakpaul 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.

LGTM. Thanks a lot!

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Jul 11, 2024

Copy link
Copy Markdown

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.

@shinetzh

shinetzh commented Jul 11, 2024

Copy link
Copy Markdown
ContributorAuthor

space in blank line have been removed

@yiyixuxu

yiyixuxu commented Jul 17, 2024

Copy link
Copy Markdown
Collaborator

thanks for fixing this for us!
can you run make style ?

@shinetzh

shinetzh commented Jul 17, 2024

Copy link
Copy Markdown
ContributorAuthor

ok, wait a minute

@shinetzh

shinetzh commented Jul 17, 2024

Copy link
Copy Markdown
ContributorAuthor

done

@shinetzh

shinetzh commented Jul 17, 2024

Copy link
Copy Markdown
ContributorAuthor

sorry for tests bug, have fixed it

@yiyixuxu

yiyixuxu commented Jul 17, 2024

Copy link
Copy Markdown
Collaborator

hey I think the failing tests are relevant here https://github.com/huggingface/diffusers/actions/runs/9970139809/job/27588409202?pr=8836#step:7:17944

can you look into them?

@shinetzh

shinetzh commented Jul 18, 2024

Copy link
Copy Markdown
ContributorAuthor

I have look into them, and this is relevant with unet_2d_condition. I change unet_2d_conditon a little, I don't know if this is appropriate. Please check it.

@shinetzh

shinetzh commented Jul 18, 2024

Copy link
Copy Markdown
ContributorAuthor

This commit: 0ede41c

yiyixuxu
yiyixuxu reviewed Jul 18, 2024
View reviewed changes
Comment threadtests/pipelines/test_pipelines_common.py Outdated
inputs = self.get_dummy_inputs(generator_device)
output_with_slicing2 = pipe(**inputs)[0]

pipe.enable_attention_slicing(slice_size=3)

@yiyixuxuyiyixuxuJul 18, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we remove the slice_size=3 test? I think the CI would pass without this, no?

yiyixuxu
yiyixuxu reviewed Jul 18, 2024
View reviewed changes
Comment threadsrc/diffusers/models/unets/unet_2d_condition.py Outdated
dim = sliceable_head_dims[i]
if size is not None and size > dim:
raise ValueError(f"size {size} has to be smaller or equal to {dim}.")
slice_size[i] = dim

@yiyixuxuyiyixuxuJul 18, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

let's try not to make this update and change the test instead (we should try not to update the user inputs for user, we always prefer to be explicit and throw an error message, )

@shinetzhshinetzhJul 18, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok, get it. if remove the slice_size=3, the CI will pass.

@shinetzh

shinetzh commented Jul 18, 2024

Copy link
Copy Markdown
ContributorAuthor

I have remove slice_size=3, and make unet_2d_condition unchanged.

@shinetzh

shinetzh commented Jul 18, 2024

Copy link
Copy Markdown
ContributorAuthor

It seems like failed on other test file.
image

@yiyixuxu
yiyixuxu merged commit 3b04cdc into huggingface:mainJul 20, 2024
@psychedeliciouspsychedelicious mentioned this pull request Aug 2, 2024
Closed
3 tasks
sayakpaul pushed a commit that referenced this pull request Dec 23, 2024
@shinetzhneoshang@sayakpaul
fix loop bug in SlicedAttnProcessor (#8836)
e86164f
* fix loop bug in SlicedAttnProcessor
---------
Co-authored-by: neoshang <neoshang@tencent.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yiyixuxuyiyixuxuyiyixuxu left review comments
@sayakpaulsayakpaulsayakpaul 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

@shinetzh@sayakpaul@HuggingFaceDocBuilderDev@yiyixuxu