Skip to content
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

feat: add ReceiverSenderProxy. #168

Merged
merged 5 commits into from
Aug 4, 2023

Conversation

zhouaihui
Copy link
Member

  • Add ReceiverSendProxyClass for bidirectional communication, e.g.g SecretFlow link.
  • Make proxy config in dict works.
  • Remove listening_address since it can be replaced by addresses param in fed.init

Copy link
Collaborator

@fengsp fengsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not get this, why are we adding a ReceiverSendProxy, what is the difference with seperated actors and do we have to keep both of them?

global _SENDER_RECEIVER_PROXY_ACTOR
global _RECEIVER_PROXY_ACTOR_NAME
_SENDER_RECEIVER_PROXY_ACTOR = SenderReceiverProxyActor.options(
name=_RECEIVER_PROXY_ACTOR_NAME, **actor_options
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is named with receiver, how send get the SendProxyActor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When SenderReceiverProxy is enabled, actor names of sender and receiver are same.

@zhouaihui
Copy link
Member Author

I do not get this, why are we adding a ReceiverSendProxy

Since some communication backends are bidirectional, meaning that the proxy can do sending/receiving, e.g. SecretFlow Link, grpc streaming. Now we need to apply SecretFlow link to RayFed. That's why we need this PR, though it's not graceful .

do we have to keep both of them.

Yea, the default is separated actors still. RayFed does not provide ReceiverSendProxy implementation.

Copy link
Collaborator

@NKcqx NKcqx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhouaihui
Copy link
Member Author

Some ut is flaky, i will open a new PR to fix #153 (comment)

@zhouaihui zhouaihui merged commit 60ab909 into ray-project:main Aug 4, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants