Skip to content
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

Support string dtype for OV backend #2602

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Support string dtype for OV backend #2602

merged 4 commits into from
Mar 26, 2024

Conversation

andrey-churkin
Copy link
Contributor

@andrey-churkin andrey-churkin commented Mar 26, 2024 •
edited
Loading

Changes

  • Support string dtype for OV backend

Reason for changes

NotImplementedError: NNCF is not yet supported OpenVINO data type: string.

Related tickets

136751

Tests

  • test_convert_to_nncf_dtype_supported_types()
  • test_convert_to_nncf_dtype_unsupported_types()

@andrey-churkin andrey-churkin requested a review from a team as a code owner March 26, 2024 10:03
@github-actions github-actions bot added NNCF Common Pull request that updates NNCF Common NNCF OpenVINO Pull requests that updates NNCF OpenVINO labels Mar 26, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 84.84%. Comparing base (b7ba5ad) to head (38ec911).
Report is 3 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2602      +/-   ##
===========================================
- Coverage    91.16%   84.84%   -6.32%     
===========================================
  Files          494      494              
  Lines        45350    45418      +68     
===========================================
- Hits         41342    38536    -2806     
- Misses        4008     6882    +2874
Files Coverage Δ
nncf/openvino/graph/nncf_graph_builder.py 0.00% <0.00%> (-99.10%) ⬇️

... and 62 files with indirect coverage changes

Flag Coverage Δ
COMMON 44.15% <ø> (ø)
ONNX 34.66% <0.00%> (+0.01%) ⬆️
OPENVINO ∅ <ø> (∅)
TENSORFLOW 30.10% <0.00%> (-0.02%) ⬇️
TORCH 65.92% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
common 93.13% <ø> (-0.66%) ⬇️
torch 93.39% <ø> (-0.09%) ⬇️
tensorflow 93.74% <ø> (ø)
onnx 92.33% <ø> (-0.70%) ⬇️
openvino 25.76% <0.00%> (-68.38%) ⬇️
ptq 69.55% <ø> (-20.58%) ⬇️

@andrey-churkin andrey-churkin requested a review from alexsu52 March 26, 2024 10:06
rkazants
rkazants reviewed Mar 26, 2024
View reviewed changes
nncf/common/graph/layer_attributes.py Outdated
@@ -19,6 +19,7 @@
class Dtype(Enum):
FLOAT = "float"
INTEGER = "int"
STRING = "string"
Copy link
Member

Choose a reason for hiding this comment

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

may you have test for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added tests to be sure that we cover all available OpenVINO data types.

@DaniAffCH
Copy link
Contributor

Hi @andrey-churkin could you please add the inverse mapping (from NNCF dtype to OV dtype) in the function convert_nncf_dtype_to_ov_dtype within nncf/experimental/common/graph/netron.py.
Otherwise, the Netron export won't work for networks containing string as dtype

@github-actions github-actions bot removed the NNCF Common Pull request that updates NNCF Common label Mar 26, 2024
@andrey-churkin
Copy link
Contributor Author

@DaniAffCH Thanks! I have updated the code recently. So everything is fine now.

@andrey-churkin andrey-churkin requested a review from rkazants March 26, 2024 11:37
alexsu52
alexsu52 approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@alexsu52 alexsu52 left a comment

Choose a reason for hiding this comment

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

LGTM

@andrey-churkin andrey-churkin merged commit f2f3bb7 into openvinotoolkit:develop Mar 26, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexsu52 alexsu52 alexsu52 approved these changes

@rkazants rkazants Awaiting requested review from rkazants

Assignees
No one assigned
Labels
NNCF OpenVINO Pull requests that updates NNCF OpenVINO
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andrey-churkin @DaniAffCH @alexsu52 @rkazants