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

sd3 pipeline support #916

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

sd3 pipeline support #916

wants to merge 23 commits into from

Conversation

eaidova
Copy link
Collaborator

@eaidova eaidova commented Sep 26, 2024

What does this PR do?

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@eaidova eaidova changed the title Ea/sd3 wip sd3 pipeline support Sep 26, 2024
@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.

@IlyasMoutawwakil
Copy link
Member

IlyasMoutawwakil commented Oct 10, 2024

The changes to be made in this PR after the refactorization are very simple and mostly will make it more readable:

  • no need to copy paste modeling from diffusers anymore, you just subclass (OVDiffusionPipeline, StabeldDiffusion3Pipeline) and it should be good in most cases.
  • adding tests is as simple as adding the architecture to the task specific tests, I see you haven't added tests yet tho...

I also suggest you take a look at the original refactorization work in huggingface/optimum#2021, I explain there issues with the previous approach and why the refactorization is not only that but also fixing many issues with the previous design (reproducibility of results & numeric consistency with diffusers), which weren't tested before or simply ignored for some architectures/tasks.

Copy link
Member

@IlyasMoutawwakil IlyasMoutawwakil left a comment

Choose a reason for hiding this comment

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

LGTM, can you run styling and add tests please.

@IlyasMoutawwakil
Copy link
Member

  • it seems sd3 still doesn't support callbacks (idk if it ever will as it doesn't have a unet), need to be skipped.
  • it's also not as numerically consistent with diffusers, which is interesting, we can increase atol and rtol to 1e-3 and 1e-1 for this architecture for now, but worth investigating, especially if each exported component is numerically consistent.
  • and finally I guess output_type="latent" is different for this model, again because it uses a transformer instead of unet https://github.com/huggingface/optimum-intel/blob/main/tests/openvino/test_diffusion.py#L419-L422

Copy link
Member

@IlyasMoutawwakil IlyasMoutawwakil left a comment

Choose a reason for hiding this comment

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

will be merged once tests are updated and passed

@eaidova
Copy link
Collaborator Author

eaidova commented Oct 22, 2024

@IlyasMoutawwakil now all relevant tests are fixed, please take a look one more time

Comment on lines -835 to +1006
ov_outputs = self.request(model_inputs, share_inputs=True).to_dict()

ov_outputs = self.request(model_inputs, share_inputs=True)
main_out = ov_outputs[0]
model_outputs = {}
for key, value in ov_outputs.items():
model_outputs[next(iter(key.names))] = torch.from_numpy(value)

if output_hidden_states:
model_outputs["hidden_states"] = []
for i in range(self.config.num_hidden_layers):
model_outputs["hidden_states"].append(model_outputs.pop(f"hidden_states.{i}"))
model_outputs["hidden_states"].append(model_outputs.get("last_hidden_state"))
model_outputs[self.model.outputs[0].get_any_name()] = torch.from_numpy(main_out)
if len(self.model.outputs) > 1 and "pooler_output" in self.model.outputs[1].get_any_name():
model_outputs["pooler_output"] = torch.from_numpy(ov_outputs[1])
if self.hidden_states_output_names and "last_hidden_state" not in model_outputs:
model_outputs["last_hidden_state"] = torch.from_numpy(ov_outputs[self.hidden_states_output_names[-1]])
if (
self.hidden_states_output_names
and output_hidden_states
or getattr(self.config, "output_hidden_states", False)
):
hidden_states = [torch.from_numpy(ov_outputs[out_name]) for out_name in self.hidden_states_output_names]
model_outputs["hidden_states"] = hidden_states
Copy link
Member

@IlyasMoutawwakil IlyasMoutawwakil Oct 22, 2024

Choose a reason for hiding this comment

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

not sure this complexity is needed, the original code does the same thing by seperating the torch.from_numpy and collecting the hidden states in a list if requested, the only change you'll have to do is to account for the missing self.config.num_hidden_layers by using self.config.num_layers otherwise for t5-encoder https://huggingface.co/stabilityai/stable-diffusion-3-medium-diffusers/blob/main/text_encoder_3/config.json#L21

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.

3 participants