Skip to content

fix(//core/runtime): Support more delimiter variants #1004

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

Merged
merged 4 commits into from
May 7, 2022

Conversation

narendasan
Copy link
Collaborator

Description

Fixes an issue where different classes of delimiters would not properly be handled when deserializing TRT engines.

Fixes #930
Fixes #982

Type of change

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

  • Bug fix (non-breaking change which fixes an issue)

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

Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]>
@narendasan narendasan requested a review from bowang007 April 27, 2022 03:31
@github-actions github-actions bot added component: core Issues re: The core compiler component: runtime labels Apr 27, 2022
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.

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/runtime/TRTEngine.cpp b/tmp/changes.txt
index 5ba3a56..70b51e0 100644
--- a/workspace/core/runtime/TRTEngine.cpp
+++ b/tmp/changes.txt
@@ -63,7 +63,10 @@ TRTEngine::TRTEngine(std::string mod_name, std::string serialized_engine, CudaDe
    auto delim = bind_name.find(".");
    if (delim == std::string::npos) {
      delim = bind_name.find("_");
-      TORCHTRT_CHECK(delim != std::string::npos, "Unable to determine binding index for input " << bind_name << "\nEnsure module was compile with Torch-TensorRT.ts");
+      TORCHTRT_CHECK(
+          delim != std::string::npos,
+          "Unable to determine binding index for input " << bind_name
+                                                         << "\nEnsure module was compile with Torch-TensorRT.ts");
    }

    std::string idx_s = bind_name.substr(delim + 1);
@@ -97,16 +100,16 @@ std::string TRTEngine::to_str() const {
  ss << "  Name: " << name << std::endl;
  ss << "  Inputs: [" << std::endl;
  for (uint64_t i = 0; i < num_io.first; i++) {
-  ss << "    id: " << i << std::endl;
-  ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
-  ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
+    ss << "    id: " << i << std::endl;
+    ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
+    ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Outputs: [" << std::endl;
  for (uint64_t o = 0; o < num_io.second; o++) {
-  ss << "    id: " << o << std::endl;
-  ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
-  ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
+    ss << "    id: " << o << std::endl;
+    ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
+    ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Device: " << device_info << std::endl;
ERROR: Some files do not conform to 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.

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/runtime/TRTEngine.cpp b/tmp/changes.txt
index 5ba3a56..70b51e0 100644
--- a/workspace/core/runtime/TRTEngine.cpp
+++ b/tmp/changes.txt
@@ -63,7 +63,10 @@ TRTEngine::TRTEngine(std::string mod_name, std::string serialized_engine, CudaDe
    auto delim = bind_name.find(".");
    if (delim == std::string::npos) {
      delim = bind_name.find("_");
-      TORCHTRT_CHECK(delim != std::string::npos, "Unable to determine binding index for input " << bind_name << "\nEnsure module was compile with Torch-TensorRT.ts");
+      TORCHTRT_CHECK(
+          delim != std::string::npos,
+          "Unable to determine binding index for input " << bind_name
+                                                         << "\nEnsure module was compile with Torch-TensorRT.ts");
    }

    std::string idx_s = bind_name.substr(delim + 1);
@@ -97,16 +100,16 @@ std::string TRTEngine::to_str() const {
  ss << "  Name: " << name << std::endl;
  ss << "  Inputs: [" << std::endl;
  for (uint64_t i = 0; i < num_io.first; i++) {
-  ss << "    id: " << i << std::endl;
-  ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
-  ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
+    ss << "    id: " << i << std::endl;
+    ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
+    ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Outputs: [" << std::endl;
  for (uint64_t o = 0; o < num_io.second; o++) {
-  ss << "    id: " << o << std::endl;
-  ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
-  ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
+    ss << "    id: " << o << std::endl;
+    ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
+    ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Device: " << device_info << std::endl;
ERROR: Some files do not conform to style guidelines

@github-actions github-actions bot added the component: api [C++] Issues re: C++ API label Apr 27, 2022
Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]>
@narendasan narendasan force-pushed the support_multiple_delimiters branch from 78f808b to 1c3c779 Compare April 27, 2022 03:39
@github-actions github-actions bot added the component: api [Python] Issues re: Python API label Apr 27, 2022
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.

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/runtime/TRTEngine.cpp b/tmp/changes.txt
index 5ba3a56..70b51e0 100644
--- a/workspace/core/runtime/TRTEngine.cpp
+++ b/tmp/changes.txt
@@ -63,7 +63,10 @@ TRTEngine::TRTEngine(std::string mod_name, std::string serialized_engine, CudaDe
    auto delim = bind_name.find(".");
    if (delim == std::string::npos) {
      delim = bind_name.find("_");
-      TORCHTRT_CHECK(delim != std::string::npos, "Unable to determine binding index for input " << bind_name << "\nEnsure module was compile with Torch-TensorRT.ts");
+      TORCHTRT_CHECK(
+          delim != std::string::npos,
+          "Unable to determine binding index for input " << bind_name
+                                                         << "\nEnsure module was compile with Torch-TensorRT.ts");
    }

    std::string idx_s = bind_name.substr(delim + 1);
@@ -97,16 +100,16 @@ std::string TRTEngine::to_str() const {
  ss << "  Name: " << name << std::endl;
  ss << "  Inputs: [" << std::endl;
  for (uint64_t i = 0; i < num_io.first; i++) {
-  ss << "    id: " << i << std::endl;
-  ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
-  ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
+    ss << "    id: " << i << std::endl;
+    ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
+    ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Outputs: [" << std::endl;
  for (uint64_t o = 0; o < num_io.second; o++) {
-  ss << "    id: " << o << std::endl;
-  ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
-  ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
+    ss << "    id: " << o << std::endl;
+    ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
+    ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Device: " << device_info << std::endl;
ERROR: Some files do not conform to 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.

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/runtime/TRTEngine.cpp b/tmp/changes.txt
index 5ba3a56..70b51e0 100644
--- a/workspace/core/runtime/TRTEngine.cpp
+++ b/tmp/changes.txt
@@ -63,7 +63,10 @@ TRTEngine::TRTEngine(std::string mod_name, std::string serialized_engine, CudaDe
    auto delim = bind_name.find(".");
    if (delim == std::string::npos) {
      delim = bind_name.find("_");
-      TORCHTRT_CHECK(delim != std::string::npos, "Unable to determine binding index for input " << bind_name << "\nEnsure module was compile with Torch-TensorRT.ts");
+      TORCHTRT_CHECK(
+          delim != std::string::npos,
+          "Unable to determine binding index for input " << bind_name
+                                                         << "\nEnsure module was compile with Torch-TensorRT.ts");
    }

    std::string idx_s = bind_name.substr(delim + 1);
@@ -97,16 +100,16 @@ std::string TRTEngine::to_str() const {
  ss << "  Name: " << name << std::endl;
  ss << "  Inputs: [" << std::endl;
  for (uint64_t i = 0; i < num_io.first; i++) {
-  ss << "    id: " << i << std::endl;
-  ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
-  ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
+    ss << "    id: " << i << std::endl;
+    ss << "      shape: " << exec_ctx->getBindingDimensions(i) << std::endl;
+    ss << "      dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(i)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Outputs: [" << std::endl;
  for (uint64_t o = 0; o < num_io.second; o++) {
-  ss << "    id: " << o << std::endl;
-  ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
-  ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
+    ss << "    id: " << o << std::endl;
+    ss << "    shape: " << exec_ctx->getBindingDimensions(o) << std::endl;
+    ss << "    dtype: " << util::TRTDataTypeToScalarType(exec_ctx->getEngine().getBindingDataType(o)) << std::endl;
  }
  ss << "  ]" << std::endl;
  ss << "  Device: " << device_info << std::endl;
ERROR: Some files do not conform to 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

@bowang007
Copy link
Collaborator

@narendasan looks good, however, I rebased locally it still cannot resolve the issue that I have now. There is no delimiter such as '.' or '_' in the bind_name that I get in the model, maybe I still need to look into what happens in this model.
At least the error message is clearer for me now.

@narendasan
Copy link
Collaborator Author

We need inputs to have an index because tensorrt does not guarantee binding order. So you will need to modify whatever is coming in to have indexes

@bowang007
Copy link
Collaborator

I checked the log where we have this in conversion process:

DEBUG: [Torch-TensorRT TorchScript Conversion Context] - Input Dimension Specs: {
    argument_1.1 : Input(shape: [3, 1024, 1024], dtype: Float32, format: NCHW\Contiguous\Linear),}
INFO: [Torch-TensorRT TorchScript Conversion Context] - Adding Input argument_1.1 (named: input_0): Input(shape: [3, 1024, 1024], dtype: Float32, format: NCHW\Contiguous\Linear) in engine (conversion.AddInputs)

it seems that we have input_0, then there comes serialization and deserialization, after deserialization the binding name became 0 for input_0.
How can we modify this? since this whole serialization and deserialization are done through TensorRT. @narendasan

@narendasan
Copy link
Collaborator Author

The binding is just 0?

@bowang007
Copy link
Collaborator

The binding is just 0?

yes.

@narendasan
Copy link
Collaborator Author

hmm there is something odd there i feel. I would expect that the name would be input_0 or something

Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]>
@github-actions github-actions bot added the component: tests Issues re: Tests label May 5, 2022
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

@narendasan
Copy link
Collaborator Author

/blossom-ci

deserialization

NOTE: This does not fully address the deserialization issue
as the root cause is TensorRT modifies the input binding names
which is leading to these cases of stoi errors.

Signed-off-by: Naren Dasan <[email protected]>
Signed-off-by: Naren Dasan <[email protected]>
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.

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/runtime/TRTEngine.cpp b/tmp/changes.txt
index dbef11b..26c755c 100644
--- a/workspace/core/runtime/TRTEngine.cpp
+++ b/tmp/changes.txt
@@ -66,8 +66,9 @@ TRTEngine::TRTEngine(std::string mod_name, std::string serialized_engine, CudaDe
      delim = bind_name.find("_");
      TORCHTRT_CHECK(
          delim != std::string::npos,
-          "Unable to determine binding index for input " << bind_name
-                                                         << "\nEnsure module was compiled with Torch-TensorRT.ts or follows Torch-TensorRT Runtime conventions");
+          "Unable to determine binding index for input "
+              << bind_name
+              << "\nEnsure module was compiled with Torch-TensorRT.ts or follows Torch-TensorRT Runtime conventions");
    }

    std::string idx_s = bind_name.substr(delim + 1);
ERROR: Some files do not conform to style guidelines

@narendasan narendasan force-pushed the support_multiple_delimiters branch from 30cb548 to 65af9d1 Compare May 7, 2022 01:43
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: api [C++] Issues re: C++ API component: api [Python] Issues re: Python API component: core Issues re: The core compiler component: runtime component: tests Issues re: Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [Bug] Failed to use stoi to get the engine ID in TRTEngine function 🐛 [Bug] Compilation causes error: ValueError: stoi
2 participants