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.

Sign up for GitHub

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

Jump to bottom

Add support for Whisper #693

Merged
merged 72 commits into from
Aug 8, 2024
Merged

Add support for Whisper #693

merged 72 commits into from
Aug 8, 2024

Conversation

TimoImhof
Copy link
Contributor

@TimoImhof TimoImhof commented Apr 30, 2024 •
edited
Loading

This PR adds adapter support for the Whisper model from openai and builds upon work done previously in #572.

Key Additions:

  1. Adapter Support for Whisper Model:

    • Incorporates adapter functionality to enhance the flexibility and adaptability of the Whisper model.

  2. AudioClassificationHead Class:

    • Introduces a new AudioClassificationHead class.
    • Note: This class is currently not utilized by any model. The WhisperForAudioClassification static model only uses the Whisper Encoder, making it incompatible with encoder-decoder AdapterModels. I am open to feedback on whether to retain or remove this class.

  3. Enhanced Head Functions:

    • Expanded the argument options for some heads by adding a layer argument with a default value.
    • This change allows for greater customization, but I'm unsure if the previous limitations were intentional. Feedback on this modification is welcome.

  4. Preprocessing Scripts for Audio Datasets:

    • Added preprocessing scripts tailored for audio datasets.
    • These scripts are now utilized in the Whisper tests within the test suite, replacing the use of arbitrary samples.
    • I am open to suggestions on whether to retain these scripts.

TimoImhof and others added 21 commits April 2, 2024 14:37
@TimoImhof
save current progress:
ad9fe2b 
- setup import structure and files
- implement mixins
- implement WithAdapter classes for modeling
@TimoImhof
Merge branch 'adapter-hub:main' into dev/whisper
7ebb5e8
@TimoImhof
Implement WhisperAdapterModel:
1e10398 
- create WhisperSdpaAttentionAdapters module
- create WhisperAdapterModel
- make style
- add whisper to CONFIG_CLASS_KEYS_MAPPING
- add whisper to model importstructure
@TimoImhof
add logger for Attention module
c775d43
@TimoImhof
Add Whisper model to documentation
23f1c68
@TimoImhof
Add WhisperDecoderWrapperAdaptersMixin:
f4b3df8 
- make style
- add proof of concept head to head_utils
- add whisper to parallel composition white list
@TimoImhof
Add tests
dc40973
@TimoImhof
save progress
a0a89ed
@TimoImhof
save progress
f36133c
@TimoImhof
overwrite get_input_samples method to fix tests requiring simple inpu…
f011bb3 
…t and add documentation
@TimoImhof
add support for speech samples with "input_features" as tensor name
08464c2
@TimoImhof
fix wrong input argument
32a6434
@TimoImhof
upload dev files for experiments
d13b6d3
@TimoImhof
upload dev files for experiments
f5e4269
@TimoImhof
update SpeechTestBase
70f7651
@TimoImhof
Add copy info and add flash attention
e38cd9e
@TimoImhof
Changes:
8587f0f 
- remove redundant files
- add minor modifications
@TimoImhof
Changes:
909fecb 
- reorganize dev dir
- experiment more with training
   - Problem: dimensions of input samples
@TimoImhof
Delete dev dir
fcaa21e
@TimoImhof
add TODOS
9bd7065
@TimoImhof
make method more general
25171bb
@TimoImhof TimoImhof mentioned this pull request Apr 30, 2024
Closed
@TimoImhof TimoImhof changed the title [WIP] Add Support for Whisper [WIP] Add support for Whisper Apr 30, 2024
@calpt calpt linked an issue Apr 30, 2024 that may be closed by this pull request
Support for openai Whisper #466
Closed
3 tasks
calpt
calpt reviewed Jun 29, 2024
View reviewed changes
Copy link
Member

@calpt calpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall, thanks so much for working on it!

Did a first pass & left a couple of comments, mainly question to understand changes you made.

docs/model_overview.md Outdated Show resolved Hide resolved
src/adapters/head_utils.py Outdated Show resolved Hide resolved
src/adapters/models/whisper/modeling_whisper.py Show resolved Hide resolved
tests/test_adapter_heads.py Outdated Show resolved Hide resolved
tests/test_adapter_heads.py Outdated Show resolved Hide resolved
tests/test_adapter.py Outdated Show resolved Hide resolved
tests/methods/base.py Outdated Show resolved Hide resolved
src/adapters/heads/language_modeling.py Show resolved Hide resolved
src/adapters/model_mixin.py Outdated Show resolved Hide resolved
tests/test_adapter_embeddings.py Outdated Show resolved Hide resolved
@julian-fong julian-fong mentioned this pull request Jul 9, 2024
Merged
lenglaender
lenglaender reviewed Jul 17, 2024
View reviewed changes
Copy link
Member

@lenglaender lenglaender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Have you already trained an adapter on a task to see that our implementation yields the expected results?

src/adapters/head_utils.py Outdated Show resolved Hide resolved
src/adapters/model_mixin.py Outdated Show resolved Hide resolved
calpt
calpt approved these changes Aug 1, 2024
View reviewed changes
Copy link
Member

@calpt calpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

calpt
calpt reviewed Aug 4, 2024
View reviewed changes
src/adapters/methods/reft.py Outdated
# if cached indexing matrices are computed for different hidden_states size -> recompute
cache_invalidated = False
if hasattr(context, "pref_idx") and hasattr(context, "suff_idx"):
cache_invalidated = context.suff_idx.size(1) != seq_len
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch! should we check the full shape here since bsz and ddim might also change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, then we have all potential cases covered 👍

Copy link
Contributor Author

@TimoImhof TimoImhof Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However I now realized that my checking logic was not correct; the indixing matrices and hidden_states do never have the same value at dim1:

  • the hidden_states[1] represent the sequence length
  • the suff_idx[1] represents the number of positions

We need to check for the actual values of suff_idx to see if the indexing values are out of bounds. I adapted the logic and added checks for the residual dimensions as well.
When the tests passed locally I will push the changes for review

@TimoImhof TimoImhof merged commit a99e47c into adapter-hub:main Aug 8, 2024
4 checks passed
TimoImhof added a commit that referenced this pull request Aug 9, 2024
@julian-fong @TimoImhof
Adding a notebook for adapters whisper support (#717)
cd9e76c 
WIP - Adding a tutorial notebook for whisper support being worked on
#693. Feedback is always appreciated!

---------

Co-authored-by: TimoImhof <62378375+TimoImhof@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calpt calpt calpt approved these changes

@hSterz hSterz Awaiting requested review from hSterz

@lenglaender lenglaender Awaiting requested review from lenglaender

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for openai Whisper
3 participants
@TimoImhof @calpt @lenglaender