Support super() for actors

[pcmoritz]

With this PR and the cloudpickle master, the following works:

import ray
ray.init()

class A(object):
    pass

@ray.remote
class B(A):
    def __init__(self):
        super(type(self), self).__init__()

B.remote()

Note that this doesn't work yet:

import ray
ray.init()

class A(object):
    pass

@ray.remote
class B(A):
    def __init__(self):
        super(B, self).__init__()

B.remote()

[AmplabJenkins]

Merged build finished. Test FAILed.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/1469/
Test FAILed.

[pcmoritz]

@mehrdadn Do you have any idea how we could make the second syntax supported?

[mehrdadn]

I think I'd first need to know the exact semantics you want before I can try to suggest how to implement this. For example, if we have:

class A(object):
	pass

@foo
@ray.remote
@bar
class B(A):
	def __init__(self):
		super(B, self).__init__()

C = B
B = A

C.remote()

What behavior do you want, for other reasonable decorators foo and bar? Do you have any preference for any behavior? Or is it perhaps entirely undefined?

Also note that the first usage is wrong for any class except the most-derived class in the hierarchy. (i.e. if someone inherited from B then this would break.)

[mehrdadn]

Or I guess an easier way to state my question is: what exactly should B refer to inside its own definition? Does it refer to the class as initially defined, the potentially-pre-decorated class right before @ray.remote is called, the decorated class after @ray.remote is called, the final class after all decorators are called, or whatever global B happens to be at call time?

[robertnishihara]

@mehrdadn good point about how the first usage could break, thanks for bringing that up!

As for the example you gave, I think I would expect B to refer to the B in the line class B(A):.

Oh I see, your point about the decorators. I think I'd still expect it to be B as originally defined, but it it should do whatever Python would normally do, right?

[shaneknapp]

test this please

[AmplabJenkins]

Merged build finished. Test FAILed.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/1470/
Test FAILed.

[mehrdadn]

Ah yeah you're right, I guess as long as the inner decorators aren't doing anything too weird it shouldn't matter.

First, note that it does seem that cloudpickle doesn't support recursion:

import cloudpickle
def pickle_unpickle(obj): return cloudpickle.loads(cloudpickle.dumps(obj))
@pickle_unpickle
def f(x): return f(x - 1) * x if x > 0 else 1
f(1)  # NameError: global name 'f' is not defined

so if I understand this right, given that such functions are working correctly with ray.remote, it means you're working around this somehow (possibly unintentionally) by virtue of the fact that you import remote functions into the global namespace and then run them, so the names are defined at that point.

So you'd need to treat classes the same way, and they should work. I'm looking at the code now to see why they don't, but my guess right now is that classes are being treated very differently from functions.

[robertnishihara]

This is related to #449.

[royf]

I agree with @mehrdadn that super(type(self), self) isn't what you want. Naming the class (or better yet, in Python3, using super()) is significant here.

In python, B refers to class B before any decoration. This indeed creates an issue for replacing decorators (returning a different class than their argument, as opposed to returning the same class modified), which is one of the reasons they should be discouraged. The case of ray.remote is particularly tricky, since the returned class doesn't have the original MRO (it seems to be derived from object), and the foo decorator above won't behave as intended.

Until and unless ray.remote preserves the original MRO, I'd say that decorating an actor is unsupported, so shouldn't matter for this issue. As for bar, if it behaves as expected, then it shouldn't care that B refers to class B — or rather, trust that it does.

[robertnishihara]

@mehrdadn and I just looked into this a bit more.

If you do the following

import ray
ray.init()

class Foo(object):
    pass

@ray.remote
class Bar(Foo):
    def __init__(self):
        super(Bar, self).__init__()

b = Bar.remote()

the constructor task prints the following error.

Traceback (most recent call last):
  File "/Users/rkn/Workspace/ray/python/ray/worker.py", line 1841, in process_task
    worker.actors[task.actor_id().id()], *arguments)
  File "<ipython-input-4-bb5ad440612a>", line 4, in __init__
TypeError: super(type, obj): obj must be an instance or subtype of type

type is Bar and obj.__class__ is NewClass. NewClass should only exist on the driver, but its showing up here because in the above code, the Bar in the line super(Bar, self).__init__() refers to the decorated Bar. That is ray.remote(Bar) returns a NewClass, which is what the Bar in the super call is referring to.

One possible solution is to be more eager and to pickle Bar inside of the remote decorator (which is what we do with remote functions).

[mehrdadn]

@royf Regarding "in Python, B refers to class B before any decoration", did you mean the Ray behavior rather than the Python behavior? Because Python looks up everything dynamically -- if I run the following, for example, it won't refer to the class at all, but to the lambda:

import cloudpickle

def decorate(Class):
	return lambda: Class

@decorate
class Foo(object):
	def __init__(self): print(Foo)

Foo()()

The Ray semantics so far has been to bind things statically at the point of ray.remote call, which seems to be what you meant? I'm not sure whether/how this affects MRO compared to regular code at the moment but the behavior can indeed differ from local execution, though the hope is that for reasonable code it should be correct.

[royf]

@mehrdadn I meant python, but I was wrong.

[shaneknapp]

test this please

[AmplabJenkins]

Merged build finished. Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/1471/
Test PASSed.

[royf]

You guys probably have this figured out, but here's my $0.02.

I like that ray.remote is a decorator, and the common usage has very clean and easy syntax. But the best way to really think about it is as a class factory, returning a new class. For fancy stuff, one should also keep the original class:

1. As we already know, you can inherit from a class intended as an actor. You can do:

   class B(object):
       pass
   
   @ray.remote
   class B_actor(B):
       pass
   
   class C(B):
       pass

   Or more elegantly B_actor = ray.remote(B).
   Any dynamic reference to B, including inside its own body, should be bound statically in B_actor to the value it had when ray.remote was called.
   You should only replace B with its remote version once you know it won't ever be inherited or referenced dynamically again. So if B refers to itself, since this reference lives on inside C, B can't ever be replaced with its remote version.

2. If you have

   @foo
   @bar
   class C(B):
       pass

   and you want to remoteify the version after bar and before foo, I think you can do:

   @bar
   class C(B):
       pass
   
   C_actor = ray.remote(C)
   C = foo(C)

   It makes sense to keep two identifiers because you have two versions of C.
   Again, any dynamic reference to C should be bound statically in C_actor to the value it had after bar.

[robertnishihara]

@royf, you're saying that for some of these more complex use cases, you really need to call ray.remote by hand and save the result with a different name because the normal usage of decorators isn't quite flexible enough, right? I think I agree with that point. I don't see a good way of achieving what you're doing above while using ray.remote as a decorator.

[royf]

@robertnishihara Yes, a class decorator is syntactic sugar for C = decorator(C), and it's specifically the assignment to the same identifier that messes things up if the returned class doesn't have the same duck-type as the argument class.

[robertnishihara]

Closing for now since this has diverged a bunch.