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 t5 adapter #182

Merged
merged 104 commits into from Oct 12, 2021
Merged

Add t5 adapter #182

merged 104 commits into from Oct 12, 2021

Conversation

AmirAktify
Copy link

Followed the pattern of Bart to add adapters to T5. One change is that whereas Bart has separate classes for encoder and decoder, T5 does not. So I am using the is_decoder for changes between encoder and decoder classes, such as adding cross_attention adapters and adding invertible adapters.

I'm working on some testing.

@AmirAktify AmirAktify mentioned this pull request May 30, 2021
@denizbeser
Copy link

@AmirAktify thanks for working on this! My research team was looking to use adapters with T5 and we found this branch you've been working on. Do you think this is in a usable stage? Thanks!

@AmirAktify
Copy link
Author

@AmirAktify thanks for working on this! My research team was looking to use adapters with T5 and we found this branch you've been working on. Do you think this is in a usable stage? Thanks!

Hi Deniz. I was running some tests yesterday with sequence classification with T5ForConditionalGeneration, and I ran into some minor errors with the base model. Lemme try and debug those today, but I am hopefully very close.

@denizbeser
Copy link

@AmirAktify thanks for working on this! My research team was looking to use adapters with T5 and we found this branch you've been working on. Do you think this is in a usable stage? Thanks!

Hi Deniz. I was running some tests yesterday with sequence classification with T5ForConditionalGeneration, and I ran into some minor errors with the base model. Lemme try and debug those today, but I am hopefully very close.

@AmirAktify That's great to hear. Thanks for letting me know so quickly. Good luck!

@calpt calpt linked an issue Jun 21, 2021 that may be closed by this pull request
@AmirAktify
Copy link
Author

Hi @AmirAktify, if you grant me access to your branch I can work on finishing the Pull Request

Done, thank you. src/test_t5_with_adapters_on_imdb.ipynb is what I've been using to confirm that the adapter is working / converging. I've been focusing on T5ForConditionalGeneration. Unit tests seem to be passing currently. Let me know if you have any questions.

@AmirAktify
Copy link
Author

Thanks for finishing this PR btw! I hope it wasn't quite as bad as just doing it from scratch.

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.

Looks nice overall :)

Besides the line-specific comments, two general points from the implementation guide document still seem to be missing (from section Testing):

  • Insert test_modeling_<model_type> into the list of tested modules in utils/run_tests.py.
  • Append <model_type> to the list in check_adapters.py.

tests/test_pipelines_feature_extraction.py Outdated Show resolved Hide resolved
tests/test_adapter.py Outdated Show resolved Hide resolved
tests/test_adapter.py Outdated Show resolved Hide resolved
@hSterz hSterz merged commit 9989f90 into adapter-hub:master Oct 12, 2021
@kanseaveg
Copy link

nice works!

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.

Add T5 support
5 participants