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

[WIP][Docs] Use DiffusionPipeline Instead of Child Classes when Loading Pipeline #2809

Merged
merged 4 commits into from
Mar 28, 2023

Conversation

dg845
Copy link
Contributor

@dg845 dg845 commented Mar 24, 2023

This PR edits the docs to use DiffusionPipeline instead of its child classes (such as StableDiffusionPipeline) where possible when loading a pipeline. The rationale behind this change is that DiffusionPipeline should always create a pipeline of the right class when loading a checkpoint and thus should be more user friendly. See #2135 for more discussion; in particular, this PR corresponds to item 4 in this comment.


Below I've documented pages with examples that use a child class of DiffusionPipeline when loading a checkpoint that I've opted not to change, along with a short explanation of why I think it should stay as is.

  1. /docs/source/en/using-diffusers/loading.mdx (link)

The examples here specifically discuss loading directly from the appropriate pipeline class (e.g. StableDiffusionPipeline).

  1. /docs/source/en/using-diffusers/schedulers.mdx (link)

The JAX/Flax example uses FlaxStableDiffusionPipeline; since it deals specifically with Flax, I think it makes sense to use the more specific class.

  1. /docs/source/en/using-diffusers/img2img.mdx (link)
  2. /docs/source/en/using-diffusers/inpaint.mdx (link)
  3. /docs/source/en/using-diffusers/depth2img.mdx (link)

Since these pages talk about specific Stable Diffusion pipelines, I think it's appropriate to reference the specific pipeline class when loading the checkpoints.

  1. /docs/source/en/using-diffusers/reproducibility.mdx (link)

The running example on this page uses DDIMPipeline, which I think is relevant to the discussion on reproducibility. I haven't tested the numbers produced if we substitute DiffusionPipeline for DDIMPipeline (presumably, they are close if not the same).

Edit: in 0.15.0.dev0 the numbers are less important to the exposition, so it might make more sense to replace DDIMPipeline with DiffusionPipeline.

  1. /docs/source/en/using-diffusers/contribute_pipeline.mdx (link)

I think it makes sense that there is an example that discusses loading a checkpoint explicitly from a custom pipeline (in this case, UnetSchedulerOneForwardPipeline).

  1. /docs/source/en/using-diffusers/using_safetensors.mdx (link)

I imagine substituting DiffusionPipeline for StableDiffusionPipeline in the loading speed example could make a difference to the final loading numbers (I don't have the required hardware to test this myself).

  1. /docs/source/en/optimization/onnx.mdx (link)

I think discussing the difference between StableDiffusionPipeline and StableDiffusionOnnxPipeline is valuable, although perhaps there could be a note about using DiffusionPipeline to load the checkpoint as well.

Edit: in 0.15.0.dev0 ORTStableDiffusionPipeline replaces StableDiffusionPipeline. I think the comment above remains valid, although I'm not sure if 'DiffusionPipelineis an acceptable substitute forORTStableDiffusionPipeline`.

  1. /docs/source/en/optimization/habana.mdx (link)

Using GaudiStableDiffusionPipeline here instead of the generic DiffusionPipeline probably makes more sense. (I'm not familiar with Habana Gaudi.)

  1. /docs/source/en/training/text_inversion.mdx (link)
  2. /docs/source/en/training/text2image.mdx (link)
  3. /docs/source/en/training/lora.mdx (link)

These pages specifically discuss fine-tuning a Stable Diffusion model, so it feels weird to replace StableDiffusionPipeline with DiffusionPipeline.

(Note that for /docs/source/en/training/dreambooth.mdx (link), some of the code snippets use DiffusionPipeline and some use StableDiffusionPipeline, so I've changed all checkpoint loading examples to use DiffusionPipeline.)

  1. /docs/source/en/api/outputs.mdx (link)

In the pipeline loading example, we follow up a call to from_pretrained(...) with a call to __call__(...), so I think it makes sense to use a specific pipeline such as DDIMPipeline rather than DiffusionPipeline (which doesn't implement a __call__ method).

(Sorry for making the PR so long!)

Edit: changed the links to point to the PR docs endpoint.

…g a checkpoint using from_pretrained() instead of a child class (e.g. StableDiffusionPipeline) where possible.
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 24, 2023

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

@dg845
Copy link
Contributor Author

dg845 commented Mar 24, 2023

Sorry, I just realized that the docs in 0.15.0.dev0 are pretty different from the docs in 0.14.0, which I was implicitly working off of (I'm not currently able to build the docs locally due to #2803). I think some of my comments above still apply, and I will take a look at the new doc pages.

@dg845 dg845 changed the title [WIP][Docs] Use DiffusionPipeline Instead of Child Classes when Loading Pipeline #2135 [WIP][Docs] Use DiffusionPipeline Instead of Child Classes when Loading Pipeline (#2135) Mar 24, 2023
@dg845 dg845 changed the title [WIP][Docs] Use DiffusionPipeline Instead of Child Classes when Loading Pipeline (#2135) [WIP][Docs] Use DiffusionPipeline Instead of Child Classes when Loading Pipeline Mar 25, 2023
@dg845
Copy link
Contributor Author

dg845 commented Mar 25, 2023

Looked at the docs for 0.15.0.dev0 and have the following notes:

  1. /docs/diffusers/en/using-diffusers/write_own_pipeline (link)

Left unchanged because I think the specific pipelines (e.g. DDPMPipeline) used are important to the exposition.

  1. /docs/diffusers/en/using-diffusers/weighted_prompts (link)

Since this page discusses the prompt_embeds argument of a DiffusionPipeline's __call__ method, it makes sense to use a specific child class (e.g. StableDiffusionPipeline) for which prompt_embeds is implemented.

  1. /docs/diffusers/en/training/controlnet (link)
  2. /docs/diffusers/en/training/instructpix2pix (link)

Similarly to the other training docs, since these pages discuss specific pipelines, I think it makes sense to use them rather than the generic DiffusionPipeline.

  1. /docs/source/en/conceptual/evaluation.mdx (link)

For the first example, using StableDiffusionPipeline probably makes more sense than DiffusionPipeline, since the page specifically discusses evaluating a text-quided image generation model, and similarly for the other examples (e.g. image-conditioned text-to-image generation, etc.).

  1. /docs/source/en/api/pipelines/overview.mdx (link)

The examples discuss specific tasks (e.g. inpainting) so I think it makes sense to use the specific pipeline for that task (e.g. StableDiffusionInpaintPipeline).

@dg845
Copy link
Contributor Author

dg845 commented Mar 25, 2023

I realize that instead of documenting specific cases where a change is or is not made, it might make more sense to use a bright line rule about when using DiffusionPipeline.from_pretrained(...) to load a checkpoint in the docs is preferred over a specific subclass of DiffusionPipeline. I think the standard I've been trying to follow is "prefer DiffusionPipeline.from_pretrained(...) unless the docs explicitly mention a specific pipeline, or depend upon a property of a specific pipeline".

(I would also greatly appreciate it if people could give feedback on the verbosity of this PR, so that I can improve in the future.)

@patrickvonplaten
Copy link
Contributor

Thanks for the PR @dg845 - I agree with your reasoning. I think we should use DiffusionPipeline whenever we can. Note sometimes we can't e.g. when showcasing StableDiffusionImg2Img we can't load it from DiffusionPipeline because it would default to the wrong one.

I'm in favor of merging this PR as it though, so LGTM. @yiyixuxu could you take a final look?

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.

LGTM!

@patrickvonplaten patrickvonplaten merged commit 663c654 into huggingface:main Mar 28, 2023
@dg845 dg845 deleted the docs-use-diffusion-pipeline branch March 28, 2023 22:13
w4ffl35 pushed a commit to w4ffl35/diffusers that referenced this pull request Apr 14, 2023
…ng Pipeline (huggingface#2809)

* Change the docs to use the parent DiffusionPipeline class when loading a checkpoint using from_pretrained() instead of a child class (e.g. StableDiffusionPipeline) where possible.

* Run make style to fix style issues.

* Change more docs to use DiffusionPipeline rather than a subclass.

---------

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
…ng Pipeline (huggingface#2809)

* Change the docs to use the parent DiffusionPipeline class when loading a checkpoint using from_pretrained() instead of a child class (e.g. StableDiffusionPipeline) where possible.

* Run make style to fix style issues.

* Change more docs to use DiffusionPipeline rather than a subclass.

---------

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
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.

None yet

4 participants