Repository navigation
[Fix][Core] Raise exception if ray.init() is called while is_client_mode_enabled is True #67407
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
base: master
Are you sure you want to change the base?
Changes from all commits
3d4e773
77ae2c7
ab50fdf
2a1fd51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,9 @@ | |
| import pytest | ||
|
|
||
| import ray | ||
| from ray._common.network_utils import build_address, parse_address | ||
| from ray._common.network_utils import build_address, find_free_port, parse_address | ||
| from ray._private import ray_constants | ||
| from ray._private.client_mode_hook import disable_client_hook | ||
| from ray._private.test_utils import persistent_gcs_test_enabled | ||
| from ray.client_builder import ClientContext | ||
| from ray.cluster_utils import Cluster | ||
|
|
@@ -359,6 +360,32 @@ def test_ray_init_with_runtime_env_as_object( | |
| assert "gcs://" in parsed_runtime_env["py_modules"][0] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "call_ray_start", | ||
| ["ray start --head --ray-client-server-port 25307 --port 0"], | ||
| indirect=True, | ||
| ) | ||
| def test_ray_init_after_client_connect_succeeded(call_ray_start): | ||
| ray.init("ray://localhost:25307") | ||
|
|
||
| try: | ||
| # A local ray.init() while the Ray Client connection is open must not raise. | ||
| ray.init() | ||
| finally: | ||
| # While client mode is on, ray.shutdown() is routed to the Ray Client. | ||
| # Bypass the client hook so ray.shutdown() disconnects the local driver instead. | ||
| with disable_client_hook(): | ||
| ray.shutdown() | ||
|
|
||
|
|
||
| def test_ray_init_after_client_connect_failed(shutdown_only): | ||
| with pytest.raises(ConnectionError): | ||
| ray.init(f"ray://localhost:{find_free_port()}") | ||
|
|
||
| with pytest.raises(RuntimeError, match="Ray Client is already initialized"): | ||
| ray.init() | ||
|
Comment on lines
+381
to
+386
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test asserts that calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Marking is_client_mode_enabled to False would be out of scope for #67142.
Comment on lines
+381
to
+386
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test asserts that calling |
||
|
|
||
|
|
||
| def test_shutdown_wait_for_processes(shutdown_only): | ||
| ray.init() | ||
| node = ray._private.worker._global_node | ||
|
|
||
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.
Issue: Misleading Error and Blocked Fallback due to Global State Leak\n\nThe check introduced here:\n
python\nif client_mode_should_convert() and num_connected_contexts() == 0:\n raise RuntimeError(\"Ray Client is already initialized in this process.\")\n\nraises a misleadingRuntimeErrorand blocks users from starting a local Ray cluster (viaray.init()) if a previous Ray Client connection attempt failed.\n\n#### Why this happens:\n1. Whenray.init(\"ray://...\")is called, it enables client mode globally by calling_explicitly_enable_client_mode().\n2. If the connection attempt fails (e.g., throwing aConnectionError), the exception is propagated, but_explicitly_disable_client_mode()is never called becauseRayAPIStub.connect()does not clean up on failure.\n3. This leavesis_client_mode_enabledasTrueglobally, even though no client connection was ever established (num_connected_contexts() == 0).\n4. Consequently, any subsequent call toray.init()(local) triggers this new check and raisesRuntimeError: Ray Client is already initialized in this process., which is incorrect (it was never successfully initialized) and prevents the user from falling back to a local cluster.\n\n#### Recommended Solution:\nInstead of adding this check toray.init(), we should fix the root cause by ensuring thatis_client_mode_enabledis cleaned up/disabled if the connection attempt fails.\n\nWe can achieve this by wrapping the connection logic inRayAPIStub.connect()(located inpython/ray/util/client/__init__.py) in atry...exceptblock:\n\npython\n def connect(self, *args, **kw_args):\n self.get_context()._inside_client_test = self._inside_client_test\n try:\n conn = self.get_context().connect(*args, **kw_args)\n global _lock, _all_contexts\n with _lock:\n _all_contexts.add(self._cxt.handler)\n return conn\n except Exception:\n global _lock, _all_contexts\n with _lock:\n if len(_all_contexts) == 0:\n _explicitly_disable_client_mode()\n raise\n\n\nIf we fix the leak there,client_mode_should_convert()will correctly returnFalseafter a failed connection, and this check inworker.pybecomes unnecessary and can be removed.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.
Same as #67407 (comment). Fixing the leak of global client mode state is not in-scope for this pr. It is also an existing issue. The goal is to raise with a clearer message in this pr.