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

Fix tf-addons for upcoming keras 3 default. #2858

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

cantonios
Copy link
Contributor

Description

Keras 3.0 will become default in TF 2.16 (and is currently default in tf-nightly). This breaks this tf-addons package. Here we make minimal changes to return functionality in a backward-compatible way.

Fixes # (issue)

None known reported.

Type of change

  • Bug fix

Checklist:

  • I've properly formatted my code according to the guidelines
    • [] By running Black + Flake8
    • By running pre-commit hooks
  • This PR addresses an already submitted issue for TensorFlow Addons
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • This PR contains modifications to C++ custom-ops

How Has This Been Tested?

Imported with tf-nightly, keras-nightly, tf_keras-nightly, setting both TF_USE_LEGACY_KERAS=0 and 1.

@bot-of-gabrieldemarmiesse

@howl-anderson @pedrolarben @SSaishruthi @hyang0129

You are owners of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

Keras 3.0 will become default in TF 2.16 (and is currently default in
tf-nightly).  This breaks this tf-addons package.  Here we make
minimal changes to return functionality in a backward-compatible way.
@@ -149,3 +149,6 @@ def _resource_scatter_operate(self, resource, indices, update, resource_scatter_
}

return resource_scatter_op(**resource_update_kwargs)

def get_config(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with keras 3 it throws an error that LazyAdam doesn't have a get_config method.

# ==============================================================================
"""Base class for RNN cells.

Adapted from legacy github.com/keras-team/tf-keras.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason this is needed? Is it because of the usage of tf.keras.layers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AbstractRNNCell only exists in tf_keras (it previously existed in keras 2, but doesn't exist in keras 3).

Copy link
Member

@qlzh727 qlzh727 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted offilne. Most of the duplication/copy here is that they are not available in keras 3.0 if installed.

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for the PR ahead of time

@seanpmorgan seanpmorgan merged commit 3380b3c into tensorflow:master Dec 13, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants