From 6a8f78967c37c5ebf27d5105233ccd99b25cd6bf Mon Sep 17 00:00:00 2001 From: Sean Morgan Date: Mon, 25 May 2020 22:08:11 -0400 Subject: [PATCH 1/7] * gelu migration RFC --- rfcs/20200525-gelu-migration.md | 75 +++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 rfcs/20200525-gelu-migration.md diff --git a/rfcs/20200525-gelu-migration.md b/rfcs/20200525-gelu-migration.md new file mode 100644 index 000000000..e2312b2bd --- /dev/null +++ b/rfcs/20200525-gelu-migration.md @@ -0,0 +1,75 @@ +# Migrate gelu activation from TensorFlow Addons to TensorFlow Core + +| Status | Proposed (Waiting for Sponsor) | +| :---------- | :------------------------------------------------------------------------------------------------- | +| **RFC #** | TBD after PR | | +| **Authors** | Tzu-Wei Sung (@WindQAQ) & Sean Morgan (@seanpmorgan) | +| **Sponsor** | TBD | +| **Updated** | 2020-05-XX | +| **Sponsorship Deadline** | 2020-07-XX (45 Days after submission) | + +## Rationale for Migration +* [Gaussian Error Linear Units (GELUs)](https://arxiv.org/pdf/1606.08415.pdf) cited 600+ times +* Used in BERT and other influential architectures +* Multiple approvals from TF side: + * https://github.com/tensorflow/tensorflow/pull/33945#issuecomment-617832325 + * https://github.com/tensorflow/tensorflow/issues/32783#issuecomment-537284266 + +## Historical Information +* Have there been signifiant issues reported to Addons that need to be adressed? + * Only ABI incompatibilities for the custom-op (not an issue if built along with core TF) +* When was it implemented in Addons? + * C++ custom-op added **2019-08-2019 (TFA 0.5.0)** + * Python composite op added **2020-02-26 (TFA 0.9.0)** +* We have [performed benchmarking of the GELU activation](https://colab.research.google.com/drive/1rLb4EuydbFg9PbhboXhCDqopcl6BmphG#scrollTo=0GL2x2S4zxW3) +which shows there it may be beneficial to retain the custom-ops, but the maintence burden has grown +to much for us to continue to support it in Addons. +* This migration is long over-due but we've struggled with finalizing the migration process. + +## Implementation Details +* Link to implementation in Addons: + * Python: [https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/activations/gelu.py](https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/activations/gelu.py) + * C++ : [https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/custom_ops/activations/cc/kernels/gelu_op.h](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. + * Are they CPU/GPU/TPU compatible? + * CPU/GPU compatible. No support for TPU. +* What is the pytest coverage of the addon? + * `tensorflow_addons/activations/gelu.py 89%` +## Changes to Implementation (If Needed) +``` +def gelu(x: types.TensorLike, approximate: bool = True) -> tf.Tensor: + x = tf.convert_to_tensor(x) + if approximate: + pi = tf.cast(math.pi, x.dtype) + coeff = tf.cast(0.044715, x.dtype) + return 0.5 * x * (1.0 + tf.tanh(tf.sqrt(2.0 / pi) * (x + coeff * tf.pow(x, 3)))) + else: + return 0.5 * x * (1.0 + tf.math.erf(x / tf.cast(tf.sqrt(2.0), x.dtype))) +``` +The above implementation would only bring over the python composite op. Since there is +[no way for us to build tfxla kernels](https://github.com/tensorflow/tensorflow/pull/33945#issuecomment-617842977) +we had no support for TPUs in Addons. [There were comments](https://github.com/tensorflow/tensorflow/pull/33945#issuecomment-625380208) +about using a "selector", but we would need guidance on how to implement that. + +We may also want to discuss the `approximate` bool and if it should be included in the +upstream version. + + +## Transition Plan +* The activation would land in [nn_ops.py](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow//python/ops/nn_ops.py) as well as in [keras advaced_activations](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow/python/keras/layers/advanced_activations.py) +* No planned changes to the parameter signatures at this time +* Addons would deprecate our activation and make a call to the core functionality. + +## Relevant GitHub Issues +https://github.com/tensorflow/tensorflow/pull/33945 +https://github.com/tensorflow/addons/issues/550 +https://github.com/tensorflow/tensorflow/issues/32783 + +## Questions and Discussion Topics +* Whom from the TF core team would sponsor this migration and ownership of the API? +* Is it worth brining over the custom-op kernels for CPU/GPU? + +## Final Decision +TBD From 8fc2dd35cf0aa291c96ff519a5110bb6310b9f80 Mon Sep 17 00:00:00 2001 From: Sean Morgan Date: Wed, 27 May 2020 19:38:41 -0400 Subject: [PATCH 2/7] typos --- rfcs/20200525-gelu-migration.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/20200525-gelu-migration.md b/rfcs/20200525-gelu-migration.md index e2312b2bd..402723127 100644 --- a/rfcs/20200525-gelu-migration.md +++ b/rfcs/20200525-gelu-migration.md @@ -22,7 +22,7 @@ * C++ custom-op added **2019-08-2019 (TFA 0.5.0)** * Python composite op added **2020-02-26 (TFA 0.9.0)** * We have [performed benchmarking of the GELU activation](https://colab.research.google.com/drive/1rLb4EuydbFg9PbhboXhCDqopcl6BmphG#scrollTo=0GL2x2S4zxW3) -which shows there it may be beneficial to retain the custom-ops, but the maintence burden has grown +which shows it may be beneficial to retain the custom-ops, but the maintence burden has grown to much for us to continue to support it in Addons. * This migration is long over-due but we've struggled with finalizing the migration process. @@ -69,7 +69,7 @@ https://github.com/tensorflow/tensorflow/issues/32783 ## Questions and Discussion Topics * Whom from the TF core team would sponsor this migration and ownership of the API? -* Is it worth brining over the custom-op kernels for CPU/GPU? +* Is it worth bringing over the custom-op kernels for CPU/GPU? ## Final Decision TBD From a3b8b6d8547c30ed1a6d51f3b6a4085cd43ea885 Mon Sep 17 00:00:00 2001 From: Sean Morgan Date: Wed, 27 May 2020 19:58:33 -0400 Subject: [PATCH 3/7] Spacing --- rfcs/20200525-gelu-migration.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rfcs/20200525-gelu-migration.md b/rfcs/20200525-gelu-migration.md index 402723127..4a7ed1dfe 100644 --- a/rfcs/20200525-gelu-migration.md +++ b/rfcs/20200525-gelu-migration.md @@ -64,7 +64,9 @@ upstream version. ## Relevant GitHub Issues https://github.com/tensorflow/tensorflow/pull/33945 + https://github.com/tensorflow/addons/issues/550 + https://github.com/tensorflow/tensorflow/issues/32783 ## Questions and Discussion Topics From 8f1af584c6ce7f6ba6edc42e28413b50420ccfee Mon Sep 17 00:00:00 2001 From: Sean Morgan Date: Wed, 27 May 2020 20:49:25 -0400 Subject: [PATCH 4/7] * Update transition --- rfcs/20200525-gelu-migration.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/rfcs/20200525-gelu-migration.md b/rfcs/20200525-gelu-migration.md index 4a7ed1dfe..ee013b757 100644 --- a/rfcs/20200525-gelu-migration.md +++ b/rfcs/20200525-gelu-migration.md @@ -28,8 +28,8 @@ to much for us to continue to support it in Addons. ## Implementation Details * Link to implementation in Addons: - * Python: [https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/activations/gelu.py](https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/activations/gelu.py) - * C++ : [https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/custom_ops/activations/cc/kernels/gelu_op.h](https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/custom_ops/activations/cc/kernels/gelu_op.h) + * Python: https://github.com/tensorflow/addons/blob/r0.10/tensorflow_addons/activations/gelu.py + * 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. @@ -61,13 +61,15 @@ upstream version. * The activation would land in [nn_ops.py](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow//python/ops/nn_ops.py) as well as in [keras advaced_activations](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow/python/keras/layers/advanced_activations.py) * No planned changes to the parameter signatures at this time * Addons would deprecate our activation and make a call to the core functionality. +* After merging to TF Core: + * Consolidate/remove https://github.com/tensorflow/models/blob/r2.2.0/official/modeling/activations/gelu.py + * Consolidate/remove https://github.com/tensorflow/models/blob/r2.2.0/official/modeling/activations/gelu_test.py + * Consolidate/remove https://github.com/tensorflow/models/blob/r2.2.0/official/nlp/xlnet/xlnet_modeling.py#L29 ## Relevant GitHub Issues -https://github.com/tensorflow/tensorflow/pull/33945 - -https://github.com/tensorflow/addons/issues/550 - -https://github.com/tensorflow/tensorflow/issues/32783 +* https://github.com/tensorflow/tensorflow/pull/33945 +* https://github.com/tensorflow/addons/issues/550 +* https://github.com/tensorflow/tensorflow/issues/32783 ## Questions and Discussion Topics * Whom from the TF core team would sponsor this migration and ownership of the API? From 0a2db5dee61be987f740687e59eb1172785bdb06 Mon Sep 17 00:00:00 2001 From: Sean Morgan Date: Tue, 2 Jun 2020 11:04:42 -0400 Subject: [PATCH 5/7] Add Keras activations --- rfcs/20200525-gelu-migration.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rfcs/20200525-gelu-migration.md b/rfcs/20200525-gelu-migration.md index ee013b757..372ef136a 100644 --- a/rfcs/20200525-gelu-migration.md +++ b/rfcs/20200525-gelu-migration.md @@ -5,8 +5,8 @@ | **RFC #** | TBD after PR | | | **Authors** | Tzu-Wei Sung (@WindQAQ) & Sean Morgan (@seanpmorgan) | | **Sponsor** | TBD | -| **Updated** | 2020-05-XX | -| **Sponsorship Deadline** | 2020-07-XX (45 Days after submission) | +| **Updated** | 2020-06-02 | +| **Sponsorship Deadline** | 2020-07-17 (45 Days after submission) | ## Rationale for Migration * [Gaussian Error Linear Units (GELUs)](https://arxiv.org/pdf/1606.08415.pdf) cited 600+ times @@ -58,7 +58,8 @@ upstream version. ## Transition Plan -* The activation would land in [nn_ops.py](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow//python/ops/nn_ops.py) as well as in [keras advaced_activations](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow/python/keras/layers/advanced_activations.py) +* The activation would land in [nn_ops.py](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow//python/ops/nn_ops.py), [keras activations](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow/python/keras/activations.py), + and possibly in [keras advaced_activation layers](https://github.com/tensorflow/tensorflow/blob/r2.2/tensorflow/python/keras/layers/advanced_activations.py) * No planned changes to the parameter signatures at this time * Addons would deprecate our activation and make a call to the core functionality. * After merging to TF Core: From 1c2bca8fbc7f70f5763b946961147d9eb04c83a0 Mon Sep 17 00:00:00 2001 From: Sean Morgan Date: Tue, 2 Jun 2020 12:04:05 -0400 Subject: [PATCH 6/7] Add RFC number --- rfcs/20200525-gelu-migration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/20200525-gelu-migration.md b/rfcs/20200525-gelu-migration.md index 372ef136a..6b133f6eb 100644 --- a/rfcs/20200525-gelu-migration.md +++ b/rfcs/20200525-gelu-migration.md @@ -2,7 +2,7 @@ | Status | Proposed (Waiting for Sponsor) | | :---------- | :------------------------------------------------------------------------------------------------- | -| **RFC #** | TBD after PR | | +| **RFC #** | [252](https://github.com/tensorflow/community/pull/252) | | | **Authors** | Tzu-Wei Sung (@WindQAQ) & Sean Morgan (@seanpmorgan) | | **Sponsor** | TBD | | **Updated** | 2020-06-02 | From cff37256efdb0c4e983fa78d6be5ba8b1fcb94b3 Mon Sep 17 00:00:00 2001 From: Sean Morgan Date: Wed, 15 Jul 2020 20:38:56 -0400 Subject: [PATCH 7/7] * Marking RFC as completed --- rfcs/20200525-gelu-migration.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rfcs/20200525-gelu-migration.md b/rfcs/20200525-gelu-migration.md index 6b133f6eb..bd6271dee 100644 --- a/rfcs/20200525-gelu-migration.md +++ b/rfcs/20200525-gelu-migration.md @@ -1,11 +1,11 @@ # Migrate gelu activation from TensorFlow Addons to TensorFlow Core -| Status | Proposed (Waiting for Sponsor) | +| Status | Accepted | | :---------- | :------------------------------------------------------------------------------------------------- | | **RFC #** | [252](https://github.com/tensorflow/community/pull/252) | | | **Authors** | Tzu-Wei Sung (@WindQAQ) & Sean Morgan (@seanpmorgan) | -| **Sponsor** | TBD | -| **Updated** | 2020-06-02 | +| **Sponsor** | @alextp | +| **Updated** | 2020-07-15 | | **Sponsorship Deadline** | 2020-07-17 (45 Days after submission) | ## Rationale for Migration