Refactor ActorManager to store underlying remote actors in dict. #29953
Conversation
Signed-off-by: Jun Gong <jungong@anyscale.com>
|
|
||
| @DeveloperAPI | ||
| def clear(self): | ||
| """Clean up managed actors.""" | ||
| while self.__actors: | ||
| del self.__actors[0] | ||
| for actor in self.__actors.values(): |
There was a problem hiding this comment.
Do you unit-test clear? Can you del a value of a dict without deling the key? why not loop through all the keys and do del self.__actors[key]?
There was a problem hiding this comment.
yeah it works. I added it to all the unit test cases.
you can't del keys while looping through a dict though.
>>> for k in a.keys(): del a[k]
...
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
RuntimeError: dictionary changed size during iteration
>>>
deletion is always tricky. you need to make sure you are not holding an iterator while deleting things.
There was a problem hiding this comment.
wait, what? this works for me?
>>> a = {'a': 1, 'b': 2}
>>> for k in a.keys(): del a[k]
There was a problem hiding this comment.
what?! let me double check.
| actor_idx = remote_actor_indices[i] | ||
| for r in ready: | ||
| # Find the corresponding actor ID for this remote call. | ||
| actor_id = remote_actor_ids[remote_calls.index(r)] |
There was a problem hiding this comment.
I don't see index method implemented for RemoteCallResults class. How does this work? what's the run time of finding the index? Should RemoteCallResults() internally be a dict so that the look up is O(1)?
There was a problem hiding this comment.
remote_calls is a List[ObjectRef]. index is an API for python ilst.
I think it's likely ok, we will never have millions of outstanding requests, so probably shouldn't complicate the API for performance reasons.
modern computer is pretty fast ... :)
Signed-off-by: Jun Gong <jungong@anyscale.com>
…-project#29953) Signed-off-by: Jun Gong <jungong@anyscale.com> Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Signed-off-by: Jun Gong jungong@anyscale.com
Why are these changes needed?
To upgrade things in a backward-compatible way, ActorManager needs to support
remove_actor()API, which make it much easier to do if we stored the underlying actor handles in a dict.Also fix a minor issue where None is a legit result for a remote call.
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.