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

Merged
merged 24 commits into from
Oct 24, 2024
Merged

sd3 pipeline support #916

merged 24 commits into from
Oct 24, 2024

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

@IlyasMoutawwakil
Copy link
Member

reference for text_encoder_3 inference test skipping : #965

@IlyasMoutawwakil IlyasMoutawwakil merged commit 86598a6 into huggingface:main Oct 24, 2024
13 of 17 checks passed
@eaidova eaidova deleted the ea/sd3_wip branch October 24, 2024 10:01
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