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.

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

Updated GPU wrapper to uninstall TF without GPU #1011

Merged
merged 3 commits into from
Nov 6, 2019

Conversation

JoelH96
Copy link
Contributor

@JoelH96 JoelH96 commented Oct 29, 2019

Updated the Python TF-GPU wrapper to uninstall Tensorflow (without GPU) after the Seldon-core install and only install Tensorflow-GPU. An install of Numpy=1.16.1 was also added to clear Warnings due to version 1.17 of Numpy with TF-GPU.

@axsaucedo
Copy link
Contributor

Looks solid - tensorflow is actually not a required install from v0.5.0 so if we wait for that release we could make sure the pip uninstall is not required, and given it's an image file we'd be able to push a snapshot for 1.0. Did you test it in a GPU instance with the loaded models?

@axsaucedo
Copy link
Contributor

@adeepsri this fixes #914

@axsaucedo
Copy link
Contributor

@JoelH96 it would be good to address #868 on this PR as well given it's quite relevant

@JoelH96
Copy link
Contributor Author

JoelH96 commented Oct 31, 2019

Ok, maybe if we wait then until the v0.5.0 release, then I will change the wrapper to ensure the seldon-core pip install is with that version, to remove the pip uninstall.

For now though the newly built seldonio/seldon-core-s2i-python3-tf-gpu does not contain the CPU version of TF, and yes I tested it using the deep mnist models in a GPU instance.

@axsaucedo
Copy link
Contributor

Now that 0.5 is out we can add this one 👍 tf doesn't come installed by default so you can just install it on top

@seldondev
Copy link
Collaborator

@JoelH96: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
lint 57e105b link /test lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@JoelH96
Copy link
Contributor Author

JoelH96 commented Nov 6, 2019

@axsaucedo - Updated the wrapper to not include the uninstall of TF. The new image also pushed to the Dockerhub repo

@axsaucedo
Copy link
Contributor

Nice one! Lint seems to be failing just on the version.py so /lgtm
/

@axsaucedo
Copy link
Contributor

/lgtm
/approve

@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: axsaucedo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants