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

Update/Fix Pipeline Mixins and ORT Pipelines #2021

Merged
merged 81 commits into from
Oct 9, 2024

Conversation

IlyasMoutawwakil
Copy link
Member

@IlyasMoutawwakil IlyasMoutawwakil commented Sep 10, 2024

What does this PR do?

This PR allows for using the same modeling in diffusers for ORT diffusion pipelines without maintaining custom mixins.
It also fixes the issues in output reproducibility and numeric consistency vs diffusers observed in #1960.
Breaking changes:

  • We export the vae encoder by outputting its latent distribution parameters instead of sampling during export, this way we can solve the above mentioned issues.

We also conduct a benchmark to prove this change doesn't hurt perf: #2021 (comment)

We also show how reproducibility is impossible with an onnx model that performs random numbers generation at export-time: #2021 (comment)

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?

Who can review?

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

…eproducibility and comparaison tests (7 failed, 35 passed)
@IlyasMoutawwakil
Copy link
Member Author

@echarlaix

optimum/onnxruntime/modeling_seq2seq.py Show resolved Hide resolved
optimum/onnxruntime/modeling_diffusion.py Outdated Show resolved Hide resolved
optimum/onnxruntime/modeling_diffusion.py Outdated Show resolved Hide resolved
Co-authored-by: Ella Charlaix <80481427+echarlaix@users.noreply.github.com>
@IlyasMoutawwakil
Copy link
Member Author

I made sure the model also accepts safety_chacker if passed to from_pretrained.
and also if some sessions are passed directly to from_pretrained like in diffusers.

Copy link
Collaborator

@JingyaHuang JingyaHuang left a comment

Choose a reason for hiding this comment

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

Thanks @IlyasMoutawwakil for raising and working on this PR, it's brilliant! I'm so inspired to improve the diffusers support in Neuron as well!

Comment on lines +388 to +395
components = {
"vae": self.vae,
"unet": self.unet,
"text_encoder": self.text_encoder,
"text_encoder_2": self.text_encoder_2,
"safety_checker": self.safety_checker,
"image_encoder": self.image_encoder,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe adding a test leveraging from_pipe() to ensure that we have enough members in the components?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And for making this to work you might need to allow passing ORTModelXXX to __init__() instead of just bare ort session. Never mind, it's a small nit, we can put it as todo anyways!

Copy link
Member Author

@IlyasMoutawwakil IlyasMoutawwakil Oct 9, 2024

Choose a reason for hiding this comment

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

yes this one will need a bit more work for compatibility, I can do it in another PR.
as you said it especially requires passing models and not sessions, which might require deprecating some stuff.

@IlyasMoutawwakil
Copy link
Member Author

IlyasMoutawwakil commented Oct 9, 2024

I uploaded a model that was exported using optimum 1.22 :

I also added the same checks in optimum-intel for missing attributes in model configs, that way even older versions can still work (e.g. hf-internal-testing/tiny-stable-diffusion-openvino was exported using "_diffusers_version": "0.14.0").

I also removed the need for separate ORTPipeline "base" class, now it's just ORTDiffusionPipeline which works as an entrypoint and parent class for other pipelines. This is more like what's done in diffusers as well.

@IlyasMoutawwakil IlyasMoutawwakil merged commit d3c56cd into main Oct 9, 2024
60 of 64 checks passed
@IlyasMoutawwakil IlyasMoutawwakil deleted the update-diffusers-mixins branch October 9, 2024 12:17
@@ -278,14 +284,26 @@ def _from_pretrained(
else:
submodels[submodel] = load_method(model_save_path)

return cls(
# same as DiffusionPipeline.from_pretraoned, if called directly, it loads the class in the config
Copy link
Collaborator

Choose a reason for hiding this comment

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

smol typo here

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