[2/X][Pipeline] Add python generation for ClassNode #22617
Conversation
There was a problem hiding this comment.
The parts that are filled in look good; the detailed comments and thoughtful abstractions are really appreciated!
Are you planning to address the TODOs/missing pieces here or in a follow-up? Let's make sure we can grep -r "TODO" and find nothing before the branch cut
| def _remove_non_default_ray_actor_options(ray_actor_options: Dict[str, Any]): | ||
| """ | ||
| In Ray DAG building we pass full ray_actor_options regardless if a field | ||
| was explicitly set. Since some values are invalid, we need to remove them | ||
| from ray_actor_options. | ||
| """ |
There was a problem hiding this comment.
interestingly, there's a small edge case we likely won't handle: if the user passed in one of these default explicitly, we won't error but will instead just purge it. not important IMO but if there's a simple way to handle it, might be worthwhile
There was a problem hiding this comment.
i see, let me leave a TODO about this, we will revisit this part soon when we get to the build() implementations.
| # TODO: (jiaodong) Need to handle deeply nested deployment in args | ||
| for arg in args: | ||
| if isinstance(arg, DeploymentNode): | ||
| init_args.append(arg._deployment_handle) | ||
| else: | ||
| init_args.append(arg) | ||
| for key, value in kwargs.items(): | ||
| if isinstance(value, DeploymentNode): | ||
| init_kwargs[key] = value._deployment_handle | ||
| else: | ||
| init_kwargs[key] = value | ||
| return init_args, init_kwargs |
There was a problem hiding this comment.
shouldn't we be able to hook into the ray DAG node arg replacement that already handles nested args?
There was a problem hiding this comment.
this would be a very big limitation, not acceptable for public release IMO
There was a problem hiding this comment.
Exactly -- that's the most feasible way I can think of as well, pickling / json serialization with hook on DAGNode type is the simplest way we handle nested args.
There're some details to be figured out that I didn't have a passing test yet, since we both 1) want to replace ClassNode with DeploymentNode 2) But for the deployment instance we construct and pass into DeploymentNode, we need to recursively replace all DeploymentNode with handle. I'm thinking maybe a two-pass solution is cleaner.
Let me work on this on branch #3 today and update this to be mergeable.
|
@jiaodong I think for the sake of reviewing it might be easier to address my nits and merge this branch w/o the full functionality, then make smaller follow-ups for things like nested args. |
|
Btw, for nested deployment args, shouldn't the rag DAG API give us an interface to handle this? This would be a common pattern to basically all DAG implementations I'd assume. cc @ericl if you have thought about this |
edoakes
added
the
@author-action-required
…e for shared deployment handle
jiaodong
added
tests-ok
- Added backbone of ray dag -> serve dag transformation and deployment extraction. - Added util functions for deployment unique name generation .. ray_actor_options, replacement of DeploymentNode with deployment handle, etc.
Biggest TBD is how to handle deeply nested Deployment used in init_args, where simple traversal of
init_argsandinit_kwargsare not sufficient. This is covered in the unit test in PR and need to be fixed. Will address in next stacked diff so this one doesn't look too large.Why are these changes needed?
Checks
scripts/format.shto lint the changes in this PR.