Skip to content

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.

Sign up for GitHub

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
Apr 9, 2022
Merged

fix: Use user provided dtype when we can't infer it from the graph #962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14650d1
fix: Use user provided dtype when we can't infer it from the graph
peri044 Apr 5, 2022
aed82a0
chore: Add bazel-TRTorch reference for users who haven't changed thei…
peri044 Apr 5, 2022
aa3f57a
chore: Handle valid dtype and tensorformat checks in cpp api level
peri044 Apr 8, 2022
b677e8d
chore: Revert previous changes
peri044 Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)};
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

} else {
if (util::TRTDataTypeToScalarType(cfg.convert_info.inputs.find(in)->second.dtype) != est_type_opt.value()) {
std::stringstream ss;
Expand Down
11 changes: 9 additions & 2 deletions core/ir/Input.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator

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

Copy link
Collaborator

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

case nvinfer1::TensorFormat::kLINEAR:
return true;
default:
return false;
}
default:
return false;
}
Expand All @@ -48,7 +55,7 @@ bool valid_dtype_format_combo(nvinfer1::DataType dtype, nvinfer1::TensorFormat f
bool valid_input_dtype(nvinfer1::DataType dtype) {
switch (dtype) {
case nvinfer1::DataType::kBOOL:
return false;
return true;
case nvinfer1::DataType::kFLOAT:
return true;
case nvinfer1::DataType::kHALF:
Expand Down Expand Up @@ -153,4 +160,4 @@ std::ostream& operator<<(std::ostream& os, const Input& input) {

} // namespace ir
} // namespace core
} // namespace torch_tensorrt
} // namespace torch_tensorrt
1 change: 0 additions & 1 deletion py/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,6 @@ def run(self):
include_dirs=[
dir_path + "torch_tensorrt/csrc", dir_path + "torch_tensorrt/include",
dir_path + "/../bazel-TRTorch/external/tensorrt/include",
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

dir_path + "/../bazel-Torch-TensorRT-Preview/external/tensorrt/include",
dir_path + "/../bazel-Torch-TensorRT/external/tensorrt/include", dir_path + "/../"
],
extra_compile_args=[
Expand Down
2 changes: 1 addition & 1 deletion py/torch_tensorrt/csrc/torch_tensorrt_py.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ PYBIND11_MODULE(_C, m) {
.value("float16", DataType::kHalf, "16 bit floating point number")
.value("int8", DataType::kChar, "8 bit integer number")
.value("int32", DataType::kInt32, "32 bit integer number")
.value("bool", DataType::kChar, "Boolean value")
.value("bool", DataType::kBool, "Boolean value")
.value("unknown", DataType::kUnknown, "Unknown data type")
.export_values();

Expand Down