Skip to content
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

[Client] Make dir() work for ClientActorHandle #16157

Merged
merged 8 commits into from
Jun 2, 2021

Conversation

ijrsvt
Copy link
Contributor

@ijrsvt ijrsvt commented May 31, 2021

Why are these changes needed?

  • dir() on a ClientActorHandle does not properly show the methods of the actual Actor's class. It only shows the internal implementation details. This PR override the __dir__() function to return the expected value!

Related issue number

Checks

  • 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 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 :(
      Tested for Client because test_actor.py is run with client mode.

@robertnishihara
Copy link
Collaborator

Nice! Would it be possible to add a test doing something like

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

f = Foo.remote()

method_names = [field for field in dir(f) if not field.startswith('_')]
assert method_names == ['method'], method_names

Not sure if we already have such a test for the non-client case, but the test case makes sense in both cases...

I suppose there are other assertions we could throw in as well to make sure tab completion works well, e.g., for Foo (should have remote, options, and maybe method ...).

@ijrsvt
Copy link
Contributor Author

ijrsvt commented May 31, 2021

@robertnishihara Added tests and ensured that f.method_one.<tab> only yields options and remote!

@ijrsvt ijrsvt changed the title [WIP] Make dir() work for ClientHandle [Client] Make dir() work for ClientActorHandle May 31, 2021
@@ -974,5 +974,30 @@ def get_ref(self):
assert ray.get(inner.ping.remote()) == "pong"


def test_actor_autocomplete(ray_start_regular_shared):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks fantastic! Does this test both the ray.init and ray.client code paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! This test_actor.py is also run against client mode (see here)!

if self._dir:
return self._dir
if ray.is_connected():
self._dir = ray.get(ray.remote(lambda x: dir(x)).remote(self))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Launching a remote function within a tab completion seems wrong to me (imagine if it blocks due to unavailable resources or something).

Looking at how this is done in the ActorHandle class, it seems that the relevant method info is passed into the ActorHandle constructor, so we don't need to fetch it on the fly. Not sure if we have something equivalent here, but we eventually will (probably) in order to do static type checking for actor methods.

ray/python/ray/actor.py

Lines 750 to 918 in 45d2331

class ActorHandle:
"""A handle to an actor.
The fields in this class are prefixed with _ray_ to hide them from the user
and to avoid collision with actor method names.
An ActorHandle can be created in three ways. First, by calling .remote() on
an ActorClass. Second, by passing an actor handle into a task (forking the
ActorHandle). Third, by directly serializing the ActorHandle (e.g., with
cloudpickle).
Attributes:
_ray_actor_language: The actor language.
_ray_actor_id: Actor ID.
_ray_method_decorators: Optional decorators for the function
invocation. This can be used to change the behavior on the
invocation side, whereas a regular decorator can be used to change
the behavior on the execution side.
_ray_method_signatures: The signatures of the actor methods.
_ray_method_num_returns: The default number of return values for
each method.
_ray_actor_method_cpus: The number of CPUs required by actor methods.
_ray_original_handle: True if this is the original actor handle for a
given actor. If this is true, then the actor will be destroyed when
this handle goes out of scope.
_ray_is_cross_language: Whether this actor is cross language.
_ray_actor_creation_function_descriptor: The function descriptor
of the actor creation task.
"""
def __init__(self,
language,
actor_id,
method_decorators,
method_signatures,
method_num_returns,
actor_method_cpus,
actor_creation_function_descriptor,
session_and_job,
original_handle=False):
self._ray_actor_language = language
self._ray_actor_id = actor_id
self._ray_original_handle = original_handle
self._ray_method_decorators = method_decorators
self._ray_method_signatures = method_signatures
self._ray_method_num_returns = method_num_returns
self._ray_actor_method_cpus = actor_method_cpus
self._ray_session_and_job = session_and_job
self._ray_is_cross_language = language != Language.PYTHON
self._ray_actor_creation_function_descriptor = \
actor_creation_function_descriptor
self._ray_function_descriptor = {}
if not self._ray_is_cross_language:
assert isinstance(actor_creation_function_descriptor,
PythonFunctionDescriptor)
module_name = actor_creation_function_descriptor.module_name
class_name = actor_creation_function_descriptor.class_name
for method_name in self._ray_method_signatures.keys():
function_descriptor = PythonFunctionDescriptor(
module_name, method_name, class_name)
self._ray_function_descriptor[
method_name] = function_descriptor
method = ActorMethod(
self,
method_name,
self._ray_method_num_returns[method_name],
decorator=self._ray_method_decorators.get(method_name))
setattr(self, method_name, method)
def __del__(self):
# Mark that this actor handle has gone out of scope. Once all actor
# handles are out of scope, the actor will exit.
worker = ray.worker.global_worker
if worker.connected and hasattr(worker, "core_worker"):
worker.core_worker.remove_actor_handle_reference(
self._ray_actor_id)
def _actor_method_call(self,
method_name,
args=None,
kwargs=None,
name="",
num_returns=None):
"""Method execution stub for an actor handle.
This is the function that executes when
`actor.method_name.remote(*args, **kwargs)` is called. Instead of
executing locally, the method is packaged as a task and scheduled
to the remote actor instance.
Args:
method_name: The name of the actor method to execute.
args: A list of arguments for the actor method.
kwargs: A dictionary of keyword arguments for the actor method.
name (str): The name to give the actor method call task.
num_returns (int): The number of return values for the method.
Returns:
object_refs: A list of object refs returned by the remote actor
method.
"""
worker = ray.worker.global_worker
args = args or []
kwargs = kwargs or {}
if self._ray_is_cross_language:
list_args = cross_language.format_args(worker, args, kwargs)
function_descriptor = \
cross_language.get_function_descriptor_for_actor_method(
self._ray_actor_language,
self._ray_actor_creation_function_descriptor, method_name)
else:
function_signature = self._ray_method_signatures[method_name]
if not args and not kwargs and not function_signature:
list_args = []
else:
list_args = signature.flatten_args(function_signature, args,
kwargs)
function_descriptor = self._ray_function_descriptor[method_name]
if worker.mode == ray.LOCAL_MODE:
assert not self._ray_is_cross_language,\
"Cross language remote actor method " \
"cannot be executed locally."
object_refs = worker.core_worker.submit_actor_task(
self._ray_actor_language, self._ray_actor_id, function_descriptor,
list_args, name, num_returns, self._ray_actor_method_cpus)
if len(object_refs) == 1:
object_refs = object_refs[0]
elif len(object_refs) == 0:
object_refs = None
return object_refs
def __getattr__(self, item):
if not self._ray_is_cross_language:
raise AttributeError(f"'{type(self).__name__}' object has "
f"no attribute '{item}'")
if item in ["__ray_terminate__", "__ray_checkpoint__"]:
class FakeActorMethod(object):
def __call__(self, *args, **kwargs):
raise TypeError(
"Actor methods cannot be called directly. Instead "
"of running 'object.{}()', try 'object.{}.remote()'.".
format(item, item))
def remote(self, *args, **kwargs):
logger.warning(f"Actor method {item} is not "
"supported by cross language.")
return FakeActorMethod()
return ActorMethod(
self,
item,
ray_constants.
# Currently, we use default num returns
DEFAULT_ACTOR_METHOD_NUM_RETURN_VALS,
# Currently, cross-lang actor method not support decorator
decorator=None)
# Make tab completion work.
def __dir__(self):
return self._ray_method_signatures.keys()

@robertnishihara
Copy link
Collaborator

The test looks great! I left a comment about the implementation. It'd be great to get another pair of eyes on that :)

Copy link
Contributor

@AmeerHajAli AmeerHajAli left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this!
Also thanks Robert for reviewing this.

return self._dir
if ray.is_connected():

@client_mode_wrap
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we guaranteed to be in client mode here?
If so, I think it would be clearer to directly use a cpu=0 ray task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense!

Copy link
Contributor

@DmitriGekhtman DmitriGekhtman left a comment

Choose a reason for hiding this comment

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

looks good, minor comment/question

@robertnishihara
Copy link
Collaborator

robertnishihara commented Jun 2, 2021

Calling a remote function and ray.get inside of __dir__ still feels like an unreasonable thing to do. Is there another way to make sure the handle has the appropriate information?

What if the actor hasn't been created yet or is blocked in some long-running method invocation? These are just some examples. Tab completion should be a really lightweight thing and not involve RPCs.

@ijrsvt
Copy link
Contributor Author

ijrsvt commented Jun 2, 2021

@robertnishihara I added more caching, but an RPC will always be necessary for some situations (detached actors are a major example).

In cases where the actor is created locally, i.e. via .options().remote() or via .remote() the methods will be pulled directly from the definition.

@ijrsvt ijrsvt merged commit 513196d into ray-project:master Jun 2, 2021
@ijrsvt ijrsvt deleted the dir-for-actor branch June 2, 2021 21:51
@robertnishihara
Copy link
Collaborator

@ijrsvt can you explain why some RPC will always be necessary for some situations? This is not the case outside of client mode (including with detached actors).

Look at how it works for the non-client case, there is no RPC

ray/python/ray/actor.py

Lines 916 to 918 in 45d2331

# Make tab completion work.
def __dir__(self):
return self._ray_method_signatures.keys()

Why can't we just treat the method names as part of the actor handle metadata and pass it around with the actor handle?

@ijrsvt
Copy link
Contributor Author

ijrsvt commented Jun 5, 2021

@robertnishihara I'm talking about the following case:

# Client One
ray.client(...).connect():
@ray.remote
class ActorOne
  def f1(self):
    pass

ActorOne.options(name="act1", lifetime="detached").remote()
# Client Two
ray.client(...).connect():
act1 = ray.get_actor("act1")
dir(act1)

@robertnishihara
Copy link
Collaborator

robertnishihara commented Jun 6, 2021

@ijrsvt what I would expect in this case is that when client 2 calls ray.get_actor("act1"), it goes to the GCS to find the relevant actor info, which includes all of the method names, so yes, there's an RPC, but it's bundled with the ray.get_actor call (and goes to the GCS, not the actor).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants