Skip to content

Conversation

@anton-l
Copy link
Member

@anton-l anton-l commented Jan 5, 2023

Now the unconditional EMA wrapper mimics the EMAModel from train_text_to_image.py. This isn't a full copy because the unconditional example uses a different decay schedule.

Fixes broken training on multiple GPUs: #1772 #1895

action="store_true",
default=True,
help="Whether to use Exponential Moving Average for the final model weights.",
)
Copy link
Member Author

@anton-l anton-l Jan 5, 2023

Choose a reason for hiding this comment

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

Also removing the wrong default, as it was done for the text2img example (issue #1654)

Copy link
Contributor

Choose a reason for hiding this comment

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

small breaking change but think that's fine!

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jan 5, 2023

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

@patil-suraj patil-suraj self-assigned this Jan 17, 2023
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.

The PR looks very nice to me! I'd also just try to make it more or less 100% backwards compatible (let's try to be role-models when it comes to that in OSS).

Think it's not too hard:

  • detect if a model is passed
  • probs have to keep a mapping of parameters to names to be able to use new logic but also return model as done previously
  • we can relatively quickly deprecate here then I think

@patil-suraj
Copy link
Contributor

The EMAModel should now be 100% backwards compatible. @patrickvonplaten @pcuenca would be nice if you could take one more look.

@patil-suraj
Copy link
Contributor

The failing tests are unrelated.

Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

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

Looks great! Just pointed out a couple nits.

@patrickvonplaten
Copy link
Contributor

Good to merge for me - thanks @patil-suraj !

@patil-suraj patil-suraj merged commit 7c82a16 into main Jan 19, 2023
@patil-suraj patil-suraj deleted the fix-unconditional-ema branch January 19, 2023 10:55
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
…ace#1930)

* improve EMA

* style

* one EMA model

* quality

* fix tests

* fix test

* Apply suggestions from code review

Co-authored-by: Pedro Cuenca <[email protected]>

* re organise the unconditional script

* backwards compatibility

* default to init values for some args

* fix ort script

* issubclass => isinstance

* update state_dict

* docstr

* doc

* Apply suggestions from code review

Co-authored-by: Pedro Cuenca <[email protected]>

* use .to if device is passed

* deprecate device

* make flake happy

* fix typo

Co-authored-by: patil-suraj <[email protected]>
Co-authored-by: Pedro Cuenca <[email protected]>
Co-authored-by: Patrick von Platen <[email protected]>
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.

6 participants