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

CI: Unpin tf-nightly and stay on Keras 2 #6684

Merged
merged 18 commits into from
Nov 16, 2023
Merged

Conversation

yatbear
Copy link
Member

@yatbear yatbear commented Nov 14, 2023

tf-nightly was pinned at #6655 due to Keras 3 release.

Keras 2 <> Keras incompatibilities (keras-team/keras#18467) are impacting the Graph plugin and Hparams plugin, since the two plugins are not being actively developed and updating the code to be Keras 3 compatible is non-trivial, enforce using Keras 2 for the time being.

Note that tf-nightly is not compatible with tf-keras and needs to be used with tf-keras-nightly instead.

Googlers, see b/309503942 for more context.

Tested internally: cl/582334596

#oncall

@yatbear yatbear changed the title CI: Unpin tf-nightly CI: Unpin tf-nightly and stay on Keras 2 Nov 15, 2023
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

Thanks for getting this working again!

@@ -30,7 +30,8 @@ env:
BUILDTOOLS_VERSION: '3.0.0'
BUILDIFIER_SHA256SUM: 'e92a6793c7134c5431c58fbc34700664f101e5c9b1c1fcd93b97978e8b7f88db'
BUILDOZER_SHA256SUM: '3d58a0b6972e4535718cdd6c12778170ea7382de7c75bc3728f5719437ffb84d'
TENSORFLOW_VERSION: 'tf-nightly==2.16.0.dev20231018'
TENSORFLOW_VERSION: 'tf-nightly'
TF_KERAS_VERSION: 'tf-keras-nightly' # Keras 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also add the dependency on tf-keras to the requirements.txt file, since there is one case where it's used beyond tests (in hparams/_keras.py).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually now that it's in the requirements.txt, I think we might need some adjustments here to the CI to avoid having both tf-keras and tf-keras-nightly installed since I would assume they probably conflict.

Perhaps the simplest would just be to skip depending on tf-keras-nightly at all (i.e. no special handling for it here). We install nightly TF in our CI, but I think that's mostly a special case given our close coordination with TF in the past and the need to anticipate breakages before release deadlines (plus it's easier, since TF isn't in our pip requirements). We don't depend on pre-releases for any other pip dependencies, so we could probably skip it for tf-keras as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah hmmm. But maybe we can't do the simple version, if tf-keras itself breaks with tf-nightly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I think we have to use tf-keras-nightly when using tf-nightly: b/306638603#comment15 :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, ok. Maybe we can do a hacky find/replace on requirements.txt before we pip install from it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks for the suggestion! Hope it doesn't look too hacky 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha it looks great - sometimes a hack is the best option 😉

@yatbear yatbear requested a review from nfelt November 16, 2023 19:21
@yatbear yatbear merged commit cf175ea into tensorflow:master Nov 16, 2023
13 checks passed
@yatbear yatbear deleted the nightly branch November 16, 2023 20:08
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.

2 participants