-
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
[Serve][Deployment Graph] Let .bind return ray DAGNode types and remove exposing DeploymentNode as public #24065
Changes from 10 commits
b25e5f5
c435936
a11ae9b
c1dba9f
76144f5
cc4ba55
3c4fd2d
efa7535
4097a4b
33e0c75
b8f3cf9
46a138b
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 |
---|---|---|
|
@@ -79,6 +79,12 @@ def _contains_input_node(self) -> bool: | |
return False | ||
|
||
def __getattr__(self, method_name: str): | ||
# User trying to call .bind() without a bind class method | ||
if method_name == "bind" and "bind" not in dir(self._body): | ||
raise AttributeError( | ||
f".bind() cannot be used again on {type(self)} " | ||
f"(args: {self.get_args()}, kwargs: {self.get_kwargs()})." | ||
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. why print args and kwargs here? Doesn't seem relevant and might be spammy. |
||
) | ||
# Raise an error if the method is invalid. | ||
getattr(self._body, method_name) | ||
call_node = _UnboundClassMethodNode(self, method_name) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
import json | ||
from ray.experimental.dag.class_node import ClassNode | ||
from ray.experimental.dag.function_node import FunctionNode | ||
from ray.experimental.dag import DAGNode | ||
from ray.experimental.dag.class_node import ClassNode # noqa: F401 | ||
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. these imports happen at deployment_graph level so from api.py we're just importing from serve module, rather than directly exposing Ideally we should have another effort to cleanup imports while eliminating circular imports. |
||
from ray.experimental.dag.function_node import FunctionNode # noqa: F401 | ||
from ray.experimental.dag.input_node import InputNode # noqa: F401 | ||
from ray.experimental.dag import DAGNode # noqa: F401 | ||
from ray.util.annotations import PublicAPI | ||
|
||
|
||
|
@@ -14,7 +15,9 @@ class RayServeDAGHandle: | |
""" | ||
|
||
def __init__(self, dag_node_json: str) -> None: | ||
from ray.serve.pipeline.json_serde import dagnode_from_json | ||
|
||
self.dagnode_from_json = dagnode_from_json | ||
self.dag_node_json = dag_node_json | ||
|
||
# NOTE(simon): Making this lazy to avoid deserialization in controller for now | ||
|
@@ -31,57 +34,8 @@ def __reduce__(self): | |
return RayServeDAGHandle._deserialize, (self.dag_node_json,) | ||
|
||
def remote(self, *args, **kwargs): | ||
from ray.serve.pipeline.json_serde import dagnode_from_json | ||
|
||
if self.dag_node is None: | ||
self.dag_node = json.loads( | ||
self.dag_node_json, object_hook=dagnode_from_json | ||
self.dag_node_json, object_hook=self.dagnode_from_json | ||
) | ||
return self.dag_node.execute(*args, **kwargs) | ||
|
||
|
||
@PublicAPI(stability="alpha") | ||
class DeploymentMethodNode(DAGNode): | ||
"""Represents a method call on a bound deployment node. | ||
These method calls can be composed into an optimized call DAG and passed | ||
to a "driver" deployment that will orchestrate the calls at runtime. | ||
This class cannot be called directly. Instead, when it is bound to a | ||
deployment node, it will be resolved to a DeployedCallGraph at runtime. | ||
""" | ||
|
||
# TODO (jiaodong): Later unify and refactor this with pipeline node class | ||
pass | ||
|
||
|
||
@PublicAPI(stability="alpha") | ||
class DeploymentNode(ClassNode): | ||
"""Represents a deployment with its bound config options and arguments. | ||
The bound deployment can be run using serve.run(). | ||
A bound deployment can be passed as an argument to other bound deployments | ||
to build a deployment graph. When the graph is deployed, the | ||
bound deployments passed into a constructor will be converted to | ||
RayServeHandles that can be used to send requests. | ||
Calling deployment.method.bind() will return a DeploymentMethodNode | ||
that can be used to compose an optimized call graph. | ||
""" | ||
|
||
# TODO (jiaodong): Later unify and refactor this with pipeline node class | ||
def bind(self, *args, **kwargs): | ||
"""Bind the default __call__ method and return a DeploymentMethodNode""" | ||
return self.__call__.bind(*args, **kwargs) | ||
|
||
|
||
@PublicAPI(stability="alpha") | ||
class DeploymentFunctionNode(FunctionNode): | ||
"""Represents a serve.deployment decorated function from user. | ||
It's the counterpart of DeploymentNode that represents function as body | ||
instead of class. | ||
""" | ||
|
||
pass |
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.
@ericl addressed, now we don't have the extra
.bind()
on@serve.deployment
, consistent with Ray Core .bind now.Only little nit i did here is to detect and surface the same DAGNode base exception message when user tries to
Actor.bind().bind()
, that previously we only surfacedtype object 'Actor' has no attribute 'bind'