[Core][Compiled Graph] Fix shutdown error #48280

rkooo567 · 2024-10-26T04:48:49Z

Why are these changes needed?

Fixes remaining shutdown issues. It also fixes some failures in the nightly we accidently merged

Related issue number

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 :(

rkooo567 · 2024-10-26T06:49:33Z

python/ray/dag/compiled_dag_node.py

-                        self.wait_teardown(kill_actors)
-                    return
+                    # self.wait_teardown(kill_actors)
+                    while True:


this was the root cause of segfault

I think there was a dangling monitor thread that was calling this API and waiting for ray.get, which is triggering segfault when the driver is shutdown

calling this API and waiting for ray.get, which is triggering segfault when the driver is shutdown

Why this function will be called after the driver process is shutdown?

This function still calls ray.get and self.wait_teardown() below. Why does the issue disappear?

it is not called "after" shutdown. but when shutdown happens, 2 threads (main and monitor) get into this wait_teardown, which calls ray.get. If the main thread one finishes, the shutdown finishes and the next test starts. If the monitor thread one is not finished before the main thread, the dangling thread still is waiting for ray.get, which basically crashes because ray is shutdown and core worker doesn't exist

Thank you for the explanation!

rkooo567 · 2024-10-26T06:49:41Z

python/ray/dag/compiled_dag_node.py

@@ -2174,14 +2186,16 @@ def teardown(self, kill_actors: bool = False):
        """Teardown and cancel all actor tasks for this DAG. After this
        function returns, the actors should be available to execute new tasks
        or compile a new DAG."""
+        if self._is_teardown:


this will make this API idempotent

rkooo567 · 2024-10-26T06:49:53Z

python/ray/dag/tests/experimental/test_torch_tensor_dag.py

-            r"Outputs cannot be transferred via NCCL because the driver cannot "
-            "participate in the NCCL group"
-        ),
+        match=(r"Driver cannot participate in the NCCL group\."),


this is a fix for accidental merge in the master

rkooo567 · 2024-10-26T06:50:04Z

python/ray/dag/tests/experimental/test_torch_tensor_dag.py

@@ -714,7 +700,7 @@ def test_torch_tensor_nccl_nested_dynamic(ray_start_regular):

    for i in range(3):
        dtype = torch.float16
-        args = [{j: (j, (10 * j,), dtype)} for j in range(1, i + 1)]
+        args = {j: (j, (10 * j,), dtype) for j in range(1, i + 1)}


another accidental merge fix in the master

kevin85421 · 2024-10-26T22:08:47Z

python/ray/experimental/compiled_dag_ref.py

@@ -84,6 +84,10 @@ def __reduce__(self):
        raise ValueError("CompiledDAGRef cannot be pickled.")

    def __del__(self):
+        # If the dag is already teardown, it should do nothing.


Suggested change

# If the dag is already teardown, it should do nothing.

# If the DAG has already been torn down, it should do nothing.

kevin85421 · 2024-10-26T22:13:55Z

python/ray/dag/tests/experimental/test_torch_tensor_dag.py

@@ -351,6 +274,73 @@ def test_torch_tensor_custom_comm(ray_start_regular):

    from cupy.cuda import nccl

+    class TestNcclGroup(GPUCommunicator):


why there are two class TestNcclGroup in this file?

this is the fix @ruisearch42 made before. I am just reverting it back (in a local test, without this, test suite cannot find this folder for some reasons)

kevin85421 · 2024-10-26T22:43:54Z

python/ray/dag/compiled_dag_node.py

-                        self.wait_teardown(kill_actors)
-                    return
+                    # self.wait_teardown(kill_actors)
+                    while True:


calling this API and waiting for ray.get, which is triggering segfault when the driver is shutdown

Why this function will be called after the driver process is shutdown?

This function still calls ray.get and self.wait_teardown() below. Why does the issue disappear?

Fixes remaining shutdown issues. It also fixes some failures in the nightly we accidently merged

Fixes remaining shutdown issues. It also fixes some failures in the nightly we accidently merged Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>

Fixes remaining shutdown issues. It also fixes some failures in the nightly we accidently merged Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

rkooo567 added 3 commits October 26, 2024 04:47

.

0578120

Merge branch 'master' into fix-gpu-test-failures

faa79ed

.

53034c1

rkooo567 added the go add ONLY when ready to merge, run all tests label Oct 26, 2024

rkooo567 added 4 commits October 26, 2024 05:14

.

daf5f2a

.

bfdb5e3

working

fb75490

done

61daf9b

rkooo567 commented Oct 26, 2024

View reviewed changes

rkooo567 assigned stephanie-wang and ruisearch42 Oct 26, 2024

fixd

8e249c2

kevin85421 reviewed Oct 26, 2024

View reviewed changes

kevin85421 self-assigned this Oct 26, 2024

kevin85421 approved these changes Oct 26, 2024

View reviewed changes

rkooo567 enabled auto-merge (squash) October 27, 2024 16:42

rkooo567 merged commit 0b1d0d8 into ray-project:master Oct 27, 2024
6 checks passed

edoakes pushed a commit to edoakes/ray that referenced this pull request Oct 30, 2024

[Core][Compiled Graph] Fix shutdown error (ray-project#48280)

9e6e5e2

Fixes remaining shutdown issues. It also fixes some failures in the nightly we accidently merged

Jay-ju pushed a commit to Jay-ju/ray that referenced this pull request Nov 5, 2024

[Core][Compiled Graph] Fix shutdown error (ray-project#48280)

349fec4

Fixes remaining shutdown issues. It also fixes some failures in the nightly we accidently merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core][Compiled Graph] Fix shutdown error #48280

[Core][Compiled Graph] Fix shutdown error #48280

rkooo567 commented Oct 26, 2024 •

edited

Loading

rkooo567 Oct 26, 2024

kevin85421 Oct 26, 2024

rkooo567 Oct 26, 2024

kevin85421 Oct 26, 2024

rkooo567 Oct 26, 2024

rkooo567 Oct 26, 2024

rkooo567 Oct 26, 2024

kevin85421 Oct 26, 2024

kevin85421 Oct 26, 2024

rkooo567 Oct 26, 2024

kevin85421 Oct 26, 2024

	# If the dag is already teardown, it should do nothing.
	# If the DAG has already been torn down, it should do nothing.

		@@ -351,6 +274,73 @@ def test_torch_tensor_custom_comm(ray_start_regular):

		from cupy.cuda import nccl

		class TestNcclGroup(GPUCommunicator):

[Core][Compiled Graph] Fix shutdown error #48280

[Core][Compiled Graph] Fix shutdown error #48280

Conversation

rkooo567 commented Oct 26, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 commented Oct 26, 2024 •

edited

Loading