-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[core][compiled-graphs] Register custom serializers for InputAttributeNodes and pass their type hints to downstream nodes #49236
Conversation
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
TorchTensorType
does not move tensors to GPUSigned-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
@@ -1088,6 +1092,9 @@ def _preprocess(self) -> None: | |||
): | |||
downstream_actor_handle = dag_node._get_actor_handle() | |||
|
|||
# Add the type hint of the upstream node to the task. | |||
task.arg_type_hints.append(upstream_task.dag_node.type_hint) |
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.
Resolve "Reason 1". In the logic below, if the upstream_task.dag_node
is an InputAttributeNode
, the upstream_task
will be set to self.idx_to_task[self.input_task_idx]
. This is why I moved this line above.
python/ray/dag/compiled_dag_node.py
Outdated
input_task.dag_node.type_hint.register_custom_serializer() | ||
|
||
# Register custom serializers for input attribute nodes. | ||
for input_attr_task_idx in self.input_attr_task_idx_list: |
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.
Resolve "Reason 2"
Signed-off-by: Kai-Hsun Chen <[email protected]>
Signed-off-by: Kai-Hsun Chen <[email protected]>
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.
Looks good. Minor comments.
Signed-off-by: Kai-Hsun Chen <[email protected]>
…eNodes and pass their type hints to downstream nodes (ray-project#49236) Signed-off-by: Kai-Hsun Chen <[email protected]> Signed-off-by: ujjawal-khare <[email protected]>
Why are these changes needed?
Issue #47251 states that the tensor will not be moved to the GPU even if users specify
TorchTensorType
on the input attribute node. The behavior is inconsistent with specifyingTorchTensorType
on theInputNode
.There are two reasons contributing to this inconsistent behavior:
InputAttributeNode
's type hint is not passed to the downstream DAG nodes.InputAttributeNode
.There are four changes in this PR:
InputNode
,InputAttributeNode
, and output nodes in a single location.Related issue number
Closes #47251
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.