Add Experimental API for setting model name #10518
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nums11
changed the title
smk2007
reviewed
Feb 17, 2022
martinb35
reviewed
Feb 17, 2022
martinb35
reviewed
Feb 17, 2022
| @@ -66,6 +66,9 @@ IModelInfo : IUnknown { | |||
| STDMETHOD(GetName) | |||
| (const char** out, size_t* len) PURE; | |||
|
|
|||
| STDMETHOD(SetName) | |||
| (const char* name) PURE; | |||
There was a problem hiding this comment.
is the convention that we expect the char* to be null terminated and therefore we don't need to pass the length?
There was a problem hiding this comment.
Yes, I assume the char* will be null terminated
martinb35
reviewed
Feb 24, 2022
| ORT_API_STATUS_IMPL(winmla::ModelSetName, _In_ const OrtModel* model, _In_ const char* const name) { | ||
| API_IMPL_BEGIN | ||
| auto model_proto = model->UseModelProto(); | ||
| ONNX_NAMESPACE::GraphProto& graph = *model_proto->mutable_graph(); |
There was a problem hiding this comment.
can the "mutable_graph()" call return null?
martinb35
reviewed
Feb 24, 2022
| @@ -279,6 +279,13 @@ LearningModel::OutputFeatures() try { | |||
| } | |||
| WINML_CATCH_ALL | |||
|
|
|||
| void LearningModel::SetName(const hstring& name) try { | |||
| auto name_std_str = _winml::Strings::UTF8FromHString(name); | |||
There was a problem hiding this comment.
what does this return when you pass in something that can't be represented in char*? like a wide-unicode character
martinb35
previously approved these changes
Feb 24, 2022
martinb35
previously approved these changes
Feb 25, 2022
smk2007
previously approved these changes
Feb 25, 2022
ryanlai2
approved these changes
Feb 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds the SetName function to the LearningModelExperimental API