Skip to content

Conversation

@sayakpaul
Copy link
Member

Closes #2377

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 2, 2023

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

ema_unet.step(unet.parameters())
assert ema_unet.optimization_step == 3

del unet, ema_unet
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't ever really manually call del on variables unless we absolutely need to force a garbage collection on a variable that is still in scope or for some reason the python garbage collector can't handle its de-allocation (I think this is sometimes the case if we have reference cycles).

In the case of these tests where we create the models at the beginning of the test and they fall out of scope at the end of the test, it looks like they're pretty straight forward for allocation and de-allocation and we shouldn't need to manually call del.

If I'm missing something and the ema class breaks python's gc and we do need to manually call del, it would be nice if we could instead fix the ema class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense! Keeping this comment open as it's useful discussion.

@williamberman
Copy link
Contributor

looks good! can we also test the serialization methods?

@sayakpaul sayakpaul marked this pull request as ready for review March 13, 2023 10:38
@sayakpaul
Copy link
Member Author

looks good! can we also test the serialization methods?

Done.

@sayakpaul sayakpaul requested a review from williamberman March 13, 2023 12:02
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.

Super nice! Thanks for adding the tests

@sayakpaul
Copy link
Member Author

@pcuenca

From this action:

FAILED tests/test_ema.py::EMAModelTests::test_serialization - RuntimeError: Placeholder storage has not been allocated on MPS device!

I can add skip_mps to test_serialization() but I wanted to know how to best handle it in this case. Here's test failure point: https://github.com/huggingface/diffusers/actions/runs/4411911784/jobs/7730867771#step:7:3173.

Could you advise?

@williamberman
Copy link
Contributor

@sayakpaul We don't support training on mps so just skip it 👍

Copy link
Contributor

@williamberman williamberman left a comment

Choose a reason for hiding this comment

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

perfect!

@sayakpaul
Copy link
Member Author

Will merge after the CI has run.

@sayakpaul sayakpaul merged commit f9cfb5a into main Mar 14, 2023
@sayakpaul sayakpaul deleted the tests/ema branch March 14, 2023 05:24
w4ffl35 pushed a commit to w4ffl35/diffusers that referenced this pull request Apr 14, 2023
* ema test cases.

* debugging maessages.

* debugging maessages.

* add: tests for ema.

* fix: optimization_step arg,

* handle device placement.

* Apply suggestions from code review

Co-authored-by: Will Berman <wlbberman@gmail.com>

* remove del and gc.

* address PR feedback.

* add: tests for serialization.

* fix: typos.

* skip_mps to serialization.

---------

Co-authored-by: Will Berman <wlbberman@gmail.com>
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
* ema test cases.

* debugging maessages.

* debugging maessages.

* add: tests for ema.

* fix: optimization_step arg,

* handle device placement.

* Apply suggestions from code review

Co-authored-by: Will Berman <wlbberman@gmail.com>

* remove del and gc.

* address PR feedback.

* add: tests for serialization.

* fix: typos.

* skip_mps to serialization.

---------

Co-authored-by: Will Berman <wlbberman@gmail.com>
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.

[Utils] Implement tests for EMAModel

5 participants