[1/X][Pipeline] Add deployment nodes#22549
Conversation
ericl
added
the
@author-action-required
edoakes
left a comment
edoakes
left a comment
There was a problem hiding this comment.
Looks good, only questions/nits. Thanks for splitting this out, makes it much easier to read.
|
|
||
| def __init__( | ||
| self, | ||
| deployment_body: Deployment, |
There was a problem hiding this comment.
not sure what a deployment body is, let's just call it deployment?
| super().__init__( | ||
| method_args, | ||
| method_kwargs, | ||
| method_options, | ||
| other_args_to_resolve=other_args_to_resolve, | ||
| ) |
There was a problem hiding this comment.
what does the parent constructor do in this case?
There was a problem hiding this comment.
it's needed to have a DAGNode level stable uuid, but this comment reminds me we can merge super().init with serve DAGNode level init.. let me change it
|
|
||
| def _execute_impl(self, *args): | ||
| """Executor of DeploymentNode by ray.remote()""" | ||
| return self._deployment_handle.options(**self._bound_options).remote( |
There was a problem hiding this comment.
do we support options to the handle?
There was a problem hiding this comment.
good question. I'm leaning towards no, I think it should be preferred to just edit the YAML for everything we put in .options(), that's the ground truth, and variants of a deployment.
Proper implementation of this can happen in diff ... #5 or so when we implement build() tho.
jiaodong
force-pushed
the
deployment_node
branch
from
e30e1c6 to
a1c9856
Compare
jiaodong
force-pushed
the
deployment_node
branch
from
a1c9856 to
41be7bd
Compare
jiaodong
removed
the
@author-action-required
edoakes
left a comment
edoakes
left a comment
There was a problem hiding this comment.
Please try to avoid force pushing, makes it harder for me to review the diffs between revisions
Ray DAG Changes - Restructured and resolves circular imports in current dag_node.py. - Moved `__str__` to each DAGNode subclass level with centralized utils imports - Removed restrictions on binding `InputNode` to `FunctionNode` and `ClassMethodNode` - Moved `_contain_input_node` to only `ClassNode` and `DeploymentNode` Serve DAG Changes - Added DeploymentNode - Cannot be directly constructed - Holds deployment func or class body as well as handle that trivially maps to `__call__` method (match current behavior) - Upon accessing an attribute, it will spawn DeploymentMethodNode node with `other_args_to_resolve` passed in to differentiate sync handle type and others - Added DeploymentMethodNode - Holds arg and deployment handle - Executing on it translate to deployment handle call on the method.
3 participants
WIP, but content is there up for review, I will add more tests on this PR.
Diff Summary
Ray DAG Changes
__str__to each DAGNode subclass level with centralized utils importsInputNodetoFunctionNodeandClassMethodNode_contain_input_nodeto onlyClassNodeandDeploymentNodeServe DAG Changes
__call__method (match current behavior)other_args_to_resolvepassed in to differentiate sync handle type and othersRelated issue number
Test Plan
Ray dag unit tests
New serve deployment node tests
Checks
scripts/format.shto lint the changes in this PR.