Move the tf.keras.layers.PeepholeLSTMCell to tfa

[qlzh727]

This cell was exported under tf core API as an experimental since 2.0, but I think tfa should be a better place for this implementation.

We are planing to deprecate and eventually remove the PeepholeLSTMCell in the tf core API once this is landed.

[bot-of-gabrieldemarmiesse]

@pedrolarben

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.

[qlzh727]

@seanpmorgan, currently the CI test is failed for 2 reasons https://github.com/tensorflow/addons/pull/1944/checks?check_run_id=796216452.

1. The typehint is not available for the new cell, since it was using the init from the keras LSTMCell. For a class doesn't have its own init(), can we suppress the warning if possible?

2. The unit test was failing since it is using some tf.compat.v1 API. I was copying the existing test since it was doing some numerical comparison with tf.v1 API for correctness test. Can we somehow allow that?

[gabrieldemarmiesse]

@qlzh727 Thanks for the pull request!

1. Sure, we can allow that, no problem. I'll need to modify the library that does the check. In the meantime, you can add this class to the list of exceptions avoid having the tests bothering you: https://github.com/tensorflow/addons/blob/master/tools/testing/source_code_test.py#L40

2. I see tf.compat.v1 is used three times. I believe it should be possible to find alternatives. Maybe using tf.random.set_seed tf.keras.layers.LSTMCell and tf.keras.initializers.Ones?

[qlzh727]

@qlzh727 Thanks for the pull request!

1. Sure, we can allow that, no problem. I'll need to modify the library that does the check. In the meantime, you can add this class to the list of exceptions avoid having the tests bothering you: https://github.com/tensorflow/addons/blob/master/tools/testing/source_code_test.py#L40

Sure, will do.

2. I see tf.compat.v1 is used three times. I believe it should be possible to find alternatives. Maybe using tf.random.set_seed tf.keras.layers.LSTMCell and tf.keras.initializers.Ones?

I can't change to keras.LSTMCell since only the tf.nn.rnn_cell.LSTMCell contains the implementation of peephole. It was the base line for me to verify the numbers. Could we allow the v1.compat in test only?

[gabrieldemarmiesse]

Yeah that makes sense. In addons, we're trying to show users how to write idiomatic tf 2.x code. Even if tf.compat is not going away anytime soon, we would prefer to use modern, future-proof TF code to minimize the technical debt. This was the rational when we decided to forbid tf.compat. In this case, a good alternative would be to hardcode some numbers in the tests, and check against that. That would prevent any regressions since we know that the current implementation is correct.

[qlzh727]

Yeah that makes sense. In addons, we're trying to show users how to write idiomatic tf 2.x code. Even if tf.compat is not going away anytime soon, we would prefer to use modern, future-proof TF code to minimize the technical debt. This was the rational when we decided to forbid tf.compat. In this case, a good alternative would be to hardcode some numbers in the tests, and check against that. That would prevent any regressions since we know that the current implementation is correct.

Done.

[WindQAQ]

LGTM!

[seanpmorgan]

LGTM Thanks!