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 adapter support for Hubert #551

Open
wants to merge 4 commits into
base: legacy
Choose a base branch
from

Conversation

bhavitvyamalik
Copy link

@bhavitvyamalik bhavitvyamalik commented May 25, 2023

This PR adds adapter support for Hubert by Meta. I have a few questions:

  • Should I also add support for LoRA? It expects PretrainedConfig in LoRALinear which is not imported in modeling_hubert.py file. Should I add that and then support LoRA too? Added support for LoRA too. Let me know if it looks fine?
  • adjust_tensors_for_parallel is used for what purpose? I see you have used this while calculating attention.
  • In src/transformers/adapters/models/hubert/adapter_model.py there are many heads and for this ASR model I found only ClassificationHead, ModelWithFlexibleHeadsAdaptersMixin, MultiLabelClassificationHead, and MultipleChoiceHead useful. Let me know if that's fine else I can add head for other tasks too

I'm working on tests, will finalise after first review. Thanks a lot!

hidden_states = attn_residual + hidden_states
hidden_states = hidden_states + self.feed_forward(self.final_layer_norm(hidden_states))
sa_output = self.dropout(sa_output)
sa_output = self.attention_adapters(sa_output, attn_residual) # (bs, seq_length, dim)
Copy link
Author

@bhavitvyamalik bhavitvyamalik May 26, 2023

Choose a reason for hiding this comment

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

I'll change it to sa_output = self.attention_adapters(sa_output, attn_residual, None) instead

@calpt
Copy link
Member

calpt commented Sep 9, 2023

Hey, thanks for your efforts in contributing new model architectures to adapter-transformers and sorry for the silence on our side.

In the last few weeks, we've been working on a large refactoring of our project, which will ultimately result in the release of Adapters, the next-generation adapters library. See #584.

As a consequence, we plan to merge any new model integrations directly to the new codebase, which currently can be found on this branch. Unfortunately, this necessitates some changes in the model integration code (detailed here, see already integrated models such as BERT, BART etc. for reference).

If you'd be willing to update your model integration to target the new library yourself, we'd be super happy to help you on this. Otherwise, we might look into upgrading and merging some of the open model integration PRs ourselves in the future. For more details, again see #584.

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

Successfully merging this pull request may close these issues.

2 participants