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

[python] Create model level virtualenv #811

Merged
merged 10 commits into from
Jun 28, 2023
Merged

[python] Create model level virtualenv #811

merged 10 commits into from
Jun 28, 2023

Conversation

xyang16
Copy link
Contributor

@xyang16 xyang16 commented Jun 7, 2023 •
edited
Loading

Description

Brief description of what this PR is about

Create model-specific virtualenv. I discussed with Frank about two options:

  1. Option 1: Create a virtual environment on the fly for each model.
  2. Option 2: Pre-create a pool of n virtual environments and allocate one to each model.

In my experiment, the time creating the venv on the fly is not noticeable. So I had this PR for option 1.

@xyang16 xyang16 changed the title [python] Create model-specific virtualenv [python] Create model level virtualenv Jun 7, 2023
@xyang16 xyang16 marked this pull request as ready for review June 7, 2023 23:46
@xyang16 xyang16 requested review from zachgk and a team as code owners June 7, 2023 23:46
@xyang16 xyang16 force-pushed the venv branch 5 times, most recently from 5fd8a01 to 553a3df Compare June 27, 2023 05:09
frankfliu
frankfliu reviewed Jun 27, 2023
View reviewed changes
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated Show resolved Hide resolved
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated Show resolved Hide resolved
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated Show resolved Hide resolved
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated Show resolved Hide resolved
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated Show resolved Hide resolved
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated Show resolved Hide resolved
engines/python/src/main/java/ai/djl/python/engine/PyProcess.java Outdated
@@ -122,6 +122,9 @@ synchronized void startPythonProcess() {
int id = restartCount.get();
int port = connections.get(0).getPort();
logger.info("Start process: {} - retry: {}", port, id);
if (pyEnv.isEnableVenv()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

create/delete venv should happen at model loading and close time, not at StartPyProcess time

one model may have multiple workers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

xyang16 and others added 6 commits June 26, 2023 22:37
@xyang16 @frankfliu
Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java
03ca322 
Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
@xyang16 @frankfliu
Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java
ac88354 
Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
@xyang16 @frankfliu
Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java
d267283 
Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
@xyang16 @frankfliu
Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java
7cf0e44 
Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
@xyang16 @frankfliu
Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java
857235d 
Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
@xyang16
Review changes
b9e64fd
frankfliu
frankfliu reviewed Jun 27, 2023
View reviewed changes
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated
/**
* Constructs a new {@code PyEnv} instance.
*
* @param mpiMode true to use MPI launcher
*/
public PyEnv(boolean mpiMode) {
this.mpiMode = mpiMode;
pythonExecutable = Utils.getenv("PYTHON_EXECUTABLE");
Copy link
Contributor

Choose a reason for hiding this comment

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

With your implementation, no need to lazy get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Show resolved Hide resolved
engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated
logger.warn("{}", logOutput);
}
} catch (IOException | InterruptedException e) {
logger.warn("Python virtual failed.", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should fail the model loading

engines/python/src/main/java/ai/djl/python/engine/PyEnv.java Outdated
setPythonExecutable(path.resolve("bin").resolve("python").toString());
venvCreated = true;
} else {
logger.warn("Failed to create virtual environment with error code: {}", ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should fail here

Copy link
Contributor

Choose a reason for hiding this comment

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

We need add a unit test

Copy link
Contributor Author

@xyang16 xyang16 Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

throwed EngineException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test

xyang16 and others added 2 commits June 26, 2023 23:47
@xyang16 xyang16 force-pushed the venv branch 3 times, most recently from 21cee2f to 84072e1 Compare June 27, 2023 23:58
@xyang16 xyang16 merged commit 7776ab5 into deepjavalibrary:master Jun 28, 2023
@xyang16 xyang16 deleted the venv branch August 2, 2023 06:26
KexinFeng pushed a commit to KexinFeng/djl-serving-forked that referenced this pull request Aug 16, 2023
@xyang16 @frankfliu
[python] Create model level virtualenv (deepjavalibrary#811)
b22d0ef 
* [python] Create model level virtualenv

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Review changes

* Review changes

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Review changes

---------

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
KexinFeng pushed a commit to KexinFeng/djl-serving-forked that referenced this pull request Aug 16, 2023
@xyang16 @frankfliu @KexinFeng
[python] Create model level virtualenv (deepjavalibrary#811)
99c522f 
* [python] Create model level virtualenv

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Review changes

* Review changes

* Update engines/python/src/main/java/ai/djl/python/engine/PyEnv.java

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>

* Review changes

---------

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maaquib maaquib maaquib left review comments

@frankfliu frankfliu frankfliu approved these changes

@lanking520 lanking520 lanking520 approved these changes

@zachgk zachgk Awaiting requested review from zachgk zachgk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xyang16 @maaquib @lanking520 @frankfliu