From c4e5bafc60de7d3b4a1e6c9b26582db3ed6bf00c Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Tue, 2 May 2023 08:37:13 -0700 Subject: [PATCH 1/8] Fixed lint Signed-off-by: SangBin Cho --- python/ray/_private/runtime_env/conda.py | 40 ++++++++++++++----- .../ray/_private/runtime_env/conda_utils.py | 1 + .../tests/test_runtime_env_conda_and_pip.py | 15 +++++++ 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/python/ray/_private/runtime_env/conda.py b/python/ray/_private/runtime_env/conda.py index f428163602dc..2f58b8973e61 100644 --- a/python/ray/_private/runtime_env/conda.py +++ b/python/ray/_private/runtime_env/conda.py @@ -18,6 +18,7 @@ create_conda_env_if_needed, delete_conda_env, get_conda_activate_commands, + get_conda_env_list, ) from ray._private.runtime_env.context import RuntimeEnvContext from ray._private.runtime_env.packaging import Protocol, parse_uri @@ -319,18 +320,35 @@ async def create( context: RuntimeEnvContext, logger: logging.Logger = default_logger, ) -> int: - if uri is None: - # The "conda" field is the name of an existing conda env, so no - # need to create one. - # TODO(architkulkarni): Try "conda activate" here to see if the - # env exists, and raise an exception if it doesn't. - return 0 - - # Currently create method is still a sync process, to avoid blocking - # the loop, need to run this function in another thread. - # TODO(Catch-Bull): Refactor method create into an async process, and - # make this method running in current loop. def _create(): + if uri is None: + # The "conda" field is the name of an existing conda env, so no + # need to create one. + # TODO(architkulkarni): Try "conda activate" here to see if the + # env exists, and raise an exception if it doesn't. + conda_env_name = runtime_env.get("conda") + if conda_env_name is None or not isinstance(conda_env_name, str): + raise ValueError( + "Unexpected conda environment was specified " + f"from a runtime env. {runtime_env}. It must " + "be an existing env, a yaml file, or a " + "corresponding dict." + ) + + # Make sure the conda env exists. + conda_env_list = get_conda_env_list() + logger.info(conda_env_list) + envs = [Path(env).name for env in conda_env_list] + if conda_env_name not in envs: + raise ValueError( + f"The given conda environment '{conda_env_name}' " + f"from a runtime env {runtime_env} doesn't " + "exist from the output of `conda env list --json`. " + "You can only specify the env that already exists. " + f"Please make sure to create a env {conda_env_name} " + ) + return 0 + logger.debug( "Setting up conda for runtime_env: " f"{runtime_env.serialize()}" ) diff --git a/python/ray/_private/runtime_env/conda_utils.py b/python/ray/_private/runtime_env/conda_utils.py index 291f20c2f407..757ca4a998dd 100644 --- a/python/ray/_private/runtime_env/conda_utils.py +++ b/python/ray/_private/runtime_env/conda_utils.py @@ -100,6 +100,7 @@ def create_conda_env_if_needed( """ if logger is None: logger = logging.getLogger(__name__) + conda_path = get_conda_bin_executable("conda") try: exec_cmd([conda_path, "--help"], throw_on_error=False) diff --git a/python/ray/tests/test_runtime_env_conda_and_pip.py b/python/ray/tests/test_runtime_env_conda_and_pip.py index ec721a8ac9d7..97f9fef09032 100644 --- a/python/ray/tests/test_runtime_env_conda_and_pip.py +++ b/python/ray/tests/test_runtime_env_conda_and_pip.py @@ -207,6 +207,21 @@ def f(): assert ray.get(f.remote()) == 0 +def test_runtime_env_conda_not_exists_not_hang(shutdown_only): + """Verify when the conda env doesn't exist, it doesn't hang Ray.""" + ray.init(runtime_env={"conda": "env_which_does_not_exist"}) + + @ray.remote + def f(): + return 1 + + with pytest.raises(ray.exceptions.RuntimeEnvSetupError) as exc_info: + ray.get(f.remote()) + assert "doesn't exist from the output of `conda env list --json`" in str( + exc_info.value + ) # noqa + + if __name__ == "__main__": if os.environ.get("PARALLEL_CI"): sys.exit(pytest.main(["-n", "auto", "--boxed", "-vs", __file__])) From c54e9c001ea175a5bfb9eb26abbc1dd4c9a09324 Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Tue, 2 May 2023 23:10:30 -0700 Subject: [PATCH 2/8] addressed code review. Signed-off-by: SangBin Cho --- python/ray/_private/runtime_env/conda.py | 36 ++++++++----------- .../tests/test_runtime_env_conda_and_pip.py | 13 ++++--- 2 files changed, 22 insertions(+), 27 deletions(-) diff --git a/python/ray/_private/runtime_env/conda.py b/python/ray/_private/runtime_env/conda.py index 2f58b8973e61..11d28333c5ae 100644 --- a/python/ray/_private/runtime_env/conda.py +++ b/python/ray/_private/runtime_env/conda.py @@ -23,6 +23,7 @@ from ray._private.runtime_env.context import RuntimeEnvContext from ray._private.runtime_env.packaging import Protocol, parse_uri from ray._private.runtime_env.plugin import RuntimeEnvPlugin +from ray._private.runtime_env.validation import parse_and_validate_conda from ray._private.utils import ( get_directory_size_bytes, get_master_wheel_url, @@ -218,12 +219,10 @@ def get_uri(runtime_env: Dict) -> Optional[str]: """Return `"conda://"`, or None if no GC required.""" conda = runtime_env.get("conda") if conda is not None: - if isinstance(conda, str): + if isinstance(conda, str) or isinstance(conda, dict): # User-preinstalled conda env. We don't garbage collect these, so # we don't track them with URIs. - uri = None - elif isinstance(conda, dict): - uri = "conda://" + _get_conda_env_hash(conda_dict=conda) + uri = f"conda://{_get_conda_env_hash(conda_dict=conda)}" else: raise TypeError( "conda field received by RuntimeEnvAgent must be " @@ -320,32 +319,25 @@ async def create( context: RuntimeEnvContext, logger: logging.Logger = default_logger, ) -> int: + if not runtime_env.has_conda(): + return 0 + def _create(): - if uri is None: - # The "conda" field is the name of an existing conda env, so no - # need to create one. - # TODO(architkulkarni): Try "conda activate" here to see if the - # env exists, and raise an exception if it doesn't. - conda_env_name = runtime_env.get("conda") - if conda_env_name is None or not isinstance(conda_env_name, str): - raise ValueError( - "Unexpected conda environment was specified " - f"from a runtime env. {runtime_env}. It must " - "be an existing env, a yaml file, or a " - "corresponding dict." - ) + result = parse_and_validate_conda(runtime_env.get("conda")) - # Make sure the conda env exists. + if isinstance(result, str): + # The conda env name is given. + # In this case, we only verify if the given + # conda env exists. conda_env_list = get_conda_env_list() - logger.info(conda_env_list) envs = [Path(env).name for env in conda_env_list] - if conda_env_name not in envs: + if result not in envs: raise ValueError( - f"The given conda environment '{conda_env_name}' " + f"The given conda environment '{result}' " f"from a runtime env {runtime_env} doesn't " "exist from the output of `conda env list --json`. " "You can only specify the env that already exists. " - f"Please make sure to create a env {conda_env_name} " + f"Please make sure to create a env {result} " ) return 0 diff --git a/python/ray/tests/test_runtime_env_conda_and_pip.py b/python/ray/tests/test_runtime_env_conda_and_pip.py index 97f9fef09032..8fde07778801 100644 --- a/python/ray/tests/test_runtime_env_conda_and_pip.py +++ b/python/ray/tests/test_runtime_env_conda_and_pip.py @@ -215,11 +215,14 @@ def test_runtime_env_conda_not_exists_not_hang(shutdown_only): def f(): return 1 - with pytest.raises(ray.exceptions.RuntimeEnvSetupError) as exc_info: - ray.get(f.remote()) - assert "doesn't exist from the output of `conda env list --json`" in str( - exc_info.value - ) # noqa + refs = [f.remote() for _ in range(5)] + + for ref in refs: + with pytest.raises(ray.exceptions.RuntimeEnvSetupError) as exc_info: + ray.get(ref) + assert "doesn't exist from the output of `conda env list --json`" in str( + exc_info.value + ) # noqa if __name__ == "__main__": From b7cf788744d9dab0a67aee7948a6e8a1e716e4ed Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Tue, 2 May 2023 23:11:12 -0700 Subject: [PATCH 3/8] done Signed-off-by: SangBin Cho --- python/ray/_private/runtime_env/conda.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python/ray/_private/runtime_env/conda.py b/python/ray/_private/runtime_env/conda.py index 11d28333c5ae..90dcc236b0ec 100644 --- a/python/ray/_private/runtime_env/conda.py +++ b/python/ray/_private/runtime_env/conda.py @@ -334,10 +334,10 @@ def _create(): if result not in envs: raise ValueError( f"The given conda environment '{result}' " - f"from a runtime env {runtime_env} doesn't " + f"from the runtime env {runtime_env} doesn't " "exist from the output of `conda env list --json`. " - "You can only specify the env that already exists. " - f"Please make sure to create a env {result} " + "You can only specify an env that already exists. " + f"Please make sure to create an env {result} " ) return 0 From cc1ae45a3c505de5a90d3287daafe42a2d377ba7 Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Thu, 10 Aug 2023 10:33:23 +0900 Subject: [PATCH 4/8] ip --- python/ray/tests/test_runtime_env_conda_and_pip.py | 1 - 1 file changed, 1 deletion(-) diff --git a/python/ray/tests/test_runtime_env_conda_and_pip.py b/python/ray/tests/test_runtime_env_conda_and_pip.py index 4c7d0ffc8d66..fb8323ff11b9 100644 --- a/python/ray/tests/test_runtime_env_conda_and_pip.py +++ b/python/ray/tests/test_runtime_env_conda_and_pip.py @@ -275,7 +275,6 @@ def test_get_requirements_file(): ], ) assert "Could not find a valid filename for the internal " in str(excinfo.value) ->>>>>>> master if __name__ == "__main__": From 7698fd019f0fb4b9be17b56643df179b1e344c71 Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Tue, 22 Aug 2023 17:29:22 +0900 Subject: [PATCH 5/8] Addressed comments. --- python/ray/_private/runtime_env/conda.py | 17 +++++++++- .../ray/tests/test_runtime_env_complicated.py | 32 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/python/ray/_private/runtime_env/conda.py b/python/ray/_private/runtime_env/conda.py index 90dcc236b0ec..f2060b62a4f5 100644 --- a/python/ray/_private/runtime_env/conda.py +++ b/python/ray/_private/runtime_env/conda.py @@ -219,7 +219,10 @@ def get_uri(runtime_env: Dict) -> Optional[str]: """Return `"conda://"`, or None if no GC required.""" conda = runtime_env.get("conda") if conda is not None: - if isinstance(conda, str) or isinstance(conda, dict): + if isinstance(conda, str): + return None + + if isinstance(conda, dict): # User-preinstalled conda env. We don't garbage collect these, so # we don't track them with URIs. uri = f"conda://{_get_conda_env_hash(conda_dict=conda)}" @@ -269,6 +272,12 @@ def __init__(self, resources_dir: str): self._installs_and_deletions_file_lock = os.path.join( self._resources_dir, "ray-conda-installs-and-deletions.lock" ) + # A set of named conda environments (instead of yaml or dict) + # that are validated to exist. + # NOTE: It has to be only used within the same thread, which + # is an event loop. + # Also, we don't need to GC this field because it is pretty small. + self._validated_named_conda_env = set() def _get_path_from_hash(self, hash: str) -> str: """Generate a path from the hash of a conda or pip spec. @@ -329,6 +338,11 @@ def _create(): # The conda env name is given. # In this case, we only verify if the given # conda env exists. + + # If the env is already validated, do nothing. + if result in self._validated_named_conda_env: + return 0 + conda_env_list = get_conda_env_list() envs = [Path(env).name for env in conda_env_list] if result not in envs: @@ -339,6 +353,7 @@ def _create(): "You can only specify an env that already exists. " f"Please make sure to create an env {result} " ) + self._validated_named_conda_env.add(result) return 0 logger.debug( diff --git a/python/ray/tests/test_runtime_env_complicated.py b/python/ray/tests/test_runtime_env_complicated.py index 5a7361a135de..2906ac362409 100644 --- a/python/ray/tests/test_runtime_env_complicated.py +++ b/python/ray/tests/test_runtime_env_complicated.py @@ -213,6 +213,38 @@ def wrapped_version(self): assert ray.get(actor.wrapped_version.remote()) == package_version +@pytest.mark.skipif( + os.environ.get("CONDA_DEFAULT_ENV") is None, + reason="must be run from within a conda environment", +) +def test_task_conda_env_validation_cached(shutdown_only): + """Verify that when a task is running with the same conda env + it doesn't validate if env exists. + """ + # The first run would be slower because we need to validate + # if the package exists. + ray.init() + # version = EMOJI_VERSIONS[0] + # runtime_env = {"conda": f"package-{version}"} + runtime_env = {"conda": "core"} + task = get_emoji_version.options(runtime_env=runtime_env) + s = time.time() + ray.get(task.remote()) + first_run = time.time() - s + # Typically takes 1~2 seconds. + print("First run took", first_run) + + # We should verify this doesn't happen + # from the second run. + s = time.time() + for _ in range(10): + ray.get(task.remote()) + second_10_runs = time.time() - s + # Typicall takes less than 100ms. + print("second 10 runs took", second_10_runs) + assert second_10_runs < first_run + + @pytest.mark.skipif( os.environ.get("CONDA_DEFAULT_ENV") is None, reason="must be run from within a conda environment", From 374b8a0ab0f74dd7a1d98d0d043694743b8eacb8 Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Tue, 22 Aug 2023 17:31:26 +0900 Subject: [PATCH 6/8] revert unnecessary changes. --- python/ray/_private/runtime_env/conda.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/python/ray/_private/runtime_env/conda.py b/python/ray/_private/runtime_env/conda.py index f2060b62a4f5..a170645affa8 100644 --- a/python/ray/_private/runtime_env/conda.py +++ b/python/ray/_private/runtime_env/conda.py @@ -220,9 +220,8 @@ def get_uri(runtime_env: Dict) -> Optional[str]: conda = runtime_env.get("conda") if conda is not None: if isinstance(conda, str): - return None - - if isinstance(conda, dict): + uri = None + elif isinstance(conda, dict): # User-preinstalled conda env. We don't garbage collect these, so # we don't track them with URIs. uri = f"conda://{_get_conda_env_hash(conda_dict=conda)}" From 59488f67861853006db6419d7af79be254a0eabe Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Wed, 23 Aug 2023 09:26:02 +0900 Subject: [PATCH 7/8] Addressed code review --- python/ray/_private/runtime_env/conda.py | 4 ++-- python/ray/tests/test_runtime_env_complicated.py | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/python/ray/_private/runtime_env/conda.py b/python/ray/_private/runtime_env/conda.py index a170645affa8..a28cc02cb046 100644 --- a/python/ray/_private/runtime_env/conda.py +++ b/python/ray/_private/runtime_env/conda.py @@ -220,10 +220,10 @@ def get_uri(runtime_env: Dict) -> Optional[str]: conda = runtime_env.get("conda") if conda is not None: if isinstance(conda, str): - uri = None - elif isinstance(conda, dict): # User-preinstalled conda env. We don't garbage collect these, so # we don't track them with URIs. + uri = None + elif isinstance(conda, dict): uri = f"conda://{_get_conda_env_hash(conda_dict=conda)}" else: raise TypeError( diff --git a/python/ray/tests/test_runtime_env_complicated.py b/python/ray/tests/test_runtime_env_complicated.py index 2906ac362409..56463c79fc3b 100644 --- a/python/ray/tests/test_runtime_env_complicated.py +++ b/python/ray/tests/test_runtime_env_complicated.py @@ -217,16 +217,15 @@ def wrapped_version(self): os.environ.get("CONDA_DEFAULT_ENV") is None, reason="must be run from within a conda environment", ) -def test_task_conda_env_validation_cached(shutdown_only): +def test_task_conda_env_validation_cached(conda_envs, shutdown_only): """Verify that when a task is running with the same conda env it doesn't validate if env exists. """ # The first run would be slower because we need to validate # if the package exists. ray.init() - # version = EMOJI_VERSIONS[0] - # runtime_env = {"conda": f"package-{version}"} - runtime_env = {"conda": "core"} + version = EMOJI_VERSIONS[0] + runtime_env = {"conda": f"package-{version}"} task = get_emoji_version.options(runtime_env=runtime_env) s = time.time() ray.get(task.remote()) From 9597b04f85eaf4e6571a05f076c7bcd7204b9422 Mon Sep 17 00:00:00 2001 From: SangBin Cho Date: Wed, 23 Aug 2023 19:32:42 +0900 Subject: [PATCH 8/8] M odify the test to stop using conda --- python/ray/tests/test_client_proxy.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/python/ray/tests/test_client_proxy.py b/python/ray/tests/test_client_proxy.py index 16211fe01ab4..5faa3b00243d 100644 --- a/python/ray/tests/test_client_proxy.py +++ b/python/ray/tests/test_client_proxy.py @@ -88,11 +88,16 @@ def test_proxy_manager_bad_startup(shutdown_only): """ pm, free_ports = start_ray_and_proxy_manager(n_ports=2) client = "client1" + ctx = ray.init(ignore_reinit_error=True) + port_to_conflict = ctx.dashboard_url.split(":")[1] pm.create_specific_server(client) - assert not pm.start_specific_server( + # Intentionally bind to the wrong port so that the + # server will crash. + pm._get_server_for_client(client).port = port_to_conflict + pm.start_specific_server( client, - JobConfig(runtime_env={"conda": "conda-env-that-sadly-does-not-exist"}), + JobConfig(), ) # Wait for reconcile loop time.sleep(2)