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

aten::split converter #2232

Merged
merged 7 commits into from
Sep 21, 2023
Merged

aten::split converter #2232

merged 7 commits into from
Sep 21, 2023

Conversation

apbose
Copy link
Collaborator

@apbose apbose commented Aug 15, 2023

aten::split converter

@github-actions github-actions bot added component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths component: tests Issues re: Tests labels Aug 15, 2023
@apbose apbose requested a review from gs-olive August 15, 2023 22:48
@github-actions github-actions bot requested a review from peri044 August 15, 2023 22:48
@apbose apbose requested review from narendasan and removed request for peri044 August 15, 2023 22:49
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

@apbose apbose force-pushed the dynamo_converter_aten_split branch from ee21e73 to 0c53e8c Compare August 15, 2023 23:00
@apbose apbose removed the request for review from narendasan August 15, 2023 23:01
@apbose apbose marked this pull request as draft August 15, 2023 23:01
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

@apbose apbose force-pushed the dynamo_converter_aten_split branch from 0c53e8c to 7be7512 Compare August 15, 2023 23:03
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

@apbose apbose force-pushed the dynamo_converter_aten_split branch from 7be7512 to a332a44 Compare August 15, 2023 23:09
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link
Collaborator

@gs-olive gs-olive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split operator looks good; made some suggested changes.

@@ -279,6 +279,22 @@ def aten_ops_unsqueeze(
)


@dynamo_tensorrt_converter(
torch.ops.aten.split.default, capability_validator=dynamic_unsupported
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to `torch.ops.aten.split.Tensor

kwargs: Dict[str, Argument],
name: str,
) -> Union[TRTTensor, Sequence[TRTTensor]]:
return impl.split(network, target, SourceIR.ATEN, name, args[0], args[1], args[2])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to impl.split.split

source_ir: Optional[SourceIR],
name: str,
input: TRTTensor,
split_size_or_sections: Union[int, List(int)],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace List(int) with List[int]

offset = 0

if len(split_sizes) == 1:
num_splits = input.shape[dim] + split_sizes[0] - 1 // split_sizes[0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parenthesize first argument (num_splits = input.shape[dim] + split_sizes[0] - 1) // split_sizes[0]

name: str,
input: TRTTensor,
split_size_or_sections: Union[int, List(int)],
dim: Optional[Any] = 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with dim: int = 0, since this is a mandatory argument, as per native_functions.yaml.

Comment on lines 33 to 35
if network.has_implicit_batch_dimension:
assert dim != 0, "Can't split on batch dim when it's implicit!"
dim -= 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this conditional, since implicit_batch_dimension is deprecated

assert input.shape[dim] != -1, "Can't chunk on dynamic shape dimension!"

split_sizes = []
if type(split_size_or_sections) == int:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with isinstance(split_size_or_sections, int)

@apbose apbose force-pushed the dynamo_converter_aten_split branch from a332a44 to 3c09956 Compare August 17, 2023 20:10
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

@@ -351,6 +351,34 @@ def aten_ops_softmax(
)


@dynamo_tensorrt_converter(
torch.ops.aten.split.Tensor, capability_validator=dynamic_unsupported_with_args([1])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamic_unsupported_with_args import may also have been removed in the rebase

@apbose apbose force-pushed the dynamo_converter_aten_split branch from 8b1d0d5 to d74c61f Compare September 20, 2023 21:27
@apbose apbose requested a review from gs-olive September 20, 2023 21:28
@apbose apbose force-pushed the dynamo_converter_aten_split branch from d74c61f to 4fed932 Compare September 20, 2023 22:23
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

gs-olive and others added 3 commits September 20, 2023 15:26
- Add support for selecting individual argument positions to check and
expand checking to include symbolic types, which are sometimes passed in
as arguments
Copy link
Collaborator

@gs-olive gs-olive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good - approved, pending CI and minor fix mentioned/any other linting fixes from mypy.

@@ -0,0 +1,81 @@
from typing import Any, Dict, List, Optional, Sequence, Tuple, Union, cast
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point, but the linting may complain about unused import for cast here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it

@apbose apbose force-pushed the dynamo_converter_aten_split branch from 5d1309a to 797d510 Compare September 21, 2023 00:58
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

@apbose apbose requested a review from gs-olive September 21, 2023 01:15
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

@apbose apbose force-pushed the dynamo_converter_aten_split branch from c3341c6 to e95e0e4 Compare September 21, 2023 01:17
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to Python style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code conforms to C++ style guidelines

Copy link
Collaborator

@gs-olive gs-olive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@apbose apbose merged commit 19aabdd into main Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths component: tests Issues re: Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants