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

[tests] make tests device-agnostic (part 3) #10437

Merged
merged 31 commits into from
Jan 21, 2025
Merged

Conversation

faaany
Copy link
Contributor

@faaany faaany commented Jan 3, 2025

What does this PR do?

Followed by PR #9399 and #9400 , this PR further makes current cuda-only tests device-agnostic.

Copy link
Collaborator

@hlky hlky left a comment

Choose a reason for hiding this comment

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

Thanks @faaany. I've left a couple comments.

tests/pipelines/controlnet/test_controlnet.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@hlky hlky left a comment

Choose a reason for hiding this comment

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

I've added some more suggestions and comments. Happy to assist with replicating the requested changes to all areas, let me know!

src/diffusers/utils/testing_utils.py Outdated Show resolved Hide resolved
src/diffusers/utils/testing_utils.py Show resolved Hide resolved
src/diffusers/utils/testing_utils.py Show resolved Hide resolved
src/diffusers/utils/testing_utils.py Outdated Show resolved Hide resolved
src/diffusers/utils/testing_utils.py Outdated Show resolved Hide resolved
tests/pipelines/controlnet/test_controlnet.py Outdated Show resolved Hide resolved
tests/pipelines/controlnet/test_controlnet.py Outdated Show resolved Hide resolved
@faaany
Copy link
Contributor Author

faaany commented Jan 7, 2025

I've added some more suggestions and comments. Happy to assist with replicating the requested changes to all areas, let me know!

Thanks so much for the suggestions! code updated. And for further replication, I will submit a follow-up PR today. And for the rest, I would be very happy if you could help.

@faaany
Copy link
Contributor Author

faaany commented Jan 7, 2025

Do you really need "BACKEND_RESET_MAX_MEMORY_ALLOCATED"? This function will all reset_peak_memory_stats anyway as can be seen from here: https://pytorch.org/docs/stable/generated/torch.cuda.reset_max_memory_allocated.html

@hlky
Copy link
Collaborator

hlky commented Jan 7, 2025

Do you really need "BACKEND_RESET_MAX_MEMORY_ALLOCATED"? This function will all reset_peak_memory_stats anyway as can be seen from here: https://pytorch.org/docs/stable/generated/torch.cuda.reset_max_memory_allocated.html

We can look at this in another PR.

@HuggingFaceDocBuilderDev

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.

@hlky hlky requested a review from DN6 January 8, 2025 08:47
@hlky hlky requested a review from sayakpaul January 8, 2025 08:47
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.

This looks very promising! Thank you!

Have you run the tests for which the changes are being introduced in this PR?

src/diffusers/utils/testing_utils.py Outdated Show resolved Hide resolved
@faaany
Copy link
Contributor Author

faaany commented Jan 8, 2025

This looks very promising! Thank you!

Have you run the tests for which the changes are being introduced in this PR?

yes, I run the tests on xpu and except backend_reset_max_memory_allocated, all tests work. But I can fix it in another PR as @hlky mentioned earlier.

@hlky
Copy link
Collaborator

hlky commented Jan 8, 2025

@faaany backend_reset_max_memory_allocated should be a no-op for non-cuda, if it's giving an error we'll need to fix that. I was referring to using reset_max_memory_allocated when that function has changed to also call reset_peak_memory_stats, we'll review that change in behavior later.

@faaany
Copy link
Contributor Author

faaany commented Jan 8, 2025

sure, let me update.

@faaany
Copy link
Contributor Author

faaany commented Jan 9, 2025

Hi @sayakpaul @hlky @DN6 , could you do final-round review? Since I will have follow-up PRs, it would be great that it could be merged this week. Thanks a lot!

@faaany
Copy link
Contributor Author

faaany commented Jan 14, 2025

Hi folks, any feedback?

@hlky hlky requested a review from yiyixuxu January 17, 2025 07:29
@hlky hlky merged commit ec37e20 into huggingface:main Jan 21, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants