can call encode_prompt with out setting a text encoder instance variable#4396
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
keturn
left a comment
There was a problem hiding this comment.
I appreciate the fix!
but copy/paste logic changes to 26 files (and counting)?!?? that doesn't seem maintainable. Is there code here that can be factored out to a support function?
| if self.text_encoder is not None: | ||
| prompt_embeds_dtype = self.text_encoder.dtype | ||
| elif self.unet is not None: | ||
| prompt_embeds_dtype = self.unet.dtype | ||
| else: | ||
| prompt_embeds_dtype = prompt_embeds.dtype | ||
|
|
||
| prompt_embeds = prompt_embeds.to(dtype=prompt_embeds_dtype, device=device) |
There was a problem hiding this comment.
I get that when this method is responsible for encoding the prompt, it gets it from text_encoder -- so converting things to text_encoder's data type is a way to enforce consistency between the different modes.
On the other hand, the output is being used as conditioning data for the unet. In which case the type of the encoding network doesn't really matter, right? This data is never going back in to that network, only onward to the unet. Is there any reason to not simplify that conditional somewhat and drop the reference to text_encoder.dtype entirely?
There was a problem hiding this comment.
Looking in the git history, b562b66 this was the commit it was introduced. I think the intention for the cast was that when prompt_embeds were passed, they would need to be cast to the equivalent type as if they had been produced by the text encoder.
I think ideally the cleanest solution here would be to just cast to the type of the unet, but we also have the case of if the pipeline doesn't have a unet, i.e. if the user is just instantiating the pipeline to encode text (not often but it's documented in some of the example IF pipeline workflows). So I think while not ideal this might be the simplest way to cover all bases
fwiw, the change is mostly automated via I'm actually generally in agreement that the text encoding logic given it doesn't actually mutate any instance state shouldn't be a part of the pipeline class or at least not an instance method (this would let it be re-used pretty easily). You can see some of my discussion here #4140 (comment) but we're going to keep it as is for now :) |
patrickvonplaten
left a comment
There was a problem hiding this comment.
Thanks for the fix
…ble (huggingface#4396) * can call encode_prompt with out setting a text encoder instance variable * fix
…ble (huggingface#4396) * can call encode_prompt with out setting a text encoder instance variable * fix
re: #4349