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

Quantization stable diffusion #84

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

thliang01
Copy link

What does this PR do?

Fixes #83

Who can review?

Feel free to tag members/contributors who may be interested in your PR.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@thliang01 thliang01 marked this pull request as ready for review April 29, 2024 14:08
@thliang01
Copy link
Author

@merveenoyan and @stevhliu can you give me some feedback about this PR

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

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Really nice example of using the new quanto library! 👏

Some feedback:

  1. I'd refine the focus of the cookbook on using quanto to quantize a diffusion model. A clearer title could be Quantize Stable Diffusion with quanto because it tells you exactly what you're doing and with which library.
  2. The "Step-by-Step Guide to Post Training Quantization (PTQ)" is kind of misleading because I thought this cookbook was going to walk through all the steps but it really only shows how to apply PTQ to the model and evaluate it against the base model. I would've liked to see more discussion around the other steps or just remove them entirely.
  3. I don't think you need to define the sections so granularly. It can be something simple like:
# Quantize Stable Diffusion with quanto
## Install and setup
## Base model
### Evaluation
## Quantized Stable Diffusion
### Evaluation
  1. It'd also be nice to print out the results so you can directly see what the memory and execution time is as well as the image quality.

@@ -0,0 +1,777 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Apr 30, 2024

Choose a reason for hiding this comment

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

I think you can very briefly explain the important steps of the process in a paragraph instead of putting like this


Reply via ReviewNB

@@ -0,0 +1,777 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Apr 30, 2024

Choose a reason for hiding this comment

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

this can be a simple markdown text and not a header (applies for many steps below


Reply via ReviewNB

@@ -0,0 +1,777 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Apr 30, 2024

Choose a reason for hiding this comment

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

you could append it to above cell instead


Reply via ReviewNB

@@ -0,0 +1,777 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Apr 30, 2024

Choose a reason for hiding this comment

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

you can write more explanation here with a better heading


Reply via ReviewNB

Copy link
Collaborator

Choose a reason for hiding this comment

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

This still holds

@@ -0,0 +1,777 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Apr 30, 2024

Choose a reason for hiding this comment

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

maybe explain them a bit and how they're calculated?


Reply via ReviewNB

@merveenoyan
Copy link
Collaborator

@thliang01 thanks a lot for this recipe! from what I see it's in a very early stage, I left very general comments that should be applied to rest of the recipe. you can ask for review once it's ready for review. does this work?

@thliang01 thliang01 marked this pull request as draft April 30, 2024 14:18
@@ -0,0 +1,1067 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Jun 5, 2024

Choose a reason for hiding this comment

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

nits: Stable Diffusion is an open-source image generation model*

It's a diffusion model that takes in text input and generates an image based on it.


Reply via ReviewNB

@@ -0,0 +1,1067 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Jun 5, 2024

Choose a reason for hiding this comment

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

Nit: Installation

no need to make it a big header, we can even just say "We will install the necessary libraries for this tutorial".


Reply via ReviewNB

@@ -0,0 +1,1067 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Jun 5, 2024

Choose a reason for hiding this comment

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

no need to say it IMO


Reply via ReviewNB

@@ -0,0 +1,1067 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Jun 5, 2024

Choose a reason for hiding this comment

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

We can move imports whenever they're used to improve readability, so the reader doesn't have to go back up to remember which class or function comes from where


Reply via ReviewNB

@@ -0,0 +1,1067 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Jun 5, 2024

Choose a reason for hiding this comment

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

No need for this


Reply via ReviewNB

@@ -0,0 +1,1067 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Jun 5, 2024

Choose a reason for hiding this comment

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

We can just create the list IMO and not extend


Reply via ReviewNB

@@ -0,0 +1,1067 @@
{
Copy link
Collaborator

@merveenoyan merveenoyan Jun 5, 2024

Choose a reason for hiding this comment

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

let's again add more and more explanations of what we'll do instead of comments inside the code


Reply via ReviewNB

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.

Quantization Stable Diffusion
4 participants