[Serve][Deployment Graph] Let .bind return ray DAGNode types and remove exposing DeploymentNode as public #24065
Conversation
| 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.
these imports happen at deployment_graph level so from api.py we're just importing from serve module, rather than directly exposing experimental folder stuff.
Ideally we should have another effort to cleanup imports while eliminating circular imports.
ericl
added
the
@author-action-required
| @@ -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): | |||
There was a problem hiding this comment.
@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 surfaced type object 'Actor' has no attribute 'bind'
jiaodong
removed
the
@author-action-required
jiaodong
added
tests-ok
| 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.
why print args and kwargs here? Doesn't seem relevant and might be spammy.
|
@jiaodong please fix error message before we merge |
Why are these changes needed?
See dag layering summary in #24061
We need to cleanup and set right ray dag -> serve dag layering where
.bind()can be called on@serve.deploymentdecorated class or func, but only returns raw Ray DAGNode type, executable by ray core and serve_dag is only available after serve-specific transformations.Thus this PR removes exposed serve DAGNode type such as DeploymentNode.
It also removes the syntax of
class.bind().bind()to return aDeploymentMethodNodethat defaults to__call__to match same behavior in ray dag building.closes #24062
Next steps
Refactor
DeploymentNodeimplementation that doesn't always bundle DAGNode, Deployment creation, handling config and replacing handle in one node. We need all of the functionalities, but shouldn't be in one place.Concerte example: all Ray DAGNode types uses
_copyto handle function calls recursively which will re-instantiate DAGNode instances upon replacement it's the mechanism used to execute the DAG as well. But we don't want to re-do everything upon each call inDeploymentNode, not just for abstraction, but also for performance.Checks
scripts/format.shto lint the changes in this PR.