-
Notifications
You must be signed in to change notification settings - Fork 610
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
Added extra requirements for tensorflow, tensorflow-gpu, and tensorflow-cpu #2114
Conversation
Do you need to add something in the README? |
Thanks for taking a look @bhack, I've added some additional docs to the README explaining the usage. |
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.
Thanks for the PR! Wanted to get a definitive answer on the question posed in the issue since it'll have a different experience for the user
README.md
Outdated
To ensure you install a compatible version of `tensorflow_addons` for your existing version of | ||
TensorFlow, you can install with the `tensorflow` extra requirement: | ||
|
||
``` | ||
pip install tensorflow-addons[tensorflow] | ||
``` |
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.
Pending the answer to #2111 (comment) I think we could even move this up to the primary "Install TFA" block
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.
As discussed in that issue, reworded this section to clarify that the effect will be to ensure a compatible version of TensorFlow is installed, not the reverse (a compatible TFA for your TF install). Also moved to the Installation
section as suggested. Let me know what you think!
README.md
Outdated
pip install tensorflow-addons[tensorflow] | ||
``` | ||
|
||
Similar extras exist for `tensorflow-gpu` and `tensorflow-cpu`. |
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.
Would be happier if tensorflow-gpu
was done away with but I think while it exists this is good to use for dependencies
# Required TensorFlow version [min, max) | ||
MIN_TF_VERSION = "2.2.0" | ||
MAX_TF_VERSION = "2.4.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.
Very happy with the single location
When to merge this? |
@seanpmorgan ready for review whenever you have some time. |
Thanks for the review, @seanpmorgan! Looks like all tests are passing now. |
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.
LGTM Thank you very much!
…ow-cpu (tensorflow#2114) * Added extra requirements for tensorflow, tensorflow-gpu, and tensorflow-cpu
Description
Brief Description of the PR:
This restores the functionality removed by #974 but makes it optional, so users can get the behavior when installing:
Fixes #2111
Type of change
Install script enhancement.
Checklist:
How Has This Been Tested?