-
Notifications
You must be signed in to change notification settings - Fork 351
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
Selectively enable different frontends #2693
Conversation
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.
Overall looks great; overhaul of dtype system and frontend selection at install is very helpful. I just need to verify on local installs as well. Left a few small comments
or torch_tensorrt.dtype.float in enabled_precisions | ||
): | ||
precision = torch.float32 | ||
if dtype.float16 in enabled_precisions or dtype.half in enabled_precisions: |
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.
dtype.float16
and dtype.half
seem to point to the same enum
object. Should this be:
if dtype.float16 in enabled_precisions or torch.float16 in enabled_precisions:
precision = torch.float32 | ||
if dtype.float16 in enabled_precisions or dtype.half in enabled_precisions: | ||
precision = dtype.float16 | ||
elif dtype.float32 in enabled_precisions or dtype.float in enabled_precisions: |
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.
Same as above comment
if dtype.float16 in enabled_precisions or dtype.half in enabled_precisions: | ||
precision = dtype.float16 | ||
elif dtype.float32 in enabled_precisions or dtype.float in enabled_precisions: | ||
precision = dtype.float32 |
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.
Same as above
0045465
to
ae453fc
Compare
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
…pes in the python package to decouple frontends Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
dynamo Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
torch.fx.passes.splitter_base._SplitterBase Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
959f3e5
to
b21fb5f
Compare
py/torch_tensorrt/_compile.py
Outdated
logger.warning( | ||
"Input graph is a Torchscript module but the ir provided is default (dynamo). Please set ir=torchscript to suppress the warning. Compiling the module with ir=torchscript" | ||
"Input is a torchscript module but the ir was not specified (default=dynamo), please set ir=torchscript to suppress the warning." | ||
) | ||
return _IRType.ts | ||
elif module_is_exportable: |
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.
Need to add feature check
py/torch_tensorrt/_compile.py
Outdated
@@ -261,7 +281,7 @@ def convert_method_to_trt_engine( | |||
method_name: str = "forward", | |||
inputs: Optional[Sequence[Input | torch.Tensor]] = None, | |||
ir: str = "default", | |||
enabled_precisions: Optional[Set[torch.dtype | dtype]] = None, | |||
enabled_precisions: Optional[Set[torch.dtype]] = None, |
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.
Correct type annotation
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[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.
Overall looks great - added some small style/clarification/logging comments
Additionally, tested on Windows E2E models and appears to cleanly dispatch to the correct runtime without needing to modify the |
A few examples, such as the one below, use
|
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[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.
LGTM. Added minor comments
py/torch_tensorrt/_enums.py
Outdated
use_default: bool, | ||
) -> Optional[Union[torch.dtype, trt.DataType, np.dtype, dtype]]: | ||
try: | ||
print(self) |
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 remove this statement
py/torch_tensorrt/_enums.py
Outdated
elif t == DeviceType: | ||
return self |
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.
I'm curious when do we need to cast EngineCapability
to DeviceType
. Any examples ?
use_fast_partitioner: bool = USE_FAST_PARTITIONER, | ||
enable_experimental_decompositions: bool = ENABLE_EXPERIMENTAL_DECOMPOSITIONS, | ||
enabled_precisions: Set[torch.dtype | dtype] | Tuple[torch.dtype | dtype] = ( | ||
dtype.float32, |
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.
Shouldn't this be _defaults.ENABLED_PRECISIONS
?
|
||
compilation_options = { | ||
"precision": precision, | ||
"enabled_precisions": enabled_precisions, |
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.
It seems like you've handled enabled_precisions to be empty scenario in compile
function. We can use the same here
"enabled_precisions": (
enabled_precisions if enabled_precisions else _defaults.ENABLED_PRECISIONS
@@ -64,7 +64,7 @@ include-package-data = false | |||
|
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.
Where would this file be used ?
Will docs be updated with the instructions as a different PR ? |
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
…2761) Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]> Co-authored-by: Naren Dasan <[email protected]>
…2761) Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]> Co-authored-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
Description
Allows users to configure which frontends/features they want to include in the
torch-tensorrt
builds.Features can be enabled as so:
A builds feature set can be accessed via the following struct
where
ENABLED_FEATURES
is anamedtuple
FeatureSet
:In order to support optional features, a number of core types have been abstracted:
torch_tensorrt.Device
has no direct dependencies on the torchscript core and can be translated totorch_tensorrt.ts.Device
to access those featurestorch_tensorrt.Input
behaves the same waydtype
,DeviceType
,memory_format
have been defined and can translate fromnumpy
,tensorrt
,torch
andtorch_tensorrt._C
(assumingtorch_tensorrt.ENABLED_FEATURES.torchscript_frontend
isTrue
)Translating between different library enums now can take the form
Fixes #1943
Fixes #2379
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: