Setup ProxyActor Config [1/N] #132
Conversation
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]>
fed/api.py
Outdated
| cross_silo_serializing_allowed_list: Dict = None, | ||
| cross_silo_send_options: Dict = None, |
There was a problem hiding this comment.
cross_silo_send_options equals actor options?
There was a problem hiding this comment.
yes, the "cross_silo" prefix may be more clear to user, what do u think ?
There was a problem hiding this comment.
Naming is ok, but cross_silo_send_options == actor.options is worth discussing.
There was a problem hiding this comment.
At high level, I don't think it's a good idea to allow users setting the options for proxies, as:
It's not that all options should be exposed to users. For instances:
- Why we need expose
name? max_concurrencyis what rayfed should control under the hood.
What I recommend is to expose the specific APIs.
|
+1. |
|
Make sense, I'll use a class to only expose certain options |
Move this to PR description? |
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]> Conflicts: fed/barriers.py
Signed-off-by: paer <[email protected]>
Signed-off-by: paer <[email protected]>
| @@ -131,6 +133,14 @@ def init( | |||
| cross_silo_serializing_allowed_list: The package or class list allowed for | |||
| serializing(deserializating) cross silos. It's used for avoiding pickle | |||
| deserializing execution attack when crossing solis. | |||
| cross_silo_send_resource_label: Customized resource label, the SendProxyActor | |||
There was a problem hiding this comment.
If we are trying to limit the user facing proxy configs, maybe exposing ProxyActorConfig would give us more extensibility？If we would like to add another config, this might look tedious, also, send_max_retries could merge into ProxyActorConfig?
There was a problem hiding this comment.
+1.
And I vote for a name ProxyConfig, which not exposing more details on the ray actor implementation.
There was a problem hiding this comment.
Thx for the advice~ @fengsp @jovany-wang
That's also what I plan to do in [2/N]: merge cross-silo RPC related config into ProxyActorConfig, and then expose it as a parameter of fed.init.
[1/N] is just trying to enable placing the ProxyActor according to its resource label.
@jovany-wang ProxyConfig is indeed a better name when exposing it to users 👍
Sure |
This is the first step of #134
This PR provides
cross_silo_send_resource_labelandcross_silo_recv_resource_labelconfig infed.initand convert them to the "resources" field of ActorClass.options.