Skip to content
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

Implement Optional Metadata support and C# test support #15314

Merged
merged 12 commits into from
Apr 11, 2023

Conversation

yuslepukhin
Copy link
Member

@yuslepukhin yuslepukhin commented Mar 31, 2023

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.

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
@yuslepukhin yuslepukhin force-pushed the yuslepukhin/optional_type_info branch from 77b5d79 to 9fb3811 Compare April 3, 2023 01:16
@yuslepukhin yuslepukhin marked this pull request as ready for review April 3, 2023 17:07
@yuslepukhin yuslepukhin requested a review from tianleiwu April 4, 2023 16:40
@yuslepukhin
Copy link
Member Author

yuslepukhin commented Apr 4, 2023

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)

@yuslepukhin
Copy link
Member Author

yuslepukhin commented Apr 4, 2023

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)

snnn
snnn previously approved these changes Apr 4, 2023
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{} {}
Copy link
Contributor

@pranavsharma pranavsharma Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for changing the formatting? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS automatically formatted

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@yuslepukhin yuslepukhin Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
return std::make_unique<OrtTensorTypeAndShapeInfo>(*this);
}

OrtTensorTypeAndShapeInfo(const OrtTensorTypeAndShapeInfo& other) = default;
Copy link
Contributor

@pranavsharma pranavsharma Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to mark them as deleted? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@yuslepukhin yuslepukhin Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the copy ctor private should fix it.

Copy link
Member Author

@yuslepukhin yuslepukhin Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pranavsharma pranavsharma Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@skottmckay skottmckay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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{} {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return std::make_unique<OrtTensorTypeAndShapeInfo>(*this);
}

OrtTensorTypeAndShapeInfo(const OrtTensorTypeAndShapeInfo& other) = default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

OrtApis::ReleaseTensorTypeAndShapeInfo(ret);
return status;
}
OrtTensorTypeAndShapeInfo::Ptr OrtTensorTypeAndShapeInfo::GetTensorShapeAndTypeHelper(ONNXTensorElementDataType type, onnxruntime::TensorShape shape,
Copy link
Contributor

@skottmckay skottmckay Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

struct OrtTensorTypeAndShapeInfo {
public:
using Ptr = std::unique_ptr<OrtTensorTypeAndShapeInfo>;
Copy link
Contributor

@skottmckay skottmckay Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@yuslepukhin yuslepukhin Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I remember.

@yuslepukhin
Copy link
Member Author

yuslepukhin commented Apr 6, 2023

/// 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)

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!");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return std::make_unique<OrtTensorTypeAndShapeInfo>(*this);
}

OrtTensorTypeAndShapeInfo(const OrtTensorTypeAndShapeInfo& other) = default;
Copy link
Contributor

@pranavsharma pranavsharma Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@yuslepukhin yuslepukhin merged commit ce3b4ea into main Apr 11, 2023
@yuslepukhin yuslepukhin deleted the yuslepukhin/optional_type_info branch April 11, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants