fix: replace add_identity by add_cast for type cast#3563
fix: replace add_identity by add_cast for type cast#3563peri044 merged 2 commits intopytorch:mainfrom
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.
@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
to
from torch_tensorrt.dynamo.conversion.converter_utils import (
has_dynamic_shape,
prepend_ones,
set_layer_name,
)
and change
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. |
| 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.
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.
Thanks for the feedback. I updated the comment for cast_trt_tensor as you mentioned.
peri044
left a comment
There was a problem hiding this comment.
1 minor comment. mostly looks good
| """ | ||
| 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.
Can you use the cast_trt_tensor function to this instead ?
There was a problem hiding this comment.
This is a patch for FX, but looks like cast_trt_tensor is only in dynamo?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
No, the subpackages should remain as distinct as possible. IMO this implementation is fine for FX as essentially all development is on the dynamo side now.
|
Also, @junstar92 please rebase with main. Some of the CI failures should be resolved |
Description
This PR updates the
type_casthelper function to ensure compatibility with TensorRT's strongly typed network mode.type_castusedadd_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_castis called byexpandfunction intorch_tensorrt/dynamo/conversion/impl/slice/ops.pywith 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: