[core] Weakref semantics for actor handles retrieved from self or by name #45699

stephanie-wang · 2024-06-03T20:50:03Z

Why are these changes needed?

The distributed ref counting protocol assumes that ObjectRefs and ActorHandles are only ever passed in-band, through task arguments and return values. However, it is possible to get actor handles "out-of-band" through two APIs:

ray.get_runtime_context().current_actor returns a handle to the self actor
ray.get_actor can get an actor handle by name

We used to assert that when a task finished, its arguments were still registered in the ref count table. That way, we could return the remaining ref count back to the task's caller. This assertion is too strong and causes crash in the following case:

Actor receives a handle to itself through its arguments.
Actor gets another handle to itself via ray.get_runtime_context().current_actor
The handle in step (2) goes out of scope -> ref removed from the ReferenceCounter
Actor task finishes -> the handle in its arguments is no longer in the ReferenceCounter -> assertion failure

This PR updates ref counting for actors to distinguish between strong refs and weak refs. Strong refs are handles passed via task args and return values from the original handle. These are tracked by the ref counting protocol and will keep the actor alive. Weak refs are handles created from the above cases and are not tracked by the ref counting protocol (doing so would require additional protocols to notify the owner of the new handle).

This PR also fixes the related #45704 by not incrementing the self ref count on the actor itself when the actor is first created.

Related issue number

Closes #45704.

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: Stephanie Wang <swang@cs.berkeley.edu>

kevin85421

Try to understand the issue:

(1) If the actor handle is obtained from "out-of-band" (either ray.get_runtime_context().current_actor or ray.get_actor), the reference count will not increment.
(2) However, when the "out-of-band" actor handles go out of the scope. The reference count will decrement.
(3) When the "in-band" actor task finishes, we can't find the actor handle in the reference count table.

Is my understanding correct?

This PR loosens the check in step (4) to only throw an assertion failure if the ObjectID is a real object, i.e. not an actor handle.

I don't have enough context about reference counting, but this solution seems a bit strange to me. Why wouldn't the reference count be incremented when there is an out-of-band actor handle? Hence, (3) will not fail.

This PR appears to not correct the incorrect value in the reference count table. Instead, it looses the assertion. What happens if the reference count value is wrong? Will it affect the correctness of Ray applications?

jjyao · 2024-06-04T16:54:53Z

src/ray/core_worker/reference_count.cc

+                                                       /*deduct_local_ref=*/true,
+                                                       &borrowed_refs);
+    if (!ref_found) {
+      RAY_CHECK(ObjectID::IsActorID(borrowed_id))


Should we make those "out-of-band" cases in-band instead so that reference counting is correct. With this PR, it's still possible that reference count table is 0 while the application still has a reference to the actor handle.

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

kevin85421 · 2024-06-06T18:12:59Z

python/ray/_private/serialization.py

            # Update ref counting for the actor handle
-            self.add_contained_object_ref(actor_handle_id)
-            return _actor_handle_deserializer, (serialized,)
+            if not weak_ref:


Is weak_ref equivalent to out-of-band? If they are equivalent, maybe we can update add_contained_object_ref instead, which already has some logic to determine whether the serialization is in-band or out-of-band.

It is not quite equivalent. add_contained_ref is used to track which ObjectRefs are passed inside other objects. But we don't want to count weakref ActorHandles stored inside other objects towards the ref count. Will add a comment.

Thank you for the explanation!

kevin85421 · 2024-06-06T19:03:13Z

python/ray/_raylet.pyx

        return self.make_actor_handle(
-            CCoreWorkerProcess.GetCoreWorker().GetActorHandle(c_actor_id))
+            CCoreWorkerProcess.GetCoreWorker().GetActorHandle(c_actor_id),
+            weak_ref)

    def get_named_actor_handle(self, const c_string &name,


Is this to handle ray.get_actor?

It's used in a few places, I think for both get_actor and getting the self actor.

kevin85421 · 2024-06-06T19:04:37Z

python/ray/_raylet.pyx

@@ -4429,13 +4436,15 @@ cdef class CoreWorker:
                    name, ray_namespace))
        check_status(named_actor_handle_pair.second)

-        return self.make_actor_handle(named_actor_handle_pair.first)
+        return self.make_actor_handle(named_actor_handle_pair.first,
+                                      weak_ref=True)

    def get_actor_handle(self, ActorID actor_id):


Is this to handle runtime_context().current_actor?

This is for get_actor by name.

kevin85421 · 2024-06-06T19:37:57Z

src/ray/core_worker/actor_manager.cc

    reference_counter_->AddOwnedObject(actor_creation_return_id,
                                       /*inner_ids=*/{},
                                       caller_address,
                                       call_site,
                                       /*object_size*/ -1,
                                       /*is_reconstructable=*/true,
-                                       /*add_local_ref=*/false);


Is /*add_local_ref=*/false a bug?

It used to be right because we would add the local ref in the call to AddActorHandle, but now we might as well do it when we add the owned object.

kevin85421 · 2024-06-06T19:45:40Z

src/ray/core_worker/actor_manager.cc

+    AddNewActorHandle(std::move(actor_handle),
+                      call_site,
+                      caller_address,
+                      /*is_detached*/ true);


Must a named actor be a detached actor? I am not sure why is_detached = True here.

Hmm I just copied this from before, but good catch, I don't think it needs to be. I'll try updating to is_detached = false and see if it breaks anything.

Oh actually maybe the arg is just named poorly. I think it is more about whether we own the actor or not, will update.

kevin85421 · 2024-06-06T19:55:23Z

A lot of CI tests fail. Maybe we need to rebase with the master branch.

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

kevin85421

The change looks good to me. The CI has a lot of failures. Maybe we need to rebase with the master branch?

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

jjyao · 2024-06-07T06:05:18Z

cpp/src/ray/runtime/abstract_ray_runtime.cc

@@ -389,7 +389,9 @@ std::string AbstractRayRuntime::DeserializeAndRegisterActorHandle(
    const std::string &serialized_actor_handle) {


So we fix the reference cycle of a <-> a but what about a <-> b?

I don't think we've ever supported GC for cycles with strong refs. But if they both got the actors by name, then it should work. Will add a test.

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

This reverts commit 958f5ab.

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

This reverts commit e3c5078.

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

This reverts commit 87cdf57.

…f or by name (ray-project#45699)" This reverts commit d729815.

…from self or by name (ray-project#45699)"" This reverts commit aaa6364.

stephanie-wang added 3 commits June 2, 2024 17:43

Ref counting bug

21a4289

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

test

09df077

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

test

1ae3a2f

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

stephanie-wang changed the title ~~[core] Fix ref counting bug when actors get actor handles out-of-band~~ [core] Fix ref counting crash when actors get their own actor handle Jun 3, 2024

stephanie-wang marked this pull request as ready for review June 3, 2024 20:50

stephanie-wang assigned jjyao, jackhumphries, kevin85421 and ruisearch42 Jun 3, 2024

kevin85421 reviewed Jun 4, 2024

View reviewed changes

jjyao reviewed Jun 4, 2024

View reviewed changes

stephanie-wang added 2 commits June 5, 2024 23:31

actor weak refs

03b0d69

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

lint

3bd530e

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

stephanie-wang changed the title ~~[core] Fix ref counting crash when actors get their own actor handle~~ [core] Weakref semantics for actor handles retrieved from self or by name Jun 6, 2024

kevin85421 reviewed Jun 6, 2024

View reviewed changes

stephanie-wang mentioned this pull request Jun 6, 2024

[core][experimental] Avoid serialization for data passed between two tasks on the same actor #45591

Merged

8 tasks

x

ab37018

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

stephanie-wang assigned stephanie-wang and unassigned jjyao, jackhumphries, kevin85421 and ruisearch42 Jun 6, 2024

kevin85421 approved these changes Jun 6, 2024

View reviewed changes

cpp

b6f3c37

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

jjyao reviewed Jun 7, 2024

View reviewed changes

stephanie-wang added 2 commits June 10, 2024 13:54

test

c09aff1

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

test

41f3745

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

stephanie-wang enabled auto-merge (squash) June 10, 2024 21:00

github-actions bot added the go add ONLY when ready to merge, run all tests label Jun 10, 2024

debug

958f5ab

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

github-actions bot disabled auto-merge June 13, 2024 19:48

stephanie-wang added 6 commits June 13, 2024 12:51

Merge remote-tracking branch 'upstream/master' into ref-count-bug

5d98ec2

Revert "debug"

e3c5078

This reverts commit 958f5ab.

Fix?

f841923

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

Revert "Revert "debug""

87cdf57

This reverts commit e3c5078.

weak ref

b2e06c9

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

fix

6e7f5e6

Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>

stephanie-wang enabled auto-merge (squash) June 14, 2024 17:06

Revert "Revert "Revert "debug"""

64ce0f0

This reverts commit 87cdf57.

github-actions bot disabled auto-merge June 14, 2024 18:16

stephanie-wang merged commit d729815 into ray-project:master Jun 14, 2024
5 of 6 checks passed

stephanie-wang deleted the ref-count-bug branch June 14, 2024 22:37

ptomecek mentioned this pull request Jun 28, 2024

[Core] Actor supports job lifetime #46333

Open

zcin mentioned this pull request Jul 3, 2024

Revert "[core] Weakref semantics for actor handles retrieved from self or by name" #46429

Closed

GeneDer mentioned this pull request Jul 9, 2024

Revert "[core] Weakref semantics for actor handles retrieved from sel… #46524

Closed

8 tasks

anyscalesam mentioned this pull request Jul 15, 2024

[core][serve] 1MB latency performance regression #46428

Closed

jjyao added a commit to jjyao/ray that referenced this pull request Jul 21, 2024

Revert "[core] Weakref semantics for actor handles retrieved from sel…

aaa6364

…f or by name (ray-project#45699)" This reverts commit d729815.

jjyao added a commit to jjyao/ray that referenced this pull request Jul 25, 2024

Revert "Revert "[core] Weakref semantics for actor handles retrieved …

433d812

…from self or by name (ray-project#45699)"" This reverts commit aaa6364.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Weakref semantics for actor handles retrieved from self or by name #45699

[core] Weakref semantics for actor handles retrieved from self or by name #45699

stephanie-wang commented Jun 3, 2024 •

edited

Loading

kevin85421 left a comment

jjyao Jun 4, 2024

kevin85421 Jun 6, 2024

stephanie-wang Jun 6, 2024

kevin85421 Jun 6, 2024

kevin85421 Jun 6, 2024

stephanie-wang Jun 6, 2024

kevin85421 Jun 6, 2024

stephanie-wang Jun 6, 2024

kevin85421 Jun 6, 2024

stephanie-wang Jun 6, 2024

kevin85421 Jun 6, 2024

stephanie-wang Jun 6, 2024

stephanie-wang Jun 6, 2024

kevin85421 commented Jun 6, 2024

kevin85421 left a comment

jjyao Jun 7, 2024

stephanie-wang Jun 7, 2024

		@@ -389,7 +389,9 @@ std::string AbstractRayRuntime::DeserializeAndRegisterActorHandle(
		const std::string &serialized_actor_handle) {

[core] Weakref semantics for actor handles retrieved from self or by name #45699

[core] Weakref semantics for actor handles retrieved from self or by name #45699

Conversation

stephanie-wang commented Jun 3, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

kevin85421 left a 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

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

kevin85421 commented Jun 6, 2024

kevin85421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanie-wang commented Jun 3, 2024 •

edited

Loading