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.

Sign up for GitHub

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

Jump to bottom

[Cog] some minor fixes and nits #9466

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

[Cog] some minor fixes and nits #9466

wants to merge 6 commits into from
+65 −61

Conversation

sayakpaul
Copy link
Member

@sayakpaul sayakpaul commented Sep 19, 2024 •
edited
Loading

What does this PR do?

  • Cleans up check_inputs()
  • Moves the generator check in prepare_latents() at the beginning of the method so that we can error as early as possible.
  • Documentation nits.

@HuggingFaceDocBuilderDev
Copy link

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.

a-r-r-o-w
a-r-r-o-w approved these changes Sep 19, 2024
View reviewed changes
Copy link
Member

@a-r-r-o-w a-r-r-o-w left a comment

Choose a reason for hiding this comment

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

Oops 😬 So, we don't have a test that passes both prompt embeds and negative prompt embeds? I believe that would have caught this, no? Surprising and very lucky that it was working for the normal prompt inputs so far.
Apologies for missing this on my part

@sayakpaul sayakpaul removed the request for review from yiyixuxu September 19, 2024 04:37
@sayakpaul sayakpaul marked this pull request as draft September 19, 2024 04:37
@sayakpaul
Copy link
Member Author

So, we don't have a test that passes both prompt embeds and negative prompt embeds? I believe that would have caught this, no?

I believe that could be added but the test would be about checking if an error is thrown if prompt and prompt_embeds are both passed and other adjacent areas.

@sayakpaul
Copy link
Member Author

Sorry, I have additional updates to make. Will let you all know here after that.

@sayakpaul sayakpaul changed the title [Cog V2V] fix positional arguments in check_inputs(). [Cog] some minor fixes and nits Sep 19, 2024
sayakpaul
sayakpaul commented Sep 19, 2024
View reviewed changes
src/diffusers/pipelines/cogvideo/pipeline_cogvideox.py
Comment on lines 580 to -581

height = height or self.transformer.config.sample_size * self.vae_scale_factor_spatial
width = width or self.transformer.config.sample_size * self.vae_scale_factor_spatial
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need it because height and width are already at their default values.

sayakpaul
sayakpaul commented Sep 19, 2024
View reviewed changes
src/diffusers/pipelines/cogvideo/pipeline_cogvideox_image2video.py
Comment on lines +210 to +212
self.vae_scaling_factor_image = (
self.vae.config.scaling_factor if hasattr(self, "vae") and self.vae is not None else 0.7
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Is beneficial for scenarios where we want to run the pipeline without the VAE.

@sayakpaul sayakpaul mentioned this pull request Sep 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-r-r-o-w a-r-r-o-w a-r-r-o-w 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.
3 participants
@sayakpaul @HuggingFaceDocBuilderDev @a-r-r-o-w