-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Enable TRT provider option configuration for C# (updated version) #7808
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
…rosoft/onnxruntime into c_sharp_trt_provider_options
|
/azp run orttraining-linux-ci-pipeline,orttraining-mac-ci-pipeline,orttraining-linux-gpu-ci-pipeline,centos7_cpu,Linux CPU Minimal Build E2E CI Pipeline,Linux Nuphar CI Pipeline,MacOS NoContribops CI Pipeline,Linux OpenVINO CI Pipeline,orttraining-distributed, orttraining-amd-gpu-ci-pipeline |
|
Pull request contains merge conflicts. |
|
|
||
| virtual void* GetInfo() { return nullptr; } // Returns a provider specific information interface if it exists | ||
|
|
||
| virtual const ProviderOptions GetProviderOptions() { return {}; } // Returns a provider specific information interface if it exists |
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 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.
fixed.
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
| // | ||
| #include "core/framework/provider_options.h" |
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.
Can you forward declare it here? As a reminder, you can use forward declarations in the folowing cases:
- pointer 2) reference 3) return by value
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.
Is it forward declare here?
In provider_options.h, I think they are all complete type:
using ProviderOptions = std::unordered_map<std::string, std::string>;
using ProviderOptionsVector = std::vector;
using ProviderOptionsMap = std::unordered_map<std::string, ProviderOptions>;
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.
ProviderOptions can be forward declared in this header. Under onnxruntime you can say
struct/class ProviderOptions.
| /** | ||
| * Use this API to create the configuration of a TensorRT Execution Provider. | ||
| */ | ||
| ORT_API2_STATUS(CreateTensorRTProviderOptions, _Outptr_ OrtTensorRTProviderOptions** out); |
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.
Please, document all the params and state how to destroy the instance when done.
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. and move the definitions to tensorrt_provider_factory.h
| * Get configuration of a TensorRT Execution Provider. | ||
| * \param allocator - a ptr to an instance of OrtAllocator obtained with CreateAllocator() or GetAllocatorWithDefaultOptions() | ||
| * the specified allocator will be used to allocate continuous buffers for output strings and lengths. | ||
| * \param ptr - is a null terminated string allocated using 'allocator'. The caller is responsible for freeing it. |
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 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. and move the definitions to tensorrt_provider_factory.h
pranavsharma
left a comment
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.
See comment.
onnxruntime/core/providers/tensorrt/tensorrt_provider_factory.cc
Outdated
Show resolved
Hide resolved
| trt_options.trt_int8_calibration_table_name = new char[internal_options.int8_calibration_table_name.size() + 1]; | ||
| strcpy((char*)trt_options.trt_int8_calibration_table_name, internal_options.int8_calibration_table_name.c_str()); | ||
| trt_options.trt_int8_calibration_table_name = new char[str_size + 1]; | ||
| strncpy((char*)trt_options.trt_int8_calibration_table_name, internal_options.int8_calibration_table_name.c_str(), str_size + 1); |
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.
strncpy won't add the null char. I would change this to strncy(dest, src, str_size); and then dest[str_size] = '\0';
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.
I was wondering why strncpy won't add the null char?
In this case, the size of src is always same as str_size, so strncpy will always copy the null char of src and then stop due to str_size + 1 is reached. (Also, the c_str() returns char array with null char, so src contains null char at the end)
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.
I think it's better to be explicit.
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.
Agree to be explicit in order to avoid potential null termination bug.
Description: Enable TRT provider option configuration for C#
Use key-value strings to configure TRT EP provider options and create session options with TRT EP specific configurations:
After configuration, call GetOptions() to get provider options as c# string to check
Note: This PR is the modification of #7179