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

Allow non-default schedulers to be easily swapped into DiffusionPipeline classes #183

Closed
patrickvonplaten opened this issue Aug 16, 2022 · 19 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@patrickvonplaten
Copy link
Contributor

Is your feature request related to a problem? Please describe.

By default the stable diffusion pipeline uses the PNDM scheduler, but one could easily use other schedulers (we only need to overwrite the self.scheduler) attribute.

This can be done with the following code-snippet:

pipe = StableDiffusionPipeline.from_pretrained("CompVis/stable-diffusion-v1-3-diffusers", use_auth_token=True)  # make sure you're logged in with `huggingface-cli login`

# use DDIM scheduler
scheduler = DDIMScheduler(beta_start=0.00085, beta_end=0.012, beta_schedule="scaled_linear", clip_sample=False, clip_alpha_at_one=False)
pipe.scheduler = scheduler

Now, that's a bit hacky and not the way we want users to do it ideally!

Describe the solution you'd like

Instead, the following code snippet should work or less for all pipelines:

from diffusers import StableDiffusionPipeline, DDIMScheduler

# Use DDIM scheduler here instead
scheduler = DDIMScheduler(beta_start=0.00085, beta_end=0.012, beta_schedule="scaled_linear", clip_sample=False, clip_alpha_at_one=False)

pipe = StableDiffusionPipeline.from_pretrained("CompVis/stable-diffusion-v1-3-diffusers", scheduler=scheduler, use_auth_token=True)  # make sure you're logged in with `huggingface-cli login`

This is a cleaner & more intuitive API. The idea should be that every class variable that can be passed to

should also be overwrite-able when using from_pretrained(...)

When currently running this command it fails:

TypeError: cannot unpack non-iterable DDIMScheduler object 

Now we can allow such behavior by adding some logic to the general DiffusionPipeline from_pretrained method here:

def from_pretrained(cls, pretrained_model_name_or_path: Optional[Union[str, os.PathLike]], **kwargs):

Also we want this approach to work not just for one pipeline and only the scheduler class, but for all pipelines and all schedulers classes.
We can achieve this by doing more or less the following in

def from_pretrained(cls, pretrained_model_name_or_path: Optional[Union[str, os.PathLike]], **kwargs):

Pseudo code:

  1. Retrieve all variables that can be passed to the class init here:
    pipeline_class = getattr(diffusers_module, config_dict["_class_name"])

    -> you should get a list of keys such as [vae, text_encoder, tokenizer, unet, scheduler]
  2. Check if any of those parameters are passed in kwargs -> if yes -> store them in a dict passed_class_obj
  3. In the loop that loads the class variables:
    if is_pipeline_module:
    add a new if statements that checkes whether the name is in passed_class_obj dict -> if yes -> simple use this instead and skip the loading part (set the passed class to loaded_sub_model )

=> after the PR this should work:

from diffusers import StableDiffusionPipeline, DDIMScheduler

# Use DDIM scheduler here instead
scheduler = DDIMScheduler(beta_start=0.00085, beta_end=0.012, beta_schedule="scaled_linear", clip_sample=False, clip_alpha_at_one=False)

pipe = StableDiffusionPipeline.from_pretrained("CompVis/stable-diffusion-v1-3-diffusers", scheduler=scheduler, use_auth_token=True)  # make sure you're logged in with `huggingface-cli login`

where as this should give a nice error message (note how scheduler is incorrectly passed to vae):

from diffusers import StableDiffusionPipeline, DDIMScheduler

# Use DDIM scheduler here instead
scheduler = DDIMScheduler(beta_start=0.00085, beta_end=0.012, beta_schedule="scaled_linear", clip_sample=False, clip_alpha_at_one=False)

pipe = StableDiffusionPipeline.from_pretrained("CompVis/stable-diffusion-v1-3-diffusers", vae=scheduler, use_auth_token=True)  # make sure you're logged in with `huggingface-cli login`

The error message can be based on the passed class not having a matching parent class with what was expected (this could be checked using this dict:

LOADABLE_CLASSES = {
)

Additional context
As suggusted by @apolinario - it's very important to allow one to easily swap out schedulers. At the same time we don't want to create too much costum code. IMO the solution above handles the problem nicely.

@patrickvonplaten
Copy link
Contributor Author

Anybody keen on taking this one? Should be rather easy

@patrickvonplaten patrickvonplaten added the good first issue Good for newcomers label Aug 16, 2022
@patil-suraj
Copy link
Contributor

I like this solution, much cleaner! But I have one concern, this solution doesn't really swap the scheduler if I understand it correctly. It rather initilizes the whole pipeline again, which is a bit slow. By swapping I would expect to just swap the scheduler while keeping everything else in-place.

Should we maybe add some method called pipeline.set_scheduler, which will only swap the scheduler. This will help users who want to quickly generating things with multiple schedulers and won't have to load everything multiple times. But this could be a bit high-level also and we might not want to add in pipeline. Curious to know what you think @patrickvonplaten @anton-l

@patrickvonplaten
Copy link
Contributor Author

@patil-suraj nono it shouldn't init the pipeline again - if implemented correctly there is no need to re-initialize anything.
I'm not in favor of a set_scheduler method as this opens the door for many problems (e.g. we would adapt the model_index config after init which is not a good idea IMO

@patil-suraj
Copy link
Contributor

Thanks for clarifying, if we don't need to re-initialize anything then let's go with it !

@pcuenca
Copy link
Member

pcuenca commented Aug 16, 2022

I think @patil-suraj might be referring to a specialized use case where someone would want to experiment with different schedulers in the same session. So they'd create a pipeline with the default scheduler, and then they'd need to create a new pipeline with a different scheduler. Is this something worthwhile doing, especially given the can of worms @patrickvonplaten mentioned?

@Jpbonino
Copy link

good morning friends, sorryx my ignorance, but I am getting an error in the HF pipeline route and I honestly don't know what commands I should write in colab, thank you very much.

@patil-suraj
Copy link
Contributor

Hi @Jpbonino , could you open a different issue and post the code-snippet and the error that you are having ?

@Jpbonino
Copy link

done @patil-suraj #193

@patrickvonplaten
Copy link
Contributor Author

Issue has been solved with #188

@reimager
Copy link
Contributor

Maybe I'm misunderstanding, but I don't understand how #188 helps 'swap' a scheduler in an existing pipeline.
It appears just to allow the caller to specify a scheduler at the time the pipeline is created, but doesn't seem to provide any way to be swapped out later.

But my reading above is that the code in the original post:

scheduler = DDIMScheduler(...) pipe.scheduler = scheduler

is OK and acceptable to change a scheduler on an existing pipeline?

The reason swapping is desirable is if you want to support X different schedulers, if you can't swap, you need to load X different pipelines, which means you need X times more memory, or break it apart into multiple processes/sessions.

@brandnewx
Copy link

brandnewx commented Dec 1, 2022

is OK and acceptable to change a scheduler on an existing pipeline?

No, you have to set it in the StableDiffusionPipeline.from_pretrained()

@patrickvonplaten
Copy link
Contributor Author

Hey @reimager,

We advise to use the following API to swap schedulers:

from huggingface_hub import login
from diffusers import DiffusionPipeline
import torch

# Now we can download the pipeline
pipeline = DiffusionPipeline.from_pretrained("runwayml/stable-diffusion-v1-5", torch_dtype=torch.float16)

from diffusers import DDIMScheduler

pipeline.scheduler = DDIMScheduler.from_config(pipeline.scheduler.config)

Note that a longer tutorial is also here: https://huggingface.co/docs/diffusers/using-diffusers/schedulers

@jslegers
Copy link

Is the PNDM scheduler still the default scheduler for SD? If so, why?

According to the Huggingface article Training Stable Diffusion with Dreambooth using 🧨 Diffusers, DDIM usually works much better than PNDM and LMSDiscrete for overfitted models. So, from an end user perspective, what are the advantages of PNDM?

@brandnewx
Copy link

brandnewx commented Dec 29, 2022

@jslegers That article talks about different schedulers during inference. Now people are already using something like DPM++ SDE @ 20 steps. It's not the noise scheduler during training. I tested PNDM and DDIM for dreambooth training, and they both work the same.

@jslegers
Copy link

jslegers commented Dec 30, 2022

@brandnewx :

Thing is, I've been working on my own finetuned general purpose SD 1..5 model, similar to Openjourney Diffusion, Dreamlike Diffusion & Seek.art_META.

In my experience, the PNDM scheduler is pretty bad for running this type of models in the DiffusionPipeline, as the DDIM Scheduler gives much better results (at least for 20 steps)... which is why it puzzles me why the PNDM scheduler is the default.

Thusfar, as an end user, I have no clue why I'd ever want to use the PNDM scheduler on any model and why I shouldn't make the DDIM or DPM++ Scheduler the default scheduler for all my finetuned models...

As a software developer who's still fairly new to AI, I find any documentation I can find on schedulers pretty cryptic and inaccessible...

@brandnewx
Copy link

brandnewx commented Dec 30, 2022

@jslegers You're just confused between noise scheduler during training and noise scheduler during inference. The article you mentioned clearly states "Using DDIM for inference seems to be more robust. "

Noise schedulers for training don't seem to matter at all, because you're setting a discrete learning rate anyway. If you switch PNDM to other schedulers, you will just overfit the model.

@jslegers
Copy link

jslegers commented Dec 30, 2022

I'm not confused. I am indeed refering to inference. Why do you presume I'm talking about training?

When you load a 1.x model into a DiffusionPipeline, it loads PNDM scheduler by default for inference, as described in the config.json.

I don't understand why this is the default if this is an inferior scheduler for inference. It took me countless hours of tinkering & a bit of luck to figure out I could greatly improve he performance & quality of my models (or any other model, really) during inference just by changing the scheduler in the json config.

@brandnewx
Copy link

brandnewx commented Dec 31, 2022

@jslegers Told you even DDIM in that article is outdated info; nobody uses it right now for SD inferences. You're supposed to replace the default with one of the latest noise schedulers, which are Euler Ancestral, PLMS, and DPM++ SDE. You also need to set your own step count that works best for the noise scheduler.

There's nothing wrong with PNDM. It's just slow and also requires more step count.

@jslegers
Copy link

jslegers commented Dec 31, 2022

You're supposed to replace the default with one of the latest noise schedulers, which are Euler Ancestral, PLMS, and DPM++ SDE.

I find that out yesterday... after wasting dozens of hours trying out different models by running them in diffusers with a PNDM scheduler.

I'm getting good results with DPM++ and changed the default for all my models to DPM++. Would you say that is the best to use as a default. If not, which would you recommend? I know the AUTOMATIC1111 GUI used to use Euler a as the default, but it's ages ago since I used it...

Pretty much every other SD diffusers model on Huggingface still uses PNDM, though. Same for scripts that convert ckpt to diffusers format. It's all PNDM... except for Seek.art_META & Dreamlike Diffusion, which use DDIM...

I suppose most devs either can't keep up with recent developments or simply don't care about the diffusers model because most end users use the ckpt file to run it on AUTOMATIC1111. Either way, as someone who's struggling to find his way through the maze that is the SD ecosystem it's pretty damn frustrating to find out dozens of hours were wasted due to outdated settings in config files...

PhaneeshB pushed a commit to nod-ai/diffusers that referenced this issue Mar 1, 2023
All the torch_models are imported to gs::shark_tank.
Scripts have been updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

10 participants