Migrate gelu to core

[WindQAQ]

Describe the feature and the current behavior/state.

tfa.activations.gelu would be moved to tf.keras.activations.gelu, as well as the CC kernels.

Relevant information

• Are you willing to contribute it (yes/no): yes
• Are you willing to maintain it going forward? (yes/no): yes
• Is there a relevant academic paper? (if so, where): yes
• Is there already an implementation in another framework? (if so, where): yes, addons
• Was it part of tf.contrib? (if so, where): no

Which API type would this fall under (layer, metric, optimizer, etc.)
activations

Who will benefit with this feature?
Whole community

Any other info.
As per tensorflow/tensorflow#32783, seems that keras is seeking for gelu activation, and accepts PR.

[WindQAQ]

cc @seanpmorgan and @facaiy.

[facaiy]

+@karmel for visibility

[karmel]

Metacomment: @ewilderj / @seanpmorgan -- I wonder if we should start tracking these moves? Seems like it's one interesting metric indicating the utility of Addons.

[ewilderj]

+1 I'd be strongly in favor of at least something like a GRADUATED.md doc that kept a list of these with the date they moved. You're right, it's great bragging rights, and also a good piece of documentation for users.

[AakashKumarNain]

I am happy that finally GeLU is moving to core. @WindQAQ let me know if you need any help in any way

[WindQAQ]

Hi all, my minor concern of this migration is that tfa's gelu is based on C++/CUDA kernel rather than a python function. So I am afraid the PR will not only affect tf.keras.* but also some cc files in tensorflow/core/* as well as tf.nn.*. Is it still okay for us to issue a single PR to cover core and keras part? Thank you all in advance!

[facaiy]

Is it still okay for us to issue a single PR to cover core and keras part?

We usually add both c++ and python codes when implementing a new OP. So I think the answer is yes. But I'm a little curious why we write c++ kernel implementation for the activation ops. Those ops looks quite simple, and it seems that they could be composed by tf python ops easily if I'm not wrong.

[WindQAQ]

Is it still okay for us to issue a single PR to cover core and keras part?

We usually add both c++ and python codes when implementing a new OP. So I think the answer is yes. But I'm a little curious why we write c++ kernel implementation for the activation ops. Those ops looks quite simple, and it seems that they could be composed by tf python ops easily if I'm not wrong.

The speed. On my local machine, the C++ kernel speed up 3 times and 10 times in forward and backward pass respectively compared with tf python ops. I know that XLA optimization could somewhat fuse multiple operations into a single kernel, but not all of the users can enjoy XLA on their own machine.

A related talk presented by NVIDIA. gelu's performance test on Colab. Similar issue requesting for prelu's C++ kernel in core TF tensorflow/tensorflow#31883.

[facaiy]

I agree that C++ implementation should run faster than python implementation, Tzu-Wei. But as the op is so small, I'm not sure whether it is worthy at the cost of maintaining a chunk of code. Moreover, the new op we add might not work well with graph optimization toolkit(say op fusion as you mentained, or TensorRT ect) because those tools might only focus on tf-core. I'm not against c++ implementation, just a little reminder: perhaps we could write c++ implementation or create a fused op later when performance sucks in the mainstream models 😄

[WindQAQ]

Thanks for pointing out! So what should we migrate for this time; a tf python operation implementation only or kernel version? Both are fine with me, but want to have your ideas 😄

[hendrycks]

Note PyTorch uses a kernel version.

…

On Fri, Oct 18, 2019 at 12:13 AM Tzu-Wei Sung ***@***.***> wrote: Thanks for pointing out! So what should we migrate for this time; a tf python operation implementation only or kernel version? Both are fine with me, but want to have your ideas 😄 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#550?email_source=notifications&email_token=ACZBITSBMIOMOAR2ZAN4EKDQPFOZXA5CNFSM4I35DB6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBTBFGQ#issuecomment-543560346>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZBITR3MBNTARHX3JKKOS3QPFOZXANCNFSM4I35DB6A> .

[WindQAQ]

Hi all, the PR has been created tensorflow/tensorflow#33945.

[AakashKumarNain]

Hi all, the PR has been created tensorflow/tensorflow#33945.

@WindQAQ Are we going with the Python implementation or with the C++ one?

[WindQAQ]

Hi all, the PR has been created tensorflow/tensorflow#33945.

@WindQAQ Are we going with the Python implementation or with the C++ one?

I use C++ implementation :D

[bhack]

+1 I'd be strongly in favor of at least something like a GRADUATED.md doc that kept a list of these with the date they moved. You're right, it's great bragging rights, and also a good piece of documentation for users.

Yes but the main issue Is the we always need to check in multiple places:
keras-team/keras#11834
keras-team/keras#12020
keras-team/keras#11835
keras-team/keras#11839

We need to find a solution for Keras repo other then contrib vs addon:
keras-team/keras-contrib#519

[seanpmorgan]

During the monthly call it was determined that the best way to move this forward is to create a Keras RFC per our new procedure:
https://github.com/tensorflow/addons/blob/master/MIGRATION_TO_CORE.md

The goal is to propose the custom op along with the accompanying performance tests. In the "alternatives considered" we can propose the python composite op for discussion.

@WindQAQ When time allows would you be able to author this RFC? I can help out as well.

[WindQAQ]

The goal is to propose the custom op along with the accompanying performance tests. In the "alternatives considered" we can propose the python composite op for discussion.

@WindQAQ When time allows would you be able to author this RFC? I can help out as well.

Of course!

[AakashKumarNain]

Lemme know if you guys need any help. I would be more than happy to help

[bhack]

@seanpmorgan

The goal is to propose the custom op along with the accompanying performance tests. In the "alternatives considered" we can propose the python composite op for discussion.
Also probably there is an impact with evolving MLIR compiler stack ecosystem about the codegen path vs c++ implemented in custom ops.

[seanpmorgan]

Closing as this will be finished up in #2005