[modular] Refactor pipeline functions #10726
Conversation
|
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. |
| @@ -3254,20 +3261,13 @@ def prepare_control_image( | |||
|
|
|||
| return image | |||
|
|
|||
| # Copied from diffusers.pipelines.stable_diffusion_xl.pipeline_stable_diffusion_xl.StableDiffusionXLPipeline.encode_prompt | |||
There was a problem hiding this comment.
I was thinking that we could have a new _encode_prompt method ( or maybe better to have a public method for this, so use a different name), and refactor the current encode_prompt to use that new method, what do you think?
There was a problem hiding this comment.
Yeah that works and I agree a public method is better for this, how about encode_single_prompt?
There was a problem hiding this comment.
sounds good! and we can discuss with the team and change it later if needed (it will be under this PR for now but I think we should change that for regular pipeline too)
There was a problem hiding this comment.
I've added encode_single_prompt and encode_prompt.
| ) | ||
| return prompt_embeds, negative_prompt_embeds, pooled_prompt_embeds, negative_pooled_prompt_embeds | ||
|
|
||
| def encode_single_prompt( |
There was a problem hiding this comment.
Nice! do you think we can maybe make this a class method while we are at this?
you will need to pass the components, and lora_scale will not be used if it is None
i.e.
def encode_single_prompt(text_encoder, prompt, device, lora_scale, clip_skip)(have this in mind that we want to potentially apply this pattern across all pipeline in the future, so maybe look over other encode_prompts to see if this makes seense)
There was a problem hiding this comment.
Would we pass text_encoder, text_encoder_2 etc. or would we call separately with each and have an additional method to handle concatenation?
There was a problem hiding this comment.
maybe make a encde_single_prompt_clip that can take a list of text_encoders/tokenizers? we're already doing that for sd3/flux https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/stable_diffusion_3/pipeline_stable_diffusion_3.py#L288
I think class method would add convenience when using with our regular pipeline (i.e, we now support creating a pipeline without unet & vae very well but it's better not having to create one to begin with). But I think it does not matter much with modular diffusers, so we don't have to make class methods too.
all are just thoughts here, I think it depends on the use case we want to support here (e.g. long prompt is an example, or any other customization users want to play with prompts). so let me know what you think
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
What does this PR do?
#9672 (review)
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@yiyixuxu