[runtime env] Async pip runtime env #22381

fyrestone · 2022-02-15T09:49:55Z

Why are these changes needed?

In order to initialize runtime env concurrently, this PR makes pip runtime env asynchronous. It includes,

New check_output_cmd in runtime env utils.
Async PipProcessor.
The asynccontextmanager from https://github.com/python-trio/async_generator for Python 3.6
Remove pip runtime env lock.
Disable pip cache.

Related issue number

#21950

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

SongGuyang

Looks great for me! Only left some little comments.

python/ray/_private/runtime_env/utils.py

SongGuyang · 2022-02-15T12:11:24Z

python/ray/_private/runtime_env/utils.py

+ str_list = [super().__str__().strip(",.")]
+ out = {
+ "stdout": self.stdout,
+ "stderr": self.stderr,


"stderr" will be empty because we redirect it to stdout https://github.com/ray-project/ray/pull/22381/files#diff-3968b5fc7121d304a253988b1bed9fdd58c910dbae0eb33bc0405e2a9a1bfec3R346 ?

This looks very similar to this part, maybe you can modify the implementation in conda_utils.py to avoid splitting the implementation

this part

Yes. But we can't assume that the stderr is always redirected to the stdout. The implementation of SubprocessCalledProcessError should cover the interface of the parent subprocess.CalledProcessError.

This looks very similar to this part, maybe you can modify the implementation in conda_utils.py to avoid splitting the implementation

The exec_cmd and exec_cmd_stream_to_logger in the conda_utils.py are used by many runtime envs, not only by the conda. I thinks it's better to put the exec utils into the utils.py instead of conda_utils.py.

But, we are going to make all the runtime env async, so exec_cmd and exec_cmd_stream_to_logger will be removed after they are not referenced.

python/ray/_private/runtime_env/utils.py

…me_env

python/ray/_private/runtime_env/pip.py

…me_env

fyrestone · 2022-02-18T07:30:26Z

@architkulkarni @edoakes @rkooo567 Hi, can you review this PR?

rkooo567

This is a nice change btw :)! I will leave the further review to @architkulkarni who should have more context in this code path

python/ray/_private/async_compat.py

edoakes · 2022-02-18T15:05:45Z

Deferring to @architkulkarni to review. Let's make sure there's no regression in logging/exception messages due to the change (hopefully our existing tests are enough here)

fyrestone · 2022-02-21T02:43:38Z

python/ray/_private/async_compat.py

+ from contextlib import asynccontextmanager
+except ImportError:
+ # Copy from https://github.com/python-trio/async_generator
+ # for compatible with Python 3.6


@rkooo567 The Python 3.6 does not support asynccontextmanager. This code is copied from async_generator.

…me_env

python/ray/_private/runtime_env/utils.py

python/ray/tests/test_runtime_env.py

architkulkarni

This looks good to me! Just have one question:

Are we sure that it's safe to remove the lock for concurrent pip installs? I think concurrently installing pip environments can cause some unexpected errors, e.g. #17086. Or is it safe now because we added --no-cache-dir?

If we can add a test to cover the case of concurrent installs, I'd feel better about this. Something like installing two runtime envs, pip: ["A", "B"] concurrently with pip: ["A", "C"]. Is it possible to add such a test to this PR?

fyrestone · 2022-02-23T03:19:20Z

This looks good to me! Just have one question:

Are we sure that it's safe to remove the lock for concurrent pip installs? I think concurrently installing pip environments can cause some unexpected errors, e.g. #17086. Or is it safe now because we added --no-cache-dir?

If we can add a test to cover the case of concurrent installs, I'd feel better about this. Something like installing two runtime envs, pip: ["A", "B"] concurrently with pip: ["A", "C"]. Is it possible to add such a test to this PR?

Concurrent installs in different virtualenvs is OK because,

Creating a virtualenv with a clean and dedicated directory for the app data. Related options: --app-data, --reset-app-data.
Disable the pip cache (Set a dedicated cache dir for pip also works, but the cache is useless because pip runtime env only install once to the virtualenv). Related options: --no-cache-dir.

I will add a test to cover the current installs.

…me_env

fyrestone · 2022-02-23T04:08:14Z

This looks good to me! Just have one question:

Are we sure that it's safe to remove the lock for concurrent pip installs? I think concurrently installing pip environments can cause some unexpected errors, e.g. #17086. Or is it safe now because we added --no-cache-dir?

If we can add a test to cover the case of concurrent installs, I'd feel better about this. Something like installing two runtime envs, pip: ["A", "B"] concurrently with pip: ["A", "C"]. Is it possible to add such a test to this PR?

I noticed that the test_runtime_env_complicated.py::test_simultaneous_install already covers the concurrent pip installs. Please refer to: https://github.com/ray-project/ray/blob/master/python/ray/tests/test_runtime_env_complicated.py#L706

fyrestone · 2022-02-23T13:32:33Z

@architkulkarni Hi, all the comments have been fixed. Can this PR be merged?

architkulkarni

Looks good to me! There were a handful of failed tests in CI but I don't think they're related to this PR; I restarted them just to have more confidence.

fyrestone · 2022-02-24T02:43:57Z

Looks good to me! There were a handful of failed tests in CI but I don't think they're related to this PR; I restarted them just to have more confidence.

Thanks.

In order to initialize runtime env concurrently, this PR makes pip runtime env asynchronous. It includes, - [x] New `check_output_cmd` in runtime env utils. - [x] Async PipProcessor. - [x] The `asynccontextmanager` from `https://github.com/python-trio/async_generator` for Python 3.6 - [x] Remove pip runtime env lock. - [x] Disable pip cache. Co-authored-by: 刘宝 <[email protected]>

…n in threads. (#22588) I recently realized that during a runtime_env creation process, a plugin/manager that is very slow to setup may block the creation of other runtime_env, so I make plugin/manager setup run in threads. [The refactor of `PipManager`](#22381) is about to be completed, so I ignore it in this PR.

刘宝 added 2 commits February 15, 2022 16:01

Async pip runtime env

3f3108d

Add test case

1d90c68

fyrestone self-assigned this Feb 15, 2022

刘宝 added 2 commits February 15, 2022 17:56

Run get_directory_size_bytes in executor

91ecd8e

Fix lint

90a1908

SongGuyang approved these changes Feb 15, 2022

View reviewed changes

刘宝 added 2 commits February 16, 2022 10:50

Merge remote-tracking branch 'origin_ray/master' into async_pip_runti…

803a9f1

…me_env

Refine SubprocessCalledProcessError

e13a503

fyrestone marked this pull request as ready for review February 16, 2022 07:18

fyrestone requested review from architkulkarni and edoakes February 16, 2022 07:18

Fix Python 3.6 compatibility

13a6cf5

Catch-Bull reviewed Feb 16, 2022

View reviewed changes

python/ray/_private/runtime_env/pip.py Outdated Show resolved Hide resolved

刘宝 added 2 commits February 16, 2022 18:43

Remove pip file lock to avoid dead lock

9311ac3

Delete uri cancel creating task

e5425c9

architkulkarni assigned architkulkarni, edoakes and rkooo567 Feb 16, 2022

刘宝 added 4 commits February 17, 2022 10:54

Fix Python compatibility

317584d

Make PipProcessor awaitable

bab5d47

Disable pip cache

443eed7

Merge remote-tracking branch 'origin_ray/master' into async_pip_runti…

9d91dc2

…me_env

rkooo567 reviewed Feb 18, 2022

View reviewed changes

python/ray/_private/async_compat.py Show resolved Hide resolved

edoakes removed their assignment Feb 18, 2022

fyrestone commented Feb 21, 2022

View reviewed changes

刘宝 added 2 commits February 21, 2022 10:49

Merge remote-tracking branch 'origin_ray/master' into async_pip_runti…

68d9830

…me_env

Merge remote-tracking branch 'origin_ray/master' into async_pip_runti…

538019f

…me_env

architkulkarni reviewed Feb 22, 2022

View reviewed changes

python/ray/_private/runtime_env/utils.py Outdated Show resolved Hide resolved

architkulkarni reviewed Feb 22, 2022

View reviewed changes

python/ray/tests/test_runtime_env.py Outdated Show resolved Hide resolved

architkulkarni reviewed Feb 22, 2022

View reviewed changes

Merge remote-tracking branch 'origin_ray/master' into async_pip_runti…

9a5b7e6

…me_env

刘宝 added 2 commits February 23, 2022 12:09

Fix

ef5bf89

Append LICENSE

dae5801

fyrestone added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Feb 23, 2022

architkulkarni approved these changes Feb 23, 2022

View reviewed changes

jovany-wang merged commit 6a9a286 into ray-project:master Feb 24, 2022

jovany-wang deleted the async_pip_runtime_env branch February 24, 2022 03:03

Catch-Bull mentioned this pull request Feb 24, 2022

[runtime env] Make plugin setup process that has not been refactor run in threads. #22588

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runtime env] Async pip runtime env #22381

[runtime env] Async pip runtime env #22381

fyrestone commented Feb 15, 2022 •

edited

Loading

SongGuyang left a comment

SongGuyang Feb 15, 2022

Catch-Bull Feb 15, 2022

fyrestone Feb 16, 2022

fyrestone Feb 16, 2022

fyrestone commented Feb 18, 2022

rkooo567 left a comment

edoakes commented Feb 18, 2022

fyrestone Feb 21, 2022

architkulkarni left a comment

fyrestone commented Feb 23, 2022

fyrestone commented Feb 23, 2022 •

edited

Loading

fyrestone commented Feb 23, 2022

architkulkarni left a comment

fyrestone commented Feb 24, 2022

[runtime env] Async pip runtime env #22381

[runtime env] Async pip runtime env #22381

Conversation

fyrestone commented Feb 15, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

SongGuyang left a comment

Choose a reason for hiding this comment

SongGuyang Feb 15, 2022

Choose a reason for hiding this comment

Catch-Bull Feb 15, 2022

Choose a reason for hiding this comment

fyrestone Feb 16, 2022

Choose a reason for hiding this comment

fyrestone Feb 16, 2022

Choose a reason for hiding this comment

fyrestone commented Feb 18, 2022

rkooo567 left a comment

Choose a reason for hiding this comment

edoakes commented Feb 18, 2022

fyrestone Feb 21, 2022

Choose a reason for hiding this comment

architkulkarni left a comment

Choose a reason for hiding this comment

fyrestone commented Feb 23, 2022

fyrestone commented Feb 23, 2022 • edited Loading

fyrestone commented Feb 23, 2022

architkulkarni left a comment

Choose a reason for hiding this comment

fyrestone commented Feb 24, 2022

fyrestone commented Feb 15, 2022 •

edited

Loading

fyrestone commented Feb 23, 2022 •

edited

Loading