[Multi-Job][2/N] Make proxy's RPC framework replaceable #140

NKcqx · 2023-07-04T02:36:49Z

[2/N] in #134

Union cross-silo rpc related config into one Config. This can do good for multi-job config management.
Make it a parameter infed.init API to reduce tha amount of RPC related config parameters.

Signed-off-by: NKcqx <[email protected]>

fed/config.py

fed/api.py

fed/config.py

zhouaihui · 2023-07-11T02:09:40Z

Could you at me when this is ready for review again @NKcqx

fengsp · 2023-07-11T02:58:34Z

fed/api.py

-            will be used. More details please refer to
-            `retry-policy <https://github.com/grpc/proposal/blob/master/A6-client-retries.md#retry-policy>`_. # noqa
-
+        enable_waiting_for_other_parties_ready: ping other parties until they


This does not match the code

Not quite understand.
If you're saying the functionality, I think it doesn't belong to the scope of this PR.

fed/config.py

fed/proxy/barriers.py

fed/config.py

Signed-off-by: paer <[email protected]>

fed/api.py

Signed-off-by: paer <[email protected]>

zhouaihui · 2023-07-13T12:38:46Z

fed/proxy/grpc_proxy.py

+        server.add_secure_port(f'[::]:{port}', server_credentials)
+    else:
+        server.add_insecure_port(f'[::]:{port}')
+        # server.add_insecure_port(f'[::]:{port}')


Remove this # server.add_insecure_port(f'[::]:{port}')

zhouaihui

Nice work. General LGTM.

NKcqx · 2023-07-14T03:23:05Z

The UT failure is not related to this PR, has filed an issue #153

jovany-wang

This is a great PR.

I left some high level comments, and I need more time to review to deep into dive.

jovany-wang · 2023-07-14T03:28:25Z

fed/_private/constants.py

-KEY_OF_CROSS_SILO_MESSAGES_MAX_SIZE_IN_BYTES = "CROSS_SILO_MESSAGES_MAX_SIZE_IN_BYTES" # noqa
-
-KEY_OF_CROSS_SILO_TIMEOUT_IN_SECONDS = "CROSS_SILO_TIMEOUT_IN_SECONDS"
+KEY_OF_CROSS_SILO_COMM_CONFIG = "CROSS_SILO_COMM_CONFIG"


Suggested change

KEY_OF_CROSS_SILO_COMM_CONFIG = "CROSS_SILO_COMM_CONFIG"

KEY_OF_CROSS_SILO_COMMON_CONFIG = "CROSS_SILO_COMMON_CONFIG"

jovany-wang · 2023-07-14T03:37:24Z

fed/_private/serialization_utils.py

@@ -63,7 +63,8 @@ def find_class(self, module, name):
 def _apply_loads_function_with_whitelist():
    global _pickle_whitelist

-    _pickle_whitelist = fed_config.get_cluster_config().serializing_allowed_list
+    _pickle_whitelist = fed_config.get_job_config() \
+        .cross_silo_comm_config.serializing_allowed_list


Should this be job config?
@zhouaihui @fengsp CC

Now that we separate ray.init from fed.init, there's no way to reach the cluster-level information, since each fed.init starts and only starts a job session.

Unless there's a global actor (or service job) that can break the job isolation and filter each job's tasks' invalid param type.

If we don't have the cluster config, why we not just use config? The question that we should answer before it getting finalized is whether we need the cluster config in the future at high level.

Let's discuss in #156

jovany-wang · 2023-07-14T03:40:39Z

fed/api.py

 )
+from fed.config import CrossSiloCommConfig


I believe the more native name is CommonCrossSiloConfig

"Comm" is short for "Communication" 😅

😅 So that we should even more not use it as it's confusing though in this conversation.

Rename to CrossSiloMsgConfig and GrpcCrossSiloMsgConfig, any other name suggestions?

proposing CrossSiloMessageConfig

fed/api.py

jovany-wang · 2023-07-14T03:42:46Z

fed/api.py

-        resource_label=cross_silo_recv_resource_label)
+    if recv_proxy_cls is None:
+        from fed.proxy.grpc_proxy import GrpcRecvProxy
+        recv_proxy_cls = GrpcRecvProxy
    # Start recv proxy
    start_recv_proxy(


It's not related to this PR changes, but start_recv_proxy function should be a private one.

fed/proxy/barriers.py

fed/proxy/grpc_proxy.py

jovany-wang · 2023-07-14T03:59:32Z

BTW, could you please update the PR title and the description? They're not match to your PR content.

fed/config.py

jovany-wang

I left some minor comments.

This PR looks good to me overall. I think it's time to merge it and then the remaining issues could be addressed in the next coming PRs:

Making it easy to support plugin a new communication component. For instance, I don't think grpc_config should be defined in fed/config as it's hard to plugin.
Removing cluster config concept, and use config as the name for job level config.
Defining how we could pass the cluster config enabling for all of the jobs.

jovany-wang · 2023-07-18T06:35:44Z

fed/_private/constants.py

-KEY_OF_CROSS_SILO_MESSAGES_MAX_SIZE_IN_BYTES = "CROSS_SILO_MESSAGES_MAX_SIZE_IN_BYTES" # noqa
-
-KEY_OF_CROSS_SILO_TIMEOUT_IN_SECONDS = "CROSS_SILO_TIMEOUT_IN_SECONDS"
+KEY_OF_CROSS_SILO_MSG_CONFIG = "CROSS_SILO_MSG_CONFIG"


Suggested change

KEY_OF_CROSS_SILO_MSG_CONFIG = "CROSS_SILO_MSG_CONFIG"

KEY_OF_CROSS_SILO_MESSAGE_CONFIG = "CROSS_SILO_MESSAGE_CONFIG"

jovany-wang · 2023-07-18T06:37:30Z

fed/_private/serialization_utils.py

@@ -63,7 +63,8 @@ def find_class(self, module, name):
 def _apply_loads_function_with_whitelist():
    global _pickle_whitelist

-    _pickle_whitelist = fed_config.get_cluster_config().serializing_allowed_list
+    _pickle_whitelist = fed_config.get_job_config() \


As discussed, we should rename the function to get_config(). But we could defer it later.

jovany-wang · 2023-07-18T06:38:00Z

fed/_private/serialization_utils.py

@@ -63,7 +63,8 @@ def find_class(self, module, name):
 def _apply_loads_function_with_whitelist():
    global _pickle_whitelist

-    _pickle_whitelist = fed_config.get_cluster_config().serializing_allowed_list
+    _pickle_whitelist = fed_config.get_job_config() \
+        .cross_silo_msg_config.serializing_allowed_list


Suggested change

.cross_silo_msg_config.serializing_allowed_list

.cross_silo_message_config.serializing_allowed_list

jovany-wang · 2023-07-18T06:40:05Z

fed/api.py

@@ -34,6 +34,8 @@
    start_recv_proxy,
    start_send_proxy,
 )
+from fed.proxy.grpc.grpc_proxy import SendProxy, RecvProxy
+from fed.config import CrossSiloMsgConfig


Suggested change

from fed.config import CrossSiloMsgConfig

from fed.config import CrossSiloMessageConfig

jovany-wang · 2023-07-18T06:42:02Z

fed/api.py

        enable_waiting_for_other_parties_ready: ping other parties until they
            are all ready if True.
-        grpc_metadata: optional; The metadata sent with the grpc request. This won't override
-            basic tcp headers, such as `user-agent`, but aggregate them together.
+        global_cross_silo_msg_config: Global cross-silo message related


Sorry for that I still don't get what the global mean here.

Because the cluster parameter in fed.init supports setting msg config for each party.
The "global" here indicates that the config applies to each party, and can be overridden by the party-specific config declared in cluster.

jovany-wang · 2023-07-18T06:44:40Z

fed/api.py

-        resource_label=cross_silo_recv_resource_label)
+    if recv_proxy_cls is None:
+        logger.debug(
+            "Not declaring recver proxy class, using `GrpcRecvProxy` as default.")


There is no receiver proxy class specified, it uses `GrpcRecvProxy` by default.

more native?

jovany-wang · 2023-07-18T06:51:04Z

fed/config.py

@@ -48,8 +39,10 @@ def __init__(self, raw_bytes: bytes) -> None:
            self._data = cloudpickle.loads(raw_bytes)

    @property
-    def grpc_metadata(self):
-        return self._data.get(fed_constants.KEY_OF_GRPC_METADATA, {})
+    def cross_silo_msg_config(self):


Suggested change

def cross_silo_msg_config(self):

def cross_silo_message_config(self):

jovany-wang · 2023-07-18T06:51:17Z

fed/config.py

@@ -80,14 +73,94 @@ def get_job_config():
    return _job_config


-class ProxyActorConfig:
+@dataclass
+class CrossSiloMsgConfig:


Suggested change

class CrossSiloMsgConfig:

class CrossSiloMessageConfig:

jovany-wang · 2023-07-18T06:54:39Z

fed/proxy/base_proxy.py

+from fed.config import CrossSiloMsgConfig
+
+
+class SendProxy(abc.ABC):


Should we rename these to SenderProxy and ReceiverProxy?

This is the following up #140 , to address the naming issues. And it's also the part of #44 --------- Signed-off-by: Qing Wang <[email protected]>

NKcqx added 3 commits June 26, 2023 12:06

tmp save

c8dd31f

union rpc related config into one

ef842a9

Signed-off-by: NKcqx <[email protected]>

union config in KV

ccf7d51

Signed-off-by: NKcqx <[email protected]>

NKcqx added the enhancement New feature or request label Jul 4, 2023

NKcqx requested review from fengsp and a team July 4, 2023 02:36

update docstr

a046d27

Signed-off-by: NKcqx <[email protected]>

fengsp reviewed Jul 4, 2023

View reviewed changes

fed/config.py Outdated Show resolved Hide resolved

fed/api.py Outdated Show resolved Hide resolved

zhouaihui reviewed Jul 4, 2023

View reviewed changes

fed/config.py Show resolved Hide resolved

zhouaihui reviewed Jul 4, 2023

View reviewed changes

fed/config.py Outdated Show resolved Hide resolved

NKcqx and others added 9 commits July 10, 2023 15:41

tmp save

aa397a8

fix conflicts

863da5f

fix conflicts

40cec65

fix conflicts

ed18a07

Pluggable cross_silo rpc impl

51e4e70

parameter passng bugs

8ff5658

lint codes

582f523

default val for Grpc config

cdc1996

default val for Grpc config

b5a3f08

fengsp reviewed Jul 11, 2023

View reviewed changes