RFC: Migrate gelu activation from Addons to Core #252
Conversation
seanpmorgan
requested review from
ematejska,
ewilderj,
martinwicke and
theadactyl
as code owners
|
cc @alextp to review whenever time allows |
| ## Relevant GitHub Issues | ||
| * https://github.com/tensorflow/tensorflow/pull/33945 | ||
| * https://github.com/tensorflow/addons/issues/550 | ||
| * https://github.com/tensorflow/tensorflow/issues/32783 |
There was a problem hiding this comment.
Could you add keras-team/keras#11834?
| * C++ : https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/custom_ops/activations/cc/kernels/gelu_op.h | ||
| * Does this include custom-op kernels? | ||
| * Yes, but currently proposing to just migrate the python composite op. This may | ||
| change with discussion in the RFC. |
There was a problem hiding this comment.
Does this mean only python implementation will be migrated to TF?
I ask this because I saw the below topic Is it worth bringing over the custom-op kernels for CPU/GPU? We really hope to add a core kernel for CPU, it will get benefit from fusion, mkldnn or other optimization easily.
There was a problem hiding this comment.
adding to the discussion Zantares(Teng Lu) mentions, there is a need for core C++ kernels over and above python was also requested and mentioned in the PR by pennporn here:
tensorflow/tensorflow#33945 (comment)
ematejska
changed the title
|
@ematejska @theadactyl I've marked this as accepted and it can be merged IMO since it was approved and the PR is going through: |
Following the yet to be merged template from: #241
This is a bit overdue so wanted to start the discussion period. Linking PR:
tensorflow/tensorflow#33945