-
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
base: ovep-develop
Are you sure you want to change the base?
Conversation
|
Looks good to me. I will comment in the mainline about the tests. |
MayureshV1
left a comment
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.
Looks good.
Dmitri from ORT team has also reviews the change.
|
This implementation would fail the test case in case single ops fallback to running on default CPU EP. needs more time to rework. Converting to draft. For now, we will revert these test change. PR #883 FYI. @yuslepukhin |
8ec98b6 to
6ae5d7e
Compare
0649789 to
4cebc5d
Compare
| // so drop the part that differs from the expected string | ||
| "kernel_shape num_dims is not compatible with W num_dims. kernel_shape: {1,1,1,5} W: {1,1,", | ||
| {kTensorrtExecutionProvider, kQnnExecutionProvider, | ||
| kDmlExecutionProvider}); // TODO: Unskip when fixed #41968513 |
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.
Would you like to comment on the reason for exclusion?
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.
This is caused by the latest OV commits add some logic to make this pattern work. But V2025.4 can pass it. I will report a new issue to get this test pass in the next release.
| #ifdef USE_OPENVINO | ||
| TEST(QuantizeLinearOpTest, OVEP_Int8_PositiveZeroPoint) { | ||
| // TODO: Unskip when fixed #41968513 | ||
| if (DefaultDmlExecutionProvider().get() != nullptr) { |
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.
This test would not run in builds where DML is present.
Meaning OpenVino EP would also not be exercised.
I wonder why DML would be involved if the configured EP below only has OpenVino?
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.
Just copied from the previous test and I will delete it and check all modified test case.
| // CUDA: result mismatch due to not implementing NHWC support | ||
| test.Run(OpTester::ExpectResult::kExpectSuccess, "", | ||
| {kCudaExecutionProvider, kCudaNHWCExecutionProvider}); | ||
| {kCudaExecutionProvider, kCudaNHWCExecutionProvider, kOpenVINOExecutionProvider}); |
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.
Would you like to comment on OpenVino exclusion reasons?
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.
Updated
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.
Pull request overview
This PR addresses provider test issues identified during code review by updating OpenVINO execution provider test exclusions and support configurations. The changes primarily involve migrating from deprecated test APIs to newer configuration methods and adding OpenVINO-specific test variants with adjusted expected outputs.
Key changes:
- Migrated test exclusion patterns from deprecated
test.Run()parameter toConfigExcludeEps()andRunWithConfig()API - Added OpenVINO-specific test cases with adapted expected values due to different quantization rounding behaviors
- Updated OpenVINO capability definitions to enable previously disabled operations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| slice_op.test.cc | Added OpenVINO to excluded providers for tests with ONNX shape disagreement |
| resize_op_test.cc | Migrated to new exclusion API and added OVEP-specific test variants for uint8 resize operations |
| quantize_linear_test.cc | Migrated to new exclusion API and added OVEP-specific quantization tests with adjusted expected outputs |
| cast_op_test.cc | Added exclusion for OpenVINO when input size is zero |
| loop_test.cc | Added OpenVINO to excluded providers for iteration count test |
| quantize_ops_test.cc | Migrated contrib op tests to new API and added OVEP-specific test variants |
| data_ops.cc | Removed operations from model-only support list and added UINT32 type support |
Comments suppressed due to low confidence (1)
onnxruntime/test/contrib_ops/quantize_ops_test.cc:1
- The expected output differs from the original test (line 535 expects
Int4x2(-1, 1)while this OVEP variant expectsInt4x2(0, 1)for the second pair). Add a comment explaining why OpenVINO's Int4 quantization produces a different value for the-3.0finput.
// Copyright (c) Microsoft Corporation. All rights reserved.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "TopK", | ||
| "Trilu"}; | ||
| "TopK" | ||
| }; |
Copilot
AI
Dec 16, 2025
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.
Inconsistent trailing whitespace before closing brace. The closing brace on line 52 should align with the opening of ops_supported_only_in_model without extra spaces, matching the style of other set initializations in the file.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 8b81a24.
| [&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; |
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.
| 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 comment
The 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?
Description
Motivation and Context
https://jira.devtools.intel.com/browse/CVS-178009