-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[data] Remove ray.kill in ActorPoolMapOperator #47752
Conversation
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
del self._num_tasks_in_flight[actor] | ||
del self._actor_locations[actor] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a note that we intend to refactor this with the pause API at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raulchen can you please link corresponding ticket
assert actor not in self._num_tasks_in_flight | ||
assert actor not in self._actor_locations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we're asserting after modified the state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the assert is for a different dict.
it's to make sure the actor handle is not being ref'ed by any of these 3 dicts
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
With ray-project#47396, now Ray Core can automatically restarts an actor when needing to resubmit tasks. This doesn't work with `ray.kill`-ed actors. This PR removes ray.kill and let ref counting garbage-collect the actors. --------- Signed-off-by: Hao Chen <[email protected]> Signed-off-by: ujjawal-khare <[email protected]>
#47752 introduced a minor, silent issue in test_map. I stumbled across it because a linter I'm using complained.
Why are these changes needed?
With #47396, now Ray Core can automatically restarts an actor when needing to resubmit tasks.
This doesn't work with
ray.kill
-ed actors.This PR removes ray.kill and let ref counting garbage-collect the actors.
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.