Fix LLMGroundedDiffusionPipeline super class arguments

[KristianMischke]

What does this PR do?

This PR makes requires_safety_checker a keyword argument so it doesn't collide with StableDiffusionPipeline.image_encoder in the parameter order.

Fixes #5992

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline?
• Did you read our philosophy doc (important for complex PRs)?
• Was this discussed/approved via a GitHub issue or the forum? Please add a link to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

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.

@TonyLianLong

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

[TonyLianLong]

Thanks for the fix. It's interesting that it somehow does not error on my end. Is it a recent change?

[TonyLianLong]

After tracing the change, I found #5713 broke the positional argument order. This fix is indeed needed. I also suggest replacing safety_checker and feature_extractor positionals with keywords in case we have future changes in the order of the arguments.

Thank you!

[TonyLianLong]

Just verified on colab and it still gives an error. Tested commit: git+https://github.com/KristianMischke/diffusers.git@8c07e30.

The original colab: https://colab.research.google.com/drive/1SXzMSeAB-LJYISb2yrUOdypLz4OYWUKj
You need to replace the diffusers version in pip install with git+https://github.com/KristianMischke/diffusers.git@8c07e30.

Steps to reproduce:

1. !wget https://raw.githubusercontent.com/KristianMischke/diffusers/8c07e30b143938491864607e47aa65955599dad0/examples/community/llm_grounded_diffusion.py -O llm_grounded_diffusion.py
2. Use local custom pipeline:

pipe = DiffusionPipeline.from_pretrained(
    "longlian/lmd_plus",
    custom_pipeline="llm_grounded_diffusion.py",
    variant="fp16", torch_dtype=torch.float16
)

The error says [the pipeline] has been incorrectly initialized or <class 'diffusers_modules.local.llm_grounded_diffusion.LLMGroundedDiffusionPipeline'> is incorrectly implemented. Expected {'tokenizer', 'unet', 'scheduler', 'safety_checker', 'text_encoder', 'feature_extractor', 'vae'} to be defined, but dict_keys(['vae', 'text_encoder', 'tokenizer', 'unet', 'scheduler', 'safety_checker', 'feature_extractor', 'image_encoder']) are defined.

at

[/usr/local/lib/python3.10/dist-packages/diffusers/pipelines/pipeline_utils.py](https://localhost:8080/#) in components(self)
   1952 
   1953         if set(components.keys()) != expected_modules:
-> 1954             raise ValueError(
   1955                 f"{self} has been incorrectly initialized or {self.__class__} is incorrectly implemented. Expected"
   1956                 f" {expected_modules} to be defined, but {components.keys()} are defined."

Somehow the commit #5713 breaks things in a more complicated way than I think it is... It introduces an image_encoder that conflicts with the original state design.

The proper way to fix this is to replace the initialization and super call with (i.e., add image_encoder):

    def __init__(
        self,
        vae: AutoencoderKL,
        text_encoder: CLIPTextModel,
        tokenizer: CLIPTokenizer,
        unet: UNet2DConditionModel,
        scheduler: KarrasDiffusionSchedulers,
        safety_checker: StableDiffusionSafetyChecker,
        feature_extractor: CLIPImageProcessor,
        image_encoder: CLIPVisionModelWithProjection = None,
        requires_safety_checker: bool = True,
    ):
        super().__init__(
            vae,
            text_encoder,
            tokenizer,
            unet,
            scheduler,
            safety_checker=safety_checker,
            feature_extractor=feature_extractor,
            image_encoder=image_encoder,
            requires_safety_checker=requires_safety_checker,
        )
        # other parts of the initialization

After that it works on my end on colab.

@KristianMischke could you help add this to the PR and check whether it works on colab?

[KristianMischke]

@TonyLianLong Thanks for investigating further! Applied your changes and it's working properly now in the colab notebook with my latest commit

[yiyixuxu]

ok by me but is there any reason we use StableDiffusionPipeline as super class, instead of DiffusionPipeline? if not maybe let's just change the super class to DiffusionPipeline? pipelines should not use StableDiffusionPipeline as base class at all

[TonyLianLong]

I use StableDiffusionPipeline as a superclass because we inherit many methods from it. If we inherit directly from DiffusionPipeline we have to copy things over (which we do not change at all, such as encode_image). It's hard to say which one will be more compatible. I am ok with both options. @yiyixuxu @KristianMischke

[yiyixuxu]

We recommend using the DiffusionPipeline and use #Copy from statement to copy over methods that are relevant to your pipeline :) We do not expect StableDiffusionPipeline to be used as a superclass at all so that's not a use case we maintain at diffusers.

Are you the author of this pipeline? I can merge this quickly for you now if that's what you want. Let me know:)

[TonyLianLong]

Thanks for your suggestions! Yes, I'm the author of this pipeline, and I'd like to make it easier to maintain. Let's inherit from DiffusionPipeline then. Do you have a reference of # Copy from statement in other pipelines?

I can take the work to update the inheritance, or before I get some time to do this @KristianMischke you can also help with this if you feel excited about it.

[yiyixuxu]

We don't have it in the doc (I think we should, though! cc @sayakpaul @stevhliu what do you think?)

but it's everywhere in our codebase; here is an example

diffusers/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion_img2img.py

Line 323 in f72b28c

# Copied from diffusers.pipelines.stable_diffusion.pipeline_stable_diffusion.StableDiffusionPipeline.encode_prompt

Once you added this #Copy from statement to the encode_prompt method of the img2img pipeline and specified where it's copying from, every time you run make fix-copies it will automatically update if there is a change in the code it's copying from (i.e., stable diffusion's encode_prompt method in this case)

[yiyixuxu]

@KristianMischke

let me know if you want to work on this - if not I will merge this PR and @TonyLianLong can open a different PR later

[stevhliu]

We don't have it in the doc (I think we should, though!)

Sounds good to me, maybe we can add it to the "How to contribute?" doc under Adding pipelines, models, and schedulers?

[KristianMischke]

@yiyixuxu and @TonyLianLong thanks for the continued correspondence! Given my limited time, I'd say merge and allow @TonyLianLong to adjust the structure later

[TonyLianLong]

Somehow the colab still does not work: https://colab.research.google.com/drive/1SXzMSeAB-LJYISb2yrUOdypLz4OYWUKj

Will look into this when I get time.

[TonyLianLong]

OK, colab fixed. Seems like we need to specify custom_revision to main in order to load the latest version of the pipeline. I will work on migrating it to DiffusionPipeline.

Example:

pipe = DiffusionPipeline.from_pretrained(
    "longlian/lmd_plus",
    custom_pipeline="llm_grounded_diffusion",
    custom_revision="main",
    variant="fp16", torch_dtype=torch.float16
)

pipe.enable_model_cpu_offload()

[Skquark]

I'd also like to add another small fix when peft is installed with diffusers, gives the TypeError: Linear.forward() got an unexpected keyword argument 'scale'
On line 168, added this:
args = () if USE_PEFT_BACKEND else (scale,)
along with importing USE_PEFT_BACKEND at the top. Simple enough, that patch is probably missing from other pipelines too...

[TonyLianLong]

Could you create an issue and assign it to me? @Skquark