Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion python/ray/experimental/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
flush_redis_unsafe, flush_task_and_object_metadata_unsafe,
flush_finished_tasks_unsafe, flush_evicted_objects_unsafe,
_flush_finished_tasks_unsafe_shard, _flush_evicted_objects_unsafe_shard)
from .named_actors import (
get_actor, register_actor
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this can all fit on one line without the parentheses


__all__ = [
"TensorFlowVariables", "flush_redis_unsafe",
"flush_task_and_object_metadata_unsafe", "flush_finished_tasks_unsafe",
"flush_evicted_objects_unsafe", "_flush_finished_tasks_unsafe_shard",
"_flush_evicted_objects_unsafe_shard"
"_flush_evicted_objects_unsafe_shard", "get_actor", "register_actor"
]
34 changes: 34 additions & 0 deletions python/ray/experimental/named_actors.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reserve assertions for checking things that should never actually happen. The errors (in both register_actor and get_actor) can happen if the user passes in the wrong values, so we should raise some sort of exception

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled.

import ray
import ray.cloudpickle as pickle
"""
This file contains functions intended to implement the named actor
"""

def _calculate_key_(name):
return b"Actor:" + str.encode(name)

def get_actor(name):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add docstrings for these methods with the format used e.g., in

ray/python/ray/worker.py

Lines 2443 to 2458 in 4584193

def get(object_ids, worker=global_worker):
"""Get a remote object or a list of remote objects from the object store.
This method blocks until the object corresponding to the object ID is
available in the local object store. If this object is not in the local
object store, it will be shipped from an object store that has it (once the
object has been created). If object_ids is a list, then the objects
corresponding to each object in the list will be returned.
Args:
object_ids: Object ID of the object to get or a list of object IDs to
get.
Returns:
A Python object or a list of Python objects.
"""

worker = ray.worker.get_global_worker()
actor_hash = _calculate_key_(name)
pickled_state = worker.redis_client.hmget(actor_hash, name)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use hget and then remove the assertion below.

assert len(pickled_state) == 1, \
"Error: Multiple actors under this name."
assert pickled_state[0] is not None, \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should raise ValueError

"Error: actor with name {} doesn't exist".format(name)
handle = pickle.loads(pickled_state[0])
return handle

def register_actor(name, actor_handle):
worker = ray.worker.get_global_worker()
actor_hash = _calculate_key_(name)
assert type(actor_handle) == ray.actor.ActorHandle, \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should raise TypeError

"Error: you could only store named-actors."
is_existed = worker.redis_client.hexists(actor_hash, name)
assert not is_existed, \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should raise ValueError

"Error: the actor with name={} already exists".format(name)
pickled_state = pickle.dumps(actor_handle)
worker.redis_client.hmset(actor_hash, {name: pickled_state})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a race condition with this approach. It's possible that another worker could add publish a named actor with the same name between the calls to hexists and hmset. You can avoid this with hsetnx. See https://redis.io/commands/hsetnx.

30 changes: 30 additions & 0 deletions test/actor_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1906,6 +1906,36 @@ def method(self):
# we should also test this from a different driver.
ray.get(new_f.method.remote())

def testRegisterAndGetActorHandle(self):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we change this to testRegisterAndGetNamedActors? It'll be easier to find by searching

# TODO(heyucongtom): We may want to test this from another driver?
# One viable way might be setting up another ray process here, connecting to the
# redis_address, sending the objectID back, and compare

ray.worker.init(num_workers=1)

@ray.remote
class Foo(object):
def method(self):
pass

f1 = Foo.remote()
# Test saving f
ray.experimental.register_actor("f1", f1)
# Test getting f
f2 = ray.experimental.get_actor("f1")
self.assertEqual(f1._actor_id, f2._actor_id)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's modify the actor to be something like

def method(self):
    self.x += 1
    return self.x

so that here we can do

self.assertEqual(ray.get(f1.method.remote(), 1)
self.assertEqual(ray.get(f2.method.remote(), 2)
self.assertEqual(ray.get(f1.method.remote(), 3)
self.assertEqual(ray.get(f2.method.remote(), 4)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled.

# Test same name register shall raise error
with self.assertRaises(AssertionError):
ray.experimental.register_actor("f1", f2)

# Test register with wrong object type
with self.assertRaises(AssertionError):
ray.experimental.register_actor("f3", 1)

# Test getting an unexist actor
with self.assertRaises(AssertionError):
err = ray.experimental.get_actor("unexisted")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unexisted -> nonexistent


class ActorPlacementAndResources(unittest.TestCase):
def tearDown(self):
Expand Down