Fix MultiOptimizer list of layers #2180

WindQAQ · 2020-09-25T19:07:26Z

Description

Fixes #2178

Type of change

Checklist:

I've properly formatted my code according to the guidelines
- By running Black + Flake8
- By running pre-commit hooks
This PR addresses an already submitted issue for TensorFlow Addons
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
This PR contains modifications to C++ custom-ops

How Has This Been Tested?

Additional test.

bot-of-gabrieldemarmiesse · 2020-09-25T19:08:13Z

@hyang0129

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

WindQAQ · 2020-09-25T19:13:41Z

tensorflow_addons/optimizers/tests/discriminative_layer_training_test.py

 @pytest.mark.with_device(["cpu", "gpu"])
-@pytest.mark.parametrize("dtype", [tf.float16, tf.float32, tf.float64])


This is not being used in the original tests, so I remove it

But we should add this test, right?

bhack

From a quick overview It seems that we are not covering the optimizer, model case?

bhack · 2020-09-25T23:19:07Z

tensorflow_addons/optimizers/discriminative_layer_training.py

-        if type(layer) == list:
+        The name of each variable is used rather than `var.ref()` to enable serialization and deserialization.
+        """
+        if isinstance(layer, list):
            weights = [var.name for sublayer in layer for var in sublayer.weights]


Is here trainable_weights?

This is discussed in #969 (comment).

I would respect the decision of code owner. Change in design is not part of this PR. /cc @hyang0129

If it was interpreted as we have weights and we set the same weights at semantic level is ok

Yep. We can wait the response from the code owner. I see the underlying fit only takes trainable variables into account. As you recommend, we can change it to trainable_weights after dicussion.

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/keras/engine/training.py#L737

Yes this is V1.
In V2 you need to pass a variable list or a callable as it has no default but they suppose as default example:
https://github.com/tensorflow/tensorflow/blob/v2.3.1/tensorflow/python/keras/optimizer_v2/optimizer_v2.py#L118-L120

Instead in our apply_gradients we are controlling internally the variable list to pass to the real apply_gradiennts of optimizers. It seems to me that we are performing a little bit like fit->minimize method or not?

We are not “controlling the var list” internally but fetching only vars that users pass in.

If we look at the argument ok we receive an user argument but internally we access to our prepared spec["weights"]:
for name in spec["weights"]:

the multi optimizer stores a reference of weight names.

when optimizing, it uses this reference to allocate grad var pairs to the correct optimizer.

it does not determine what weights are passed to the multi optimizer.

So you may have a situation where 5 vars are passed but spec['weights'] contains 10 var names and only 3 of them match. Thus, only 3 are passed on.

The original design is intended to be used in the model.fit function, not a custom optimization loop. Even in a custom optimization loop, the user is expected to specify the var list to include only trainable weights. I am fairly certain that when an optimizer is called in a fit loop, the var list passed includes only trainable variables. Thus, the var list passed to the multi optimizer includes only trainable variables. Finally, the multi optimizer only passes on to its sub optimizers the variables in var list it received that match a variable in spec['weights'] for that particular optimizer. If this is true, then a non trainable variable will never make it to the multi optimizer or sub optimizers.

This behavior has been tested in a colab notebook. When a layer's trainable attribute has been set to false, the multi optimizer's sub optimizer assigned to that layer does not optimizer the weights for that layer.

If it Is a case where weight is a superset to check the name probably ok.

But I still don't catch when if var.name == name needs to handle not trainable weights.

bhack · 2020-09-25T23:19:29Z

tensorflow_addons/optimizers/discriminative_layer_training.py

-        if type(layer) == list:
+        The name of each variable is used rather than `var.ref()` to enable serialization and deserialization.
+        """
+        if isinstance(layer, list):
            weights = [var.name for sublayer in layer for var in sublayer.weights]
        else:
            weights = [var.name for var in layer.weights]


WindQAQ · 2020-09-25T23:28:06Z

From a quick overview It seems that we are not covering the optimizer, model case?

Not sure what this case is. Can you elaborate it?

bhack · 2020-09-25T23:52:35Z

I meant it seems that tf.Model could be an input right? Do we have a test for this input case?

WindQAQ · 2020-09-26T00:07:09Z

I meant it seems that tf.Model could be and input right? Do we have a test for this input case?

Thanks for the info. Updated the related tests :-)

bhack · 2020-09-26T00:10:21Z

tensorflow_addons/optimizers/discriminative_layer_training.py

+    def create_optimizer_spec(
+        cls,
+        optimizer: tf.keras.optimizers.Optimizer,
+        layer: Union[


Also the naming here It Is a little bit confusing also in the internal code cause if layers with s It could cover the list case but what about tf.keras.Model?

Sure, the naming is not so good to me either, but I cannot come up with a new one... do you have any suggestion?

Ugly as ugly we already have optimizers_and_layers.
With another boolean we could have layers_or_model 😄

Can you share the full input signature you propose?

that's a good idea. please go ahead with the renaming.

layer -> layers_or_model

Hi, tf.keras.Model is a subclass of tf.keras.layers.Layer. Do we still need to do this?

It is a borderline case cause it is multi inheritance:
class Model(base_layer.Layer, version_utils.ModelVersionSelector):
We are only using layer the single base class features but I don't know about readability.

So as you want cause we cannot upcast to Layer in python or It Is generally a borderline pratice

Updated the naming. See if this is better now :-)

hyang0129 · 2020-09-26T00:31:17Z

Sorry was on vacation. I'll take a look tomorrow.

tensorflow_addons/optimizers/tests/discriminative_layer_training_test.py

hyang0129 · 2020-09-26T21:52:14Z

tensorflow_addons/optimizers/tests/discriminative_layer_training_test.py

+    multi_optimizer = MultiOptimizer(optimizers_and_layers)
+    model.compile(multi_optimizer, loss="mse")
+
+    x = np.random.rand(128, 4)


I would recommend using a signal rather than complete noise. The purpose of this test is to demonstrate that the model weights will move when there is a signal based on the optimizer setup.

Here, when you use np rand, you are generating noise without any signal. Technically, the signal is the average, but the model will likely memorize the input based on the model size and number of examples of x.

You can choose to leave it as is because it will test the multi optimizer. In the future, people might be confused (temporarily) as to what signal the model is trying to learn.

Nice suggestion :-) Updated

hyang0129 · 2020-09-27T01:30:35Z

tensorflow_addons/utils/test_utils.py

@@ -217,6 +217,25 @@ def pytest_collection_modifyitems(items):
                item.add_marker(pytest.mark.skip("The gpu is not available."))


+def assert_not_allclose(a, b, **kwargs):


hyang0129 · 2020-09-27T15:56:06Z

tensorflow_addons/optimizers/discriminative_layer_training.py

+    Each optimizer will optimize only the weights associated with its paired layer.
+    This can be used to implement discriminative layer training by assigning
+    different learning rates to each optimizer layer pair.
+    `(tf.keras.optimizers.Optimizer, List[tf.keras.layers.Layer])` pairs are also supported.


List[tf.keras.layers.Layer]) -> List([tf.keras.layers.Layer]). Was missing a (

It's ( optimizer, List[layer] ), where () stands for Tuple.

hyang0129 · 2020-09-27T15:57:07Z

@bhack I think this is good to go.

* Fix MultiOptimizer list of layers * Fix name * Remove unused tests * Change list to iterable * Update doc * Update code snippet * Update doc * Back to list * Update error message * Update doc * Fix tmpdir fixture * Fix tmpdir * Update doc * Add test on tf.keras.Model * Add nested model tests * Better naming * Add custom subclass model tests * Inherit from Layer * Move assert_not_allclose to test_utils * Change input to ones * Inherit from Model * Test all weights instead of first one * Update doc

WindQAQ added 2 commits September 25, 2020 12:02

Fix MultiOptimizer list of layers

77ee266

Fix name

7ba62e1

boring-cyborg bot added the optimizers label Sep 25, 2020

googlebot added the cla: yes label Sep 25, 2020

WindQAQ requested a review from a team September 25, 2020 19:07

WindQAQ mentioned this pull request Sep 25, 2020

Fix assert for list of layers #2179

Closed

16 tasks

Remove unused tests

8217c81

WindQAQ commented Sep 25, 2020

View reviewed changes

WindQAQ added 10 commits September 25, 2020 12:27

Change list to iterable

9496b88

Update doc

db132b7

Update code snippet

015d581

Update doc

10cc299

Back to list

049ee25

Update error message

a95bf2f

Update doc

919390c

Fix tmpdir fixture

3dbcaa8

Fix tmpdir

1eb159e

Update doc

9689116

bhack requested changes Sep 25, 2020

View reviewed changes

Add test on tf.keras.Model

d334fab

bhack reviewed Sep 26, 2020

View reviewed changes

tensorflow_addons/optimizers/tests/discriminative_layer_training_test.py Outdated Show resolved Hide resolved

Add nested model tests

ecb90c1

bhack reviewed Sep 26, 2020

View reviewed changes

tensorflow_addons/optimizers/tests/discriminative_layer_training_test.py Show resolved Hide resolved

Better naming

d7c3c09

Add custom subclass model tests

31aa6ef

hyang0129 reviewed Sep 27, 2020

View reviewed changes

WindQAQ added 4 commits September 26, 2020 18:18

Inherit from Layer

7036ecc

Move assert_not_allclose to test_utils

e6bbaa7

Change input to ones

2de24df

Inherit from Model

1733448

hyang0129 previously approved these changes Sep 27, 2020

View reviewed changes

Test all weights instead of first one

f644308

WindQAQ dismissed hyang0129’s stale review via f644308 September 27, 2020 01:35

Update doc

7db5362

hyang0129 reviewed Sep 27, 2020

View reviewed changes

bhack approved these changes Sep 27, 2020

View reviewed changes

WindQAQ merged commit 392f36c into tensorflow:master Sep 27, 2020

		@pytest.mark.with_device(["cpu", "gpu"])
		@pytest.mark.parametrize("dtype", [tf.float16, tf.float32, tf.float64])

		@@ -217,6 +217,25 @@ def pytest_collection_modifyitems(items):
		item.add_marker(pytest.mark.skip("The gpu is not available."))


		def assert_not_allclose(a, b, **kwargs):

Fix MultiOptimizer list of layers #2180

Fix MultiOptimizer list of layers #2180

Conversation

WindQAQ commented Sep 25, 2020

Description

Type of change

Checklist:

How Has This Been Tested?

Uh oh!

bot-of-gabrieldemarmiesse commented Sep 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhack 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!

hyang0129 Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

WindQAQ commented Sep 25, 2020

Uh oh!

bhack commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WindQAQ commented Sep 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WindQAQ Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WindQAQ Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

WindQAQ Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhack Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hyang0129 commented Sep 26, 2020

Uh oh!

Uh oh!

hyang0129 Sep 27, 2020 •

edited

Loading

bhack commented Sep 25, 2020 •

edited

Loading

WindQAQ Sep 26, 2020 •

edited

Loading

WindQAQ Sep 26, 2020 •

edited

Loading

WindQAQ Sep 26, 2020 •

edited

Loading

bhack Sep 26, 2020 •

edited

Loading