-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[tests] speed up animatediff tests #8846
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. |
…tuple_outputs_equivalent
Here's a before and after speed comparison of the SD1.5 AnimateDiff tests. I will improve the SDXL ones later since some errors after my changes were not straightforward to debug. The results below are from an A100 machine. The improvements are even better when running on just my CPU locally.
|
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.
I think we should try to implement test_from_pipe_consistent_config()
in this PR itself rather than skipping it.
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. For the value slices, could you follow https://www.notion.so/huggingface2/SSH-into-our-runners-d9c24de77d0843b2a308f28171106001?
@sayakpaul Fixed the expected slice values. Should be good to merge now. Failing tests are unrelated |
@@ -127,6 +133,36 @@ def get_dummy_inputs(self, device, seed=0): | |||
} | |||
return inputs | |||
|
|||
def test_from_pipe_consistent_config(self): | |||
assert self.original_pipeline_class == StableDiffusionPipeline |
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.
@a-r-r-o-w
is the only difference the repo? any reason we would not update the test in the tester mixin?
if self.original_pipeline_class == StableDiffusionPipeline: |
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.
We only made the model size smaller here, for AnimateDiff, no? It can definitely be applied across everything though to gain a good speed up. In fact, we can still make the model smaller with 1 layer per block instead of 2, smaller block out channels, smaller cross attn, etc. Will be some effort if we'd like to do it, but always good to have these tests running faster IMO
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.
I think for now it will be pretty easy and safe to update the from pipe tester mixin here
class PipelineFromPipeTesterMixin: |
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.
Won't updating that cause most, if not all, tests inheriting from PipelineFromPipeTesterMixin to fail?
For example, in the test test_from_pipe_consistent_config
we use hf-internal-testing/tiny-stable-diffusion-pipe
which has larger model sizes than the one we're using here (hf-internal-testing/tinier-stable-diffusion-pipe
).
In order to use the latter, we will have to update all model configurations in all tests as well as expected_slice values.
* speed up animatediff tests * fix pia test_ip_adapter_single * fix tests/pipelines/pia/test_pia.py::PIAPipelineFastTests::test_dict_tuple_outputs_equivalent * update * fix ip adapter tests * skip test_from_pipe_consistent_config tests * fix prompt_embeds test * update test_from_pipe_consistent_config tests * fix expected_slice values * remove temporal_norm_num_groups from UpBlockMotion --------- Co-authored-by: Sayak Paul <spsayakpaul@gmail.com> Co-authored-by: Dhruv Nair <dhruv.nair@gmail.com>
What does this PR do?
In the past, we tried speeding up tests with #7707 but that never got merged due to inactivity on my part. Picking it up again to run AnimateDiff tests much much faster than before.
Continuing from #8789 (comment). 8789 can probably be merge after we fix some of the broken tests here due to overlapping changes.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@DN6