feat: Wrap dynamic size handling in a compilation flag #1851

peri044 · 2023-04-21T19:57:53Z

Description

This PR does the following

The aten::size dynamic shape handling is now disabled by default. allow_shape_tensors=True will enable it.
Modifications to aten::size include preserving the static dimensions in the input, thereby not converting any known types eg: Int into ITensors

Type of change

Please delete options that are not relevant and/or add your own.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project (You can use the linters)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas and hacks
I have made corresponding changes to the documentation
I have added tests to verify my fix or my feature
New and existing unit tests pass locally with my changes
I have added the relevant labels to my PR in so that relevant reviewers are notified

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

bowang007

LGTM

bowang007 · 2023-04-21T21:41:06Z

core/conversion/evaluators/prim.cpp

@@ -40,6 +40,8 @@ auto prim_registrations =
               // Outputs is an IValue which has list of tensors which can be found in ctx->evaluated_value_map
               const torch::jit::IValue* outputs = args.at(n->input()).IValue();
               auto outputVec = outputs->toList().vec();
+               LOG_DEBUG("==== OUTPUT VEC: " << outputVec);
+               TORCHTRT_THROW_ERROR("========= FORCED ERROR ");


These 2 lines need to be removed

bowang007 · 2023-04-21T21:41:42Z

core/conversion/evaluators/prim.cpp

@@ -40,6 +40,8 @@ auto prim_registrations =
               // Outputs is an IValue which has list of tensors which can be found in ctx->evaluated_value_map
               const torch::jit::IValue* outputs = args.at(n->input()).IValue();
               auto outputVec = outputs->toList().vec();
+               LOG_DEBUG("==== OUTPUT VEC: " << outputVec);


remove this line

…en::size Signed-off-by: Dheeraj Peri <peri.dheeraj@gmail.com> chore: Add allow-shape-tensors option to torchtrtc, preserve static dimensions in the aten::size layer Signed-off-by: Dheeraj Peri <peri.dheeraj@gmail.com> chore: Linter fixes Signed-off-by: Dheeraj Peri <peri.dheeraj@gmail.com> chore: remove debug throw error Signed-off-by: Dheeraj Peri <peri.dheeraj@gmail.com>

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

narendasan · 2023-05-12T18:25:08Z

core/conversion/evaluators/aten.cpp

+                     if (ctx->settings.allow_shape_tensors) {
+                       return dynamic_size_layer(ctx, n, args);
+                     } else {
+                       LOG_WARNING("There may be undefined behavior using dynamic shape and aten::size ");


Is this undefined behavior due to using aten::size with dynamic shape without shape tensors or the other way around. Might need rewording

narendasan · 2023-05-12T18:27:47Z

core/conversion/evaluators/aten.cpp

@@ -270,7 +270,11 @@ auto aten_registrations TORCHTRT_UNUSED =
                 if (tensor_var.isITensor()) {
                   auto tensor = tensor_var.ITensor();
                   if (ctx->input_is_dynamic) {
-                     return dynamic_size_layer(ctx, n, args);
+                     if (ctx->settings.allow_shape_tensors) {


We should be checking if input is dynamic, shape tensors are enabled and the input contains a placeholder dimension before using the shape tensor code path. Otherwise static aten size is sufficient and will cause less errors

narendasan · 2023-05-12T18:29:32Z

core/conversion/evaluators/aten.cpp

@@ -286,7 +290,11 @@ auto aten_registrations TORCHTRT_UNUSED =
                 auto dim = args.at(n->input(1)).unwrapToInt();
                 if (tensor_var.isITensor()) {
                   if (ctx->input_is_dynamic) {
-                     return dynamic_size_layer(ctx, n, args);
+                     if (ctx->settings.allow_shape_tensors) {


Same comments here from above about conditions and warnings

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

feat: Wrap dynamic size handling in a compilation flag

peri044 requested a review from narendasan April 21, 2023 19:57

facebook-github-bot added the cla signed label Apr 21, 2023

This comment was marked as resolved.

Sign in to view

peri044 force-pushed the exp_size branch from 69472db to c8f285f Compare April 21, 2023 20:03

github-actions bot approved these changes Apr 21, 2023

View reviewed changes

peri044 force-pushed the exp_size branch from c8f285f to 105c892 Compare April 21, 2023 23:05

github-actions bot approved these changes Apr 21, 2023

View reviewed changes

Christina-Young-NVIDIA added the release: v1.4 Tagged to be included in v1.4 label Apr 24, 2023

github-actions bot requested a review from bowang007 April 24, 2023 20:28

bowang007 approved these changes May 12, 2023

View reviewed changes

bowang007 force-pushed the exp_size branch from 105c892 to 2e2e292 Compare May 12, 2023 00:15

github-actions bot approved these changes May 12, 2023

View reviewed changes

narendasan requested changes May 12, 2023

View reviewed changes

chore: rewording warning message

ed7fd99

github-actions bot approved these changes May 15, 2023

View reviewed changes

bowang007 merged commit fcb6890 into main May 15, 2023

bowang007 added a commit that referenced this pull request May 15, 2023

Merge pull request #1851 from pytorch/exp_size

40dfa81

feat: Wrap dynamic size handling in a compilation flag

gs-olive mentioned this pull request May 16, 2023

🐛 [Bug] Compilation failure for SSD300 model with dynamic batch #1555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Wrap dynamic size handling in a compilation flag #1851

feat: Wrap dynamic size handling in a compilation flag #1851

peri044 commented Apr 21, 2023

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

bowang007 left a comment

bowang007 Apr 21, 2023

bowang007 Apr 21, 2023

github-actions bot left a comment

github-actions bot left a comment

narendasan May 12, 2023

narendasan May 12, 2023

narendasan May 12, 2023

github-actions bot left a comment

github-actions bot left a comment

feat: Wrap dynamic size handling in a compilation flag #1851

feat: Wrap dynamic size handling in a compilation flag #1851

Conversation

peri044 commented Apr 21, 2023

Description

Type of change

Checklist:

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

bowang007 left a comment

Choose a reason for hiding this comment

bowang007 Apr 21, 2023

Choose a reason for hiding this comment

bowang007 Apr 21, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

narendasan May 12, 2023

Choose a reason for hiding this comment

narendasan May 12, 2023

Choose a reason for hiding this comment

narendasan May 12, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment