-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
191d6a4
unpin tf-nightly
yatbear 09e2654
Update Keras test, Model() no longer supports deeply nested input/out…
yatbear 37ad910
`maybe_layers` will not be a nested list
yatbear 752a566
Revert "`maybe_layers` will not be a nested list"
yatbear 5e56b9b
Revert "Update Keras test, Model() no longer supports deeply nested i…
yatbear 9e4fd17
stay on Keras 2 for the time being
yatbear 9fd869a
oops, added the code to the wrong file, fixing
yatbear 01541f8
install tf-keras
yatbear 50781d6
rm white space
yatbear 32f879d
update relevant test code to use Keras 2
yatbear 3730c28
rm accidentally added newline
yatbear df4ae08
use tf-keras-nightly
yatbear c33e46a
fix
yatbear 3def2eb
use keras 2
yatbear 76f3af3
add tf-keras as an requirement for Hparams plugin
yatbear 8c35eb3
keep the alphabetical order
yatbear 6604db4
use tf-keras-nightly when using tf-nightly
yatbear 62a5cf8
match and replace the whole line
yatbear File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Thanks!
There was a problem hiding this comment.
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
andtf-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 fortf-keras
as well.There was a problem hiding this comment.
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 withtf-nightly
?There was a problem hiding this comment.
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 usingtf-nightly
: b/306638603#comment15 :(There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😞
There was a problem hiding this comment.
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 😉