-
Notifications
You must be signed in to change notification settings - Fork 366
fix: Use user provided dtype when we can't infer it from the graph #962
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
Conversation
Signed-off-by: Dheeraj Peri <[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.
Code conforms to Python style guidelines
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.
Code conforms to C++ style guidelines
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.
Code conforms to Python style guidelines
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.
Code conforms to C++ style guidelines
@@ -239,8 +239,6 @@ def run(self): | |||
libraries=["torchtrt"], | |||
include_dirs=[ | |||
dir_path + "torch_tensorrt/csrc", dir_path + "torch_tensorrt/include", | |||
dir_path + "/../bazel-TRTorch/external/tensorrt/include", |
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.
Might as well keep this since if people dont redownload the repo after the namechange its going to break the build
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.
added it back. But it would be nice to clean this up at some point in time.
@@ -328,7 +328,8 @@ void MapInputsAndDetermineDTypes( | |||
spec.dtype = nvinfer1::DataType::kFLOAT; | |||
} else if (spec.dtype_is_user_defined && cfg.partition_info.enabled) { | |||
if (!est_type_opt) { | |||
LOG_INFO("Cannot infer input tensor dtype in graph, unable to verify user input dtype settings"); | |||
LOG_INFO("Cannot infer input tensor dtype in graph. Using user provided input dtype settings"); | |||
first_use_type_map[in] = {util::TRTDataTypeToScalarType(cfg.convert_info.inputs.find(in)->second.dtype)}; |
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.
how does this work with this patch? #902
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.
Your patch works in the case where the estimated type is not same as the user provided type. In this case, you override with user provided dtype.
In this PR. if the user dtype is defined (dtype_is_user_defined=True) and if the est_type_opt is null (which happens for the bitwise OR graph I linked in the PR where inferred type is empty, I use user provided types.
@@ -40,6 +40,13 @@ bool valid_dtype_format_combo(nvinfer1::DataType dtype, nvinfer1::TensorFormat f | |||
default: | |||
return false; | |||
} | |||
case nvinfer1::DataType::kBOOL: // Supports Linear (NCHW) | |||
switch (format) { |
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.
We probably should be more clear to the user why a BOOL / NHWC combo doesnt work
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.
NHWC is only bound to FP32 according to https://docs.nvidia.com/deeplearning/tensorrt/api/c_api/namespacenvinfer1.html#ac3e115b1a2b1e578e8221ef99d27cd45
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.
Yeah, I understand, I mean we need to throw a descriptive error or something
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.
Probably put this at the API level
…r local repo name Signed-off-by: Dheeraj Peri <[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.
Code conforms to C++ style guidelines
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.
Code conforms to Python style guidelines
Signed-off-by: Dheeraj Peri <[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.
There are some changes that do not conform to C++ style guidelines:
diff --git a/workspace/cpp/src/types.cpp b/tmp/changes.txt
index d60f6bd..993a9e5 100644
--- a/workspace/cpp/src/types.cpp
+++ b/tmp/changes.txt
@@ -161,7 +161,7 @@ Input::Input(std::vector<int64_t> shape, DataType dtype, TensorFormat format) {
this->shape = shape;
this->dtype = dtype;
this->format = format;
- if(dtype == DataType::kBool && format == TensorFormat::kChannelsLast){
+ if (dtype == DataType::kBool && format == TensorFormat::kChannelsLast) {
TORCHTRT_THROW_ERROR("Input datatype (Bool) is not currently supported with Tensorformat NHWC (kChannelsLast)");
}
this->input_is_dynamic = false;
@@ -184,7 +184,7 @@ Input::Input(c10::IntArrayRef shape, DataType dtype, TensorFormat format) {
this->max_shape = torch_tensorrt::core::util::toVec(shape);
this->shape = torch_tensorrt::core::util::toVec(shape);
this->dtype = dtype;
- if(dtype == DataType::kBool && format == TensorFormat::kChannelsLast){
+ if (dtype == DataType::kBool && format == TensorFormat::kChannelsLast) {
TORCHTRT_THROW_ERROR("Input datatype (Bool) is not currently supported with Tensorformat NHWC (kChannelsLast)");
}
this->format = format;
@@ -220,7 +220,7 @@ Input::Input(
this->shape = torch_tensorrt::core::util::toVec(
torch_tensorrt::core::ir::Input(this->min_shape, this->opt_shape, this->max_shape).input_shape);
this->dtype = dtype;
- if(dtype == DataType::kBool && format == TensorFormat::kChannelsLast){
+ if (dtype == DataType::kBool && format == TensorFormat::kChannelsLast) {
TORCHTRT_THROW_ERROR("Input datatype (Bool) is not currently supported with Tensorformat NHWC (kChannelsLast)");
}
this->format = format;
@@ -250,7 +250,7 @@ Input::Input(
this->shape = torch_tensorrt::core::util::toVec(
torch_tensorrt::core::ir::Input(this->min_shape, this->opt_shape, this->max_shape).input_shape);
this->dtype = dtype;
- if(dtype == DataType::kBool && format == TensorFormat::kChannelsLast){
+ if (dtype == DataType::kBool && format == TensorFormat::kChannelsLast) {
TORCHTRT_THROW_ERROR("Input datatype (Bool) is not currently supported with Tensorformat NHWC (kChannelsLast)");
}
this->format = format;
@@ -263,7 +263,7 @@ Input::Input(at::Tensor tensor) {
this->max_shape = tensor.sizes().vec();
this->shape = tensor.sizes().vec();
this->dtype = tensor.scalar_type();
- if(dtype == DataType::kBool && format == TensorFormat::kChannelsLast){
+ if (dtype == DataType::kBool && format == TensorFormat::kChannelsLast) {
TORCHTRT_THROW_ERROR("Input datatype (Bool) is not currently supported with Tensorformat NHWC (kChannelsLast)");
}
TORCHTRT_ASSERT(
@@ -282,7 +282,7 @@ Input::Input(at::Tensor tensor) {
/* ==========================================*/
torch_tensorrt::core::ir::Input to_internal_input(Input& i) {
- if(i.dtype == DataType::kBool && i.format == TensorFormat::kChannelsLast){
+ if (i.dtype == DataType::kBool && i.format == TensorFormat::kChannelsLast) {
TORCHTRT_THROW_ERROR("Input datatype (Bool) is not currently supported with Tensorformat NHWC (kChannelsLast)");
}
return torch_tensorrt::core::ir::Input(
ERROR: Some files do not conform to style guidelines
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.
Code conforms to Python style guidelines
Signed-off-by: Dheeraj Peri <[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.
Code conforms to C++ style guidelines
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.
Code conforms to Python style guidelines
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
Signed-off-by: Dheeraj Peri [email protected]
Description
Use user provided dtype when we can't infer it from the graph
Fixes #911 (comment)
Type of change
Checklist: