-
Notifications
You must be signed in to change notification settings - Fork 328
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
[Ray] Ray client channel get recv when first complied #2740
[Ray] Ray client channel get recv when first complied #2740
Conversation
Merge branch fix_channel_hang of git@gitlab.alipay-inc.com:ray-project/mars.git into master https://code.alipay.com/ray-project/mars/pull_requests/36 Signed-off-by: 慕白 <chaokun.yck@antgroup.com> * fix hang * revert * fix import error
We use the latest version of black(22.1.0) to format the code, could you please format your code first? |
sure! |
@chaokunyang @fyrestone This PR seems ready to review, could u plz take a look at it? |
self._todo.remove(future) | ||
self._done.put_nowait(future) | ||
|
||
future = asyncio.ensure_future(handle_task(message, object_ref)) |
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.
ensure_future
will create an asyncio.Task, which may have some cost. Wonder whether we should use direct ray call for ray channel, like we did in #2690
Since #2690 break the abstraction of oscar actor pool, and is not merged until now, I think we can merge this first, and discuss whether we should use dirsct call in #2690. @Catch-Bull @qinxuye @wjsi Any ideas? |
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.
LGTM
As now intra-process actor calls are made into direct calls, and #2690 still a WIP (also seems not a very huge PR), personally I would like to merge right now. Try resolving conflicts and rerun checks. |
…plied # Conflicts: # mars/oscar/backends/ray/communication.py
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.
LGTM
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.
LGTM
What do these changes do?
Ray client channel get return of function
recv
when first complied, This PR can improve efficiency and avoid some potential hang risksRelated issue number
Fixes #xxxx
Check code requirements