[runtime env] Fix Ray hangs when nonexistent conda environment is specified #28105 #34956

rkooo567 · 2023-05-02T15:38:30Z

Why are these changes needed?

When a conda name is given to the runtime env, we assume the env already exits. However, there are times the env doesn't exist, and if that happens, it hangs forever. This fixes the issue by always checking conda env list before creating a conda runtime env.

Related issue number

Closes #28105

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
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 added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
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 :(

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rkooo567 · 2023-05-02T15:39:28Z

python/ray/_private/runtime_env/conda.py

        def _create():
+            if uri is None:


I moved it to inside a function because I want get_conda_env_list to run in a separate thread to avoid blocking agent main thread.

architkulkarni

Looks great! Just a couple of nits/questions.

By the way, do you happen to know the impact of this on worker startup time? My impression is that conda activate already takes a couple seconds, and this check might take several more seconds depending on how many envs there are, but this performance impact still seems worth the correctness tradeoff.

architkulkarni · 2023-05-02T15:46:02Z

python/ray/_private/runtime_env/conda.py

+                # TODO(architkulkarni): Try "conda activate" here to see if the
+                # env exists, and raise an exception if it doesn't.


Suggested change

# TODO(architkulkarni): Try "conda activate" here to see if the

# env exists, and raise an exception if it doesn't.

Thanks for fixing this :)

python/ray/_private/runtime_env/conda.py

jjyao · 2023-05-02T17:37:10Z

Could you fix the PR title?

rkooo567 · 2023-05-02T22:55:49Z

By the way, do you happen to know the impact of this on worker startup time? My impression is that conda activate already takes a couple seconds, and this check might take several more seconds depending on how many envs there are, but this performance impact still seems worth the correctness tradeoff.

Hmm actually a good point. Maybe I can make it do only once? Maybe we can create a URI that has the state about it.

architkulkarni · 2023-05-02T23:33:35Z

I think despite the additional complexity, your URI approach is the correct solution. That's what we do for pip currently. I'm fine with leaving it as a followup issue, unless you think the performance regression in this PR is unacceptable.

Note that for production use cases, we don't recommend runtime_env anyway (we recommend using a container image). If the user must use an existing conda environment and they don't need multiple environments, we can recommend they call conda activate before ray start.

rkooo567 · 2023-05-03T05:40:58Z

Yeah I think it is a perf regression if we merge it this way. Let me see how complicated it is to support the URI.

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rkooo567 · 2023-05-03T06:11:41Z

@architkulkarni can you check it again? I addressed the problem ^. I think testing is a bit tough (do we have a test that already has conda env?), so I manually verified it, but lmk if you have an idea for testing.

architkulkarni

I just realized a concern about deletion, can you take a look?

python/ray/_private/runtime_env/conda.py

stale · 2023-06-10T05:06:13Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

stale · 2023-08-10T05:14:35Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

architkulkarni

My main concern is that using time.time() in the test is risky, it might make the test flaky. I'll let you decide the urgency, given that branch cut is tomorrow.

python/ray/_private/runtime_env/conda.py

python/ray/tests/test_runtime_env_complicated.py

rkooo567 · 2023-08-23T13:05:12Z

Failed tests seem unrelated

…cified ray-project#28105 (ray-project#34956) When a conda name is given to the runtime env, we assume the env already exits. However, there are times the env doesn't exist, and if that happens, it hangs forever. This fixes the issue by always checking conda env list before creating a conda runtime env. Signed-off-by: e428265 <arvind.chandramouli@lmco.com>

…cified ray-project#28105 (ray-project#34956) When a conda name is given to the runtime env, we assume the env already exits. However, there are times the env doesn't exist, and if that happens, it hangs forever. This fixes the issue by always checking conda env list before creating a conda runtime env. Signed-off-by: Victor <vctr.y.m@example.com>

#34956 fixed a conda hanging issue by checking existence of the conda env name. However it only checks by conda env name, not path - if the user specifies an absolute path, it no longer works. This is an regression because it should support full paths without a problem. This PR reads conda CLI for the full paths and names. A special treatment is done for `base` which is just the conda base prefix; and others are base conda_prefix/envs/name. Adds tests for both `base` and full path of it; and also name and full paths for non base envs. Signed-off-by: Ruiyang Wang <rywang014@gmail.com> Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com>

Fixed lint

c4e5baf

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

rkooo567 assigned jjyao and architkulkarni May 2, 2023

rkooo567 commented May 2, 2023

View reviewed changes

architkulkarni approved these changes May 2, 2023

View reviewed changes

architkulkarni reviewed May 2, 2023

View reviewed changes

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

rkooo567 changed the title ~~Fixed lint~~ [runtime env] Fix Ray hangs when nonexistent conda environment is specified #28105 May 2, 2023

rkooo567 added 3 commits May 2, 2023 22:44

Merge branch 'master' into runtime-env-conda-error

b1d34d8

addressed code review.

c54e9c0

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

done

b7cf788

Signed-off-by: SangBin Cho <rkooo567@gmail.com>

architkulkarni requested changes May 3, 2023

View reviewed changes

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

python/ray/_private/runtime_env/conda.py Show resolved Hide resolved

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 10, 2023

rkooo567 removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 21, 2023

rkooo567 added 2 commits August 10, 2023 10:32

Merge branch 'master' into runtime-env-conda-error

78f0828

ip

cc1ae45

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Aug 10, 2023

rkooo567 added 2 commits August 22, 2023 16:57

Merge branch 'master' into runtime-env-conda-error

8d9df6d

Addressed comments.

7698fd0

stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Aug 22, 2023

revert unnecessary changes.

374b8a0

architkulkarni approved these changes Aug 22, 2023

View reviewed changes

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

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

rkooo567 added 2 commits August 23, 2023 09:24

Merge branch 'master' into runtime-env-conda-error

0312d10

Addressed code review

59488f6

rkooo567 added 2 commits August 23, 2023 18:34

Merge branch 'master' into runtime-env-conda-error

ff01796

M odify the test to stop using conda

9597b04

rkooo567 merged commit 69ed38b into ray-project:master Aug 23, 2023
1 of 2 checks passed

rynewang mentioned this pull request May 25, 2024

[core][runtime_env] Allow full path in conda runtime env. #45550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runtime env] Fix Ray hangs when nonexistent conda environment is specified #28105 #34956

[runtime env] Fix Ray hangs when nonexistent conda environment is specified #28105 #34956

rkooo567 commented May 2, 2023

rkooo567 May 2, 2023

architkulkarni left a comment

architkulkarni May 2, 2023

jjyao commented May 2, 2023

rkooo567 commented May 2, 2023

architkulkarni commented May 2, 2023

rkooo567 commented May 3, 2023

rkooo567 commented May 3, 2023 •

edited

Loading

architkulkarni left a comment

stale bot commented Jun 10, 2023

stale bot commented Aug 10, 2023

architkulkarni left a comment

rkooo567 commented Aug 23, 2023

		# TODO(architkulkarni): Try "conda activate" here to see if the
		# env exists, and raise an exception if it doesn't.

[runtime env] Fix Ray hangs when nonexistent conda environment is specified #28105 #34956

[runtime env] Fix Ray hangs when nonexistent conda environment is specified #28105 #34956

Conversation

rkooo567 commented May 2, 2023

Why are these changes needed?

Related issue number

Checks

rkooo567 May 2, 2023

Choose a reason for hiding this comment

architkulkarni left a comment

Choose a reason for hiding this comment

architkulkarni May 2, 2023

Choose a reason for hiding this comment

jjyao commented May 2, 2023

rkooo567 commented May 2, 2023

architkulkarni commented May 2, 2023

rkooo567 commented May 3, 2023

rkooo567 commented May 3, 2023 • edited Loading

architkulkarni left a comment

Choose a reason for hiding this comment

stale bot commented Jun 10, 2023

stale bot commented Aug 10, 2023

architkulkarni left a comment

Choose a reason for hiding this comment

rkooo567 commented Aug 23, 2023

rkooo567 commented May 3, 2023 •

edited

Loading