Skip to content

[OVEP] Fix for precision accuracy #603

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 11, 2025
Merged

[OVEP] Fix for precision accuracy #603

merged 1 commit into from
Mar 11, 2025

Conversation

@jatinwadhwa921
Copy link

@jatinwadhwa921 jatinwadhwa921 commented Mar 6, 2025 •
edited by MayureshV1
Loading

This PR handles precision for devices "AUTO:,MULTI: and HETERO:"

HAFP-2913

preetha-intel
preetha-intel reviewed Mar 6, 2025
View reviewed changes
@jatinwadhwa921 jatinwadhwa921 force-pushed the jatin_fix_precison_acc_issue branch from 9967507 to f5428af Compare March 7, 2025 07:08
sfatimar
sfatimar reviewed Mar 7, 2025
View reviewed changes
sfatimar
sfatimar reviewed Mar 7, 2025
View reviewed changes
@sfatimar
Copy link

sfatimar commented Mar 7, 2025

Please move parsing out of the main file.

@sfatimar
Copy link

sfatimar commented Mar 7, 2025

Tested with AUTO, Multi Heter, GPU and CPU and NPU with Accuracy . AUTO:GPU, CPU. Need to check for GPU.0 1and GPU.1

preetha-intel
preetha-intel reviewed Mar 7, 2025
View reviewed changes
onnxruntime/core/providers/openvino/openvino_provider_factory.cc Outdated
<< " HETERO/MULTI/AUTO options and set 'precision' separately. \n";
auto delimit = device_type.find("_");
device_type = device_type.substr(0, delimit);
return device_type.substr(delimit + 1);
Copy link

@preetha-intel preetha-intel Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it directly returns the precision passed by the user or there is a validity check?

Copy link
Author

@jatinwadhwa921 jatinwadhwa921 Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No validity check, just return whatever is written after "_"

@jatinwadhwa921 jatinwadhwa921 force-pushed the jatin_fix_precison_acc_issue branch from f5428af to 204163d Compare March 7, 2025 12:07
@jatinwadhwa921
Copy link
Author

jatinwadhwa921 commented Mar 7, 2025

Please move parsing out of the main file.

fixed

@MayureshV1
Copy link

MayureshV1 commented Mar 7, 2025

@sfatimar , @jatinwadhwa921 - Please do not merge till there is a corresponding test written and merged for the Accuracy feature.

@sfatimar
Copy link

sfatimar commented Mar 10, 2025

Approved from my side.

sfatimar
sfatimar approved these changes Mar 10, 2025
View reviewed changes
Copy link

@sfatimar sfatimar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@sfatimar
Copy link

sfatimar commented Mar 10, 2025

Please make sure to rebase it

@jatinwadhwa921 jatinwadhwa921 force-pushed the jatin_fix_precison_acc_issue branch from 204163d to 61b36ef Compare March 10, 2025 12:52
@jatinwadhwa921
Copy link
Author

jatinwadhwa921 commented Mar 10, 2025

@sfatimar , @jatinwadhwa921 - Please do not merge till there is a corresponding test written and merged for the Accuracy feature.

I have tested it from my end, it works fine, test case has been added in the repo

@sfatimar sfatimar merged commit ea13a05 into ovep-develop Mar 11, 2025
6 of 12 checks passed
@MayureshV1
Copy link

MayureshV1 commented Mar 11, 2025 •
edited
Loading

@jatinwadhwa921 . Can you please share the tests with me and @preetha-intel.

These are the 3 scenarios that should be covered in validation for mixed precision models (FP32/ FP16):

  1. Objective accuracy test to ensure FP32 like accuracy is retained once Accuracy mode gets set correctly in OV
  2. Performance test to ensure perf is better than FP32 and closer to FP16 execution on GPU- Accuracy mode actually picks based on model precision not FP32 static
  3. Regression testing- Perf comparison between OV EP generations (if possible) to ensure the behavior has not changed between two OV and OV EP versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MayureshV1 MayureshV1 MayureshV1 left review comments

@preetha-intel preetha-intel preetha-intel left review comments

@sfatimar sfatimar sfatimar approved these changes

@ankitm3k ankitm3k Awaiting requested review from ankitm3k

@vthaniel vthaniel Awaiting requested review from vthaniel

@saurabhkale17 saurabhkale17 Awaiting requested review from saurabhkale17

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jatinwadhwa921 @sfatimar @MayureshV1 @preetha-intel