Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add sdpa and flash_attention2 support to speech2text #33716

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

avishaiElmakies
Copy link
Contributor

I added support for sdpa and flash_attention2 to speech2text model (I started with sdpa but beacuse of "#copied" thought it would just be easier to add flash_attentnion2 as well). copy from bart.
addresses #26350 and #28005

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@amyeroberts @fxmarty

Notes:

  1. there was a bug in the class Speech2TextSinusoidalPositionalEmbedding which caused test_eager_matches_sdpa_generate to fail. I fixed it. The class was copied in SpeechT5. so I fixed the bug there as well
  2. the test test_flash_attn_2_generate_reuse_cache was failing because he wasn't meant for speech2text model. I copied and tweaked the test from whisper.
  3. the only test currently failing on my machine is test_flash_attn_2_from_config with the error ValueError: Unrecognized configuration class <class 'transformers.models.speech_to_text.configuration_speech_to_text.Speech2TextConfig'> for this kind of AutoModel: AutoModelForCausalLM. . speech2text doesn't have a model for causalLM but i couldn't find what other models that have the same situation and what they do with the test. bart has BartForCausalLM. so would love some guidance here

@amyeroberts
Copy link
Collaborator

cc @ylacombe for first review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants