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.

Sign up for GitHub

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

Jump to bottom

Added extra requirements for tensorflow, tensorflow-gpu, and tensorflow-cpu #2114

Merged
merged 4 commits into from
Sep 2, 2020
Merged

Added extra requirements for tensorflow, tensorflow-gpu, and tensorflow-cpu #2114

merged 4 commits into from
Sep 2, 2020

Conversation

tgaddair
Copy link
Contributor

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:

pip install tensorflow_addons[tensorflow]

Fixes #2111

Type of change

Install script enhancement.

Checklist:

  • I've properly formatted my code according to the guidelines
    • By running Black + Flake8
    • By running pre-commit hooks
  • This PR addresses an already submitted issue for TensorFlow Addons
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • This PR contains modifications to C++ custom-ops

How Has This Been Tested?

pip install '.[tensorflow]'

@bhack
Copy link
Contributor

bhack commented Aug 24, 2020

Do you need to add something in the README?

@tgaddair
Copy link
Contributor Author

Thanks for taking a look @bhack, I've added some additional docs to the README explaining the usage.

seanpmorgan
seanpmorgan reviewed Aug 25, 2020
View reviewed changes
Copy link
Member

@seanpmorgan seanpmorgan left a comment •
edited
Loading

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
Comment on lines 71 to 76
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]
```
Copy link
Member

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

Copy link
Contributor Author

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`.
Copy link
Member

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

tensorflow_addons/version.py
Comment on lines +17 to +20
# Required TensorFlow version [min, max)
MIN_TF_VERSION = "2.2.0"
MAX_TF_VERSION = "2.4.0"

Copy link
Member

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

@zh794390558
Copy link

When to merge this?

@zh794390558
Copy link

#864

@zh794390558 zh794390558 mentioned this pull request Aug 28, 2020
Closed
@tgaddair
Copy link
Contributor Author

@seanpmorgan ready for review whenever you have some time.

seanpmorgan
seanpmorgan requested changes Sep 1, 2020
View reviewed changes
setup.py Outdated Show resolved Hide resolved
@tgaddair
Copy link
Contributor Author

tgaddair commented Sep 1, 2020

Thanks for the review, @seanpmorgan! Looks like all tests are passing now.

seanpmorgan
seanpmorgan approved these changes Sep 2, 2020
View reviewed changes
Copy link
Member

@seanpmorgan seanpmorgan left a 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!

@seanpmorgan seanpmorgan merged commit 61ebf40 into tensorflow:master Sep 2, 2020
jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
@tgaddair
Added extra requirements for tensorflow, tensorflow-gpu, and tensorfl…
5ab03f6 
…ow-cpu (tensorflow#2114)

* Added extra requirements for tensorflow, tensorflow-gpu, and tensorflow-cpu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanpmorgan seanpmorgan seanpmorgan approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

List TensorFlow as an "extra" requirement to install the correct TFA for a given TF version
5 participants
@tgaddair @bhack @zh794390558 @seanpmorgan @googlebot