Adding EncT5 model for non-autoregressive tasks
Author: hackyon
Pull request: yes

Adding a EncT5 model/framework for non-autoregressive tasks, as described in https://arxiv.org/abs/2110.08426 (Algorithm 1 for now, but I plan to follow up with support for Algorithm 2).  There is already a T5ForSequenceClassification variant, but EncT5 uses less params (just a single decoder layer instead) and, according to the paper, achieves similar results in the benchmarks as BERT and T5.

Context in #14097 


## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [x] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? 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](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@ArthurZucker @amyeroberts @sgugger @sjrl 


Comments:
I think this could be a small but interesting addition, but wanted to get some feedback on whether this is good idea before I write tests and update the documentation. 

@ArthurZucker @amyeroberts @sgugger

Thanks!

I'm not on the HF team, but I think this would be great! I was also interested in adding T5Encoder only variants of both SequenceClassification and Extractive QuestionAnswering. 

Hey! This is a fairly old model, not entirely sure this is still relevant let's see if the community requests this! 

Sounds good. There were some interest in the original issue #14097, and I pinged the thread again to see if anyone else is still interested in it.

When I was playing around with this code, I was also tempted to just implement EncT5 as a feature in T5ForSequenceClassification instead. That would require a new config.use_enct5 and some extra if-statements to gate the differences in logic, but there's a bunch of reusable code too (like the loss computation). The advantage here is that we won't need another class just for EncT5, with less "model sprawl". Let me know your thoughts about this other approach.

Hi, yes EncT5 was designed to be more composable through configs and should require little code change by reuse existing components especially multi-label and tagging problems.

You might find the following results from ACL2023 interesting.
[An Exploration of Encoder-Decoder Approaches to Multi-Label Classification for Legal and Biomedical Text](https://aclanthology.org/2023.findings-acl.360.pdf) showed that EncT5 is quite competitive when it comes to multilabel. See Table 6. 
Perhaps their [implementation](https://github.com/coastalcph/Multi-Label-Classification-T5) can be helpful. 








I would also vote for adding it into Transformers :hugs: 

Thanks @frederick0329 and @stefan-it! 

@ArthurZucker - it seems like there's still a fair bit of interest in this, both on this PR and also in #14097 . The paper Frederick linked, which is based on EncT5, was also published only a couple of months ago.

I'm motivated to see this through and get the implementation, tests, and docs ready. Should I go ahead and move forward with the implementation? Thank you! 🙏

I am not really sure (see my next comment). Also we are going to have a little bit of an issue with the naming. We can't go with EncT5. T5Enc is too close to T5Encoder, so no as well. 😅 

If there is only a single layer of decoder what is the difference between defining an `T5ForSequenceClassification` and just setting the number of decoder layers to 1 in the config? 



Thanks @ArthurZucker.

EncT5 was designed to reuse as much of the original T5 code and with as little change as possible. That's one of the advantages of EncT5.

With that said, there are several main differences from the existing T5ForSequenceClassification:
* EncT5 requires a reinitialized decoder embedding - with a smaller vocab size, just only what is necessary to trigger the proper classification. This vocab size changes depending on whether the problem type is regression, single-label classification, or multi-label classification.
* There needs to be a way to reinitialize the decoder embedding, decoder layer (and only the decoder layer), and classification head weights for EncT5. The user can access the internal state of the T5ForSequenceClassification model to do this, but it would be easier for them if we created a private helper method and provide documentation for it.
* The decoder input ids need to be set accordingly (either 0, or range of labels depending on context). Granted this is not difficult, but we can provide the proper default values with a separate EncT5 model.
* The classification head will also change from the existing one for the multi-label classification case (or maybe even the single-label case). Each label in multi-label classification will require their own head, and so it won't make sense to reuse the existing one as there is a [sizable dense layer](https://github.com/huggingface/transformers/blob/51042ae8e5df8275d16b3eaff36c20fb9c191655/src/transformers/models/t5/modeling_t5.py#L781) in it. We will need something more like [this](https://github.com/coastalcph/Multi-Label-Classification-T5/blob/4741edcf4e86fa6023ce125f24dd0bb3a8ebfd2f/models/t5_classifier.py#L82) (which is mostly just a projection without the extra dense layer).
* In multi-label classification, because of the need to provide separate heads per label, we will need to some reshaping of outputs to make things work. For single-label classification and regression, the index of the output to examine is also different from the existing T5ForSequenceClassification. 
* There may be additional changes required. I think the main one that comes to mind is the default casual mask which may need to be disabled for the EncT5 case. I still need to look more into this issue though.

As I mentioned above [comment](https://github.com/huggingface/transformers/pull/26683#issuecomment-1756943763), I think we can include these changes in the existing T5ForSequenceClassification, but provide a use_enct5 flag in the config and flag the logic when necessary. We'd see some extra if-statements in the code, and the logic will be harder to follow.

Or perhaps we could use a name like "EncT5ForSequenceClassification"? To me, that doesn't conflict with T5Encoder, and also implies that the new class is a variant of T5ForSequenceClassification. 

Hey! Thanks for iterating. 
> There needs to be a way to reinitialize the decoder embedding, decoder layer (and only the decoder layer), and classification head weights for EncT5. The user can access the internal state of the T5ForSequenceClassification model to do this, but it would be easier for them if we created a private helper method and provide documentation for it.

Is that something that needs to be done during inference? Or even during runtime? If yes it's a completely different logic from T5 and would need a new model. If not, there's absolutely no reason to just properly convert the checkpoints. 

> The decoder input ids need to be set accordingly (either 0, or range of labels depending on context). Granted this is not difficult, but we can provide the proper default values with a separate EncT5 model.

Again this just means that the user has to give something he is already supposed to give no? 


>  There may be additional changes required. I think the main one that comes to mind is the default casual mask which may need to be disabled for the EncT5 case. I still need to look more into this issue though.

Mmm still seem like the user can just pass the correct attention mask no? 

Thanks for checking in again! Let me know if you have any other questions. I'm happy to do a 1/2-pager write-up design if you think it'd help.

> Hey! Thanks for iterating.
> 
> > There needs to be a way to reinitialize the decoder embedding, decoder layer (and only the decoder layer), and classification head weights for EncT5. The user can access the internal state of the T5ForSequenceClassification model to do this, but it would be easier for them if we created a private helper method and provide documentation for it.
> 
> Is that something that needs to be done during inference? Or even during runtime? If yes it's a completely different logic from T5 and would need a new model. If not, there's absolutely no reason to just properly convert the checkpoints.

The reinitialization happens after loading a pre-trained model and before fine-tuning. The weights don't need to be changed or reinitialized during inference or runtime.

I think we could convert some checkpoints that already have the weights re-initialized. However, would the converted checkpoints support different config values (num_labels, problem_types, and projection sizes) that may be supported by EncT5? It's my understanding that the weight matrices would have different shapes depending on these config values, so I'm not sure converted checkpoints would work for all the cases (although I don't know enough about converted checkpoints to know if this is possible).

Fundamentally, the "purely model" differences between T5 and EncT5 is in the classification head and the decoder embedding (which isn't shared with the encoder embedding). The other differences can be worked around with the proper config and inputs, but the user will likely need to dig into the internals of the implementation to figure out how to supply the proper config and inputs. 

> 
> > The decoder input ids need to be set accordingly (either 0, or range of labels depending on context). Granted this is not difficult, but we can provide the proper default values with a separate EncT5 model.
> 
> Again this just means that the user has to give something he is already supposed to give no?

Yup, this is purely for convenience. With that said, the decoder_input_ids that are given to EncT5 are fundamentally different from those given to T5, so it should be easier for the user.

> 
> > There may be additional changes required. I think the main one that comes to mind is the default casual mask which may need to be disabled for the EncT5 case. I still need to look more into this issue though.
> 
> Mmm still seem like the user can just pass the correct attention mask no?

I took another look and it seems like the [default causal mask logic](https://github.com/huggingface/transformers/blob/c030fc891395d11249046e36b9e0219685b33399/src/transformers/modeling_utils.py#L917-L922) would not be triggered if the user provided [a 3 dimensional attention mask](https://github.com/huggingface/transformers/blob/main/src/transformers/modeling_utils.py#L913-L914). It is possible, but not easy for the user to know about this. We could probably use this mechanism ourselves for EncT5 to supply the attention mask and override the default causal masking logic.




Hey, I don't think we are aligned with what a new model implies. 
A new model in transformers means that the model is different (no in terms of weights, vocab size or decoder / encoder embeddings), not in terms of which inputs are given to the model, but in terms of architecture. So if there is indeed a difference, then we might need to add some code.

> Fundamentally, the "purely model" differences between T5 and EncT5 is in the classification head and the decoder embedding (which isn't shared with the encoder embedding).  

- the shared embedding can be controlled by loading the proper weights in the proper layers. 
- the classification head should already be controlled with `config.num_labels)`. I might have missed the other changes?

If the model you are adding needs a new conversion script, you could also just link the script in the ressources for example or add the script here. 

Last but not least, let me iterate again, `EncT5` can never fit in the T5 repo, it's just not consistent with the entire library! 
🤗 

Model is an overloaded term, and so I can see where the confusion happens! Sorry if I haven't been clear in my explanation of EncT5. Perhaps we can think of EncT5 as a feature/variant of the existing T5ForSequenceClassification, rather than a completely new "model".

When I said the two "purely model" differences, what I meant were the architectural differences that require code change. I don't think there is a way to get around this without code change (but please correct me if I'm wrong):

- Shared Word Embedding (specifically, the word embedding that converts input_ids to input_embeds): T5 currently uses a [shared embedding](https://github.com/huggingface/transformers/blob/32f799db0d625ec5cf82624ff2604c5a891ebf61/src/transformers/models/t5/modeling_t5.py#L1365-L1378) between the encoder and decoder. They are the same instance in the code. For EncT5, we will need two different embeddings. This means we will need to instantiate a complete new embedding, and set it for the decoder. 

- Classification Head: There are a couple of (smaller) differences here. For regression and single-label classification, the classification head logic should be similar to the existing T5ForSequenceClassification, but we need to change [this part](https://github.com/huggingface/transformers/blob/32f799db0d625ec5cf82624ff2604c5a891ebf61/src/transformers/models/t5/modeling_t5.py#L2070-L2078) since the eos_mask it computes does not work for EncT5. For multi-label classification, EncT5 uses a separate classification head per label, instead of a single classification head that projects out to multiple labels. 

I am definitely in favor of keeping the T5 repo clean, and am just hoping iterate with you on a proper design that would work! Let's drop the EncT5 naming and extra class if it doesn't fit inside the T5 repo. 

Since the architectural changes are minimal, perhaps we can use my earlier [proposal](https://github.com/huggingface/transformers/pull/26683#issuecomment-1756943763) of adding these architectural changes into the existing T5ForSequenceClassification, but gating it on a flag? I can make this change so it would be easier for you to visualize the necessary architectural changes.


Nice the last comment is perfect thanks! 
You can:
- add a new Class called T5ForXXXX with an appropriate name at the end of the file
- add the necessary changes (nothing should be changed in previous T5code)
- make sure you have tests (integrations tests) and documentation regarding what this new class adds and allows people to do (basically translating the motivations behind adding this). 

The T5ForSequenceClassifcation should not be changed 😉 

Sounds good, thanks @ArthurZucker! 🙏

I'll iterate and write the necessary tests and documentation for it over the coming week.

FYI: a medical issue came up and I'll need a couple more days, but I'm still working on it.

@ArthurZucker 

As I was working through the tests, I ended up reading up more on some other variants of the T5 library, such umT5, mT5, ByT5, and LongT5. It seems like many of these share a lot of code T5, and are just forked from the T5 directory with a lot of "fix-copies" dependencies. 

Thinking about it more, how would you feel if we created a new directory named encT5, with a fork of the basic T5 directory code and "fix-copies" dependencies? 

The relationship between T5 and mT5 (with mT5 being a variant of T5) is similar to the relationship between T5 and encT5 (with encT5 being a variant of T5). This feels more consistent than adding a T5ForEncoderBasedSequenceClassification class to the T5 repo.

Thanks for your input!

> @ArthurZucker
> 
> As I was working through the tests, I ended up reading up more on some other variants of the T5 library, such umT5, mT5, ByT5, and LongT5. It seems like many of these share a lot of code T5, and are just forked from the T5 directory with a lot of "fix-copies" dependencies.
> 
> Thinking about it more, how would you feel if we created a new directory named encT5, with a fork of the basic T5 directory code and "fix-copies" dependencies?
> 
> The relationship between T5 and mT5 (with mT5 being a variant of T5) is similar to the relationship between T5 and encT5 (with encT5 being a variant of T5). This feels more consistent than adding a T5ForEncoderBasedSequenceClassification class to the T5 repo.
> 
> Thanks for your input!

Here is a draft PR for this particular idea:
https://github.com/huggingface/transformers/pull/27472/files

There seems to be more code, but in reality, it's more-or-less the same as this one, but with fix-copies on the T5Models. This looks cleaner IMHO, especially given the new configurations, and also isolates the changes to a new file rather than updating the existing T5 models.

Let me know your thoughts, thanks! 🙏

Thanks for looking over the change!

The classification head class is indeed one of 2 architectural changes, the other being the decoder embedding (with a different vocab_size for the decoder). Overall, the changes indeed look similar to ForTokenClassification, but the use case is quite different and we are still doing sequence classification. Furthermore, for our model, the decoder_input_ids are constants, rather than shifted input_ids.

One big takeaway is that it could be worth creating a new configuration class for our model, so we can properly set the num_decoder_layer and decoder_vocab_size. I think this new approach of creating a new model directory, and using fix-copies on the existing t5, can help achieve this: https://github.com/huggingface/transformers/pull/27472/files. Do you mind taking a look at that change and see if it makes sense to you?

Thanks!  🙏

Hey! 
No I think the best path here is to add `T5ForTokenClassification` and create a Config (an instance not a class) with something like:
```python 
>>> from transformers import T5Config
>>> config = T5Config(
    num_decoder_layers = 1
)
```
and pushing this to the hub. 
Then the checkpoints should be converted and loaded into a `T5ForTokenClassification` which yes does not have the appropriate name but can be used for both tasks so that's a plus! 
If the decoder_input_ids are constant, they ca be passed to the model (and saved) through the GenrationConfig see [here](https://huggingface.co/docs/transformers/main_classes/text_generation#transformers.GenerationConfig.decoder_start_token_id)


Thanks for the input. 

I don't think ForTokenClassification will work in this case. Aside from having an entirely different name and use case as intended, the loss calculation is also completely different. 

As for the config, the existing T5Config does not have a decoder_vocab_size variable, so we won't be able to control that configuration. We can manually set it to 1 or num_labels depending on the problem_type (this would be the default value of the decoder_vocab_size anyways), so this is a viable solution if need be.

The loss computation can be done outside of the model and is usally what we recommend, the labels can just not be passed for each custom usage. 

The missing attributes can be dealt by just saving them in the config (which is a general class that can save and load other attributes see:
```python 
from transformers import T5Config
config = T5Config(my_special_param = "my_param")
config.save_pretrained("/tmp/tests")
 T5Config.from_pretrained("/tmp/tests")
```


Hey @hackyon tldr is I think ForTokenClassification will be the best way to solve the issue 🤗 do you still feel like adding it? 

Yea sure, I can go ahead and add the ForTokenClassification variant. It looks simple enough and might be useful. I'll start another PR for that.

Feel free to mention in the documenbtation of the t5.md that this class should be use for EncT5 tasks for example 

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

This one's closed, but just added a final write-up on the issue:

* EncT5 doesn't really fit into the T5 official HuggingFace repo, as it's inconsistent with the entire library.

* Instead, we added T5ForTokenClassification in #28443. T5ForTokenClassification is encoder only, and has a classification head and it should be possible to repurpose it for encoder-only sequence classification tasks as well.

* I ended up cleaning up and pushing the code in this PR onto the HuggingFace model hub at https://huggingface.co/hackyon/enct5-base (needs fine tuning). It should be easily accessible through `model = AutoModelForSequenceClassification.from_pretrained("hackyon/enct5-base", trust_remote_code=True)`. The code is also available on [github](https://github.com/hackyon/EncT5). Feel free to play around with it, and let me know what you think.

Thanks for everyone's help and support on this issue!