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

Support CLIP skip for SDXL (see description) #12518

Closed
wants to merge 1 commit into from

Conversation

catboxanon
Copy link
Collaborator

@catboxanon catboxanon commented Aug 13, 2023

Description

Note: making this PR just to bring this to discussion.

This PR adds a form of supporting CLIP skip with SDXL. I added this since scripts like the XYZ plot support changing this and I don't think users should be mislead to believe all the CLIP layers behave the same, but this does have several issues:

  • SDXL/Stability trained with the penultimate clip layer in mind (CLIP skip = 2)
  • Most users likely don't know the aforementioned, and the webui defaults to CLIP skip = 1. This would basically cause a lot of users to produce much worse results than the model is capable of.
    • (the webui forces the penultimate layer currently so for current users reading this there is nothing to worry about)
  • This only allows for the last layer or penultimate layer to be used. The return value of encode_with_transformer only returns last, penultimate, and pooled (I don't know enough about SDXL's arch to know what this is honestly). The way Stability handles this also implies the last and penultimate are the only layers that should ever be used. https://github.com/Stability-AI/generative-models/blob/45c443b316737a4ab6e40413d7794a7f5657c19f/sgm/modules/encoders/modules.py#L442-L447

Also some potentially useful info, courtesy of Anonymous: https://boards.4channel.org/g/thread/95329616#p95329692

Checklist:

This supports a form of CLIP skip for SDXL, but has several issues.
Making this commit mostly just to bring this to discussion in a PR.
@catboxanon catboxanon changed the title Support CLIP slip for SDXL (see description) Support CLIP skip for SDXL (see description) Aug 13, 2023
@AUTOMATIC1111
Copy link
Owner

We have to make clip skip possible for SD1 because we can't tell from a checkpoint which clip skip it needs. For SDXL we can - it needs the next to last layer. Enabling clip skip support brings problems and as far as I know does not bring any benefits.

@catboxanon
Copy link
Collaborator Author

catboxanon commented Aug 13, 2023

I'm in agreement with that, but what should we do then for when users expect this to change results? https://boards.4channel.org/g/thread/95326794#p95329057

Maybe the display name should be changed? I'm not sure but it needs to somehow be communicated to the user since most aren't going to know this. "CLIP skip" is just a term that's thrown around ubiquitously to the average user.

@AUTOMATIC1111
Copy link
Owner

Can add explanation in settings...

@catboxanon
Copy link
Collaborator Author

Option already links back to the wiki so I guess I can add an explantation there. I think you and I both know users aren't going to read that though so I'll try to think of something for this, depending on if this comes up often enough. Closing.

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.

2 participants