Skip to content

Conversation

@wbruna
Copy link
Contributor

@wbruna wbruna commented May 24, 2025

CLIPTextModel ignored setting clip_skip back to -1, instead retaining whatever value was previously defined.

This is not relevant to command-line sd, since it's currently not possible to change clip_skip between generations, but could affect any frontend that reuses model instances for multiple images.

@LostRuins
Copy link
Contributor

I don't think this fix is correct. It's breaking some models now.

@wbruna
Copy link
Contributor Author

wbruna commented May 30, 2025

@LostRuins , could you please check if those models work with clip_skip explicitly set to 2?

I did some additional tests, and I suspect this change ended up setting the default clip_skip value to 1 for all models. All SD 1.5 models I've tested seem OK, but some SDXL models do produce black, or pure noise images with clip_skip=1, even before this change (that seems related to their base models: CyberRealistic_Pony and CyberIllustrious are broken, while CyberRealisticXL works fine).

I don't know if making all SDXL models default to clip_skip 2 is "right", but that'd be outside the scope of a fix anyway; I'll change the PR.

@esolithe
Copy link

I've left a comment on the KCPP PR, but as @LostRuins mentions I have experienced issues with the changes outlined.

Manually setting it to 2 works, but I am unsure if this is the correct behaviour to default to - I know the existing logic used to generate images consistently with the SDXL models I use, but beyond that I'm afraid I don't have much more information.

@wbruna
Copy link
Contributor Author

wbruna commented May 30, 2025

Reopened at #697 .

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.

3 participants