Skip to content
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

🐛 [Bug] aten.convolution fails when 1D stride is tuple #2185

Closed
gs-olive opened this issue Aug 9, 2023 · 3 comments
Closed

🐛 [Bug] aten.convolution fails when 1D stride is tuple #2185

gs-olive opened this issue Aug 9, 2023 · 3 comments
Assignees
Labels
bug Something isn't working component: converters Issues re: Specific op converters

Comments

@gs-olive
Copy link
Collaborator

gs-olive commented Aug 9, 2023

Bug Description

When encountering a Conv1D operator in Dynamo, PyTorch can switch integer components, like dilation, into tuples. This is problematic for the Conv1D operator, since the extend_attr_to_tuple used here, will not work:

# Expand parameters manually for Conv1D computations
if is_conv1d:
padding = tuple(padding) + (0,)
stride = extend_attr_to_tuple(stride, 2)
dilation = extend_attr_to_tuple(dilation, 2)

Specifically, the function extend_attr_to_tuple, shown below, needs to be modified to do the following. It should be able to handle length-1 lists or tuples and extend those to the necessary length specified by the user.

def extend_attr_to_tuple(
val: Any,
num_elem: int,
) -> Tuple[Any, ...]:
"""
If `val` is not a tuple or a list, then we make a tuple of size `num_elem` by
replicating `val` `num_elem` times.
Args:
val (Any): Value that we want to process.
Returns:
A tuple.
"""
if not isinstance(val, (tuple, list)):
val = (val,) * num_elem
if isinstance(val, list):
val = tuple(val)
return val

To Reproduce

Compile a model with a Conv1D operator using one of the Dynamo paths.

Expected behavior

Models with Conv1D operators should trace and compile successfully with AOT/Dynamo.

Environment

  • Torch-TensorRT Version (e.g. 1.0.0): 8c62fca
  • PyTorch Version (e.g. 1.0): 2.1.0.dev20230803+cu121
@gs-olive gs-olive added bug Something isn't working component: converters Issues re: Specific op converters labels Aug 9, 2023
@gs-olive gs-olive changed the title 🐛 [Bug] aten.convolution fails when 1D dilation is tuple 🐛 [Bug] aten.convolution fails when 1D stride is tuple Aug 9, 2023
@apbose apbose self-assigned this Aug 16, 2023
@zewenli98
Copy link
Collaborator

It should be able to handle length-1 lists or tuples and extend those to the necessary length specified by the user.

Sorry I don't get it. Could you give an example for this?

@gs-olive
Copy link
Collaborator Author

@zewenli98 - sure, for instance in the conv1d case, we can get inputs like stride=[1] and dilation=[0]. In those cases, the function extend_attr_to_tuple will not extend those lists because they are not single values. Specifically, extend_attr_to_tuple([1], 2) returns [1], but we want it to return [1, 1]. It would help to have some logic in the extend_attr_to_tuple function which detects singleton lists/tuples and treats them like single elements. For instance:

if isinstance(val, (tuple, list)) and len(val) == 1:
  val = val[0]

@zewenli98
Copy link
Collaborator

zewenli98 commented Aug 24, 2023

@gs-olive Thanks for the detailed explanation! It's fixed in the PR #2252

zewenli98 added a commit to zewenli98/TensorRT that referenced this issue Aug 24, 2023
zewenli98 added a commit to zewenli98/TensorRT that referenced this issue Aug 24, 2023
fix a squeeze bug

minor fix and issue pytorch#2185
zewenli98 added a commit to zewenli98/TensorRT that referenced this issue Aug 29, 2023
fix a squeeze bug

minor fix and issue pytorch#2185

add conv validator
@peri044 peri044 closed this as completed Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component: converters Issues re: Specific op converters
Projects
None yet
Development

No branches or pull requests

4 participants