Skip to content

Fix default parameter of text_encoder_projection_dim in _get_add_time_ids#10028

Open
viiika wants to merge 1 commit intohuggingface:mainfrom
viiika:fix_sdxl_text_encoder_dim_from_none_to_1280
Open

Fix default parameter of text_encoder_projection_dim in _get_add_time_ids#10028
viiika wants to merge 1 commit intohuggingface:mainfrom
viiika:fix_sdxl_text_encoder_dim_from_none_to_1280

Conversation

@viiika
Copy link
Copy Markdown

@viiika viiika commented Nov 26, 2024

fix default parameter of text_encoder_projection_dim from None to 1280

@sayakpaul sayakpaul requested a review from yiyixuxu November 28, 2024 10:47
@b-burton
Copy link
Copy Markdown

b-burton commented Dec 2, 2024

I think this change also needs to be made to made to pipline_controlnet_inpaint_sd_xl.py (and possibly others): https://github.com/huggingface/diffusers/blob/6db33337a49c2f20db1b8d2ad069cca10c552c68/src/diffusers/pipelines/controlnet/pipeline_controlnet_inpaint_sd_xl.py#L1072C5-L1117C1

Same issue that text_encoder_projection_dim=None should be text_encoder_projection_dim=1280

see:

def _get_add_time_ids(
self,
original_size,
crops_coords_top_left,
target_size,
aesthetic_score,
negative_aesthetic_score,
dtype,
text_encoder_projection_dim=None,
):


However I think there is an additional issue here,

passed_add_embed_dim = (
self.unet.config.addition_time_embed_dim * len(add_time_ids) + text_encoder_projection_dim
)

Should probably be changed to something like:

if text_encoder_projection_dim is not None:
     passed_add_embed_dim = (
           self.unet.config.addition_time_embed_dim * len(add_time_ids) + text_encoder_projection_dim
      )
else:
     passed_add_embed_dim = self.unet.config.addition_time_embed_dim * len(add_time_ids)

In order to handle len(add_time_ids) always being an int and then trying to add a NoneType (TypeError: unsupported operand type(s) for +: 'int' and 'NoneType')

@hlky
Copy link
Copy Markdown
Contributor

hlky commented Dec 4, 2024

When calling _get_add_time_ids we always pass in text_encoder_projection_dim, as you've pointed out the argument is actually required so I'm not sure why it was made optional, maybe this should be changed instead cc @yiyixuxu

@hlky hlky changed the title Update pipeline_stable_diffusion_xl.py Fix default parameter of text_encoder_projection_dim in _get_add_time_ids Dec 4, 2024
@yiyixuxu
Copy link
Copy Markdown
Collaborator

yiyixuxu commented Dec 4, 2024

yeah agree with @hlky

ok to remove = None part

@viiika
Copy link
Copy Markdown
Author

viiika commented Dec 6, 2024

yeah agree with @hlky

ok to remove = None part

but for sdxl, it seems all usage of text_encoder_projection_dim are set to 1280. So let users set a value might be redundant and confusing as many of the dependent codes are written with its default form.

Copy link
Copy Markdown
Contributor

@hlky hlky left a comment

Choose a reason for hiding this comment

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

We'll need to run make fix-copies after this change

Copy link
Copy Markdown
Author

@viiika viiika left a comment

Choose a reason for hiding this comment

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

this may damage some external dependencies

@hlky
Copy link
Copy Markdown
Contributor

hlky commented Dec 6, 2024

We consider functions prefixed with _ to be non-public, however any external dependency using _get_add_time_ids must be passing text_encoder_projection_dim as the function doesn't work without, removing = None will have no effect, the function is called in the same way.

@github-actions
Copy link
Copy Markdown
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions Bot added the stale Issues that haven't received updates label Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Issues that haven't received updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants