-
Notifications
You must be signed in to change notification settings - Fork 895
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
Fixes necessary to upgrade to TF2.2 / Python 3.8 / Ubuntu 20.04 #249
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@elkhrt @fzvinicius @rfaulkner @finbarrtimbers @jblespiau do you mind taking a look at this when you have a chance? No rush. Even if the Travis-CI tests pass, we should discuss the best time to import it (Travis-CI does not officially support Ubuntu 20.04 yet). |
@@ -1,17 +1,16 @@ | |||
# The single source of truth for the OpenSpiel pip dependencies. | |||
pip >= 20.0.2 | |||
absl-py == 0.9.0 | |||
tensorflow < 2.0, >= 1.15.1 | |||
dm-sonnet == 1.32 | |||
tensorflow >= 1.15.1 |
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.
For this line and the others, why are you changing the versions to be >=? For Tensorflow, I understand. Why not bump it to 2.0?
For the others, I usually prefer having a specific version, except for libraries that have very strong guarantees about backward compatibility, because by having a very large space for possible number versions, it's more likely that at least one set of version fails (because the cross-product of available versions is very large).
I expect the packages to introduce breaking changes between major versions. thus, at the very least we should use attrs >= 19.1.1, < 20.0.0 to prevent potential breakages.
Thus, I would suggest to
(a) either use specific versions. It's simple, everyone understands it, and it's deterministic.
(b) If you want to give more freedom, please make sure to have a single major version. For example:
cvopt ~=1.2 will accept versions after 1.2 and before 2.0.
For simplicity, I would suggest (a) for all most of the libs. For tensorflow we can keep >= as you tested with both (why not jump to 2.0?).
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 just change what you need for this CL? Likely just TF, we can look at whether to upgrade other dependencies/do the upgrade of other dependencies when you are back. I have an internal CL).
|
||
"""Simple network classes for Tensorflow based on tf.Module.""" | ||
|
||
from __future__ import absolute_import |
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.
Remove the future imports.
All the "tf.disable_v2_behavior()" are indeed correct. Internally, we have this done by a dependency in the BUILD file, which is not exported, which is why it was breaking. As far as I have understood, Which is why you created the alternative MLP and other classes? I did not look into Sonnet, but is the same version of sonnet working with both TF1 an TF2? It is still possible to use sonnet instead of having extra classes? Otherwise, everything seems good to me. |
Thanks @jblespiau , most of these have been resolved by email. For everyone else: the choices to loosen the required versions is to help with this transition period, i.e. since TF2.2 needs python >= 3.8 (which at least requires installing separately on Ubuntu 18.04, but also seemed to cause problems for us, on Debian 10), I'd prefer to keep Python 3.6/3.7 + TF1.15 support until it is clearly safe to remove it. I'm running the Travis tests now to see if these changes will still work on Ubuntu 18.04 + MacOS. |
They passed! I'll import it soon, likely some time this week. |
These are some fixes requires to get OpenSpiel compiling and running on Ubuntu 20.04 under TF 2.2 to address #166. Note that TF2.2 requires Python 3.8.
In particular, this PR changes the following:
tf.disable_v2_behavior
in several placessimple_nets.py
, a very simple pure TF replacement for sonnet (MLPs and dense layers)Tested (so far):
kuhn_nfsp.py
to ensure the graph looks like what we expectkuhn_policy_gradient.py
Not done yet: