-
Notifications
You must be signed in to change notification settings - Fork 363
fix: replace add_identity by add_cast for type cast #3563
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
base: main
Are you sure you want to change the base?
Conversation
Thanks @junstar92 for the contribution. Instead of modifying the FX path, we should import these utilities from the dynamo path since it is actively being developed. So, instead can you modify this change so that the prepend_ones is imported from dynamo/conversion/converter_utils instead ? from torch_tensorrt.dynamo.converters.converter_utils import (
has_dynamic_shape,
prepend_ones,
set_layer_name,
) |
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.
@junstar92 Thanks for your contribution! As @peri044 mentioned, we have switched our attention to Dynamo path. In this PR, instead of importing from fx, can you change
TensorRT/py/torch_tensorrt/dynamo/conversion/impl/slice/ops.py
Lines 26 to 30 in f09be72
from torch_tensorrt.fx.converters.converter_utils import ( | |
has_dynamic_shape, | |
prepend_ones, | |
set_layer_name, | |
) |
to
from torch_tensorrt.dynamo.conversion.converter_utils import (
has_dynamic_shape,
prepend_ones,
set_layer_name,
)
and change
ctx.net, |
to
ctx
accordingly?
Besides, I noticed that you are using from torch.export._trace import _export
instead of from torch.export import export
in your repro. May I know the reason?
LGTM apart from the changes mentioned above |
@peri044 @zewenli98 Thanks for the suggestion. As you mentioned, I changed fx's conversion utilities to dynamo's. |
There's no special reason, it's just how I've been doing it. |
@@ -909,7 +909,6 @@ def type_cast( | |||
""" | |||
This function helps to cast the input type to cast_type | |||
""" | |||
layer_i = network.add_identity(input) | |||
layer_i.set_output_type(0, cast_type) | |||
layer_i = network.add_cast(input, cast_type) | |||
set_layer_name(layer_i, target, f"{name}_dtype_change") |
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.
Thanks for the quick change @junstar92. LGTM as such. Just a minor change, since now we use the cast_trt_tensor
in py/torch_tensorrt/dynamo/conversion/converter_utils.py and the above change is related to that, you could change the comment there -
Adds an Identity layer to the network which performs the conversion
if the input's dtype is different from the cast type. Otherwise returns
input unchanged
to something like
Adds a Cast layer to the network to convert the input tensor to the specified dtype.
If the input tensor already has the desired dtype, it is returned unchanged.
Otherwise, a Cast layer is added to perform the conversion
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.
Thanks for the feedback. I updated the comment for cast_trt_tensor
as you mentioned.
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.
LGTM
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.
1 minor comment. mostly looks good
@@ -909,7 +909,6 @@ def type_cast( | |||
""" | |||
This function helps to cast the input type to cast_type | |||
""" | |||
layer_i = network.add_identity(input) | |||
layer_i.set_output_type(0, cast_type) | |||
layer_i = network.add_cast(input, cast_type) |
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.
Can you use the cast_trt_tensor
function to this instead ?
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.
This is a patch for FX, but looks like cast_trt_tensor
is only in dynamo?
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.
@peri044 As @zewenli98 mentioned, cast_trt_tensor
is in Dynamo path. So it needs to import dynamo.conversion.converter_utils
in FX path. It this what you intended? If not, would you prefer me to implement cast_trt_tensor
just like in Dynamo path and use it instead of type_cast
?
Also, @junstar92 please rebase with main. Some of the CI failures should be resolved |
Description
This PR updates the
type_cast
helper function to ensure compatibility with TensorRT's strongly typed network mode.type_cast
usedadd_identity()
followed byset_output_type()
to perform the data type cast. However, in strongly typed mode, callingset_output_type()
on the identity layer causes an error below:type_cast
is called byexpand
function intorch_tensorrt/dynamo/conversion/impl/slice/ops.py
with dynamic dimension index.TensorRT/py/torch_tensorrt/dynamo/conversion/impl/slice/ops.py
Lines 232 to 237 in f09be72
The following code snippet reproduces the error:
To address this, the function now uses
add_cast()
to explicitly insert a cast layer that converts the input tensor to the desiredcast_type
.If there was a specific reason for using
add_identity()
, please let me know, as this change assumes that the identity layer was not essential beyond type casting.Type of change
Checklist: