Skip to content

Conversation

@hadaev8
Copy link
Contributor

@hadaev8 hadaev8 commented Oct 27, 2022

With current codebase this snippet would return error

num_images = 2
prompt = ["a photograph of an astronaut riding a horse"] * num_images

images = pipe(prompt, negative_prompt=prompt).images

Number of positive and negative prompts already checked in line elif batch_size != len(negative_prompt):
So no reason to repeat it by batch size.

@hadaev8 hadaev8 changed the title Fix wrong repeat of negative promt Fix wrong repeat of negative prompt Oct 27, 2022
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@hadaev8
Copy link
Contributor Author

hadaev8 commented Oct 28, 2022

Im not sure whats wrong with code quality.

@Thomas-MMJ
Copy link

Im not sure whats wrong with code quality.

Have you tried running

python -m black source_file_name.py
and seeing what changes?

@shirayu
Copy link
Contributor

shirayu commented Oct 28, 2022

Anther formatting way is to run

make style

@pcuenca
Copy link
Member

pcuenca commented Oct 28, 2022

Hi @hadaev8, thanks a lot! However I think this was already in progress in #1002 (issue #779), which was just missing some tests. Can you please take a look there and see if there's anything missing?

@hadaev8
Copy link
Contributor Author

hadaev8 commented Oct 29, 2022

@pcuenca Yes, I checked and change is same, but I fixed all (at least what I found) pipes.
Is it possible to merge our PRs?

@hadaev8
Copy link
Contributor Author

hadaev8 commented Oct 29, 2022

@Thomas-MMJ
It added whitespace.


            removed_text = self.tokenizer.batch_decode(text_input_ids[:, self.tokenizer.model_max_length :])
            removed_text = self.tokenizer.batch_decode(text_input_ids[:, self.tokenizer.model_max_length:])

Whitespace was removed by vs code and I considered its fine.
Why whitespace should be here? Or is the problem on the side of code quality check?
Im confused.

@Thomas-MMJ
Copy link

Whitespace was removed by vs code and I considered its fine.
Why whitespace should be here? Or is the problem on the side of code quality check?
Im confused.

It just happens to be the style standard used for this code base. There are a variety of essentially arbitrary choices for consistency in style and this happens to be one.

@patrickvonplaten
Copy link
Contributor

@pcuenca could you take a look here as it's very similar to your open PR here: #1002

Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

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

Looks good to me! It's the same as the other PR but includes all the pipelines.

The only thing remaining would be to create some tests, as outlined by @patil-suraj here: #1002 (review).

@hadaev8 will you have time to work on it? Otherwise happy to take it :)

Copy link
Contributor

@patil-suraj patil-suraj left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! I have same comments as @pcuenca, good for merge once those are resolved.

@hadaev8
Copy link
Contributor Author

hadaev8 commented Nov 2, 2022

@pcuenca
I would do it, should you point me to an example of test?

@pcuenca
Copy link
Member

pcuenca commented Nov 3, 2022

@hadaev8 For now I'd use something like maybe https://github.com/huggingface/diffusers/blob/main/tests/test_pipelines.py#L265 to exercise a small pipeline with different inputs, and verify that no exceptions are raised and the number of images returned is correct. These tests would include all the cases:

  • Single items for positive and negative prompts.
  • Single items + num_prompts_per_image
  • Lists for both.
  • Mismatched cardinality between positive and negative (verify an error is returned).
  • Lists + num_prompts_per_image

A better test would be to actually run a full stable diffusion pipeline and verify some slice of the results, but this is somewhat more involved and could be done later in my opinion.

@pcuenca
Copy link
Member

pcuenca commented Nov 3, 2022

I made the final changes here, I hope you don't mind @hadaev8, we want to include this very nice fix in our next release :) I also opened #1117 to create the tests so this PR should be ready to merge now. Thanks a lot @hadaev8!

@pcuenca
Copy link
Member

pcuenca commented Nov 3, 2022

Running the fast and slow tests locally.

@hadaev8
Copy link
Contributor Author

hadaev8 commented Nov 3, 2022

@pcuenca
No, i dont mind.

So about this reshape, if repeat doesnt suit, where should be another function to avoid reshaping. Maybe expand or something.

How do you think, does it make sense for another pr or leave it as it is?

pcuenca added a commit that referenced this pull request Nov 3, 2022
* remove batch size from repeat

* repeat empty string if uncond_tokens is none

* fix inpaint pipes

* return back whitespace to pass code quality

* Apply suggestions from code review

* Fix typos.

Co-authored-by: Had <[email protected]>
@pcuenca
Copy link
Member

pcuenca commented Nov 3, 2022

I had to open #1120 to fix a typo I made in my suggestion and pass the tests locally. Already merged, so we can close this one.

Thanks again @hadaev8, this is very very helpful!

@anton-l
Copy link
Member

anton-l commented Nov 3, 2022

Continued in #1120

@anton-l anton-l closed this Nov 3, 2022
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* remove batch size from repeat

* repeat empty string if uncond_tokens is none

* fix inpaint pipes

* return back whitespace to pass code quality

* Apply suggestions from code review

* Fix typos.

Co-authored-by: Had <[email protected]>
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.

8 participants