Skip to content

[docs] Clean up pipeline apis #3905

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

Merged
merged 13 commits into from
Jul 21, 2023
Merged

Conversation

stevhliu
Copy link
Member

@stevhliu stevhliu commented Jun 29, 2023

Clean up the pipeline API docs to:

  • Move DiffusionPipeline to the pipeline overview page and remove bloat there (a lot of this info concerns design decisions that users are not necessarily interested in when they want to look up how a specific function works). Also, removed the long table because users can find this in the left sidebar and on the main index page.
  • Remove some duplicated info on each pipeline page to make it easier and quicker for users to find what they’re looking for.
  • General fixes to clean up docstrings and make sure they’re consistent.
  • Standardize each pipeline page a bit; for example, some pipeline docs have usage examples, and some don’t. This should provide a more consistent user experience.
  • Adds FlaxDiffusionPipeline to the pipeline overview and adds Flax equivalent pipelines where available
  • Adds missing example usage in some of the pipeline's __call__ method
  • Adds a PipelineOutput to each pipeline doc

Starting with StableDiffusionPipeline first because it'll be easier to propagate the changes to the rest of the pipelines that copy from it.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 29, 2023

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

@stevhliu stevhliu force-pushed the clean-pipeline-apis branch 2 times, most recently from eea0dbd to 3a172d3 Compare July 7, 2023 22:40
@stevhliu
Copy link
Member Author

stevhliu commented Jul 7, 2023

I finished a first pass through the pipeline APIs, and would love it if you could take a look at it now and let me know what you think! Some of the pipelines (DiffEdit, IF, Kandinsky, Pix2Pix Zero, Shap-E, Stable UnCLIP, and UniDiffuser) have a lot of useful "how-to" content on it so I think it may be a good idea to spin those off into their own docs in the Pipelines for inference section.

@sayakpaul
Copy link
Member

Can we tackle this PR in parts maybe? Because going over 116 file changes is a little cumbersome and is also error-prone.

So, I would maybe open multiple PRs in parts to ensure the experience is smooth for you and the reviewers.

Let me know if that's okay otherwise, okay for me to continue on this PR.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Sorry, I am not comfortable with the changes that correspond to deleting the additional notes and the usage examples from the pipeline documentation pages. Slightly worries that it might lead to valuable information loss.

However, I really like the idea of keeping the docstring formatting consistency across the different pipelines.

@stevhliu
Copy link
Member Author

Slightly worries that it might lead to valuable information loss.

Very valid concern! I'll also address your comment here :)

For some of the additional notes/usage, I feel like they add more general nice-to-know type info than actual content that is relevant to that specific pipeline. I'm a bit more worried that the pipeline pages will begin to feel cluttered with the extra info so that's why I removed them. For some models (like AudioLDM), I left the tip there because it is specific to the model. And for some models (Kandinsky/IF), I haven't touched the usage sections yet because I think that should be on a separate page in the docs.

As an example, on the AltDiffusion page the tips/usage are:

  1. a single sentence that says it is conceptually similar (not very useful)
  2. how to run it (I included this in the __call__ method for each pipeline)
  3. how to use different schedulers and how to load the same components into a different pipeline (we already have docs for these, so maybe we can add a Tip with a link to them instead? or I can also add a section to the Pipeline overview page a section like this so users know you can do this for pipelines)

Can we tackle this PR in parts maybe?

Oof, apologies for that! I should've asked what you would have preferred. 🤦 Is there a way to keep the changes I've already made while splitting this PR into smaller chunks?

@sayakpaul
Copy link
Member

Thanks for understanding!

And for some models (Kandinsky/IF), I haven't touched the usage sections yet because I think that should be on a separate page in the docs.

Do you have an idea of where it could live?

In the standalone usage sections, we often share memory optimization related code. So, we need to make sure to provide those but also in a self-contained manner.

Is there a way to keep the changes I've already made while splitting this PR into smaller chunks?

Maybe just keep this branch as a backup and keep this PR open as a way to track the changes?

@stevhliu
Copy link
Member Author

Do you have an idea of where it could live?

I think it can live in the Pipelines for inference section since most of it corresponds to how to use the pipeline.

I'll start by overhauling the Stable Diffusion pipelines in this section in a first PR since a lot of the other pipelines copy the docstrings of it so it'll be easier to fix them with make fix-copies, and I'll also incorporate your feedback about the memory-optimization code if that sounds good to you?

@sayakpaul
Copy link
Member

Before you start, I would also like to get opinions from @patrickvonplaten @yiyixuxu and @pcuenca.

@patrickvonplaten
Copy link
Contributor

Can we tackle this PR in parts maybe? Because going over 116 file changes is a little cumbersome and is also error-prone.

So, I would maybe open multiple PRs in parts to ensure the experience is smooth for you and the reviewers.

Let me know if that's okay otherwise, okay for me to continue on this PR.

Since this PR is focussed on cleaning up all the pipeline API i think it's fine to have it in one big PR.

I agree with Sayak that some of the information that is removed is valuable and should probably stay.

I'd argue for:

  • Leave the tips section and the code examples on the pipeline APIs
  • Stick to removing all the table with link to model file and paper (think they are indeed not useful)
  • Very happy about the changes that were done directly to the pipeline code files

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.

Love the changes to the code files!

@stevhliu
Copy link
Member Author

I'll continue working on this PR then if you don't mind!

About removing the tips and code snippets for usage, what do you think about:

  1. For the scheduler/reusing component tips apply to all pipelines, I added a <Tip> with a link to the relevant code snippet (see here for example).
  2. For tips that apply to a specific pipeline, it remains unchanged (see here for example).
  3. All code snippets related to pipeline usage are included in the __call__ method (see here for example).

This way, we aren't losing any information except for repeated stuff (like showing example code usage in both the doc and __call__ method docstring) while keeping the doc relatively clean and lightweight imo. Let me know how you feel about this, and if we aren't onboard, I can undo it and just keep the changes to the pipeline code files! :)

@sayakpaul
Copy link
Member

I'll continue working on this PR then if you don't mind!

Sure, works for me if that's the best way to go about this!

For the scheduler/reusing component tips apply to all pipelines, I added a with a link to the relevant code snippet (see here for example).

We need to be very careful about not removing any specific tips from the sections, though. Any common tips -- I am in agreement.

This way, we aren't losing any information except for repeated stuff (like showing example code usage in both the doc and call method docstring) while keeping the doc relatively clean and lightweight imo. Let me know how you feel about this, and if we aren't onboard, I can undo it and just keep the changes to the pipeline code files! :)

Yes, I am okay with that given we don't accidentally miss out on any info (including tips, results, etc.).

@stevhliu stevhliu force-pushed the clean-pipeline-apis branch from ab362c1 to 9779237 Compare July 18, 2023 18:38
@stevhliu stevhliu marked this pull request as ready for review July 18, 2023 18:52

## DanceDiffusionPipeline
[[autodoc]] DanceDiffusionPipeline
- all
- __call__

## AudioPipelineOutput
[[autodoc]] pipelines.AudioPipelineOutput
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it makes sense to elevate StableDiffusionPipelineOutput in pipelines init so that it can be accessed by pipelines.StableDiffusionPipelineOut rather than pipelines.stable_diffusion.StableDiffusionPipelineOutput.

@patrickvonplaten WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be ok for me!

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Looking great!

@stevhliu stevhliu force-pushed the clean-pipeline-apis branch from 9779237 to f0a6728 Compare July 21, 2023 17:43
@stevhliu stevhliu merged commit a69754b into huggingface:main Jul 21, 2023
@stevhliu stevhliu deleted the clean-pipeline-apis branch July 21, 2023 18:01
orpatashnik pushed a commit to orpatashnik/diffusers that referenced this pull request Aug 1, 2023
* start with stable diffusion

* fix

* finish stable diffusion pipelines

* fix path to pipeline output

* fix flax paths

* fix copies

* add up to score sde ve

* finish first pass of pipelines

* fix copies

* second review

* align doc titles

* more review fixes

* final review
orpatashnik pushed a commit to orpatashnik/diffusers that referenced this pull request Aug 1, 2023
* start with stable diffusion

* fix

* finish stable diffusion pipelines

* fix path to pipeline output

* fix flax paths

* fix copies

* add up to score sde ve

* finish first pass of pipelines

* fix copies

* second review

* align doc titles

* more review fixes

* final review
orpatashnik pushed a commit to orpatashnik/diffusers that referenced this pull request Aug 1, 2023
* start with stable diffusion

* fix

* finish stable diffusion pipelines

* fix path to pipeline output

* fix flax paths

* fix copies

* add up to score sde ve

* finish first pass of pipelines

* fix copies

* second review

* align doc titles

* more review fixes

* final review
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* start with stable diffusion

* fix

* finish stable diffusion pipelines

* fix path to pipeline output

* fix flax paths

* fix copies

* add up to score sde ve

* finish first pass of pipelines

* fix copies

* second review

* align doc titles

* more review fixes

* final review
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* start with stable diffusion

* fix

* finish stable diffusion pipelines

* fix path to pipeline output

* fix flax paths

* fix copies

* add up to score sde ve

* finish first pass of pipelines

* fix copies

* second review

* align doc titles

* more review fixes

* final review
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