-
Notifications
You must be signed in to change notification settings - Fork 56
CVS-178009: Fix provider tests based on code review #880
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
Changes from 11 commits
c572ba2
8952ea3
790e552
4cebc5d
bf10bd2
8b81a24
850f17b
71e1247
7514048
ac1e9f3
81ba841
d0916b9
23a0364
c8045f2
d462616
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,8 @@ struct OnnxToOvNetworkBindings { | |
| [&onnx_name](const auto& ov_parameter_info) { return ov_parameter_info.get_names().contains(onnx_name); }); | ||
| bool matched_names = it != ov_parameters.end(); | ||
|
|
||
| if (it == ov_parameters.end()) continue; | ||
|
|
||
| // For Stateful Model Compilation, the ONNX model includes KV cache (past/present) tensors. | ||
| // However, these tensors are internally converted to a stateful representation, which removes them. | ||
| // It's also possible that the onnx model does not contain tensors such as beam_idx, whereas our converted | ||
|
|
@@ -110,6 +112,11 @@ struct OnnxToOvNetworkBindings { | |
|
|
||
| info.SetFullyDynamic(has_fully_dynamic); | ||
| info.SetBoundedDynamic(has_bounded_dynamic); | ||
| } else { | ||
| auto shape_size = ov::shape_size(shape.get_shape()); | ||
| if (0 == shape_size) { | ||
| has_dynamic_io_ = true; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate a bit more on this case? The shape came from the OV compiled model, and the shape is reported as static but has a zero dimension? Or maybe no dimension? Does that actually make it dynamic?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another way is we can disable this test case due to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer we don't have specific handling in i/o binding for it a case ov doesn't support, but if we keep it, just comment why it's there so it can be more easily evaluated if we want to change it in the future. |
||
| } | ||
|
|
||
| input_output_map.push_back(std::move(info)); | ||
|
|
||

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.
If we can't match onnx io names to ov io names we shouldn't just skip it. We should fail.
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.
In this case, the OV has optimized out this input because it is unused. How to handle this case?
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.
Hmm, in that case, perhaps we only allow skipping if there's fewer ov i/o parameters than onnx. We should also log a warning that we couldn't find the onnx i/o name in the resulting ov compiled model and that it may have been optimized out.
Thoughts?
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.
Can we add a mechanism in OV ONNX FE to avoid the parameter be optimized out? This need more investigation to research how to keep the unused
ParameterThere 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 mean sure, ideally, OV doesn't remove I/O but in the time between now and a potential future that it doesn't, I'd prefer that we don't silently skip binding I/O.
As an aside, do we know the FE is removing the input? Or are you proposing that there be ONNX FE setting that prevents later passes from removing model I/O?
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 have tried to remove the code in OV ONNX FE, the
remove_dangling_parameterswill delete the floating parameters in the model. Because the unused parameter may0-sizedbuffer, which will have an issue when allocating the tensors. How about just output some warnings?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.
@sgbihu , If we need an OV change we will not intercept this for ORT1.24 and can plan those test changes later.
Any changes we make to the test, Can we ensure functionality with OV 2025.4.