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

Make LoRACompatibleConv padding_mode work. #6031

Merged
merged 9 commits into from
Feb 27, 2024
Merged

Make LoRACompatibleConv padding_mode work. #6031

merged 9 commits into from
Feb 27, 2024

Conversation

jinghuan-Chen
Copy link
Contributor

@jinghuan-Chen jinghuan-Chen commented Dec 3, 2023

What does this PR do?

fix #5957

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.

@sayakpaul
Copy link
Member

Thanks for your PR. Could you:

  • Run make style && make quality so that the CI is green.
  • Add a fast test to verify the implementation correctness?

Once done, I will run our SLOW tests (such as https://github.com/younesbelkada/diffusers/blob/peft-part-2/tests/lora/test_lora_layers_peft.py) to ensure nothing broke.

Comment on lines 358 to 363
if self.padding_mode != 'zeros':
return F.conv2d(F.pad(hidden_states, self._reversed_padding_repeated_twice, mode=self.padding_mode),
self.weight, self.bias, self.stride,
(0, 0), self.dilation, self.groups)
return F.conv2d(hidden_states, self.weight, self.bias, self.stride,
self.padding, self.dilation, self.groups)
Copy link
Member

Choose a reason for hiding this comment

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

Can we compute original_outputs one time and the reuse it? That way, I think the code will remain cleaner.

Copy link
Contributor Author

@jinghuan-Chen jinghuan-Chen Dec 4, 2023

Choose a reason for hiding this comment

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

Thanks for your guidance.
There is currently no better, cleaner code solution. If you have, please give me some guidance.

testing script

import torch
from diffusers import DiffusionPipeline


# Modify the padding mode of Conv2d
def set_pad_mode(network, mode="circular"):
    for _, module in network.named_children():
        if len(module._modules) > 0:
            set_pad_mode(module, mode)
        else:
            if isinstance(module, torch.nn.Conv2d):
                module.padding_mode = mode


base = DiffusionPipeline.from_pretrained(
    "stabilityai/stable-diffusion-xl-base-1.0",
    torch_dtype=torch.float16,
    variant="fp16",
    use_safetensors=True,
)
base.to("cuda")
n_steps = 30
prompt = "interior design, Equirectangular, Panoramic, Panorama and 360"

set_pad_mode(base.vae, "circular")
set_pad_mode(base.unet, "circular")


image = base(
    prompt=prompt,
    height=1024,
    width=2048,
    num_inference_steps=n_steps,
    output_type="pil",
).images[0]

Copy link
Member

Choose a reason for hiding this comment

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

I don't this comment was addressed:

#6031 (comment)

Also, by testing, I meant adding a test to our testing suite. In this case, you add one here and here.

Does that make sense?

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@patrickvonplaten
Copy link
Contributor

What exactly is the use case here that we're trying to solve? Why do we need this padding_mode in the first place?

@jinghuan-Chen
Copy link
Contributor Author

What exactly is the use case here that we're trying to solve? Why do we need this padding_mode in the first place?

360 panoramic image generation. The rightmost and the leftmost sides of the 360 panoramic image should be continued,as shown below:
grocery
Due to the above characteristics, padding_mode needs to be changed to "circular" mode when generating a panorama to provide context and alleviate seam problems.

@@ -355,13 +355,34 @@ def forward(self, hidden_states: torch.Tensor, scale: float = 1.0) -> torch.Tens
if self.lora_layer is None:
# make sure to the functional Conv2D function as otherwise torch.compile's graph will break
# see: https://github.com/huggingface/diffusers/pull/4315
if self.padding_mode != "zeros":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we give this branch a better name? E.g. self.padding_mode == "reversed_adding" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The padding mode of torch conv2d only supports 'zeros', 'reflect', 'replicate' or 'circular'.Default: 'zeros'

@sayakpaul
Copy link
Member

@patrickvonplaten could you review this once?

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.

Can we maybe try to simplify the code a bit?

Simplify the code by patrickvonplaten.

Co-authored-by: Patrick von Platen <[email protected]>
@jinghuan-Chen
Copy link
Contributor Author

Can we maybe try to simplify the code a bit?

Thank you for your new implementation ideas.However, if the user switches the padding mode from "circular" to "zeros", problems may occur because self.padding has been set to (0, 0).

Comment on lines 365 to 378
if self.padding_mode != "zeros":
original_outputs = F.conv2d(
F.pad(hidden_states, self._reversed_padding_repeated_twice, mode=self.padding_mode),
self.weight,
self.bias,
self.stride,
(0, 0),
self.dilation,
self.groups,
)
else:
original_outputs = F.conv2d(
hidden_states, self.weight, self.bias, self.stride, self.padding, self.dilation, self.groups
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if self.padding_mode != "zeros":
original_outputs = F.conv2d(
F.pad(hidden_states, self._reversed_padding_repeated_twice, mode=self.padding_mode),
self.weight,
self.bias,
self.stride,
(0, 0),
self.dilation,
self.groups,
)
else:
original_outputs = F.conv2d(
hidden_states, self.weight, self.bias, self.stride, self.padding, self.dilation, self.groups
)
if self.padding_mode != "zeros":
original_outputs = F.conv2d(
F.pad(hidden_states, self._reversed_padding_repeated_twice, mode=self.padding_mode),
self.weight,
self.bias,
self.stride,
(0, 0),
self.dilation,
self.groups,
)
else:
original_outputs = F.conv2d(
hidden_states, self.weight, self.bias, self.stride, self.padding, self.dilation, self.groups
)

Can we refactor this part in the same way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patrickvonplaten Hi, thank you for your guidance. I have refactored the code, please review it.

Comment on lines 355 to 374
if self.padding_mode != "zeros":
hidden_states_pad = F.pad(hidden_states, self._reversed_padding_repeated_twice, mode=self.padding_mode)
original_outputs = F.conv2d(
hidden_states_pad,
self.weight,
self.bias,
self.stride,
(0, 0),
self.dilation,
self.groups,
)
else:
# make sure to the functional Conv2D function as otherwise torch.compile's graph will break
# see: https://github.com/huggingface/diffusers/pull/4315
original_outputs = F.conv2d(
hidden_states, self.weight, self.bias, self.stride, self.padding, self.dilation, self.groups
)
if self.lora_layer is None:
return original_outputs
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if self.padding_mode != "zeros":
hidden_states_pad = F.pad(hidden_states, self._reversed_padding_repeated_twice, mode=self.padding_mode)
original_outputs = F.conv2d(
hidden_states_pad,
self.weight,
self.bias,
self.stride,
(0, 0),
self.dilation,
self.groups,
)
else:
# make sure to the functional Conv2D function as otherwise torch.compile's graph will break
# see: https://github.com/huggingface/diffusers/pull/4315
original_outputs = F.conv2d(
hidden_states, self.weight, self.bias, self.stride, self.padding, self.dilation, self.groups
)
if self.lora_layer is None:
return original_outputs
else:
if self.padding_mode != "zeros":
hidden_states = F.pad(hidden_states, self._reversed_padding_repeated_twice, mode=self.padding_mode)
padding = (0, 0)
else:
padding = self.padding
original_outputs = F.conv2d(
hidden_states, self.weight, self.bias, self.stride, self.padding, self.dilation, self.groups
)
if self.lora_layer is None:
return original_outputs
else:

Wouldn't that be easier?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is the padding wrong is this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to your opinion, the code on lines 361-363 should use padding instead of self.padding?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you apply @patrickvonplaten 's suggestion here?
It does exactly the same thing but simplifies your code - that's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,I think his suggestion is great and agree with his modifications, but I think should use

original_outputs = F.conv2d(
             hidden_states, self.weight, self.bias, self.stride, padding, self.dilation, self.groups
         ) 

instead of

original_outputs = F.conv2d(
            hidden_states, self.weight, self.bias, self.stride, self.padding, self.dilation, self.groups
        )

Is that so?

Copy link
Member

Choose a reason for hiding this comment

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

Gentle ping here: @yiyixuxu

Copy link
Collaborator

Choose a reason for hiding this comment

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

hey @jinghuan-Chen
yes you're right!

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Feb 10, 2024
@github-actions github-actions bot closed this Feb 19, 2024
@sayakpaul sayakpaul reopened this Feb 19, 2024
@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Feb 24, 2024

hey @jinghuan-Chen
sorry we let this go stale and closed unexpectedly. You're absolutely correct about #6031 (comment)

we will merge this once you resolve the merge conflicts and tests pass :)

@yiyixuxu yiyixuxu removed the stale Issues that haven't received updates label Feb 24, 2024
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.

thanks!

@yiyixuxu
Copy link
Collaborator

ohh tests fail - can you look into fixing them?

@yiyixuxu yiyixuxu merged commit 88aa7f6 into huggingface:main Feb 27, 2024
13 checks passed
@jinghuan-Chen jinghuan-Chen deleted the make-LoRACompatibleConv-paddingmode-work branch February 27, 2024 01:56
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.

Make LoRACompatibleConv padding_mode work
5 participants