Skip to content

Conversation

@DN6
Copy link
Collaborator

@DN6 DN6 commented Feb 5, 2024

What does this PR do?

The Deepfloyd IF slow tests runs all pipelines in a single test. This is a bit difficult to debug when there are timeouts/failed tests. Splitting these into individual tests.

Fixes # (issue)

Before submitting

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.

@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.

@DN6 DN6 requested a review from sayakpaul February 5, 2024 10:49
mask_image = floats_tensor((1, 3, 256, 256), rng=random.Random(1)).to(torch_device)

output = pipe_2(
output = pipe_super_res(
Copy link
Member

Choose a reason for hiding this comment

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

Decouple this one as well?


mem_bytes = torch.cuda.max_memory_allocated()
assert mem_bytes < 10 * 10**9
assert mem_bytes < 13 * 10**9
Copy link
Member

Choose a reason for hiding this comment

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

Why?

image = floats_tensor((1, 3, 64, 64), rng=random.Random(0)).to(torch_device)

output = pipe_2(
output = pipe_super_res(
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't do super-resolution here.

Instead, it could make sense to pre-serialize the inputs and pass them accordingly to the pipeline for testing. This should reduce some timing, too.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I should just move them to their individual files. We have fast tests for the individual pipelines. So the Slow tests can live there as well.

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 IS the way to go! Thanks!

My only concern is about how we're doing super-resolution after every core pipeline tasks. I don't think that is necessary. I have left a comment about it.

@sayakpaul sayakpaul requested a review from yiyixuxu February 5, 2024 12:03
Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

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

makes sense and PR looks good to me:)
I'm ok if you want to split it into the existing individual files too

@DN6 DN6 merged commit e6a48db into main Feb 6, 2024
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
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.

5 participants