Remove value error

[jplu]

What does this PR do?

This PR update the behavior of the input. We should not raise an error if the name is not among the parameters but act like if there was no name, this is more elegant and less annoying.

[sgugger]

Could you elaborate on the use case? It seems dangerous and magical to me. When passing a parameter to a function that is not in the signature, the user gets a ValueError.

[jplu]

Sure! an EagerTensor doesn't have a .name attribute so we assume for that case that the values are given in the parameters order. That's ok because we don't have the choice, but why not having the same behavior in case someone decides to name the tensors as he wishs.

This is very picky, and I won't fight at all if not accepted ahah

[sgugger]

Mmm, but in this test we're not eager tensors since there is a .name attribute, or am I missing something?

[jplu]

While I was trying to explain this, a use case came to my mind, and indeed this behavior is not correct for an edge use case:

from transformers import AutoTokenizer, TFBertForSequenceClassification, BertConfig
import tensorflow as tf
import datasets

config = BertConfig.from_pretrained("bert-base-cased", num_labels=6)
tokenizer = AutoTokenizer.from_pretrained("bert-base-cased")
ds = datasets.load_dataset('emotion')
encoded_train = ds['train'].map(lambda examples: tokenizer(examples['text'], truncation=True, padding='max_length', max_length=128), batched = True)
encoded_train.set_format(type='tensorflow', columns=['input_ids', 'attention_mask', 'label'])
features_train = {x: encoded_train[x].to_tensor(default_value=0, shape=[None, 128]) for x in ['input_ids', 'attention_mask']}
train_ds = tf.data.Dataset.from_tensor_slices((features_train, encoded_train["label"])).batch(16)
input_ids = tf.keras.Input(shape=(128,), dtype='int32', name="input_ids")
attention_mask = tf.keras.Input(shape=(128, ), dtype='int32', name="attention_mask")
transformer = TFBertForSequenceClassification.from_pretrained("bert-base-cased", num_labels=6)
encoded = transformer([input_ids, attention_mask])
logits = encoded[0]
model = tf.keras.models.Model(inputs = [input_ids, attention_mask], outputs = logits)

model.compile(optimizer=tf.keras.optimizers.Adam(learning_rate=3e-5, epsilon=1e-08, clipnorm=1.0), 
              loss=tf.keras.losses.SparseCategoricalCrossentropy(from_logits=True), 
              metrics=[tf.keras.metrics.SparseCategoricalAccuracy('accuracy')])
model.fit(train_ds, epochs=1, steps_per_epoch=1)

We get:

ValueError: The tensor named IteratorGetNext:1 does not belong to the authorized list of names ['input_ids', 'attention_mask', 'token_type_ids', 'position_ids', 'head_mask', 'inputs_embeds', 'output_attentions', 'output_hidden_states', 'return_dict', 'labels', 'training'].

Which is normal because .fit() wraps the dataset into an iterator, and then the tensors are renamed accordingly. Thanks @sgugger for asking the question :)

[sgugger]

Thanks for explaining, I understand better now :-)

[jplu]

Ok,just realized it is even worse, the inputs gets an ID, here IteratorGetNext:1 and IteratorGetNext:0 but the order of the list is never guaranteed. I'm trying to think to a fix for this.

[jplu]

Ok, as long as we are naming the inputs accordingly to the parameters, the order is safe. For example:

input_ids = tf.keras.Input(shape=(128,), dtype='int32', name="input_ids")
attention_mask = tf.keras.Input(shape=(128, ), dtype='int32', name="attention_mask")

model = tf.keras.models.Model(inputs = [input_ids, attention_mask], outputs = ...)

Is perfectly fine and works as expected, but:

input_ids = tf.keras.Input(shape=(128,), dtype='int32')
attention_mask = tf.keras.Input(shape=(128, ), dtype='int32')

model = tf.keras.models.Model(inputs = [input_ids, attention_mask], outputs = ...)

Brings an undefined behavior into the order.

Nevertheless, there is still an issue. Let's imagine this case:

input_embeds = tf.keras.Input(shape=(768,), dtype='float32')
attention_mask = tf.keras.Input(shape=(128, ), dtype='int32')

model = tf.keras.models.Model(inputs = [input_embeds, attention_mask], outputs = ...)

Won't work because internally, the input_ids parameter will take the value of the input_embeds input. This can be solved by integrating the names of each parameter directly inside the model, but we cannot do this because of a bug in TF <= 2.4, and will be solved in the TF 2.5 release. So as long as this release is not out, we cannot fix this, so we have to live with this bug, even though this is an edge use case.

What do you think?

[sgugger]

I think we should document that this does not work and encourage users to use named inputs then.

[jplu]

I have completed the documentation of the input_processing function. Does-it sounds enough as explanation for you?

[sgugger]

LGTM!

[patrickvonplaten]

from the comment that was added above it seems like the tensor_name has to be in parameter_names no? But we still allow tensors without any parameter name?

[patrickvonplaten]

should we then maybe at least raise a warning here at the moment? And extend the functions docstring with a sentence that there might be unexpected behavior if the inputs have no names?

[jplu]

As you can see in the example, the tensors during explicite model creation cannot have a name that belongs to any parameter name. So yes we allow tensors that have a different name than the parameters. But, the input names must have a name that belongs to the parameters in order to be able to have a proper order in the list. It is more obvious when reading the full example above.

[jplu]

@patrickvonplaten I have updated the comment in order to make it clearer. Does-it sounds more understandable for you? I think there was a confusion between the terms symbolic input and tensor.

[patrickvonplaten]

should we instead raise a warning here maybe?

[jplu]

No need of an error or warning, as now, we allow tensors that have a different name. We don't have the choice because all the tensors that will be represented by a symbolic input simply cannot have a name that belongs to the parameters. So basically a warning will be raised everytime you will use a tf.data.dataset and for each batch of the dataset, it will simply pollute your standard output for nothing.

[jplu]

More specifically in the example given here #8985 (comment)

the symbolic inputs are:

input_ids = tf.keras.Input(shape=(128,), dtype='int32', name="input_ids")
attention_mask = tf.keras.Input(shape=(128, ), dtype='int32', name="attention_mask")

And the tensors that represents there symbolic inputs are respectively named IteratorGetNext:0 and IteratorGetNext:1 and these two names cannot be changed.

[jplu]

LGTM! @LysandreJik feel free to merge if the PR gets your approval!

[LysandreJik]

OK!