feat(//core/): Add support for tensorrt plugin and implement instance… #285

ChaoFang-TRI · 2021-01-14T08:48:06Z

… norm layer

Signed-off-by: Chao Fang [email protected]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

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

… norm layer Fix pytorch#210 Signed-off-by: Chao Fang <[email protected]>

ChaoFang-TRI · 2021-01-14T08:52:21Z

@narendasan The tensorRT plugin support need more tests. It would be nice if any NVIDIA folks could help.

ChaoFang-TRI · 2021-01-14T08:53:06Z

I keep getting linter erro:
ERROR: Skipping '//tools/linter:cpplint_diff': error loading package 'tools/linter': Unable to find package for @pylinter_deps//:requirements.bzl: The repository '@pylinter_deps' could not be resolved.

narendasan · 2021-01-14T19:08:59Z

I keep getting linter erro:
ERROR: Skipping '//tools/linter:cpplint_diff': error loading package 'tools/linter': Unable to find package for @pylinter_deps//:requirements.bzl: The repository '@pylinter_deps' could not be resolved.

Do you have the Python dependencies in the WORKSPACE enabled? It should just be picking up the requirements.txt file in //tools/linter

https://github.com/NVIDIA/TRTorch/blob/72bf74b2a2e425e6c83542f99c92b9e551148149/WORKSPACE#L142

narendasan · 2021-01-14T19:12:13Z

@narendasan The tensorRT plugin support need more tests. It would be nice if any NVIDIA folks could help.

Yeah for sure we can take a look here. Next test that would be good to see is if a TRT plugin based model is portable. So compile a module, save to disk, start a new process, load the module and see if it runs or something similar

narendasan

Thanks! This is a great addition. Left some preliminary comments. Would also like @peri044 to take a look. I think we need to think a bit about how to make sure plugins are available both at conversion and runtime.

narendasan · 2021-01-14T19:13:31Z

core/conversion/conversionctx/ConversionCtx.cpp

@@ -45,6 +48,25 @@ ConversionCtx::ConversionCtx(BuilderSettings build_settings)
          "[TRTorch Conversion Context] - ",
          util::logging::get_logger().get_reportable_severity(),
          util::logging::get_logger().get_is_colored_output_on()) {
+    // Get list of all available plugin creators
+
+    initLibNvInferPlugins(&logger, "");


I suspect that this will need to occur in a more global manner since it will need to cover cases were we only use the runtime and the conversion context is therefore never created. We also probably should use the global logger in that case as well instead of the conversion logger.

Ideally this would be located somewhere in the runtime module and run on library load since we would also want this for libtrtorchrt.so as well.

I am not sure where is the best place to initialize the plugin. Can you suggest a specific place?

narendasan · 2021-01-14T19:17:45Z

core/conversion/conversionctx/ConversionCtx.h

@@ -66,6 +66,9 @@ struct ConversionCtx {
  // copy of the values
  std::vector<void*> builder_resources;

+  //Registry of official tensorrt plugin layers.
+  std::unordered_map<std::string, nvinfer1::IPluginCreator*> mPluginRegistry;


Considering we are starting to ship our own TRTorch specific plugins as well maybe we could unify the various registries in a separate module which is a dep of conversion and runtime. It might solve the global init issue as well. @peri044 thoughts?

That's a nice idea. We can have a single registry and our own version of initLibNvInferPlugins which can initialize TRT and TRTorch plugins.
But here are some questions in mind
Do we plan on integrating custom plugins with libtrtorch.so as well ? or do we have something like libtrtorch_plugins.so ?

For the former case, If the extracted TRT engine has a plugin that doesn't depend on aten:calls and uses custom cuda kernels (as for general plugins), this sample needs to load libnvinfer.so, libtrtorch.so which depends on libtorch.so ?

In the later case, the sample can load libnvinfer.so and libtrtorch_plugins.so. Maybe this would be ideal from a deployment perspective ?

Seems this is touching the core requirement of TRTorch. I like the second idea to load load libnvinfer.so and libtrtorch_plugins.so.

narendasan · 2021-01-14T19:20:52Z

tests/core/conversion/converters/test_batch_norm.cpp

+  ASSERT_TRUE(trtorch::tests::util::almostEqual(jit_results[0], trt_results[0].reshape_as(jit_results[0]), 2e-6));
+}
+
+TEST(Converters, ATenInstanceNormWithConvertsCorrectly) {


Another good test would be to make sure we are hitting both paths of the instance norm converter

peri044

The testcase which hits one path of instance norm works on my local.

peri044 · 2021-01-21T20:07:57Z

core/conversion/converters/impl/batch_norm.cpp

+         fc.fields = f.data();
+         nvinfer1::IPluginV2* pluginV2 = ctx->mPluginRegistry.at(pluginName)->createPlugin("instancenorm", &fc);
+
+         TRTORCH_CHECK(pluginV2, "Unable to create interpolation plugin from node" << *n);


interpolation plugin name -> instance norm plugin. To distinguish from line 143 check, this message can be changed to something like "Unable to create instance norm plugin from TensorRT plugin registry"

peri044 · 2021-01-21T20:56:00Z

core/conversion/conversionctx/ConversionCtx.h

@@ -66,6 +66,9 @@ struct ConversionCtx {
  // copy of the values
  std::vector<void*> builder_resources;

+  //Registry of official tensorrt plugin layers.
+  std::unordered_map<std::string, nvinfer1::IPluginCreator*> mPluginRegistry;


That's a nice idea. We can have a single registry and our own version of initLibNvInferPlugins which can initialize TRT and TRTorch plugins.
But here are some questions in mind
Do we plan on integrating custom plugins with libtrtorch.so as well ? or do we have something like libtrtorch_plugins.so ?

For the former case, If the extracted TRT engine has a plugin that doesn't depend on aten:calls and uses custom cuda kernels (as for general plugins), this sample needs to load libnvinfer.so, libtrtorch.so which depends on libtorch.so ?

In the later case, the sample can load libnvinfer.so and libtrtorch_plugins.so. Maybe this would be ideal from a deployment perspective ?

peri044 · 2021-01-27T10:08:31Z

@ChaoFang-TRI Thanks for the PR. Any thoughts on the above review comments ? Can you address some of them ?

ChaoFang-TRI · 2021-03-03T05:09:27Z

@ChaoFang-TRI Thanks for the PR. Any thoughts on the above review comments ? Can you address some of them ?

Hi Peri044, I was occupied for family issue in the past month, sorry for the delayed response. Please feel free to jump in on addressing comments! I will push some commits as well to address some commits.

peri044 · 2021-04-03T00:58:52Z

I tried to add this instance norm to the plugin suite PR . The testcase provided in this PR does not use TensorRT plugin but rather hits the else section (using native TRT layers).
The following testcase graph uses plugin block in the instance norm code in this PR; but upon testing this, I face seg fault issue. So I'm holding off this plugin for now and later add this once the issues are fixed.

const auto graph = R"IR(
      graph(%0 : Tensor,
            %1: Float(5, strides=[1]),
            %2: Float(5, strides=[1]),
            %3: Float(5, strides=[1]),
            %4: Float(5, strides=[1])):
        %9 : Tensor? = prim::Constant()
        %5 : bool = prim::Constant[value=1]()
        %6 : float = prim::Constant[value=0.10000000000000001]()
        %7 : float = prim::Constant[value=1.0000000000000001e-05]()
        %8 : Tensor = aten::instance_norm(%0, %1, %2, %9, %9, %5, %6, %7, %5)
        return (%8))IR";

narendasan · 2021-04-30T22:00:41Z

We should be merging in support for nvinfer plugin in #425, Can we reduce the scope of this PR to only include the additional converters?

narendasan · 2021-08-20T21:22:26Z

Closing in favor of #573

feat(//core/): Add support for tensorrt plugin and implement instance…

4e4717a

… norm layer Fix pytorch#210 Signed-off-by: Chao Fang <[email protected]>

narendasan requested a review from peri044 January 14, 2021 19:09

narendasan reviewed Jan 14, 2021

View reviewed changes

peri044 requested changes Jan 21, 2021

View reviewed changes

ruoqianguo mentioned this pull request Feb 21, 2021

adding support for gelu converter in activation.cpp #357

Closed

6 tasks

narendasan force-pushed the master branch from e705c4f to 8cea634 Compare May 13, 2021 23:42

narendasan force-pushed the master branch from be24049 to 254eab2 Compare July 28, 2021 17:54

zsef123 mentioned this pull request Aug 11, 2021

Add instance norm #573

Merged

6 tasks

narendasan closed this Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(//core/): Add support for tensorrt plugin and implement instance… #285

feat(//core/): Add support for tensorrt plugin and implement instance… #285

ChaoFang-TRI commented Jan 14, 2021 •

edited

Loading

ChaoFang-TRI commented Jan 14, 2021

ChaoFang-TRI commented Jan 14, 2021

narendasan commented Jan 14, 2021

narendasan commented Jan 14, 2021

narendasan left a comment

narendasan Jan 14, 2021

narendasan Jan 14, 2021

ChaoFang-TRI Mar 3, 2021

narendasan Jan 14, 2021

peri044 Jan 21, 2021

ChaoFang-TRI Mar 3, 2021

narendasan Jan 14, 2021

peri044 left a comment

peri044 Jan 21, 2021

peri044 Jan 21, 2021

peri044 commented Jan 27, 2021

ChaoFang-TRI commented Mar 3, 2021

peri044 commented Apr 3, 2021

narendasan commented Apr 30, 2021

narendasan commented Aug 20, 2021

feat(//core/): Add support for tensorrt plugin and implement instance… #285

feat(//core/): Add support for tensorrt plugin and implement instance… #285

Conversation

ChaoFang-TRI commented Jan 14, 2021 • edited Loading

Description

Type of change

Checklist:

ChaoFang-TRI commented Jan 14, 2021

ChaoFang-TRI commented Jan 14, 2021

narendasan commented Jan 14, 2021

narendasan commented Jan 14, 2021

narendasan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peri044 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peri044 commented Jan 27, 2021

ChaoFang-TRI commented Mar 3, 2021

peri044 commented Apr 3, 2021

narendasan commented Apr 30, 2021

narendasan commented Aug 20, 2021

ChaoFang-TRI commented Jan 14, 2021 •

edited

Loading