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

[Feat] Enable State Dict For Textual Inversion Loader #3439

Conversation

ghunkins
Copy link
Contributor

Enable passing a pre-loaded state dict via the TextualInversionLoaderMixin.

Benchmarked the implementation of LoraLoaderMixin.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 15, 2023

The documentation is not available anymore as the PR was closed or merged.

@patrickvonplaten
Copy link
Contributor

Hey @ghunkins,

What exactly is the use case to allow for this? Would be great to better understand why we need it :-)

@ghunkins
Copy link
Contributor Author

ghunkins commented May 17, 2023

Hi @patrickvonplaten,

This addition simplifies the loading of a textual inversion if you already have the textual inversion in memory. Currently, you have to save the textual inversion to disk and then pass in the filename. This addition removes that intermediary step.

Use cases this simplifies:

  • Fetching and loading a Textual Inversion via an API
  • Fetching and loading a Textual Inversion via a memory store (e.g. Redis)
  • Custom serialization/deserialization schemes

Additionally, this also gives this loader feature parity with the LoraLoaderMixin where this is already enabled!

@ghunkins
Copy link
Contributor Author

Tried to re-run the tests to get CI to pass, but I am guessing #3449 is the fix for that.

Requesting review before this gets lost, thanks @patrickvonplaten @patil-suraj @williamberman! 🙏

@patrickvonplaten
Copy link
Contributor

Hey @ghunkins,

Generally, this PR works for me! Could you maybe also add a quick test to this test function:

def test_text_inversion_download(self):
?

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Changes look good for me. Like @patrickvonplaten mentioned, would be nice to add some tests to ensure that the changes are robust.

I'd also like a review from @pdoane since they helped us with batch loading of text inversion parameters (#3193).

Copy link
Contributor

@pdoane pdoane left a comment

Choose a reason for hiding this comment

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

Looks good!

@patrickvonplaten
Copy link
Contributor

@ghunkins let me know if you need more clarification on how to add a test :-)

@ghunkins
Copy link
Contributor Author

@patrickvonplaten Great call, tests are added! Let me know if this works.

cc @sayakpaul @pdoane

@ghunkins ghunkins requested review from sayakpaul and pdoane May 27, 2023 14:04
Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

What a comprehensive coverage of tests! Love'em, just love'em!

Thanks so much!

@ghunkins
Copy link
Contributor Author

Great, thanks @sayakpaul! So much easier when great tests are already there to build off of and benchmark against 🚀

Let me know if there's anything else I can do to get this across the finish line. Looks like only contributors with write access can merge.

cc @patrickvonplaten @pdoane

@patrickvonplaten
Copy link
Contributor

Very cool PR, good job! @ghunkins

@patrickvonplaten patrickvonplaten merged commit 799f5b4 into huggingface:main May 30, 2023
7 checks passed
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* enable state dict for textual inversion loader

* Empty-Commit | restart CI

* Empty-Commit | restart CI

* Empty-Commit | restart CI

* Empty-Commit | restart CI

* add tests

* fix tests

* fix tests

* fix tests

---------

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* enable state dict for textual inversion loader

* Empty-Commit | restart CI

* Empty-Commit | restart CI

* Empty-Commit | restart CI

* Empty-Commit | restart CI

* add tests

* fix tests

* fix tests

* fix tests

---------

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
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.

None yet

5 participants