Adding inputs_embeds argument and switch to paddle.nn.TransformerEncoder for Electra models

[sijunhe]

PR types

Function optimization

PR changes

APIs

Description

Addressing part of #3382

• Adding inputs_embeds argument for Electra models to provide more control over how to convert input_ids indices
  into the embedding space. This is particularly useful for use cases such as P-Tuning.
• Remove TransformerEncoderPro and switch to paddle.nn.TransformerEncoder

[CLAassistant]

All committers have signed the CLA.

[guoshengCS]

LGTM

[wj-Mcat]

Your PR looks so great, and let's discuss about two small suggestions. Waiting for your comments.

[wj-Mcat]

I think this code block can be improved to:

        if input_ids is None:
            inputs_embeds = self.word_embeddings(input_ids)

and in the forward method, rename input_embeddings to inputs_embeds. In this way, the code looks more concise. how do you think about it?

[guoshengCS]

Should the input_embeddings = self.word_embeddings(input_ids) in following original code be removed

        if token_type_ids is None:
            token_type_ids = paddle.zeros_like(input_ids, dtype="int64")
        input_embeddings = self.word_embeddings(input_ids)
        position_embeddings = self.position_embeddings(position_ids)
        token_type_embeddings = self.token_type_embeddings(token_type_ids)
        embeddings = input_embeddings + position_embeddings + token_type_embeddings

[sijunhe]

Thanks for the review folks.
@wj-Mcat While I agree that renaming input_embeddings to inputs_embeds makes the code more concise, it also makes it less explicit/readable. Therefore I prefer the way it is now.
@guoshengCS good call. Removed the redundant line of code.

[wj-Mcat]

if not use_inputs_embeds, it should not prepare the input_ids tensor in prepare_config_and_inputs method.

[sijunhe]

Addressed

[sijunhe]

Addressed both comments. I think this PR should be ready for merging.

[wj-Mcat]

There is another to-do under the TransformerEncoderPro class, you can refer to :

PaddleNLP/paddlenlp/transformers/model_outputs.py

Lines 147 to 152 in f43cfd0

	if cache is None and getattr(self, "_use_cache", False):
	cache = [tuple(self.layers[0].gen_cache(src))] * len(self.layers)
	# To be compatible with `TransformerEncoder.forward`, `_use_cache` defualts
	# to True when cache is not None.
	new_caches = [] if cache is not None and getattr(self, "_use_cache",
	True) else None

you set the _use_cache attribute in ElectraModel but not implementing the handler for it. So this block should be changed, how do you think about it? @sijunhe

[sijunhe]

There is another to-do under the TransformerEncoderPro class, you can refer to :

PaddleNLP/paddlenlp/transformers/model_outputs.py

Lines 147 to 152 in f43cfd0

	if cache is None and getattr(self, "_use_cache", False):
	cache = [tuple(self.layers[0].gen_cache(src))] * len(self.layers)
	# To be compatible with `TransformerEncoder.forward`, `_use_cache` defualts
	# to True when cache is not None.
	new_caches = [] if cache is not None and getattr(self, "_use_cache",
	True) else None

you set the _use_cache attribute in ElectraModel but not implementing the handler for it. So this block should be changed, how do you think about it? @sijunhe

Good catch!
As this to-do pertains to the use of cache and past_key_values, it is outside the scope of this PR. To incorporate the change you asked, I'd like to change not only the handle, but the unit test as well. I think we should merge this PR first and I'll create a new PR for the implementation.

Regarding the to-do:
I looked at _transformer_encoder_fwd you linked and the TransformerEncoderPro in ELECTRA and seems like they are identical. I should be able to directly use the existing patched paddle.nn.Transformer instead of creating one for electra, right?

[wj-Mcat]

There are some thing I want to tell you:

1. the implementation of paddle.nn.Transformer** don't contian full features we wanted, so here is why TransformerEncoderPro was born.
2. _transformer_encoder_fwd the TransformerEncoderPro are actually identical and should be refactored into paddlenlp/layers/transformer.py as the TransformerEncoderPro class.

and there are some modules that using paddle.nn.TransformerEncoder、paddle.nn.TransformerEncoderLayer、paddle.nn.TransformerDecoder、paddle.nn.TransformerDecoderLayer, you can change the related module to make it more unified.

I prefer that you do it in this pr. how do you think about it？ @sijunhe @guoshengCS

[wj-Mcat]

In order to make this pr merged, you can make some changes in TransformerEncoderPro class under the electra.modeling module. The works of refactoring can be done in next few weeks. @sijunhe

[sijunhe]

In order to make this pr merged, you can make some changes in TransformerEncoderPro class under the electra.modeling module. The works of refactoring can be done in next few weeks. @sijunhe

I noticed that before #3411, TransformerEncoderPro is basically paddle.nn.TransformerEncoder without the cache input and output. By adding the cache functionalities in #3411 and #3401, TransformerEncoderPro would be identical to paddle.nn.TransformerEncoder. Hence I think we can just use paddle.nn.TransformerEncoder in electra

[wj-Mcat]

LGTM