-
Notifications
You must be signed in to change notification settings - Fork 5.9k
[Kolors] Add PAG #8934
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
[Kolors] Add PAG #8934
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
add a test?
Added the fast tests and I fixed the old ones because I needed to get the original Kolors Pipeline to test the AutoPipeline. In short, I had to add all the missing tests I had as TODO for kolors. @yiyixuxu @sayakpaul should I add the slow tests for PAG? this one has a really big text encoder which is not in transformers yet. |
I would say, it'd be okay skipping the SLOW tests for now. |
We should try and merge this before next release, no? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's add the pipeline to the list of pipelines we support in the docs?
@unk_token.setter | ||
def unk_token(self, value: str): | ||
self._unk_token = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like these should have been done in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I don't do this, the tests fails (throws AttributeError), I'm ok with doing this in a separate PR but then I'll have to skip the tests in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay. Then no worries.
# throws AttributeError: property 'eos_token' of 'ChatGLMTokenizer' object has no setter | ||
# not sure if it is worth to fix it before integrating it to transformers | ||
def test_save_load_optional_components(self): | ||
# TODO (Alvaro) need to fix later | ||
pass | ||
super().test_save_load_optional_components(expected_max_difference=2e-4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Probably should have been done in a separate PR.
* txt2img pag added * autopipe added, fixed case * style * apply suggestions * added fast tests, added todo tests * revert dummy objects for kolors * fix pag dummies * fix test imports * update pag tests * add kolor pag to docs --------- Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
What does this PR do?
Adds PAG to kolors
Note: Kolors with PAG behaves different than with SDXL, I had to use a lower scale and different layers to get good results.
How to test
Txt2Img
After the PAG refactor, the image changed a bit, not sure why:
Who can review?
@yiyixuxu @a-r-r-o-w @sunovivid @sayakpaul