Model templates by LysandreJik · Pull Request #7636 · huggingface/transformers

LysandreJik · 2020-10-07T12:26:07Z

This PR adds a cookiecutter-based utility to generate configuration/modeling/tokenization files, the test suites and the statements across the library necessary for adding a new model.

This PR's goal is to make adding a new model way simpler, by having a simple CLI request information, generate files, that will then need to be edited to implement the changes relative to BERT. The test suites are implemented and run.

Left to do:

Possible improvements:

Ask the user whether they want to support token_type_ids

For reviewers

If you review this PR, the simplest would be to review the add_new_model.py file, and to generate model files using the utility:

transformers-cli add_new_model

And review the generated files.
Reviewing the current files with {{cookiecutter.lowercase_modelname}} doesn't seem reviewer-friendly to me.

sgugger

This super cool!
I mainly have comments on some docstrings not updated to the new standards (Et tu Brute).

sgugger · 2020-11-03T19:25:59Z

+
+    def run(self):
+        # Ensure that there is no other `cookiecutter-template-xxx` directory in the current working directory
+        directories = [directory for directory in os.listdir() if "cookiecutter-template-" in directory[:22]]


Any reason not to use == in the test?

sgugger · 2020-11-03T19:34:40Z


 MODEL_NAMES_MAPPING = OrderedDict(
    [
+        # Add cased model names here


Suggested change

# Add cased model names here

# Add full (and cased) model names here

sgugger · 2020-11-03T19:35:56Z

@@ -0,0 +1,91 @@
+# Using the `cookiecutter` to generate models


The "the" is weird. Either "Using cookiecutter to generate models" or "Using the cookiecutter templates to generate models"

sgugger · 2020-11-03T19:37:06Z

+        encoder_intermediate_dim (:obj:`int`, `optional`, defaults to 4096):
+            Dimensionality of the "intermediate" (often named feed-forward) layer in decoder.
+        {% endif -%}
+        hidden_act (:obj:`str` or :obj:`function`, optional, defaults to "gelu"):


Suggested change

hidden_act (:obj:`str` or :obj:`function`, optional, defaults to "gelu"):

hidden_act (:obj:`str` or :obj:`function`, `optional`, defaults to :obj:`"gelu"`):

sgugger · 2020-11-03T19:37:33Z

+        {% endif -%}
+        hidden_act (:obj:`str` or :obj:`function`, optional, defaults to "gelu"):
+            The non-linear activation function (function or string) in the encoder and pooler.
+            If string, "gelu", "relu", "swish" and "gelu_new" are supported.


Suggested change

If string, "gelu", "relu", "swish" and "gelu_new" are supported.

If string, :obj:`"gelu"`, :obj:`"relu"`, :obj:`"selu"` and :obj:`"gelu_new"` are supported.

sgugger · 2020-11-03T19:47:42Z

+            Position outside of the sequence are not taken into account for computing the loss.
+        end_positions (:obj:`torch.LongTensor` of shape :obj:`(batch_size,)`, `optional`):
+            Labels for position (index) of the end of the labelled span for computing the token classification loss.
+            Positions are clamped to the length of the sequence (`sequence_length`).


Suggested change

Positions are clamped to the length of the sequence (`sequence_length`).

Positions are clamped to the length of the sequence (:obj:`sequence_length`).

sgugger · 2020-11-03T19:48:48Z

+
+
+# Public API
+def _get_shape(t):


Should this be after public API since it's private?

sgugger · 2020-11-03T19:49:19Z

+        r"""
+        start_positions (:obj:`torch.LongTensor` of shape :obj:`(batch_size,)`, `optional`):
+            Labels for position (index) of the start of the labelled span for computing the token classification loss.
+            Positions are clamped to the length of the sequence (`sequence_length`).


Suggested change

Positions are clamped to the length of the sequence (`sequence_length`).

Positions are clamped to the length of the sequence (:obj:`sequence_length`).

sgugger · 2020-11-03T19:49:26Z

+            Position outside of the sequence are not taken into account for computing the loss.
+        end_positions (:obj:`torch.LongTensor` of shape :obj:`(batch_size,)`, `optional`):
+            Labels for position (index) of the end of the labelled span for computing the token classification loss.
+            Positions are clamped to the length of the sequence (`sequence_length`).


Suggested change

Positions are clamped to the length of the sequence (`sequence_length`).

Positions are clamped to the length of the sequence (:obj:`sequence_length`).

sgugger · 2020-11-03T19:50:47Z

+from .configuration_{{cookiecutter.lowercase_modelname}} import {{cookiecutter.uppercase_modelname}}_PRETRAINED_CONFIG_ARCHIVE_MAP, {{cookiecutter.camelcase_modelname}}Config
+# End.
+
+# Below: "# Add cased model names here"


If name is changed in the corresponding file, this line should be adapted.

patrickvonplaten · 2020-11-03T20:21:31Z



 logger = logging.get_logger(__name__)
+logger.setLevel(logging.INFO)


is that a leftover or should this be merged?

patrickvonplaten · 2020-11-03T20:24:02Z

+```
+modelname [LysBERT]:
+uppercase_modelname [LYSBERT]: 
+lowercase_modelname [<model_name>]: 


model_name or lys_bert ?

Excited to have Lysbert in the library soon!

patrickvonplaten · 2020-11-03T20:26:07Z

+
+Choose if your model is an encoder-decoder, or an encoder-only architecture.
+
+If your model is an encoder-only architecture, the generated architecture will be based on the BERT model. 


Should we base "decoder-only" a.k.a. GPT2 also on BERT for now? or just leave them out?

patrickvonplaten · 2020-11-03T20:34:40Z

+        intermediate_size (:obj:`int`, optional, defaults to 3072):
+            Dimensionality of the "intermediate" (i.e., feed-forward) layer in the Transformer encoder.
+        {% else -%}
+        encoder_layers (:obj:`int`, `optional`, defaults to 12):


can we go for num_encoder_layers similar to num_hidden_layers and same for decoder_attention_heads => num_decoder_attention_heads?

patrickvonplaten · 2020-11-03T20:36:18Z

+        max_position_embeddings (:obj:`int`, optional, defaults to 512):
+            The maximum sequence length that this model might ever be used with.
+            Typically set this to something large just in case (e.g., 512 or 1024 or 2048).
+        type_vocab_size (:obj:`int`, optional, defaults to 2):


Is type_vocab_size actually used for Encoder-Decoder? I don't think this param is used in Bart

maybe add a if-else here as well if possible

patrickvonplaten · 2020-11-03T20:38:12Z

+            Dimensionality of the "intermediate" (i.e., feed-forward) layer in the Transformer encoder.
+        {% else -%}
+        encoder_layers (:obj:`int`, `optional`, defaults to 12):
+            Number of encoder layers, 6 are used for the `bart-base` model.


maybe delete the "6 are used for the bart-base model part"

patrickvonplaten · 2020-11-03T20:38:26Z

+        encoder_layers (:obj:`int`, `optional`, defaults to 12):
+            Number of encoder layers, 6 are used for the `bart-base` model.
+        decoder_layers (:obj:`int`, `optional`, defaults to 12):
+            Number of decoder layers, 6 are used for the `bart-base` model.


same here bart-base model should be removed probably

patrickvonplaten · 2020-11-03T20:39:55Z

+        hidden_dropout_prob=0.1,
+        attention_probs_dropout_prob=0.1,
+        max_position_embeddings=512,
+        type_vocab_size=2,


I'd throw this out for encoder-decoder type_vocab_size - there is currently no encoder-decoder model that uses this param

patrickvonplaten · 2020-11-03T20:45:02Z

+        return self.{{cookiecutter.lowercase_modelname}}.encoder
+
+    def get_output_embeddings(self):
+        return _make_linear_from_emb(self.{{cookiecutter.lowercase_modelname}}.shared)  # make it on the fly


this is not the good way of getting the output embedding. A nn.Embedding class should not be instantiated each time the function is called but rather tied in the very beginning at __init__ this should be changed in Bart as well. But I can update this part later if you want

patrickvonplaten · 2020-11-03T20:45:58Z

+            "use_cache": use_cache,  # change this to avoid caching (presumably for debugging)
+        }
+
+    def adjust_logits_during_generation(self, logits, cur_len, max_length):


very Bart-specific function that I'd probably throw out here, but I think it's okey for now!

patrickvonplaten · 2020-11-03T20:48:56Z

+        layer_state[self.cache_key] = {
+            "prev_key": k.view(bsz, self.num_heads, -1, self.head_dim),
+            "prev_value": v.view(bsz, self.num_heads, -1, self.head_dim),
+            "prev_key_padding_mask": key_padding_mask if not static_kv else None,


masks should not be in the cache - can update this part of the code though as well

patrickvonplaten

Amazing tool! Excited to try it out on a new model.

I think some BART code should be different for new encoder-decoder models, e.g.

Encoder and Decoder should each be a PretrainedModel on its own so that these models can be used as stand-alone models.
The caching is overly complex in Bart at the moment (attention_masks don't need to be cached...)

But I can take care of these things after the PR is merged

LysandreJik force-pushed the model-templates branch from bff5bbd to d661133 Compare October 15, 2020 12:22

LysandreJik marked this pull request as ready for review October 16, 2020 11:48

LysandreJik force-pushed the model-templates branch 2 times, most recently from 0355f30 to 1bd4ae9 Compare October 28, 2020 17:36

LysandreJik added 14 commits November 2, 2020 18:02

Model templates

7ef5cd5

TensorFlow

1cb130f

Remove pooler

efcdc5f

CI

ebd2c7f

Tokenizer + Refactoring

7eda3a1

Encoder-Decoder

e77e584

Let's go testing

cac1713

Encoder-Decoder in TF

333a037

Let's go testing in TF

85b21f7

Documentation

73cc0aa

README

a986942

Fixes

2901700

Better names

e928962

Style

ca60113

LysandreJik force-pushed the model-templates branch from fe0fdeb to 83f9a1e Compare November 2, 2020 23:19

LysandreJik added 3 commits November 2, 2020 18:20

Update docs

f969f98

Choose to skip either TF or PT

ed584d8

Code quality fixes

377d082

LysandreJik force-pushed the model-templates branch 2 times, most recently from e82579b to 4a602db Compare November 2, 2020 23:29

LysandreJik added 5 commits November 2, 2020 18:29

Add to testing suite

4a602db

Update file path

f12bd2b

Cookiecutter path

2f53251

Update transformers path

68104c8

Handle rebasing

ee998fb

LysandreJik force-pushed the model-templates branch from 1bff00e to ee998fb Compare November 3, 2020 15:23

LysandreJik requested review from patrickvonplaten and sgugger November 3, 2020 19:15

sgugger approved these changes Nov 3, 2020

View reviewed changes

patrickvonplaten reviewed Nov 3, 2020

View reviewed changes

patrickvonplaten approved these changes Nov 3, 2020

View reviewed changes

LysandreJik mentioned this pull request Nov 12, 2020

Model templates encoder only #8509

Merged

LysandreJik closed this Nov 12, 2020

LysandreJik deleted the model-templates branch April 27, 2022 15:42

	# Add cased model names here
	# Add full (and cased) model names here

		@@ -0,0 +1,91 @@
		# Using the `cookiecutter` to generate models

	hidden_act (:obj:`str` or :obj:`function`, optional, defaults to "gelu"):
	hidden_act (:obj:`str` or :obj:`function`, `optional`, defaults to :obj:`"gelu"`):

	If string, "gelu", "relu", "swish" and "gelu_new" are supported.
	If string, :obj:`"gelu"`, :obj:`"relu"`, :obj:`"selu"` and :obj:`"gelu_new"` are supported.

	Positions are clamped to the length of the sequence (`sequence_length`).
	Positions are clamped to the length of the sequence (:obj:`sequence_length`).



		logger = logging.get_logger(__name__)
		logger.setLevel(logging.INFO)


		Choose if your model is an encoder-decoder, or an encoder-only architecture.

		If your model is an encoder-only architecture, the generated architecture will be based on the BERT model.

Uh oh!

Conversation

LysandreJik commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For reviewers

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickvonplaten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LysandreJik commented Oct 7, 2020 •

edited

Loading