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

Add LMSDiscreteSchedulerTest #467

Merged
merged 9 commits into from
Sep 16, 2022

Conversation

sidthekidder
Copy link
Contributor

@sidthekidder sidthekidder commented Sep 11, 2022

PR for #338, still requires some fixes

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 11, 2022

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

Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

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

Hi @sidthekidder thanks a lot for diving into these tests! The LMS scheduler differs quite a bit in its API from other schedulers (and needs refactoring, to be honest), so this is quite a challenge!

I've left some suggestions to make the full_loop test work, let me know if something's not clear :)
In general, since this scheduler is unique to Stable Diffusion at the moment, feel free to reference the SD pipeline for how it's used in practice.

self.check_over_configs(beta_start=beta_start, beta_end=beta_end)

def test_schedules(self):
for schedule in ["linear"]:
Copy link
Member

Choose a reason for hiding this comment

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

We should check over all shedules that are currently supported / in use

Suggested change
for schedule in ["linear"]:
for schedule in ["linear", "scaled_linear"]:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to add scaled_linear (squaredcos_cap_v2 is not implemented for LMSDiscreteScheduler)

Comment on lines 867 to 926

num_trained_timesteps = len(scheduler)

model = self.dummy_model()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
num_trained_timesteps = len(scheduler)
model = self.dummy_model()
model = self.dummy_model()
num_inference_steps = 10
scheduler.set_timesteps(num_inference_steps)

This scheduler requires a .set_timesteps() call. Reference: https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion.py#L225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

num_trained_timesteps = len(scheduler)

model = self.dummy_model()
sample = self.dummy_sample_deter
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sample = self.dummy_sample_deter
sample = self.dummy_sample_deter * scheduler.sigmas[0]

The initial sample needs to be multiplied by max sigma to make sure that it's within the supported range. Ref: https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion.py#L229

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, updated

model = self.dummy_model()
sample = self.dummy_sample_deter

for t in reversed(range(num_trained_timesteps - 1)):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for t in reversed(range(num_trained_timesteps - 1)):
for i, t in enumerate(scheduler.timesteps):

Can't get rid of enumeration yet, need both i and t for the loop in the current implementation 😅

Comment on lines 874 to 877
# 1. predict noise residual
residual = model(sample, t)
# print("residual: ")
# print(residual)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# 1. predict noise residual
residual = model(sample, t)
# print("residual: ")
# print(residual)
with torch.no_grad():
sigma = scheduler.sigmas[i]
sample = sample / ((sigma**2 + 1) ** 0.5)
model_output = model(sample, t)

Need to rescale the model input here too, to conform to the ODE equation used in K-LMS. Ref: https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion.py#L246

Comment on lines 879 to 887
# 2. predict previous mean of sample x_t-1
pred_prev_sample = scheduler.step(residual, t, sample).prev_sample

# if t > 0:
# noise = self.dummy_sample_deter
# variance = scheduler.get_variance(t) ** (0.5) * noise
#
# sample = pred_prev_sample + variance
sample = pred_prev_sample
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# 2. predict previous mean of sample x_t-1
pred_prev_sample = scheduler.step(residual, t, sample).prev_sample
# if t > 0:
# noise = self.dummy_sample_deter
# variance = scheduler.get_variance(t) ** (0.5) * noise
#
# sample = pred_prev_sample + variance
sample = pred_prev_sample
output = scheduler.step(model_output, i, sample)
sample = output.prev_sample

The LMS sampler takes i instead of an explicit timestep

Comment on lines 893 to 894
assert abs(result_sum.item() - 259.0883) < 1e-2
assert abs(result_mean.item() - 0.3374) < 1e-3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert abs(result_sum.item() - 259.0883) < 1e-2
assert abs(result_mean.item() - 0.3374) < 1e-3
assert abs(result_sum.item() - 1006.3885) < 1e-2
assert abs(result_mean.item() - 1.3104) < 1e-3

These are the reference values if all of the above suggestions are applied, make sure they match on your hardware! (if not - we'll need to adjust)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted values as necessary with the new output

@sidthekidder
Copy link
Contributor Author

sidthekidder commented Sep 13, 2022

Thanks for the detailed comments @anton-l! I didn't realize we could follow the implementation in pipeline_stable_diffusion.py, thanks for linking to the references - helped me understand the flow better.

Let me know if I am missing any test combinations or edge cases.

@sidthekidder sidthekidder changed the title [WIP] Add LMSDiscreteSchedulerTest Add LMSDiscreteSchedulerTest Sep 13, 2022
Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

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

Thank you @sidthekidder! I think all that's left is to implement test_pytorch_equal_numpy(), could you add that? :)
And feel free to modify the scheduler if it fails this test for some reason!

@sidthekidder
Copy link
Contributor Author

Added test_pytorch_equal_numpy similar to the PNDMSchedulerTest.test_pytorch_equal_numpy fn.

@patrickvonplaten
Copy link
Contributor

Nice looks good to me :-)

@patrickvonplaten
Copy link
Contributor

@anton-l if you could review one more time this would be great :-)

Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

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

I'll just remove the unused past residuals (they're needed only in the PNDM tests) and we're good :)

tests/test_scheduler.py Outdated Show resolved Hide resolved
tests/test_scheduler.py Outdated Show resolved Hide resolved
@anton-l
Copy link
Member

anton-l commented Sep 16, 2022

Thank you @sidthekidder!

@anton-l anton-l merged commit a54cfe6 into huggingface:main Sep 16, 2022
PhaneeshB pushed a commit to nod-ai/diffusers that referenced this pull request Mar 1, 2023
* [WEB] CSS changes to the web-ui (huggingface#465)

This commit updates UI with styling.

Signed-Off-by: Gaurav Shukla <[email protected]>

Signed-off-by: Gaurav Shukla <[email protected]>

* [WEB] Update the title (huggingface#466)

* [WEB] Add support for long prompts (huggingface#467)

* [WEB] fix background color

Signed-Off-by: Gaurav Shukla

* [WEB] Remove long prompts support

It removes support to long prompts due to higher lag in loading long prompts.

Signed-Off-by: Gaurav Shukla <gaurav@nod-labs>

* [WEB] Update nod logo and enable debug feature.

Signed-Off-by: Gaurav Shukla <[email protected]>

Signed-off-by: Gaurav Shukla <[email protected]>
Signed-off-by: Gaurav Shukla
Signed-off-by: Gaurav Shukla <gaurav@nod-labs>
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.

4 participants