keras-team / tf-keras

The TensorFlow-specific implementation of the Keras API, which was the default Keras from 2019 to 2023.
Apache License 2.0
63 stars 28 forks source link

Using output names instead of node names for loss, exporting and serving #528

Open eugen-vusak opened 2 years ago

eugen-vusak commented 2 years ago

If you open a GitHub issue, here is our policy:

It must be a bug, a feature request, or a significant problem with the documentation (for small docs fixes please send a PR instead). The form below must be filled out.

Here's why we have that policy:.

Keras developers respond to issues. We want to focus on work that benefits the whole community, e.g., fixing bugs and adding features. Support only helps individuals. GitHub also notifies thousands of people when issues are filed. We want them to see you communicating an interesting problem, rather than being redirected to Stack Overflow.

System information. Linux 5.18.9-arch1-1

TensorFlow version (you are using): 2.9.0 Are you willing to contribute it (Yes/No) : Yes

Describe the feature and the current behavior/state.

When I have custom layer that returns dict of tensors then I would like for loss to use those names because order could change if outputs change.

For example, referencing the code bellow, if I set my_layer_1_mse as a monitor variable in early stopping and change other outputs this will no longer work if out2 changes from my_layer_1_mse to e.g. my_layer_2_mse

I believe that loss and metrics be named out1_loss and out2_mse in this specific case. Or at least my_layer_out1_loss and my_layer_out2_loss

Another problem is that I would like for model outputs to follow same naming convention if possible when exporting model to, for example, to tfjs. Currently outputs will be named my_layer and my_layer_1 instead of out1 and out2 respectively. Again requesting an web page to update every time underlying implementation is changed.

This is possible to do using Model sub-classing instead of Functional API but then some other problems arise because Functional API is just so convenient.

Same problem is with tensorflow-serving. Outputs are named based on node names and if I return outputs that are dict, e.g. {"out1": out1_var, "out2": out2_var} than REST API response will not be {"output1": [...], "output2": [...]} but it will be some random complex node name that can change in the future. This can be achieved by using custom signatures when saving a model but I think this should default behavior to use names from outputs for both.

I think this will make everything more consistent and allow for better deployment and monitoring solutions.

Here is a simple code snippet explaining situation:

import tensorflow as tf

class Layer(tf.keras.layers.Layer):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)

    def call(self, inputs):
        batch_size = tf.shape(inputs)[0]
        return {"out1": tf.repeat(1, batch_size), "out2": tf.repeat(2, batch_size)}

def build_model():

    input = tf.keras.layers.Input(shape=(1,))
    out = Layer(name="my_layer")(input)

    return tf.keras.Model(
        inputs=input,
        outputs=out,
    )

model = build_model()

model.compile(
    optimizer=tf.keras.optimizers.Adam(),
    loss={"out1": "mse"},
    metrics={"out2": "mse"},
)

data = (
    # x
    [5, 5, 5, 5, 5, 5, 5, 5],
    # y
    {
        "out1": [5, 5, 5, 5, 5, 5, 5, 5],
        "out2": [5, 5, 5, 5, 5, 5, 5, 5],
    },
)

ds = tf.data.Dataset.from_tensor_slices(data)
ds = ds.batch(2)

model.fit(ds)

and when training loss is called by node name which can be seen here:

4/4 [==============================] - 0s 2ms/step - loss: 16.0000 - my_layer_loss: 16.0000 - my_layer_1_mse: 9.0000

Will this change the current api? How? This will change default loss name and exported model output names for everybody that use named outputs.

Who will benefit from this feature? Everybody who uses named outputs and doesn't want for loss names and exported model output names change with implementation of the same model.

Contributing

sushreebarsa commented 2 years ago

@gadagashwini I was able to replicate the issue, please find the gist here . Thank you!

rchao commented 2 years ago

Thanks for the report @eugen-vusak - we don't see an immediate priority needed for this but if you're interested, we welcome a PR from you and take a look if the request makes sense.