Merging Stable diffusion pipelines just makes sense

[leszekhanusz]

Following the Philosophy, it has been decided to keep different pipelines for Stable Diffusion for txt-to-img, img-to-img and inpainting.

Here is the result:

• PR Add an argument "negative_prompt" #549 : code duplicated 4 times (onnx included)
• PR stable diffusion using < 2.3GB of GPU memory #537 : modification only applied to txt-to-img pipeline
• PR Add callback parameters for Stable Diffusion pipelines #521 : code duplicated 4 times (onnx included)
• PR Removing autocast for 35-25% speedup. (autocast considered harmful). #511 : modification only applied to txt-to-img pipeline
• PR Add better compatibility with diffusers-interpret (and possibly other use cases!) #506 : modification only applied to txt-to-img pipeline
• PR Warning for too long prompts in DiffusionPipelines (Resolve #447) #472 : code duplicated 4 times (onnx included)
• PR Optimize Stable Diffusion #371 : modification only applied to txt-to-img pipeline

It's getting even worse! Now with onnx and the obvious request to have img-to-img and inpainting pipelines with onnx (#510), that would mean that every modification will need to be duplicated 6 times in total, with 6 times the number of tests.

This in untenable...

[leszekhanusz]

I just noticed that in PR #549, the code was not duplicated exactly the same way. Sometimes multiplying by batch_size, sometimes not.

Proof again than duplicating code is never good and introduce bugs.

[zledas]

I'm new here, but I also noticed, that most of the code is just duplicated, which is very hard when you want to make changes to all three :(

[shirayu]

Good suggestion. I agree with you.

[anton-l]

Hi @leszekhanusz you raise an important question, as the number of Stable Diffusion pipelines is rising, and we don't have a robust copy mechanism like the one in transformers yet.

Inviting @patil-suraj @patrickvonplaten @pcuenca to discuss possible solutions here :)

In the meantime, I'll address the mismatching pipeline issues manually, thank you for a thorough compilation!

[patrickvonplaten]

We can just add copy-mechanism, we have in Transformers.

Quite strongly against creating a unified pipeline as it makes it impossible down the road to define classes like AutoPipelineForText2Image.

Also note that default values cannot be coherently defined anymore. In text2image we use 50 inference steps in image2image just 30 => I don't want to clutter the code with if type == ... statements.

We'll sure not merge onnx and non onnx pipeline because one should not have onnx installed to be able to use the pipeline and also optimization might very well be pipeline specific.

For more general arguments regarding this design see also here: https://huggingface.co/blog/transformers-design-philosophy

Overall:

• Happy to add a "one-size-fits-it-all" community pipeline which we'll also support in pip install diffusers in 1,2 weeks by mirroring community pipelines an the Hub. If people use this extensively, we can also add this to src/diffusers
• User experience is much more important for us than maintenance. We should not write code that is only concerned with improving our maintenance, we should mostly write code that is easy to use and won't have us run into big backwards breaking problems in the future.

What do you think @leszekhanusz ?

[leszekhanusz]

I've read your blog post and you make some good points but some of it seems to contradict you:

we decided to adopt a different design principle which we like to call the single model file policy. The single model file policy states that all code necessary for the forward pass of a model is in one and only one file.

Here we have a single model (Stable diffusion) which has 3 different pipelines (ignoring onnx for now) in different files doing more or less the same thing.
We could argue that having all the code in a single pipeline is actually following your single model file policy.

Did you look at what the code looks like in the unified pipeline? You have if init_image is None: and if mask_image is not None: conditions and it's really not complicated to follow those branches.
On the contrary, now it's getting easier to see the difference between img-to-img and inpainting than to try to compare code in different files.

For someone like me which is quite new to diffusion models, what's complicated is to understand the model, what is classifier free guidance, schedulers, torch broadcasting and shapes of tensors. Tuning out a block if mask_image is not None: is a piece of cake in comparison and allow to see more easily the difference between the different usages.

We can just add copy-mechanism, we have in Transformers.

To be honest, from an external eye, this seems like a hack to me.

Quite strongly against creating a unified pipeline as it makes it impossible down the road to define classes like AutoPipelineForText2Image.

I don't see why. If you set init_image and mask_image to None, then the pipeline is the same as the text-to-img pipeline.

Also note that default values cannot be coherently defined anymore. In text2image we use 50 inference steps in image2image just 30 => I don't want to clutter the code with if type == ... statements.

First of all, for most use-cases the user will provide the number of inference steps himself as it is quite an important parameter which he would probably like to make configurable.

But if you want to keep backward compatibility and allow custom default for each pipeline, I propose to use create a StableDiffusionCommonPipeline which will contain the unified pipeline. Then the current StableDiffusionPipeline used only for text-to-image could be replaced by something like:

class StableDiffusionPipeline(StableDiffusionCommonPipeline):

    def __call__(                                                                                                       
        self,                                                                                                           
        prompt: Union[str, List[str]],                                                                                  
        height: Optional[int] = 512,                                                                                    
        width: Optional[int] = 512,                                                                                     
        num_inference_steps: Optional[int] = 50,                                                                        
        guidance_scale: Optional[float] = 7.5,                                                                          
        eta: Optional[float] = 0.0,                                                                                     
        generator: Optional[torch.Generator] = None,                                                                    
        latents: Optional[torch.FloatTensor] = None,                                                                    
        output_type: Optional[str] = "pil",                                                                             
        return_dict: bool = True,                                                                                       
        **kwargs,                                                                                                       
    ): 
    	super().__call__(
    		prompt=prompt,
    		height=height,
    		...
    		init_image=None,
    		mask_image=None,
    		...
    	)

We'll sure not merge onnx and non onnx pipeline because one should not have onnx installed to be able to use the pipeline and also optimization might very well be pipeline specific.

Sure, I agree.

Happy to add a "one-size-fits-it-all" community pipeline which we'll also support in pip install diffusers in 1,2 weeks by mirroring community pipelines an the Hub. If people use this extensively, we can also add this to src/diffusers

That's great! From the user point of view that is really important. That "one-size-fits-all" pipeline should really be inside src/diffusers so that it could be integrated easily in other projects and automatically updated with an upgrade of diffusers.

User experience is much more important for us than maintenance. We should not write code that is only concerned with improving our maintenance, we should mostly write code that is easy to use and won't have us run into big backwards breaking problems in the future.

That's good to hear, but the user experience could be better right now.

[okalldal]

Another aspect to this from a user experience perspective is that right now naively loading all pipelines via from_pretrained takes three times the GPU memory. If you want to use all three in the same script or gradio interface you have to either load the models separately and init the pipelines via the initializer manually or juggle the pipelines in and out of your GPU memory.

[leszekhanusz]

@okalldal it's possible to merge all the existing pipelines into a single one. Available here.

[patrickvonplaten]

Very good points here! Thanks a lot for writing all of this down. I still don't agree with most of it here though 😅
Writing my thoughts down a final time before giving in haha.

We could argue that having all the code in a single pipeline is actually following your single model file policy.
Clear no. Single model file policy means exactly to not build one model that fits it all but to have multiple modeling files. This was misunderstood. This was written for transformers so it cannot be applied one-to-one here.

On the contrary, now it's getting easier to see the difference between img-to-img and inpainting than to try to compare code in different files.
Don't agree either. We now cannot set sensible defaults anymore, clutter the __call__ with input args (like strength latents mask_image ....) How would one know what guidance scale should be used for the pipelines, what num inference steps to set? It's not true that most people understand num_inference_steps well enough that it doesn't require a good default. People will mix functionality for different tasks and we'll get issues about I believe. Let's see!

A point that is not taken into account a lot here IMO: Users of diffusers include both "end-users" and "packages".
"packages" are the ones that build on top of diffusers and many good "packages" is more important than direct "end-users". PyTorch is so highly used because every ML package can super easily build on it, it has an intuitive design, ... not because they started adding a stable diffusion model in their core library.

• "end-users" usually prefer very much the "one-size-fits-it-all" solutions which makes a lot of sense! Writing the same code every time for your use case is annoying! But note that we will never be able to full fill everyone's use case!
• "packages" are libraries that integrate this library into other libraries & Python packages and thus favor a cleaner logical API, having a clear distinction between different tasks, unified API, no backwards breaking problems. Unified API will be broken with this pipeline because it's not a "task" pipeline anymore but a "diffusion model specific" pipeline class. It won't fit in this table: https://github.com/huggingface/diffusers/tree/main/src/diffusers/pipelines#pipelines-summary -> we'd have to give it multiple task names. This will have more consequences in the future. It won't fit in a "AutoText2ImagePipeline" class.
  E.g. having all image-to-image, in-painting, and text2image is very stable diffusion specific. Having everything in one pipeline make us prone to future breaking changes. Most arguments here fall away for the "library" users:
• I can build a 10-line one size fits it all pipeline that uses no more memory:

from diffusers import StableDiffusionPipeline, StableDiffusionImg2ImgPipeline, StableDiffusionInpaintPipeline
from diffusers import DiffusionPipeline

stable_diffusion = DiffusionPipeline.from_pretrained("CompVis/stable-diffusion-v1-4")
sub_models = {k: v for k,v in vars(pipeline).items() if not k.startswith("_")}

text2img = StableDiffusionPipeline(**sub_models)
img2img = StableDiffusionImg2ImgPipeline(**sub_models)
inpaint = StableDiffusionInpaintPipeline(**sub_models)

def all_in_one(type, *args, **kwargs):
    if type == "text2img":
         return text2img(*args, **kwargs)
    ...

Note: {k: v for k,v in vars(pipeline).items() if not k.startswith("_")} is very hacky but we could very well provide a nice function for this in the downstream DiffusionPipeline class (cc @patil-suraj @anton-l ) I think this would be useful anyways.

• Why is this better? This way the diffusers repo can focus on making clean task-specific pipelines and doesn't have to put the burden of maintenance higher lever functionality on us. E.g. now image stable diffusion has two more task that can be done with the same model => having an official one-fits-it-all pipeline puts us then in a tough spot: "where do we draw the line", "can we add the new features without making the code unreadable? Do we have to break existing behavior?" => it becomes obvious that the one-size-fits it all will quickly break
• I think a big problem is that we don't advertise well enough how diffusers is intended to be used. E.g. it's really not that hard to build something on top of diffusers that doesn't require 3x the memory usage IMO

Nevertheless, I do want to have this library be community driven, but I think the above aspects and the long-term vision for the library is not taken into account enough.

=> So if you want, happy to follow your advice, but added it as a "experimental" pipeline. However I won't maintain the pipeline and ping you quite a bit on issues I think @leszekhanusz haha ;-). Thanks for taking the time to write everything down though - "easy-of-use" for such a young library might indeed be the winning argument in the end of the day.

Think this is a cool discussion, so will also asked the Twitter community on their opinion :-)

Also cc @keturn @shirayu @johnowhitaker @jonatanklosko @JoaoLages @pcuenca @patil-suraj @anton-l @kashif @sgrigory very interested in hearing your final thoughts!

[patrickvonplaten]

Also here a twitter poll: https://twitter.com/PatrickPlaten/status/1572980566579941381

[johnowhitaker]

I think I'm on team 'three classes'.
If you want to do a quick demo of one application, easy to just load, say, text2img.
If you want multiple your example is very helpful, it'd be nice to document what the sub_models are and how to avoid loading duplicates like you do there.
I also think a lot of people trying more complex things won't be loading three pipelines, they'll be defining their own sampling process anyway - where they can add all the extra features they want.
That being said, I'm not the one having to maintain the code ;)

[JoaoLages]

I understand the '3 classes' arguments but, as @leszekhanusz mentioned, having the code duplicated for all of them is very error-prone.
IMO, the solution proposed by @leszekhanusz of having the backbone class is what I thought of.

To exemplify:

class CoreStableDiffusionPipeline:
    ...
    def _forward(prompt, init_image: Optional = None, mask_image: Optional = None, ...):
        ....

class StableDiffusionPipeline(CoreStableDiffusionPipeline):
    ...
    def __call__(prompt, ...):
        return self._forward(prompt, ...)

class StableDiffusionImg2ImgPipeline(CoreStableDiffusionPipeline):
    ...
    def __call__(prompt, init_image, ...):
        assert init_image is not None # avoid incorrect class usage
        return self._forward(prompt, init_image, ...)

class StableDiffusionInpaintPipeline(CoreStableDiffusionPipeline):
    ...
    def __call__(prompt, init_image, mask_image, ...):
        assert init_image is not None and mask_image is not None  # avoid incorrect class usage
        return self._forward(prompt, init_image, mask_image, ...)

This way we would still keep the 3 classes separated and the code would be way easier to maintain.
Do you guys see any disadvantages in this solution?

[patrickvonplaten]

Yes! People now need to switch between multiple files CoreStableDiffusionPipeline and StableDiffusionInpaintPipeline . Maintaining different files is really not that big of a problem and much less of a concern in OSS than in industry settings

[JoaoLages]

Yes! People now need to switch between multiple files CoreStableDiffusionPipeline and StableDiffusionInpaintPipeline .

If that's a problem, the CoreStableDiffusionPipeline subclasses could be all in the same file.
This solution is better to avoid adding/removing functionality in one of the pipelines but not in the others, that's the way I see it.