-
Notifications
You must be signed in to change notification settings - Fork 863
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
Enable model custom dependency installation using virtual environment #2910
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.
Some minor comments on the python command you're constructing, I did not review the new path manipulation utils
List<String> commandParts = new ArrayList<>(); | ||
commandParts.add(EnvironmentUtils.getPythonRunTime(model)); | ||
commandParts.add("-m"); | ||
commandParts.add("venv"); |
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.
should you make this name customizable? Part of the appeal of this PR is different workers should be able to have different virtual environments
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.
Currently this PR creates a virtual environment on a per model basis at model load time. All workers for a given model use the same virtual environment. This replaces installing dependencies on a per model basis in a target directory and is backwards compatible with the existing behavior with no change to customer experience. Although the same name venv
is used, they are located within the individual model directories, for ex: /tmp/models/test-model/venv
.
Would it be useful to extend this implementation to support separate venv
per worker?
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.
No the isolation is fine as is I think
commandParts.add(EnvironmentUtils.getPythonRunTime(model)); | ||
commandParts.add("-m"); | ||
commandParts.add("venv"); | ||
commandParts.add("--clear"); |
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.
why clear? Seems beneficial to allow users to install their dependencies beforehand
It was always quite weird how we pip installed a bunch of stuff on launching a model
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.
Sounds good, will check if venvs
can safely be made portable, since the symlinks in the bin
directory of the venv, for ex: venv/bin/python
may need to be updated for them to work, since the python binary path may not be the same on the host on which the venv is created and the host on which the venv is used.
From the official docs: https://docs.python.org/3/library/venv.html
Warning: Because scripts installed in environments should not expect the environment to be activated, their shebang lines contain the absolute paths to their environment’s interpreters. Because of this, environments are inherently non-portable, in the general case. You should always have a simple means of recreating an environment (for example, if you have a requirements file requirements.txt, you can invoke pip install -r requirements.txt using the environment’s pip to install all of the packages needed by the environment).
commandParts.add("-m"); | ||
commandParts.add("venv"); | ||
commandParts.add("--clear"); | ||
commandParts.add("--system-site-packages"); |
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.
why system site?
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.
This is to make sure that the venv
can see packages that are already installed in the base python environment and not have to install them again. If a newer version of an existing package is required or a non existing package is required, they will get installed to the venv site-packages
and will take precedence over system-site-packages
. This does not affect the base python environment. I've added more details with an example in the PR description: #2910 (comment)
commandParts.add("-t"); | ||
commandParts.add(dependencyPath.getAbsolutePath()); | ||
commandParts.add("--upgrade-strategy"); | ||
commandParts.add("only-if-needed"); |
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 think this is a good change but do you mind just telling me why you needed to make it?
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 prior versions of pip (i.e pip<10.0) when pip install -U -r requirements.txt
is used, all the packages listed in requirements.txt
and their dependencies are upgraded since by default the --upgrade-strategy
was eager
. --upgrade-strategy
applies to the handling of dependencies of the packages specified in requirements.txt
. In pip>=10.0
, the default --upgrade-stragegy
is only-if-needed
. This change is to explicitly make the --upgrade-strategy
as only-if-needed
irrespective of pip version.
From the pip docs: https://pip.pypa.io/en/stable/user_guide/#only-if-needed-recursive-upgrade
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.
it is expensive to create python venv for each model and potentially increases the model loading latency. Do you know what the root cause that the existing pip install dependency installs entire pytorch again? Can we have a lightweight solution?
@namannandan I like the overall idea. A couple of questions
|
Agreed, creation of virtual environment adds latency to the model load. Root cause of existing pip install dependency installs entire pytorch again is as follows: This command will install all the packages listed in For ex: if we have say, This is expected behavior of pip when using the Can we have a lightweight solution? Pros:
Cons:
@lxning, @msaroufim, @agunapal what are your thoughts on the above approach? |
|
Latency and storage analysis
Creation of empty virtual environment:
Using the Installing custom dependencies to target directory (Torchserve v0.9.0)
Installing custom dependencies using a virtual environment (using the implementation in this PR)
Summary
|
@namannandan thanks for the analysis, let's add this python venv option in model level config (ie. model-config.yaml). |
Hi @namannandan Can you please update the PR description with how the user experience is going to be with/without venv and show any relevant logs which indicates that its working as expected and there is no BC issue |
frontend/server/src/main/java/org/pytorch/serve/util/messages/EnvironmentUtils.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/util/messages/EnvironmentUtils.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/wlm/ModelManager.java
Outdated
Show resolved
Hide resolved
92908fd
to
750f2e0
Compare
Updated implementation to support |
Included a user experience section in the summary with logs: #2910 (comment) Also included details about |
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
Review comments have been addressed.
Description
The current implementation to handle installation of custom dependencies using a
requirements.txt
file installs packages to a target directory as follows:serve/frontend/server/src/main/java/org/pytorch/serve/wlm/ModelManager.java
Lines 226 to 234 in a07b7d9
And includes the target directory in the
PYTHONPATH
environment variable:serve/frontend/server/src/main/java/org/pytorch/serve/util/messages/EnvironmentUtils.java
Lines 52 to 58 in a07b7d9
The outcome of this approach is:
requirements.txt
along with a copy of all their dependencies are installed to the target directory irrespective of the packages and their dependencies already available in the base python environment(site-packages).requirements.txt
is installed in the target directory irrespective of a supported version already being present in the base python environment(site-packages).The above approach can be improved for the following reasons:
torch
binary that supports the combination of cuda version + GPU driver version on the target platform, it is desirable to use the pip package already available in the base python environment rather than upgrade to the most up to date version which may break compatibility with existing packages.For example:
In the pytorch inference deep learning container
763104351884.dkr.ecr.us-west-2.amazonaws.com/pytorch-inference:2.1.0-gpu-py310
from https://github.com/aws/deep-learning-containers/blob/master/available_images.mdAlthough
torch-2.1.0+cu118
is already available in the base python environment and supportssegment-anything-py==1.0
,torch-2.1.2+cu121
is installed, which may break compatibility with the GPU driver on the host. This is expected behavior since when installing pip packages to a target directory, the requested packages and all their latest dependencies will get installed. Further, since the target directory is added toPYTHONPATH
,torch-2.1.2+cu121
maskstorch-2.1.0+cu118
.On the other hand, when using a virtual environment with access to system site packages to install custom dependencies, we see the following:
We can see that the
torch
binary installed in the base python environment already satisfies the dependency ofsegment-anything-py==1.0
ontorch
and is not reinstalled:User Experience
requirements.txt
should not be affected and dependencies will be installed in the specific target model directory(same as the existing behavior). Note: the logging has been improved to show what packages were downloaded and installed, this is shown below.useVenv: true
inmodel-config.yaml
. Logs for virtual environment creation and dependency installation is shown below.With
useVenv
disabled inmodel-config.yaml
:All custom packages and their dependencies are installed to the target directory.
With
useVenv
enabled inmodel-config.yaml
:Packages already available in the base python environment are not downloaded and reinstalled.
Type of change
Feature/Issue validation/testing
serve/frontend/server/src/test/java/org/pytorch/serve/ModelServerTest.java
Line 1143 in 4b69459
serve/frontend/server/src/test/java/org/pytorch/serve/ModelServerTest.java
Line 1153 in 4b69459
serve/frontend/server/src/test/java/org/pytorch/serve/WorkflowTest.java
Line 400 in 4b69459
serve/frontend/server/src/test/java/org/pytorch/serve/WorkflowTest.java
Line 413 in 4b69459