Positional embedding is always part of the models, though, not the pipeline so I think we should not move the positional embedding to pipeline here

for rotary embedding, there is indeed some discrepancy, hunyuan-dit, lumina and stable audio and now cogvideox all create it in pipeline but flux make it in transformer

design-wise, I think the flux method is the most nice (making a positional embedding that works for both rotary and sincos), it might have a slowdown due to the fact that we recompute it every step, but should be negotiable and we do that for other conditions anyway - should we consider that design if we want to make a refactor here?

Hmm... you are right that we do it for other conditions so saving some here isn't going to make a difference. My reasoning for this change was that I don't want there to be a limit on the number of frames imposed by the pipeline (previously, it was limited by CogVideoXTransformer3D which created the sincos embeds based on sample_frames parameter, and so just to give a cleaner error I put this bit of code here). I also know that you can change the parameter at initialization in .from_pretrained(..., sample_frames=<WHATEVER MAX LIMIT YOU WANT>) to get around this, but it seems like an extra step for anyone wanting to generate a different number of frames and not something you'd expect a user to know

I think that the FluxPosEmbed method is the best to go forward with, since you would know the shapes in the forward pass. Just to confirm, this would remove image_rotary_emb from the pipeline, and based on the transformer config use_rotary_positional_embedding, we make CogVideoXPosEmbed decide whether to return positional or rotary embeds?

for positional embedding, I think we can move it inside CogVideoXPatchEmbed (see how our PatchEmbed works here)

from there, we have a few choices:

• you can generate the pos_embed during initialization based on sample_frames and then re-generate it during inference time whennum_frames user passed is larger than the sample_frames(similar to here) - I don't like this very much. I think if we want to keep this sample_frames config, it is better to throw an error at inference time and ask user to override the config with from_pretrained(...) method because it is not a config you need to change on every generation as it is just a maximum, i.e. if you set it at 100, you can generate any number of frames <=100

• If you want to change it to generate the positional embedding on the fly for each generation based on the num_frames config, I'm okay with that too! As long as it is tested and we're sure that it will not result in worse performance in terms of memory and speed.

We can do Rotary embedding in a separate PR if we want to refactor it because the current implementation is consistent with most pipelines, and if we want to change it, we would need to change for all of them

I don't like the idea of re-generating it per forward pass either, as it seems like unnecessary extra time added. I think the from_pretrained(..., sample_frames=X) method is the best way to do it then.

Let's hold on a bit with the RoPE refactor, and just use this PR to remove the num_frame limitation and pos embeds to CogVideoXPatchEmbed to make it consistent. I think what we have currently in the pipeline is nice since it saves the recomputation and consistent with the rest. FluxPosEmbed like method could be used in all pipelines eventually - I have a design idea to introduce something at the DiffusionPipeline level that would allow us to cache the conditions that are re-computed at each denoising step (encoder_hidden_states intermediates, for example), even though they don't have to be. I will try and open a PoC soon that should not be intrusive to any of the existing modeling code but help make inference speed go brrrr

I have a design idea to introduce something at the DiffusionPipeline level that would allow us to cache the conditions that are re-computed at each denoising step (encoder_hidden_states intermediates, for example), even though they don't have to be. I will try and open a PoC soon that should not be intrusive to any of the existing modeling code but help make inference speed go brrrr

would be interesting to see some experiments on speed improvement if you have time, but

1. we tested it before and found the speed difference is very insignificant. That's why we went with the current design
2. if the difference is indeed significant, we have the option to have all the projection layers as a separate module module so they are only computed once - so no need for cache I think

thanks! is there any discussion/PR for reference on this so I can better understand what's already been tried and set up some expectations? I am particularly curious about deep stacked models like Flux and Cog which might really benefit from this. i'll try and get some numbers end of the week

Hmm, so I gave it a shot for Cog. I forgot that the joint text+video attention recomputes the encoder_hidden_states which is a must at each step, and so we can't save any time in the projection layers there. The only place where this is usable is the patch embed projection and first transformer layer norm, and fwiw I tried it out to save a mere ~1 second :( I see what you mean about having tested in the past and finding it insignificant