-
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
Conversation
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.
Hi, thank you for looking into this. Could you please share more information on the kind of model that triggers this failure to distinguish between a scalar or a tensor with unspecified shape? I would have thought that onnxruntime would remove this ambiguity internally. A small repro model (or a description of one) would be very useful. Thanks!
|
||
if (dim_params != nullptr) { | ||
type_and_shape->dim_params = *dim_params; | ||
type_and_shape->has_shape = true; |
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.
Being able to set has_shape
to true when dim_params
is not null seems like a convenient coincidence. Perhaps it would be good to pass that information to this function? I haven't fully thought it through but perhaps something like the following could work:
- Add a
has_shape
parameter to thisGetTensorShapeAndTypeHelper
function. - Callers of
GetTensorShapeAndTypeHelper
can useonnx::TypeProto_Tensor::has_shape()
to pass this information toGetTensorShapeAndTypeHelper
.
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.
abs_lostdim_case.zip
This is the test model, you can use the abs_0d_input.onnx
as input. The abs_0d_lostdim.onnx
is the dumped model after serialization.
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.
For your suggestion, could you take a look at onnxruntime/core/framework/onnxruntime_typeinfo.cc:L269
, L285:L310
. Here already convert the HasShape
to nullptr
or a valid pointer. After you review that code, I can add it if you think a new parameter is necessary.
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.
Yes, I think a new parameter is better. Right now, using dim_params != nullptr
seems like a coincidence. When you upstream to ORT, others may have other suggestions, but I do think this is preferable.
Note that the line utils::HasShape()
internally calls onnx::Typeproto_Tensor::has_shape()
(as I pointed out in my original message). This seems like the value we want to pass down to GetTensorShapeAndType()
.
One other thing to note: the GetTensorShapeAndTypeHelper
function is also called in core/session/custom_ops.cc:L83
. This location also has access to onnx::TypeProto_Tensor::has_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.
Thanks again for fixing this and providing the model.
@sgbihu Do we really need this in intel development branch, I think you can directly raise to MSFT ? Do you need our team help to do that |
auto symbolic_dims = GetSymbolicDims(shape_proto); | ||
input_type_shapes_.emplace_back( | ||
OrtTensorTypeAndShapeInfo::GetTensorShapeAndTypeHelper(elem_type, tensor_shape, &symbolic_dims).release()); | ||
OrtTensorTypeAndShapeInfo::GetTensorShapeAndTypeHelper(elem_type, tensor_shape, &symbolic_dims, type_proto.has_shape()).release()); |
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.
utils::HasShape
const auto* tensor_data_type = tensor.DataType(); | ||
if (tensor_data_type != nullptr) { | ||
auto type_shape = OrtTensorTypeAndShapeInfo::GetTensorShapeAndType(tensor.Shape(), *tensor_data_type); | ||
auto type_shape = OrtTensorTypeAndShapeInfo::GetTensorShapeAndType(tensor.Shape(), *tensor_data_type, true); |
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
and TensorShape
classes are runtime classes. They will always have a shape that will be inferred, otherwise we cannot run.
IsScalar()
is the method on TensorShape
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.
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.
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?)
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 the OrtGraph
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 @sgbihu
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.
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.
This PR is being deprecated in favor of microsoft#26164 |
Description
The
OrtTensorTypeAndShapeInfo
struct lost the scalar information when converted from proto. The existing structure can't distinguish thedynamic rank
andscalar
cases. That makes the serialized graph lost shape area and the OV can't distinguish the scalar type.Original tensor in prototxt:
After serialized prototxt:
has_shape
forOrtTensorTypeAndShapeInfo
Original Model
The graph after
OrtEpUtils::OrtGraphToProto
Motivation and Context