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

Expanded init fields in StableDiffusionPipeline cause incompatibilities with many/most inherited pipelines #6969

Open
vladmandic opened this issue Feb 13, 2024 · 19 comments
Assignees
Labels
bug Something isn't working contributions-welcome good first issue Good for newcomers help wanted Extra attention is needed wip

Comments

@vladmandic
Copy link
Contributor

vladmandic commented Feb 13, 2024

Describe the bug

class StableDiffusionPipeline has its init section as:

    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,
    ):

and here image_encoder was recently introduced thus changing the class signature
but most community pipelines do not include init field for recently included image_encoder and thus order or params is wrong

for example, examples/community/regional_prompting_stable_diffusion.py has this in its init:

        super().__init__(
            vae,
            text_encoder,
            tokenizer,
            unet,
            scheduler,
            safety_checker,
            feature_extractor,
            requires_safety_checker,
        )

which means bool value from requires_safety_checker is going to be passed as image_encoder and pipeline will fail during initialization like this:

> diffusers/pipelines/pipeline_utils.py:546 in _fetch_class_library_tuple
AttributeError: 'bool' object has no attribute '__module__'

this is a conceptual problem with changing master class signature while all inherited classes pass args list as simple list.
i don't see a simple solution as going back is bad and going forward requires updates to a lot of pipelines.

but at the very basic, at least add error handling to _fetch_class_library_tuple so invalid type does not cause entire solution to crash.

Reproduction

load nearly any community pipeline or any pipeline inherited from StableDiffusionPipeline, but not updated to use new signature in its super().__init__ call

Logs

No response

System Info

diffusers==0.26.3

Who can help?

@yiyixuxu @DN6 @sayakpaul @patrickvonplaten

@vladmandic vladmandic added the bug Something isn't working label Feb 13, 2024
@sayakpaul
Copy link
Member

Cc: @yiyixuxu

@yiyixuxu
Copy link
Collaborator

Hi @vladmandic
It only breaks pipelines that inherit from StableDiffusionPipeline instead of DiffusionPipeine - all our pipelines should inherit from DiffusionPipeline.

I don't consider this a breaking change for this reason. However would very much like to make the community pipelines work correctly again. We can ask the community to help refactor these PRs to use DiffusionPipeline as a base class instead - does this plan work for you?

cc @sayakpaul @DN6 here
do you guys have any good ideas?

I think we should start to check the base classes for community pipeline PRs and don't merging in unless they use DiffusionPipeline

@sayakpaul
Copy link
Member

What you suggested is the best case here as it's the simplest.

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Feb 14, 2024

@vladmandic

Is this something can be done in SD.Next for a temporary solution while we trying to update all the community pipelines to use DiffusionPipeline?

but at the very basic, at least add error handling to _fetch_class_library_tuple so invalid type does not cause entire solution to crash.

@vladmandic
Copy link
Contributor Author

vladmandic commented Feb 14, 2024

thanks - i agree with action plan.
i'll try monkey-patching _fetch_class_library_tuple from sdnext in the meantime.

@vladmandic
Copy link
Contributor Author

i confirm that monkey-patching works as as stop-gap solution

from diffusers.pipelines import pipeline_utils

def hijack_register_modules(self, **kwargs):
    for name, module in kwargs.items():
        if module is None or isinstance(module, (tuple, list)) and module[0] is None:
            register_dict = {name: (None, None)}
        elif isinstance(module, bool):
            pass
        else:
            library, class_name = pipeline_utils._fetch_class_library_tuple(module)
            register_dict = {name: (library, class_name)}
        self.register_to_config(**register_dict)
        setattr(self, name, module)

pipeline_utils.DiffusionPipeline.register_modules = hijack_register_modules

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Mar 15, 2024
@sayakpaul
Copy link
Member

I think this is not stale. @yiyixuxu can you confirm?

@sayakpaul sayakpaul removed the stale Issues that haven't received updates label Mar 15, 2024
Copy link

github-actions bot commented Apr 9, 2024

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Apr 9, 2024
@vladmandic
Copy link
Contributor Author

ping.

@github-actions github-actions bot removed the stale Issues that haven't received updates label Apr 10, 2024
Copy link

github-actions bot commented May 4, 2024

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label May 4, 2024
@vladmandic
Copy link
Contributor Author

ping.

@github-actions github-actions bot removed the stale Issues that haven't received updates label Oct 3, 2024
@asomoza
Copy link
Member

asomoza commented Oct 4, 2024

@vladmandic do yo maybe have a list of the community pipelines that are you interested in fixing? Probably will be better if we open an issue with a specific list than a generic one.

Regional prompting is the main one right? For some time I wanted to take a look into it and maybe do a new one with the same for controlnet.

@vladmandic
Copy link
Contributor Author

i'd need to go over the list of community pipelines to see which ones are most useful.
regional prompting was a great idea a long time ago, but original pipeline was never ported to sdxl so its value has diminished.

@asomoza
Copy link
Member

asomoza commented Oct 4, 2024

yeah, I'll probably take that one, I mean the SDXL one, I don't use SD 1.5 anymore but I can still fix it as a bonus while making the SDXL one.

Copy link

github-actions bot commented Nov 2, 2024

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Nov 2, 2024
@a-r-r-o-w a-r-r-o-w removed the stale Issues that haven't received updates label Nov 2, 2024
Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Nov 27, 2024
@vladmandic
Copy link
Contributor Author

ping to remove stale

@a-r-r-o-w a-r-r-o-w removed the stale Issues that haven't received updates label Nov 27, 2024
@asomoza
Copy link
Member

asomoza commented Nov 27, 2024

added the wip so it doesn't get stale.

Also added the contributions tags so if people want to contribute we would really appreciate it. This is a good first issue for new comers.

@asomoza asomoza added the good first issue Good for newcomers label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working contributions-welcome good first issue Good for newcomers help wanted Extra attention is needed wip
Projects
None yet
Development

No branches or pull requests

5 participants