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

[CLIPPER-154] Make a pip package #165

Merged
merged 7 commits into from
May 22, 2017

Conversation

nishadsingh1
Copy link

@nishadsingh1 nishadsingh1 commented May 19, 2017

To test out the functionality of this PR:

Create accounts on TestPyPI and PyPI:
https://testpypi.python.org/pypi?%3Aaction=login_form
https://pypi.python.org/pypi?%3Aaction=login_form

Create a file ~/.pypirc with the following contents:

[distutils]
index-servers =
  pypi
  testpypi

[pypi]
repository=https://pypi.python.org/pypi
username=your_username
password=your_password

[pypitest]
repository=https://testpypi.python.org/pypi
username=your_username
password=your_password

Because this file contains your credentials in plaintext, change its read/write permissions:
chmod 600 ~/.pypirc

Upload our pip package to PyPI Testing server:
python setup.py register -r testpypi
python setup.py sdist upload -r testpypi

Install from PyPi Testing server:
pip install -i https://testpypi.python.org/pypi clipper_admin

Give it a go:

import clipper_admin.clipper_manager as cm
clipper = cm.Clipper("localhost", "", "")
clipper.start()

Upload our pip package to real PyPi server:
python setup.py register -r pypi
python setup.py sdist upload -r pypi

Install from real PyPi server:
pip uninstall clipper_admin (to get rid of the one from the testing server)
pip install clipper_admin

https://clipper.atlassian.net/projects/CLIPPER/issues/CLIPPER-154

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/298/
Test FAILed.

@nishadsingh1 nishadsingh1 force-pushed the pip_package branch 2 times, most recently from 4598512 to 7a6f5e4 Compare May 19, 2017 21:09
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/299/
Test FAILed.

setup.py Outdated
url = 'https://github.com/ucbrise/clipper',
packages = ['clipper_admin'],
keywords = ['prediction', 'model', 'management'],
install_requires = ['requests', 'pyparsing', 'appdirs', 'pprint', 'subprocess32', 'sklearn', 'numpy', 'scipy', 'fabric', 'conda', 'pyyaml']
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My code formatting script is hanging for some reason -- figured it was best to push this change up early and figure out the formatting issue while it's getting reviewed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I can format it in a sec.

@dcrankshaw
Copy link
Contributor

Okay just formatted the code

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/301/
Test FAILed.

@dcrankshaw
Copy link
Contributor

jenkins test this please

1 similar comment
@dcrankshaw
Copy link
Contributor

jenkins test this please

Copy link
Contributor

@dcrankshaw dcrankshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I need to do some testing, but here's a first round of comments.

@@ -3,7 +3,7 @@ FROM clipper/py-rpc:latest
MAINTAINER Dan Crankshaw <dscrankshaw@gmail.com>

COPY python_container.py python_container_entry.sh /container/
COPY pywrencloudpickle.py python_container_conda_deps.txt /lib/
COPY ../../clipper_admin/pywrencloudpickle.py python_container_conda_deps.txt /lib/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work. All the paths copied into the container need to be in the directory tree whose root is the directory containing the Dockerfile (https://docs.docker.com/engine/reference/commandline/build/#build-with-path). This is also important to make sure that the automated builds on Docker hub work. Probably the easiest thing to do is to move all the container dockerfiles to the top level of the directory. You'll need to modify the paths in the Dockerfiles, as well as the script that builds all the files.

@@ -148,7 +148,7 @@
"# clipper_manager must be on your path:\n",
"import sys\n",
"import os\n",
"sys.path.append(os.path.abspath('../../management/'))\n",
"sys.path.append(os.path.abspath('../../clipper_admin/'))\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should import the module directly like this or make them install the pip package?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem cleaner to make them install the pip package, especially since the intended tutorial user is a Clipper user, not developer.

@@ -23,7 +23,7 @@
"# clipper_manager must be on your path:\n",
"import sys\n",
"import os\n",
"sys.path.append(os.path.abspath('../../management/'))\n",
"sys.path.append(os.path.abspath('../../clipper_admin/'))\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about direct import vs make them install the pip package

setup.py Outdated
setup(
name='clipper_admin',
version='0.1',
description='A low latency online prediction serving system',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the description "Admin commands for the Clipper prediction-serving system."

setup.py Outdated
version='0.1',
description='A low latency online prediction serving system',
author='Dan Crankshaw',
author_email='dcrankshaw@berkeley.edu',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use crankshaw@cs.berkeley.edu for my email

setup.py Outdated
description='A low latency online prediction serving system',
author='Dan Crankshaw',
author_email='dcrankshaw@berkeley.edu',
url='https://github.com/ucbrise/clipper',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the url http://clipper.ai

setup.py Outdated
author_email='dcrankshaw@berkeley.edu',
url='https://github.com/ucbrise/clipper',
packages=['clipper_admin'],
keywords=['prediction', 'model', 'management'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add clipper as a keyword

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/310/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/317/
Test FAILed.

@nishadsingh1 nishadsingh1 force-pushed the pip_package branch 2 times, most recently from 77ca261 to f55d05c Compare May 20, 2017 06:12
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/318/
Test FAILed.

Copy link
Contributor

@dcrankshaw dcrankshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update examples/tutorial/requirements.txt to reflect the new set of Python dependencies that include the clipper-admin package?

time docker build -t clipper/noop-container -f ../../NoopDockerfile ../../
time docker build -t clipper/python-container -f ../../PythonContainerDockerfile ../../
time docker build -t clipper/sklearn_cifar_container -f ../../SklearnCifarDockerfile ../../
time docker build -t clipper/tf_cifar_container -f ../../TensorFlowCifarDockerfile ../../
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consolidate this script into bin/build_docker_images.sh

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/319/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/320/
Test PASSed.

@dcrankshaw
Copy link
Contributor

Tested with pip install --extra-index-url https://testpypi.python.org/pypi clipper_admin and it works great. LGTM. Will merge once tests pass. This will break our container builds on docker hub so I'll need to update those as well.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/326/
Test PASSed.

@dcrankshaw dcrankshaw merged commit bb49084 into ucbrise:develop May 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants