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

[fix] FreeInit step index out of bounds #8969

Merged
merged 5 commits into from
Jul 26, 2024
Merged

[fix] FreeInit step index out of bounds #8969

merged 5 commits into from
Jul 26, 2024

Conversation

a-r-r-o-w
Copy link
Member

What does this PR do?

When using schedulers that maintain an internal _step_index, FreeInit with fast mode disabled fails with an out of bounds error. Discovered here

Code to reproduce
import cProfile

import torch
from diffusers import MotionAdapter, AnimateDiffPipeline, DDIMScheduler, DPMSolverMultistepScheduler
from diffusers.utils import export_to_gif


def main():
    adapter = MotionAdapter.from_pretrained("guoyww/animatediff-motion-adapter-v1-5-2")
    model_id = "SG161222/Realistic_Vision_V5.1_noVAE"
    pipe = AnimateDiffPipeline.from_pretrained(model_id, motion_adapter=adapter, torch_dtype=torch.float16).to("cuda")
    # pipe.scheduler = DDIMScheduler.from_pretrained(
    pipe.scheduler = DPMSolverMultistepScheduler.from_pretrained(
        model_id,
        subfolder="scheduler",
        algorithm_type="dpmsolver++",
        beta_schedule="linear",
        clip_sample=False,
        timestep_spacing="linspace",
        steps_offset=1
    )

    pipe.enable_free_init(use_fast_sampling=False)

    output = pipe(
        prompt="a panda playing a guitar, on a boat, in the ocean, high quality",
        negative_prompt="bad quality, worse quality",
        num_frames=16,
        guidance_scale=7.5,
        num_inference_steps=20,
        generator=torch.Generator("cpu").manual_seed(42),
    )

    frames = output.frames[0]
    export_to_gif(frames, "animation.gif")

# main()
cProfile.run("main()", filename="log.txt")

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

@a-r-r-o-w a-r-r-o-w requested a review from DN6 July 25, 2024 10:02
@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.

nice catch!
should we add a test for when fast mode is disabled?

@a-r-r-o-w
Copy link
Member Author

nice catch! should we add a test for when fast mode is disabled?

thanks. yes, i think we should have another tiny freeinit test for atleast one scheduler that manages an internal state _step_index. will add it shortly

@a-r-r-o-w
Copy link
Member Author

a-r-r-o-w commented Jul 25, 2024

@yiyixuxu WDYT? Without the change in free_init_utils.py, the test here fails. I think DPMSolver and DDIM (used in every test except the one just added) cover all internal scheduler implementation cases. Added LCM just for safety since it is widely used with AnimateLCM. If it's alright, I'll add it to the other pipeline tests.

@yiyixuxu
Copy link
Collaborator

@a-r-r-o-w
That sounds good to me
I thought the issue was not with specific schedulers, but simply that when you use use_fast_sampling=False (the timesteps are not set in that case), I would imagine making another test_free_init with use_fast_sampling=False is already sufficient; but testing with more schedulers is better if it does not slow down too much :)

@a-r-r-o-w
Copy link
Member Author

You're partially right - when use_fast_sampling is False, the timesteps and _step_index are never reset, but that's not a problem with something like DDIM. For DPMSolver, however, the _step_index continues to increase unless reset, which we do now in the PR.

I don't think it's really necessary to add any more schedulers because I've rarely come across newer workflows with recommended settings being anything but Euler, Euler A, DDIM, DPMSolver variants, LCM and Align Your Steps for AnimateDiff.

i also think FreeInit is one of the lesser used features so we shouldn't need to burden our fast tests with too many tests for all settings of it. It can stabilize videos from models that don't work too well for video, but people wouldn't be using bad models in serious use cases anyway. AnimateLCM atleast pretty much doesn't seem to benefit much from the overhead of extra steps added IMO. We can revisit if it helps in specific workflows in the future :)

@DN6 DN6 merged commit 57a021d into main Jul 26, 2024
18 checks passed
@a-r-r-o-w a-r-r-o-w deleted the freeinit-bug branch July 26, 2024 11:16
a-r-r-o-w added a commit that referenced this pull request Jul 26, 2024
a-r-r-o-w added a commit that referenced this pull request Jul 26, 2024
DN6 pushed a commit that referenced this pull request Jul 26, 2024
* initial sparse control model draft

* remove unnecessary implementation

* copy animatediff pipeline

* remove deprecated callbacks

* update

* update pipeline implementation progress

* make style

* make fix-copies

* update progress

* add partially working pipeline

* remove debug prints

* add model docs

* dummy objects

* improve motion lora conversion script

* fix bugs

* update docstrings

* remove unnecessary model params; docs

* address review comment

* add copied from to zero_module

* copy animatediff test

* add fast tests

* update docs

* update

* update pipeline docs

* fix expected slice values

* fix license

* remove get_down_block usage

* remove temporal_double_self_attention from get_down_block

* update

* update docs with org and documentation images

* make from_unet work in sparsecontrolnetmodel

* add latest freeinit test from #8969

* make fix-copies

* LoraLoaderMixin -> StableDiffsuionLoraLoaderMixin
a-r-r-o-w added a commit that referenced this pull request Jul 30, 2024
* add animatediff controlnet to core

* make style; remove unused method

* fix copied from comment

* add tests

* changes to make tests work

* add utility function to load videos

* update docs

* update pipeline example

* make style

* update docs with example

* address review comments

* add latest freeinit test from #8969

* LoraLoaderMixin -> StableDiffusionLoraLoaderMixin

* fix docs

* Update src/diffusers/utils/loading_utils.py

Co-authored-by: Dhruv Nair <[email protected]>

* fix: variable out of scope

---------

Co-authored-by: Dhruv Nair <[email protected]>
sayakpaul pushed a commit that referenced this pull request Dec 23, 2024
* fix step index out of bounds

* add test for free_init with different schedulers

* add test to vid2vid and pia
sayakpaul pushed a commit that referenced this pull request Dec 23, 2024
* initial sparse control model draft

* remove unnecessary implementation

* copy animatediff pipeline

* remove deprecated callbacks

* update

* update pipeline implementation progress

* make style

* make fix-copies

* update progress

* add partially working pipeline

* remove debug prints

* add model docs

* dummy objects

* improve motion lora conversion script

* fix bugs

* update docstrings

* remove unnecessary model params; docs

* address review comment

* add copied from to zero_module

* copy animatediff test

* add fast tests

* update docs

* update

* update pipeline docs

* fix expected slice values

* fix license

* remove get_down_block usage

* remove temporal_double_self_attention from get_down_block

* update

* update docs with org and documentation images

* make from_unet work in sparsecontrolnetmodel

* add latest freeinit test from #8969

* make fix-copies

* LoraLoaderMixin -> StableDiffsuionLoraLoaderMixin
sayakpaul pushed a commit that referenced this pull request Dec 23, 2024
* add animatediff controlnet to core

* make style; remove unused method

* fix copied from comment

* add tests

* changes to make tests work

* add utility function to load videos

* update docs

* update pipeline example

* make style

* update docs with example

* address review comments

* add latest freeinit test from #8969

* LoraLoaderMixin -> StableDiffusionLoraLoaderMixin

* fix docs

* Update src/diffusers/utils/loading_utils.py

Co-authored-by: Dhruv Nair <[email protected]>

* fix: variable out of scope

---------

Co-authored-by: Dhruv Nair <[email protected]>
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