-
Notifications
You must be signed in to change notification settings - Fork 641
Add support for TensorFlow 2.17 #974
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
base: master
Are you sure you want to change the base?
Add support for TensorFlow 2.17 #974
Conversation
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.
I had one question about Keras in the requirements, but otherwise, LGTM.
I just realized that this uses a slightly different way of generating requirements.txt. I will see if I can get MBB to review #945 today; in the meantime, can you try to generate requirements.txt using the script generate_requirements.sh in that PR? I think it will be merged soon, and so if this PR for 2.17 is merged afterwards, it will update the requirements using the same procedure, and nothing more will need to be done. (Conversely, if this is merged before the other PR, then we might have to regenerate the requirements using that script and do another PR just to update requirements.txt a 3rd time.)
| tensorflow>=2.16,<2.17 | ||
| tf-keras~=2.16.0 | ||
| tensorflow>=2.17,<2.18 | ||
| keras==3.12.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.
Out of curiosity, why does Keras need to be included here? Doesn't TensorFlow bring it in itself? Or is the problem getting a specific version of Keras?
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.
In the CI check using python 3.10 it has issues getting non specific version of Keras, it tried to get keras>3.13 and caused the issues in the CI checks.
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.
In the CI check using python 3.10 it has issues getting non specific version of Keras, it tried to get keras>3.13 and caused the issues in the CI checks.
Oh, okay. Makes sense to pin it, then.
Thanks.
| http_archive( | ||
| name = "eigen", | ||
| sha256 = "35ba771e30c735a4215ed784d7e032086cf89fe6622dce4d793c45dd74373362", | ||
| sha256 = "0992b93a590c39e196a9efdb5b4919fbf3fb485e7e656c6a87b21ddadb7f6ad2", |
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.
What is the version of Eigen this corresponds to? (It'd be worth adding a comment about that. I find myself having to know the version number so that I can match it in qsim.)
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.
Eigen 3.4.90 (development snapshot after 3.4.0)
pinned at commit c1d637433e3b3f9012b226c2c9125c494b470ae6
|
One last tiny request: could you make the title slightly more descriptive? E.g., spell out TensorFlow and say it's about supporting version 2.17. I don't mean to be picky – it's just to help make the commit history be clearer. |
Summary
requirements.inand usepip-compileto producerequirements.txtKey changes
@org_tensorflowto 2.17.1requirements.in:requirements.txtis produced by runningpip-compile --upgrade --generate-hashes -o requirements.txt requirements.inTesting
bazel build ... release:build_pip_packagesucceedsscripts/test_all.sh&./scripts/ci_validate_tutorials.sh) passNotes