-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[client][placement groups] Client placement group hooks, attempt #3 #15382
Changes from 18 commits
4f46960
11a9460
c403968
ca500f0
15979b2
82dc004
19baf05
ff10523
d7e6339
1a836ee
d78fd69
caaec1c
714ff7d
77be665
fe465bf
86c62a7
2753a83
5d456f1
5351eba
663837d
b9a0054
3b15e1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,26 @@ def client_mode_should_convert(): | |
return client_mode_enabled and _client_hook_enabled | ||
|
||
|
||
def client_mode_wrap(func): | ||
"""Decorator to wrap a function call in a task. | ||
|
||
This is useful for functions where the goal isn't to delegate | ||
module calls to the ray client equivalent, but to instead implement | ||
ray client features that can be executed by tasks on the server side. | ||
""" | ||
from ray.util.client import ray | ||
|
||
@wraps(func) | ||
def wrapper(*args, **kwargs): | ||
if client_mode_should_convert(): | ||
f = ray.remote(num_cpus=0)(func) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still concerned about why we need to submit these tasks to the cluster, it just seems kinda unnecessary and pretty decent chunk of overhead/complication. I understand that we can't run these functions on the client, but the client server is a driver in the cluster so it should be fine right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The goal of this PR is to make public placement group APIs available on the client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So a user should be able to do the following locally import ray
from ray.util.placement_group import placement_group
ray.util.connect(...)
pg = placement_group(...)
... Currently, that would throw an error here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wuisawesome I agree that this is slower than having a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wuisawesome Is this ok to resolve for now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this seem like an abuse of Client tasks? Is this iffiness of this that tasks should be used for heavy computations There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess there's a reason There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There is definitely more overhead with submitting a Ray task (as opposed to a RPC call). That being said I think the extra overhead isn't that big of a deal given that if people are trying to create many, many placement groups, the real bottleneck will all likelihood be in waiting for the cluster to scale to the appropriate size to support all the placement groups.
For computationally light tasks. The functions being wrapped are either |
||
ref = f.remote(*args, **kwargs) | ||
return ray.get(ref) | ||
return func(*args, **kwargs) | ||
|
||
return wrapper | ||
|
||
|
||
def client_mode_convert_function(func_cls, in_args, in_kwargs, **kwargs): | ||
"""Runs a preregistered ray RemoteFunction through the ray client. | ||
|
||
|
@@ -80,7 +100,10 @@ def client_mode_convert_function(func_cls, in_args, in_kwargs, **kwargs): | |
from ray.util.client import ray | ||
|
||
key = getattr(func_cls, RAY_CLIENT_MODE_ATTR, None) | ||
if key is None: | ||
|
||
# Second part of "or" is needed in case func_cls is reused between Ray | ||
# client sessions in one Python interpreter session. | ||
if (key is None) or (not ray._converted_key_exists(key)): | ||
key = ray._convert_function(func_cls) | ||
setattr(func_cls, RAY_CLIENT_MODE_ATTR, key) | ||
client_func = ray._get_converted(key) | ||
|
@@ -98,7 +121,9 @@ def client_mode_convert_actor(actor_cls, in_args, in_kwargs, **kwargs): | |
from ray.util.client import ray | ||
|
||
key = getattr(actor_cls, RAY_CLIENT_MODE_ATTR, None) | ||
if key is None: | ||
# Second part of "or" is needed in case actor_cls is reused between Ray | ||
# client sessions in one Python interpreter session. | ||
if (key is None) or (not ray._converted_key_exists(key)): | ||
key = ray._convert_actor(actor_cls) | ||
setattr(actor_cls, RAY_CLIENT_MODE_ATTR, key) | ||
client_actor = ray._get_converted(key) | ||
|
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.
I'm having a little trouble parsing this doc string. So is this meant for ray client's internal functionality that needs to run on the cluster?
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.
Correct. I think the doc string is supposed to distinguish this wrapper from
client_mode_hook
which delegates public ray APIs to Ray client's RayApiStubI'll update the doc string.
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.
Let me know if the new doc string is clearer.