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

[Pipiline] Wuerstchen v3 aka Stable Cascasde pipeline #6487

Merged
merged 123 commits into from
Mar 6, 2024

Conversation

kashif
Copy link
Contributor

@kashif kashif commented Jan 8, 2024

What does this PR do?

Add Wuerstchen v3 pipeline

@kashif
Copy link
Contributor Author

kashif commented Jan 8, 2024

cc @dome272

@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.

.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still quite a bit of clean-up to do until we can merge this I think

@DN6 DN6 merged commit 40aa47b into huggingface:main Mar 6, 2024
15 checks passed
@vladmandic
Copy link
Contributor

all of the recent class renames broke actual model as https://huggingface.co/stabilityai/stable-cascade/blob/main/model_index.json refers to class names that no longer exist.
for example StableCascadeUnet vs StableCascadeUNet

@aa956
Copy link

aa956 commented Mar 6, 2024

all of the recent class renames broke actual model as https://huggingface.co/stabilityai/stable-cascade/blob/main/model_index.json refers to class names that no longer exist. for example StableCascadeUnet vs StableCascadeUNet

If I understand correctly this was expected and will be fixed by merging this PR on model repo?

@kashif
Copy link
Contributor Author

kashif commented Mar 6, 2024

correct @aa956 and @vladmandic

@vladmandic
Copy link
Contributor

feels like a sledgehammer solution to self-inflicted problem - users will need to manually delete existing copy of stable cascade and redownload it.

@sayakpaul
Copy link
Member

FWIW, it was from a PR branch and not even from the main branch. So, this should not be too surprising.

@vladmandic
Copy link
Contributor

FWIW, it was from a PR branch and not even from the main branch. So, this should not be too surprising.

fair point, i forgot that we needed a pr branch to start with.

@vladmandic
Copy link
Contributor

vladmandic commented Mar 6, 2024

also, what are plans to support loading different model variations? for each stage there is full or light and fp32 and bf16 for each (and unofficial fp16) right now, even with the pr, there is only one frozen variation available on huggingface as that is the only variation in actual hf folder-style format.

perhaps diffusers team could republish the model so all variations are actually available as variations and can be loaded using from_pretrained by specifying variation - that is what variation was designed for?

the current prs to model itself (44 for decoder and 2 for prior) only make it worse since they force fp32 version which is double the size and unusable by majority of users.

@FurkanGozukara
Copy link

FurkanGozukara commented Mar 7, 2024

yes we need load from single point (e.g. a single safetensors file). this is really really needed

@vladmandic
Copy link
Contributor

@DN6 @sayakpaul what are the plans to make SC actually available at https://huggingface.co/stabilityai/stable-cascade ?
PRs to modify model are still open and it doesn't look like StabilityAI has much interest in it - and even if it were merged, my previous comments still stand - if diffusers format is preferred, we need to make it actually available.

@sayakpaul
Copy link
Member

The merge decision lies with StabilityAI. Once merged that will become available. But until then we have to use the revision mechanism. I know that is not ideal but that is the best we have got so far.

You can also check out the open PRs in the repository where we are adding support for Stable Cascade single file checkpoint loading.

Cc: @apolinario

@vladmandic
Copy link
Contributor

The merge decision lies with StabilityAI.

Not necessarily - StabilityAI uploaded only single variant of the model in diffusers format.
And even if PR is accepted, its still going to be a single variant.
If diffusers prefer diffusers folder-style format, then why not re-publish the model with all available variants as expected in diffusers folder-style?
Its not like Huggingface team never republished an existing model.

Switching blame does not help users that want to use the model.

@sayakpaul
Copy link
Member

You want us to host the model on our org? Sorry, I didn’t mean to blame anyone and I apologise if it sounded like that.

Also, which variant in the diffusers format is currently missing?

@vladmandic
Copy link
Contributor

as you know, both stage_b and stage_c exist in 4 variants each: full fp32, full bf16, lite fp32, lite bf16
but if i look in stabilityai/stable-cascade/decoder or stabilityai/stable-cascade-prior/prior as originally published, i only see one variant published

the pr:44 in decoder and pr:2 in prior adds variants, but places lite variants in different folder - how can they be used with from_pretrained?

and yes, ideally stabiltiyai should accept those two prs (and even better, they should publish models with all variants in the first place). as it is, model is now 1 month from publication and still not usable (and pr is one week old) - so yes, if stabilityai does not accept pr, then perhaps huggingface should re-publish the model under hugggingface org.

@sayakpaul
Copy link
Member

the pr:44 in decoder and pr:2 in prior adds variants, but places lite variants in different folder - how can they be used with from_pretrained?

These will be separate pipelines for convenience. Refer to the scripts/convert_stable_cascade_lite.py in https://github.com/huggingface/diffusers/pull/7271/files.

and yes, ideally stabiltiyai should accept those two prs (and even better, they should publish models with all variants in the first place). as it is, model is now 1 month from publication and still not usable (and pr is one week old) - so yes, if stabilityai does not accept pr, then perhaps huggingface should re-publish the model under hugggingface org.

Duly noted. Thanks for explaining!

@yiyixuxu
Copy link
Collaborator

hi @vladmandic
for using lite version, we included an example here https://github.com/huggingface/diffusers/pull/7257/files#diff-abde182084148a55a5a5edf9df250650923a1c1686e3f138e9f40d587e74ec2c

how can they be used with from_pretrained?

@vladmandic
Copy link
Contributor

vladmandic commented Mar 12, 2024

a bit messy as it requires manual load of unet from subfolder and then create a pipeline
why not have lite simply as a variants as well?
btw, i think example is wrong, from diffusers import StableCascadeUNet doesn't exist, class is in diffusers.models

and does StableCascadeCombinedPipeline support lite?
i'm getting corrupt outputs, almost like vqgan scaling is off (no issues with full).

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Mar 12, 2024

@vladmandic

i'm getting corrupt outputs, almost like vqgan scaling is off (no issues with full).

we have a bug in the pipeline, this PR should fix it #7287

why not have lite simply as a variants as well?

ohh I like this idea, I think it will be easier to use indeed, that would require us to duplicate the checkpoint for other components though, cc @DN6 and @sayakpaul here, let me know what you guys think!

@yiyixuxu
Copy link
Collaborator

@vladmandic

and it should work with combined pipeline

prior_unet = StableCascadeUNet.from_pretrained("stabilityai/stable-cascade-prior", subfolder="prior_lite")
decoder_unet = StableCascadeUNet.from_pretrained("stabilityai/stable-cascade", subfolder="decoder_lite")

pipe_combined = DiffusionPipeline.from_pretrained("stabilityai/stable-cascade", prior_prior=prior_unet, decoder=decoder_unet, ...)

@sayakpaul
Copy link
Member

ohh I like this idea, I think it will be easier to use indeed, that would require us to duplicate the checkpoint for other components though, cc @DN6 and @sayakpaul here, let me know what you guys think!

Sounds good to me! In line with https://huggingface.co/stabilityai/stable-diffusion-xl-base-1.0/tree/main/vae_1_0.

@yiyixuxu
Copy link
Collaborator

@vladmandic
I realize the lite version has a different config so it won't work as a variant out-of-box
we may still want to explore this in the future though if more models are getting distributed this way

@bghira
Copy link
Contributor

bghira commented Mar 13, 2024

having a key for variant-specific values in the config might work?

something like

{
... other config values...
"variant_config": {
    "lite": {
        ....lite values....
    }
}

@sayakpaul
Copy link
Member

The "variant" keyword is reserved for loading checkpoints belonging to certain numerical precisions IIUC. So, I would like to keep it that way given that a model variant (in the sense that it's architecturally different) can also be loaded with subfolder as shown by @yiyixuxu in #6487 (comment).

Would love to know why and how that's not we're looking for here.

@bghira
Copy link
Contributor

bghira commented Mar 13, 2024

well, that's true. the subfolder can have config.json in there and it should use that to load that model type

@yiyixuxu
Copy link
Collaborator

@sayakpaul
variant is just used to load different versions of the model - it does not have to be limited to numerical precision, and there is no reason to limit it to that.

I'm not saying we should use variant here but that should not be a factor

@sayakpaul
Copy link
Member

variant is just used to load different versions of the model - it does not have to be limited to numerical precision, and there is no reason to limit it to that.

This can only be done when:

  1. The number of parameters of the underlying model is exactly the same.
  2. The config of the underlying model is exactly the same.

Model versions mean different things and can vary with respect to numerical precision, architecture, etc. I think the sole assumption behind "variant" is that we're operating with a model that respects the two points I enlisted above.

If those two are respected, then sure -- we definitely shouldn't be restrictive about it. But I don't think that is the case for "lite" here.

@bghira
Copy link
Contributor

bghira commented Mar 13, 2024

i might propose that model_index.json could have a list of subfolders to point to for a new keyword. something like collection.

model = DiffusionModel.from_pretrained('username/reponame', collection='lite')

model_index.json:

{
    "collections": {
        "lite": {
            "prior": "prior_lite",
            ...
        }
    }
}

@vladmandic
Copy link
Contributor

@sayakpaul i agree with your points. i was looking for a quick-easy solution and overloading variant is not it.
but we should have a way to specify "flavor" (full or lite) somehow. using subfolder to load unet and and then pass that to pipe later is not really elegant, its definitely not a single step and in application layer leads to many if/then branches.

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.