Not copying the model in inference and assigning ema_weights seems incorrect.

[dasayan05]

Describe the bug

This recent commit https://github.com/huggingface/diffusers/commit/44f6bc81c764897406952bf75263b866de7c8cd3 regarding the PR #2166 removed copying the model while doing inference. Is this correct ? Aren't we substituting EMA weight to the underlying model (in the next line) which is being trained ?

cc: @patil-suraj @pcuenca

PS: @stathius noticed this bug.

Reproduction

The example file itself.

Logs

No response

System Info

• diffusers version: 0.13.0.dev0
• Platform: Linux-5.4.0-26-generic-x86_64-with-glibc2.31
• Python version: 3.9.16
• PyTorch version (GPU?): 1.13.1+cu117 (True)
• Huggingface_hub version: 0.11.1
• Transformers version: 4.26.0
• Accelerate version: 0.15.0
• xFormers version: not installed
• Using GPU in script?:
• Using distributed or parallel set-up in script?:

[patrickvonplaten]

@williamberman @patil-suraj could you take a look here?

[williamberman]

I think @dasayan05 is correct here. My understanding is that the underlying parameters in the prepared model and the unwrapped model are still the same. EMAModel.copy_to will then copy the ema tensors over the non-ema tensors in the existing parameters. So every time we do this sample, we then start training over the ema values.

@patil-suraj @pcuenca is this correct?

[sayakpaul]

Yeah, this is indeed a bug. Probably with https://github.com/huggingface/diffusers/pull/2302 merged, we can modify the script accordingly. A working example of how it might look like in practice is available here. Is is what you had in mind?

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

[patrickvonplaten]

Is this issue solved or do we still need to do something here? cc @sayakpaul

[sayakpaul]

Yeah this issue is solved now with https://github.com/huggingface/diffusers/pull/2302

[williamberman]

I think this is still a bug as store() and restore() are not called in train_unconditional

[patrickvonplaten]

Thanks for the follow-up PR @williamberman !