🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/112381

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❌ 17 New Failures

As of commit a50f8dd with merge base 0d669f0 ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

This PR needs a release notes: label

If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

I think we want to do this, we need to properly spit the APIs I.e. add this: #110546

And this and then you would add an impl to the "nested" registration for CPU

cc @cpuhrsch

I think we want to do this, we need to properly spit the APIs I.e. add this: #110546

And this and then you would add an impl to the "nested" registration for CPU

cc @cpuhrsch

So we expect pytorch to have two different flash attn APIs. I think I should commit the changes after #110546 merges, Is that okay?

For the support of attention mask, I suppose that using cum_seq_q or cum_seq_k is not appropriate because they do not mean the same thing as attention mask.

Maybe it's better to implement _scaled_dot_product_efficient_attention CPU version as it has attn_bias as input? There are some early discussions here #103826 (comment). I could help do this if needed.

cc @jgong5

For the support of attention mask, I suppose that using cum_seq_q or cum_seq_k is not appropriate because they do not mean the same thing as attention mask.

Maybe it's better to implement _scaled_dot_product_efficient_attention CPU version as it has attn_bias as input? There are some early discussions here #103826. I could help do this if needed.

cc @jgong5

Agreed with @Valentine233 . Overloading the meaning of cum_seq_q or cum_seq_k for attention mask doesn't seem the right way to support the mask. Since the algorithms for flash attention and efficient attention are very similar, I don't see the benefit of implement another version for efficient attention on CPU. Would it be a viable option to provide an overload for _scaled_dot_product_flash_attention to support attention mask with an added arg for attn_mask on CPU? @drisspg

Hi @drisspg, do you have any opinion on the API to support CPU flash attention with mask?

Hey, so like @jgong5 said I think it doesn't really make sense too add a new backend if we are going to be modifying the existing kernel code.

If it makes it easier we can decouple the signature of the kernel from cpu and cuda. We should probably register a new stem function could and call it "sdpa_fused_attention" or something where we only register a cpu backend.

I think this would also clean up the meta registration for sdpa as we won't be forcing the cpu code to abide by the constraints of the cuda code

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

Closing in favor of: #115913