Support Multi/InfiniteTalk #10179
Support Multi/InfiniteTalk #10179
Conversation
|
Y'all, this project's bout to make Comfyui famous — they been too quiet on voice tech, sheesh :) |
There was a problem hiding this comment.
Hey, thanks for the PR and sorry for the wait! I reviewed this PR with comfy just now. I left individual comments, but here is the summary:
- Make
block_idxgo int transformer_options instead in the for loops of the forward function. This will be something we want for attention tricks in the near future so will keep from block_idx being implemented in multiple ways. - Even though it removes a lot of the useability right now, remove the looping components from this PR and instead make it work wiht just the first 'window' without the looping. I will consult with you and Jacob on implementing the standard for looping stuff in ComfyUI as a separate PR, that we can then use to bring back this functionality for this + self forcing models.
- q and k should not be returned as it is an unknown whether it will cause higher memory peaks for all Wan stuff. Instead, the optimized_attention call should be overridable so that the extra attention call thingy can be done here, with the q and k that you need stored in transformer_options for the cross_attn patch to use.
More details are included in the comments. Let me know if you have any questions!
|
|
||
| x = self.o(x) | ||
| return x | ||
| return x, q, k |
There was a problem hiding this comment.
There is some uncertainty about whether returning this will in general increase the memory peak of WAN within native ComfyUI. Instead, comfy suggests that you add a patch to replace the x = optimized_attention(...) call on line 81 byreusing the ModelPatcher.set_model_attn1_replace functionality (in unet, attn1 is self, attn2 is cross), which can then do the optimized_attention call + the partial attention thing that happens inside the cross_attn patch. To get the q + k for the cross_attn patch, you can store the q and k values in transformer_options instead and then pop them out after usage.
The transformer_index can stay None (not given) since that was something unique to unet models.
It would probably be more optimal to not call optimized_attention anymore and just reuse the logic of hte slower partial attention thingy in this code, but comfy said he would be fine if you didn't go that far and just kept both within that attention replacement function.
| return (out, out_denoised) | ||
|
|
||
|
|
||
| class LoopingSamplerCustomAdvanced: |
There was a problem hiding this comment.
How to handle models that need looping is something we want to properly handle, so for now comfy wants this PR to not implement any looping, and just keep the implementation as if it was handling just the first 'window' of the loop. So, this node and the looping wrapper can be omitted from this PR - it is something we want, we will just want to make sure we design it properly so we can reuse the general mechanism for stuff like self-forcing etc.
I will coordinate with both you and Jacob to see how we want to handle looping in ComfyUI, ideally via loop helper nodes (and we can store the group of nodes in a published subgraph.
| frame_offset = previous_frames.shape[0] - self.motion_frame_count | ||
|
|
||
| # add/replace current cross-attention patch to model options | ||
| model_options["transformer_options"].setdefault("patches", {}).setdefault("cross_attn", []).append( |
There was a problem hiding this comment.
This patch would be done outside of the wrapper, on the model patcher itself (especially since comfy wants the looping aspects of this PR removed for now).
You can use ModelPatcher.set_model_patch to do this cleanly without needing to set defaults, etc.
| init_previous_frames = previous_frames[:, :, :, :3] | ||
|
|
||
|
|
||
| model_patched.add_wrapper_with_key( |
There was a problem hiding this comment.
Ditto on the removal of looping (for now).
| self.qk_norm = qk_norm | ||
| self.cross_attn_norm = cross_attn_norm | ||
| self.eps = eps | ||
| self.block_idx = block_idx |
There was a problem hiding this comment.
Instead of having the block_idx as a class parameter, can you make it an entry in transformer_options? This can be done within the for loop where the blocks are executed in the forward function. VACE already has the block_id variable, so the alternative would be to also call this block_id instead, but for some of the attention stuff I want to support (different attention in different blocks), adding block_idx to transformer_options instead makes the most sense I think.
3 participants
This is somewhat work in progress PR that adds initial basic Multi/InfiniteTalk support:
model_patchobjectsI've chosen not to do the MultiTalk long generation method as I find it redundant due to InfiniteTalk, MultiTalk model itself will load, but further functionality need to be added to add the audio control to other models such as VACE.
As I'm not fully versed in how everything works yet, especially memory management wise, I'll be relying on feedback from @comfyanonymous and @Kosinkadink to finish this PR.