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.

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

Mochi docs #9934

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Mochi docs #9934

wants to merge 4 commits into from

Conversation

DN6
Copy link
Collaborator

@DN6 DN6 commented Nov 15, 2024

What does this PR do?

Update Mochi docs

Fixes # (issue)

Before submitting

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.

@DN6 DN6 requested a review from sayakpaul November 15, 2024 11:38
@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

@Ednaordinary Ednaordinary left a comment

Choose a reason for hiding this comment

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

Noticed a few things this PR would be helpful to change

prompt = "Close-up of a chameleon's eye, with its scaly skin changing color. Ultra high resolution 4k."

with torch.autocast("cuda", torch.bfloat16, cache_enabled=False):
frames = pipe(prompt, num_frames=84).frames[0]

Choose a reason for hiding this comment

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

num_frames=84 should be num_frames=85, (14 * 6 + 1) like mentioned here


## Using a lower precision variant to save memory

The following example will use the `bfloat16` variant of the model and requires 22GB VRAM to run. There is a slight drop in the quality of the generated video as a result.

Choose a reason for hiding this comment

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

should bf16 be used? fp16 seems to have better precision, unless the autocasted weights compute in fp32 and somehow provide better precision?

Copy link
Collaborator Author

@DN6 DN6 Nov 16, 2024

Choose a reason for hiding this comment

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

We followed the official repo, which cast to bf16.
https://github.com/genmoai/models/blob/1a96b5df0c83018fcbe7aebbbb1b34781e179921/src/genmo/mochi_preview/pipelines.py#L344

I haven't fully compared bf16 vs fp16 for full downcasting though. Are you seeing much better results with fp16?

Choose a reason for hiding this comment

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

I am indeed, though I'm also doing mixed weight precision between float8 and f16. It could be beneficial to test further.

Copy link
Member

Choose a reason for hiding this comment

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

We can keep it to bfloat16 for now (as done in this PR) following the official recommendations. Then @Ednaordinary you could maybe help us opening a PR to the docs including your findings? WDYT?

Choose a reason for hiding this comment

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

Sure, that works for the moment

@@ -25,6 +25,50 @@ Make sure to check out the Schedulers [guide](../../using-diffusers/schedulers.m

Copy link

@Ednaordinary Ednaordinary Nov 15, 2024

Choose a reason for hiding this comment

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

(One line above this) Only FlowMatchEulerDiscreteScheduler has invert_sigmas, so anything else wouldn't work as of now as I understand it

pipe.enable_vae_tiling()

prompt = "Close-up of a chameleon's eye, with its scaly skin changing color. Ultra high resolution 4k."
frames = pipe(prompt, num_frames=84).frames[0]

Choose a reason for hiding this comment

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

same thing, num_frames=85

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Comments from @Ednaordinary are already great, so let's resolve them. Maybe we could add a section on how to reproduce some of their videos generated with the original inference code and params? I think most people would be interested in that.

Additionally, it seems like we should suggest using a maximum sequence length of 256?
#9769 (comment)

Already the case:

max_sequence_length: int = 256,

@DN6 DN6 mentioned this pull request Nov 27, 2024
6 tasks
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.

4 participants