Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate the CI warnings caused from the migration of gelu activation in TensorFlow #8063

Closed
wants to merge 2 commits into from

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Feb 26, 2021

Description:
This PR is part of the effort to close this Issue#7738 hence to reduce the amount of warnings that are raised when running all tests in the CI. From the first investigation, it seems that this warning (DeprecationWarning: gelu activation has been migrated to core TensorFlow) for which the current PR was opened, has the most occurrences (it appears in total in 2142 tests as seen in the log file below).

Potential related issues/PRs to this migration:

  • In this Issue#550 they describe the intention of migrating gelu activation to core TensorFlow.
  • one of the PRs of the implementation.
  • Deprecation of gelu Issue#2005.
  • Maybe this from TensorFlow docs is useful.

Warnings as they appear in the CI log file (3_Run Tests (ubuntu-latest, 3.8).txt):

2021-02-22T12:29:04.2560900Z .venv/lib/python3.8/site-packages/tensorflow_addons/activations/gelu.py:70: 1775 tests with warnings
2021-02-22T12:29:04.2562970Z   /home/runner/work/rasa/rasa/.venv/lib/python3.8/site-packages/tensorflow_addons/activations/gelu.py:70: DeprecationWarning: gelu activation has been migrated to core TensorFlow, and will be deprecated in Addons 0.13.
2021-02-22T12:29:04.2564350Z     warnings.warn(
2021-02-22T12:29:04.2564680Z 
2021-02-22T12:29:04.2623302Z .venv/lib/python3.8/site-packages/tensorflow/python/autograph/impl/api.py:493: 367 tests with warnings
2021-02-22T12:29:04.2625352Z   /home/runner/work/rasa/rasa/.venv/lib/python3.8/site-packages/tensorflow/python/autograph/impl/api.py:493: DeprecationWarning: gelu activation has been migrated to core TensorFlow, and will be deprecated in Addons 0.13.

Proposed changes:

  • Update TensorFlow core and Addons 0.13 maybe ?
  • The code changes in this PR are not solving the issue but are indicative of the places in the code where gelu activation is called. So if an update is done, these are the calls (at least these ones) that will need to be updated.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@Imod7 Imod7 marked this pull request as draft February 26, 2021 14:49
@Imod7 Imod7 added area:rasa-oss 🎡 Anything related to the open source Rasa framework priority:high type:maintenance 🔧 Improvements to tooling, testing, deployments, infrastructure, code style. labels Feb 26, 2021
@Imod7
Copy link
Contributor Author

Imod7 commented Mar 1, 2021

@twerkmeister ?

@twerkmeister
Copy link
Contributor

It seems that the gelu function was moved from the addons to the core of tensorflow. The warnings come because we are still using it from the addons package, and they warn us that it will be gone soon. So potentially we just need to stop using the gelu function from the addon package and instead use the gelu function that you mentioned tf.keras.activations.gelu or tf.nn.gelu in its place.

Is this helpful and gives you new ideas? @Imod7

@Imod7
Copy link
Contributor Author

Imod7 commented Mar 3, 2021

I tried to call the gelu activation function with both statements and I get the same type of errors :

  • When calling gelu with tf.keras.activations.gelu, the error message is :
    AttributeError: module 'tensorflow.keras.activations' has no attribute 'gelu'
  • When calling with tf.nn.gelu, the error message is :
    AttributeError: module 'tensorflow._api.v2.nn' has no attribute 'gelu'

For keras I do not know why it cannot find the gelu function since I can see the definition here.

@twerkmeister
Copy link
Contributor

Try to figure out if it is already there in the version of tensorflow we are using at the moment. Is it somewhere else in our version? If not, what's the minimum version that we would need to upgrade to? Who is most concerned with the tf version? Do they already have plans to upgrade? Do they see issues with upgrading?

@Imod7
Copy link
Contributor Author

Imod7 commented Mar 3, 2021

@twerkmeister very good feedback and very good "important-things-to-have-in-mind" type of questions! 💯 Thank you!
@tczekajlo When updating some tensorflow calls related to gelu (due to updates in tf2) I get these type of errors which look strange since I can see this function in the tensorflow GitHub code. Is there a way to see the versions of tensorflow or keras that we use in the CI ? or even check the code somewhere so I can double check that the gelu is actually present in tf.keras.activations.gelu or tf.nn.gelu ? Thank you so much in advance for your feedback!

@tczekajlo
Copy link

@twerkmeister very good feedback and very good "important-things-to-have-in-mind" type of questions! 💯 Thank you!
@tczekajlo When updating some tensorflow calls related to gelu (due to updates in tf2) I get these type of errors which look strange since I can see this function in the tensorflow GitHub code. Is there a way to see the versions of tensorflow or keras that we use in the CI ? or even check the code somewhere so I can double check that the gelu is actually present in tf.keras.activations.gelu or tf.nn.gelu ? Thank you so much in advance for your feedback!

You can find it here https://github.com/RasaHQ/rasa/blob/main/poetry.lock

@Imod7
Copy link
Contributor Author

Imod7 commented Mar 4, 2021

@twerkmeister @wochinge You were both right! 💯
From the poetry lock file mentioned by Tomasz, the version of Tensorflow that we are using at the moment is 2.3.2 and :

  • tf.keras.activations.gelu needs 2.4.1 based on this page of the docs.
  • tf.nn.gelu needs also 2.4.1 based on this page of the docs.

So, that is why the gelu calls that I tried are not working.

"If not, what's the minimum version that we would need to upgrade to?"
Based on the corresponding pages on the docs, the minimum required update for tensorflow would be from 2.3.2 to 2.4.1.

"Who is most concerned with the tf version? Do they already have plans to upgrade? Do they see issues with upgrading?"
I am not aware of that so I think @wochinge can answer that.

@wochinge
Copy link
Contributor

wochinge commented Mar 5, 2021

Do you have concerns updating TensorFlow @Ghostvv @dakshvar22 @koernerfelicia ?

From what I gather we'd also need to update the following for these:

  • tensorflow-estimator (compatible release available)
  • tensorflow-probability (compatible release available)
  • tensorflow-addons (compatible release available)
  • tensorflow-text (compatible release available)

@koernerfelicia
Copy link
Contributor

Unfortunately afaik our update is blocked by a bug in tensorflow. TF said they plan to fix this for the 2.5 release

@wochinge
Copy link
Contributor

wochinge commented Mar 9, 2021

@koernerfelicia Do you know if the 2.4 branch also fixes above warnings?

@koernerfelicia
Copy link
Contributor

Not sure, @Ghostvv might know? If not, I can try and checkout the 2.4 branch and see what happens. Also, to be clear and avoid misunderstandings, TF doesn't expect to fix the bug I mentioned above for 2.4, so our update will not happen until 2.5

@Ghostvv
Copy link
Contributor

Ghostvv commented Mar 9, 2021

I don't know about the warnings

@wochinge
Copy link
Contributor

wochinge commented Mar 9, 2021

@twerkmeister @Imod7 I'd suggest to apply your fix to research's tf-2.4 branch then, what do you think? This way it'll automatically be done once they merge this to main.

@Imod7
Copy link
Contributor Author

Imod7 commented Mar 10, 2021

Thanx @wochinge Ok then I will change the source branch from main to tf-2.4 in the current PR.

I guess this standard warning message (while trying to change source branch)
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
does not involve any negative side effects right? Just double checking!

@wochinge
Copy link
Contributor

wochinge commented Mar 10, 2021

How are you changing the source branch? You got two options:

  • rebasing (which means force pushing and sometimes gets a bit tricky)
  • creating a new PR of tf-2.4 and cherry pick your commit from this branch (requires a new PR)

@Imod7
Copy link
Contributor Author

Imod7 commented Mar 10, 2021

Ok, then I can go with the second option! However, if you Edit the title of this PR it gives you also the option to choose another base branch.

@wochinge
Copy link
Contributor

However, if you Edit the title of this PR it gives you also the option to choose another base branch.

This doesn't change your branch and the commits on it at all. It just changes the target of the pull request.

Example:
Consider main has a commit new_feature . You branch of main and your feature on top of it with commit dominique so that the branch now has commits [new_feature, dominique]. There is also bugfix branch last-minor for the last minor which currently has no commits.

If you now do a PR against main this would merge the new commit dominique into main which results in main having commits [new_feature, dominique].
If you change the base of the PR to last-minor and would merge the PR, the branch last-minor would suddenly have commits [new_feature, dominique] which means that you brought changes from main into the bugfix branch of the last minor 💥

@Imod7
Copy link
Contributor Author

Imod7 commented Mar 15, 2021

@wochinge Closing this PR since in branch tf-2.4 all gelu calls have been replaced with the new call tf.nn.gelu. I also updated the doc.

@Imod7 Imod7 closed this Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:maintenance 🔧 Improvements to tooling, testing, deployments, infrastructure, code style.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants