Implement Optional Metadata support and C# test support #15314

yuslepukhin · 2023-03-31T21:01:10Z

Description

Implement Optional Type metadata support in the library.
Implement optional support in C# API along with metadata.
Implement Sequence, Map, Optional test data support
and test execution.

Prune tests and provide more details for failing tests in C# code.

Note, this PR does not enable running onnx test models in C++.

Motivation and Context

Opset18 optional type support.

onnxruntime/core/framework/onnxruntime_typeinfo.h

csharp/src/Microsoft.ML.OnnxRuntime/Microsoft - Backup.ML.OnnxRuntime.csproj

Add native methods from the merge Add Test Protobuf data Implement test sequence input loading Optimize Input/Output names conversion and validation Introduce OnnxValue to NamedOnnxValue, rename NativeOnnxTensorMemory Rework ToOrtValue interface Implement ManagedProjections Make sure all required map types are supported Generate input OrtValue using ManagedOnnxType Implement optional support, partial map support. Fix optional issues. Provide details for failing tests Comment out two tests due to invalid test data

Provide a workaround for keras_prelu_ImageNet_small

csharp/src/Microsoft.ML.OnnxRuntime/InferenceSession.shared.cs

yuslepukhin · 2023-04-04T18:30:25Z

public class DisposableNamedOnnxValue : NamedOnnxValue, IDisposable

Refactor the code to be able to feed data into a specified NamedOnnxValue #Closed

Refers to: csharp/src/Microsoft.ML.OnnxRuntime/DisposableNamedOnnxValue.shared.cs:65 in 2e11189. [](commit_id = 2e11189, deletion_comment = False)

csharp/src/Microsoft.ML.OnnxRuntime/OrtValue.shared.cs

yuslepukhin · 2023-04-04T18:39:46Z

Doxyfile 1.9.4

Performed an upgrade with the Doxygen version to match the one used in production #Resolved

Refers to: tools/ci_build/github/Doxyfile_csharp.cfg:1 in 2e11189. [](commit_id = 2e11189, deletion_comment = False)

pranavsharma · 2023-04-05T19:45:44Z

include/onnxruntime/core/session/onnxruntime_c_api.h

-  OrtOpenVINOProviderOptions() : device_type{}, enable_vpu_fast_compile{}, device_id{},
-                                 num_of_threads{}, cache_dir{},
-                                 context{}, enable_opencl_throttling{}, enable_dynamic_shapes{} {}
+  OrtOpenVINOProviderOptions() : device_type{}, enable_vpu_fast_compile{}, device_id{}, num_of_threads{}, cache_dir{}, context{}, enable_opencl_throttling{}, enable_dynamic_shapes{} {}


Any reason for changing the formatting? #Pending

VS automatically formatted

nit: 120 char limit

VS is not your master. You can beat it!

Setup clang-format with format on save and you can more easily play around with what it wants to keep line breaks so the line isn't too long. it's painful about these initializer lists and wants them all to be one line or individual lines. Add a line break after : device_type() and it will split them to the latter.

My understanding, it was clang-format that does it to me. It did not notify.

Our clang-format's config file doesn't wrap long times, and Github's PR review UI does not show such problems too. So they are easy to become unnoticed. If it often bothers, shall we consider to update the config file?

onnxruntime/core/framework/tensor_type_and_shape.h

pranavsharma · 2023-04-05T22:33:41Z

onnxruntime/core/framework/tensor_type_and_shape.h

+    return std::make_unique<OrtTensorTypeAndShapeInfo>(*this);
+  }
+
+  OrtTensorTypeAndShapeInfo(const OrtTensorTypeAndShapeInfo& other) = default;


Did you intend to mark them as deleted? #Pending

No, the code is correct. Clone() in this case makes use of the copy __ctor. It is a code pattern to use Clone in TypeInfo impl.

Not following. What's the reason for undeleting the copy ctor and copy assignment operator? If they need to be undeleted, why bother with Clone?

I could maybe understand if it the copy ctor was private so there were no accidental copies, but it isn't, so Clone doesn't appear to add any value as the user can copy construct a new instance directly anyway.

I will make it private the copy _ctor private. We need to make a copy, but we need it on the heap. At least that's how the structure of the code has been,
Copy _ctor is a natural place to make a copy, because the class itself does not have clonable members, Clone() is to make it on the heap. What is it here not to understand? :)

Making the copy ctor private should fix it.

Can't make it private because std::make_unique requires them to be accessible.
Frankly speaking, there is not a good reason to make them private.

The existence of Clone() still seems superfluous to me given that I can still make a copy of the object on the heap/stack with a public copy ctor. I suppose you can still return a unique_ptr using 'new' without making the copy ctor and assignment op public. But I get that usage of 'new' is getting flagged. In that case, can we at least document in the header file that Clone() exists only to satisfy existing patterns?

skottmckay

Have only had time to review a small part of the PR.

skottmckay · 2023-04-06T06:28:35Z

include/onnxruntime/core/session/onnxruntime_c_api.h

-  OrtOpenVINOProviderOptions() : device_type{}, enable_vpu_fast_compile{}, device_id{},
-                                 num_of_threads{}, cache_dir{},
-                                 context{}, enable_opencl_throttling{}, enable_dynamic_shapes{} {}
+  OrtOpenVINOProviderOptions() : device_type{}, enable_vpu_fast_compile{}, device_id{}, num_of_threads{}, cache_dir{}, context{}, enable_opencl_throttling{}, enable_dynamic_shapes{} {}


nit: 120 char limit

VS is not your master. You can beat it!

Setup clang-format with format on save and you can more easily play around with what it wants to keep line breaks so the line isn't too long. it's painful about these initializer lists and wants them all to be one line or individual lines. Add a line break after : device_type() and it will split them to the latter.

skottmckay · 2023-04-06T06:35:29Z

onnxruntime/core/framework/tensor_type_and_shape.h

+    return std::make_unique<OrtTensorTypeAndShapeInfo>(*this);
+  }
+
+  OrtTensorTypeAndShapeInfo(const OrtTensorTypeAndShapeInfo& other) = default;


I could maybe understand if it the copy ctor was private so there were no accidental copies, but it isn't, so Clone doesn't appear to add any value as the user can copy construct a new instance directly anyway.

skottmckay · 2023-04-06T09:11:50Z

onnxruntime/core/framework/tensor_type_and_shape.cc

-    OrtApis::ReleaseTensorTypeAndShapeInfo(ret);
-    return status;
-  }
+OrtTensorTypeAndShapeInfo::Ptr OrtTensorTypeAndShapeInfo::GetTensorShapeAndTypeHelper(ONNXTensorElementDataType type, onnxruntime::TensorShape shape,


nit: line length in a lot of places.

https://marketplace.visualstudio.com/items?itemName=PaulHarrington.EditorGuidelinesPreview can be used to add a vertical ruler at 120 chars #Pending

skottmckay · 2023-04-06T09:18:54Z

onnxruntime/core/framework/tensor_type_and_shape.h

 struct OrtTensorTypeAndShapeInfo {
 public:
+  using Ptr = std::unique_ptr<OrtTensorTypeAndShapeInfo>;


nit: IMHO this alias unnecessarily abstracts what sort of pointer is involved, which makes it harder to understand code that uses it, as it's not clear if it's a raw pointer, unique pointer or shared pointer. someone reviewing the usage code also has to go and find this using statement to discover it's a unique_ptr and no call to 'delete' was required.

IIRC you updated a bunch of places where IAllocatorPtr was unnecessarily passed by value, because the fact it was a shared_ptr was hidden by the alias and the cost of the reference count increment wasn't clear. This alias creates that same sort of issue. #Pending

I do not recall any shared_ptr in the past.
I felt this change would make the code more robust.
It did need to allocate using smart pointers and it did not need to return 'no so smart' OrtStatus, because this code itself is not a public API.

Ptr is to save on typing. So you suggest to eliminate the typedef?

Ah, now I remember.

onnxruntime/core/framework/tensor_type_and_shape.cc

onnxruntime/core/framework/onnxruntime_map_type_info.h

onnxruntime/core/framework/onnxruntime_map_type_info.cc

csharp/src/Microsoft.ML.OnnxRuntime/NamedOnnxValue.shared.cs

csharp/src/Microsoft.ML.OnnxRuntime/DisposableNamedOnnxValue.shared.cs

yuslepukhin · 2023-04-06T18:22:03Z

/// It extends NamedOnnxValue, exposes the OnnxValueType and Tensor type

remove

In reply to: 1499452385

Refers to: csharp/src/Microsoft.ML.OnnxRuntime/DisposableNamedOnnxValue.shared.cs:59 in 2e9c365. [](commit_id = 2e9c365, deletion_comment = False)

csharp/src/Microsoft.ML.OnnxRuntime/NamedOnnxValue.shared.cs

csharp/src/Microsoft.ML.OnnxRuntime/DisposableNamedOnnxValue.shared.cs

csharp/src/Microsoft.ML.OnnxRuntime/InferenceSession.shared.cs

csharp/test/Microsoft.ML.OnnxRuntime.Tests.NetCoreApp/InferenceTest.netcore.cs

pranavsharma · 2023-04-10T22:30:02Z

onnxruntime/core/framework/onnxruntime_map_type_info.cc

+    const ONNX_NAMESPACE::TypeProto& type_proto) {
+  auto value_case = type_proto.value_case();
+  if (value_case != ONNX_NAMESPACE::TypeProto::kMapType) {
+    ORT_THROW("type_proto is not of type map!");


Sequence uses ORT_ENFORCE and this uses ORT_THROW. Any reason they should be different?

pranavsharma · 2023-04-10T23:09:31Z

onnxruntime/core/framework/tensor_type_and_shape.h

+    return std::make_unique<OrtTensorTypeAndShapeInfo>(*this);
+  }
+
+  OrtTensorTypeAndShapeInfo(const OrtTensorTypeAndShapeInfo& other) = default;


The existence of Clone() still seems superfluous to me given that I can still make a copy of the object on the heap/stack with a public copy ctor. I suppose you can still return a unique_ptr using 'new' without making the copy ctor and assignment op public. But I get that usage of 'new' is getting flagged. In that case, can we at least document in the header file that Clone() exists only to satisfy existing patterns?

rebase off main

ae93f2e

yuslepukhin requested review from skottmckay, pranavsharma, natke, edgchen1 and snnn March 31, 2023 21:01

yuslepukhin commented Mar 31, 2023

View reviewed changes

onnxruntime/core/framework/onnxruntime_typeinfo.h Outdated Show resolved Hide resolved

yuslepukhin commented Mar 31, 2023

View reviewed changes

csharp/src/Microsoft.ML.OnnxRuntime/Microsoft - Backup.ML.OnnxRuntime.csproj Outdated Show resolved Hide resolved

yuslepukhin force-pushed the yuslepukhin/optional_type_info branch from 77b5d79 to 9fb3811 Compare April 3, 2023 01:16

yuslepukhin marked this pull request as ready for review April 3, 2023 17:07

yuslepukhin added 2 commits April 3, 2023 17:51

Make test_BERT_Squad test work

6587309

Provide a workaround for keras_prelu_ImageNet_small

Merge branch 'main' into yuslepukhin/optional_type_info

2e11189

yuslepukhin requested a review from tianleiwu April 4, 2023 16:40

yuslepukhin commented Apr 4, 2023

View reviewed changes

csharp/src/Microsoft.ML.OnnxRuntime/InferenceSession.shared.cs Show resolved Hide resolved

yuslepukhin commented Apr 4, 2023

View reviewed changes

csharp/src/Microsoft.ML.OnnxRuntime/InferenceSession.shared.cs Outdated Show resolved Hide resolved