forked from microsoft/onnxruntime
-
Notifications
You must be signed in to change notification settings - Fork 55
Fix onnxruntime 0D tensor issue on openvino endpoint #816
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
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
The decision to put true or false in this file is not clear to me. The type_protos here are not from the original protobuf, but rather from generic type registrations. How do we know that this particular tensor had real shape in the original protobuf since all we have here is the OrtValue which obviously has a real shape.
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 set this value based on http://github.com/intel/onnxruntime/blob/ovep-develop/include/onnxruntime/core/framework/tensor_shape.h#L109 , because the tensor doesn't have an API to indicate this is a scalar. Do you know an API can do this?
And I think the purpose of adding the
has_shape
is to fix the information lost at model conversion stage. If we already have the shape, it is not a dynamic rank case. I think this can get the correct shapes when dumping models.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 there is a mix of two different things here.
Tensor
andTensorShape
classes are runtime classes. They will always have a shape that will be inferred, otherwise we cannot run.IsScalar()
is the method onTensorShape
that will tell you if it is a scalar or not based on the shape that was inferred or contained (not exposed via public API) since it can also be found out from the shape vector returned.The public APIs that are you are dealing with have to do with specific concrete OrtValues that contain tensors that are a result of either creating a feeding input to the model or receiving an output. So they do not have anything to do with the protobuf of the original model. The same would apply to intermediate values, which is a new path that appeared with EP plug-ins.
They are not direct counterparts of what is found in protobuf which is a metadata and this is what you are probably trying to get.
It would be helpful to see an example of the model that you are trying to deal with and what exactly you are trying to achieve (serialize the model?)
As far as the model input/output goes, Session API can return ValueInfo that contains TypeInfo. That can tell you the dimension count but does not tell you if it has the shape, perhaps it is an omission.
Uh oh!
There was an error while loading. Please reload this page.
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.
As far as I understand, an OrtValue is already known to have a shape, so passing true here makes sense to me.
The onnx model file has been provided in this comment: #816 (comment)
The plugin EP gets the model as a
OrtGraph
via the new APIs. The EP then converts theOrtGraph
to protobuf so that it can be further processed by openvino compiler, which currently only supports protobuf models (not OrtGraph). Please correct me if I'm wrong @sgbihuThere 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.
Yes, OV only handle the protobuf models for now. I also pasted a prototxt at the PR description, and the shape information is lost. That means a scalar to a dynamic rank tensor.