Change actor.__ray_kill__() to ray.kill(actor)#7360
Change actor.__ray_kill__() to ray.kill(actor)#7360edoakes merged 3 commits intoray-project:masterfrom
Conversation
|
Can one of the admins verify this patch? |
|
Test FAILed. |
|
cc @mitar |
pcmoritz
left a comment
There was a problem hiding this comment.
LGTM but notice the comment I left
|
Test FAILed. |
| you can call ``actor.__ray_terminate__.remote()`` instead to queue a | ||
| termination task. | ||
|
|
||
| If this actor is reconstructable, it will be attempted to be reconstructed. |
There was a problem hiding this comment.
This behavior sounds right to me. Just saw this PR which might be doing the opposite (though I didn't check) #7346.
There was a problem hiding this comment.
Can this be optional? Like an argument to kill? Or maybe ray.restart(actor) when you want to reconstruct it? I find it strange for this to happen automatically?
| immediately. Any atexit handlers installed in the actor will still be run. | ||
|
|
||
| If you want to kill the actor but let pending tasks finish, | ||
| you can call ``actor.__ray_terminate__.remote()`` instead to queue a |
There was a problem hiding this comment.
Egh, can we also rename __ray_terminate__? This is really not a good idea to use double underscores. Those are reserved for Python use.
There was a problem hiding this comment.
Maybe the API should be ray.terminate(actor) and ray.kill(actor). It is OK then for ray.terminate to call actor.__ray_terminate__ and for subclasses of actors to subclass __ray_terminate__ to customize its behavior. That would be aligned with Python best practices. But public API should never call __ray_terminate__ directly.
There was a problem hiding this comment.
Yes, but let's discuss/do that separately.
Why are these changes needed?
Double-underscore methods are generally reserved for builtin keywords.
Related issue number
Checks
scripts/format.shto lint the changes in this PR.