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.

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

Align NPU to CPU #2560

Merged
merged 14 commits into from
May 6, 2024
Merged

Conversation

KodiaqQ
Copy link
Collaborator

@KodiaqQ KodiaqQ commented Mar 8, 2024

Changes

  • Aligned NPU configuration to CPU for 8bit quantization;

Reason for changes

  • NPU supports CPU configuration;
  • CPU configuration as the base;
  • Removed unused configurations (ConvolutionBackpropData, GroupConvolutionBackpropData);

Related tickets

  • 132512

Tests

  • TBD

@github-actions github-actions bot added the NNCF Common Pull request that updates NNCF Common label Mar 8, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

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

Project coverage is 29.94%. Comparing base (ac15a65) to head (e5cfe06).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #2560       +/-   ##
============================================
- Coverage    62.07%   29.94%   -32.13%     
============================================
  Files          494      494               
  Lines        45775    45791       +16     
============================================
- Hits         28413    13714    -14699     
- Misses       17362    32077    +14715     
Files Coverage Δ
nncf/common/utils/dot_file_rw.py 58.33% <0.00%> (-31.95%) ⬇️
nncf/quantization/algorithms/min_max/algorithm.py 20.17% <9.09%> (-76.10%) ⬇️

... and 262 files with indirect coverage changes

Flag Coverage Δ
COMMON ?
ONNX ?
OPENVINO ?
TENSORFLOW 29.94% <8.33%> (-0.01%) ⬇️

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

Components Coverage Δ
common 76.35% <0.00%> (-12.08%) ⬇️
torch 0.01% <ø> (-32.85%) ⬇️
tensorflow 93.74% <ø> (ø)
onnx 0.00% <ø> (-93.07%) ⬇️
openvino 0.00% <ø> (-94.23%) ⬇️
ptq 15.24% <9.09%> (-65.68%) ⬇️

@KodiaqQ KodiaqQ marked this pull request as ready for review March 8, 2024 11:18
@KodiaqQ KodiaqQ requested a review from a team as a code owner March 8, 2024 11:18
@KodiaqQ
Copy link
Collaborator Author

KodiaqQ commented Mar 8, 2024

Linters fails due to #2561

@KodiaqQ KodiaqQ requested review from alexsu52 and ljaljushkin March 8, 2024 11:19
@KodiaqQ KodiaqQ force-pushed the nm/align_npu_with_cpu branch from 9d4ec97 to 4e6bbf9 Compare March 8, 2024 14:00
@KodiaqQ KodiaqQ marked this pull request as draft March 11, 2024 10:08
@ljaljushkin
Copy link
Contributor

Should NPU config also contain this?

            "attributes": {
                "scales": "unified"
            },

@ljaljushkin
Copy link
Contributor

Also noticed this difference
image

@KodiaqQ
Copy link
Collaborator Author

KodiaqQ commented Mar 11, 2024

Should NPU config also contain this?

            "attributes": {
                "scales": "unified"
            },

Maybe @alexsu52 and @AlexKoff88 can answer this question.

@KodiaqQ
Copy link
Collaborator Author

KodiaqQ commented Mar 11, 2024

Also noticed this difference image

What should we do with this? Are these parameters used for NPU?

@KodiaqQ KodiaqQ marked this pull request as ready for review March 11, 2024 16:10
@github-actions github-actions bot added NNCF TF Pull requests that updates NNCF TensorFlow NNCF PT Pull requests that updates NNCF PyTorch labels Mar 11, 2024
@KodiaqQ KodiaqQ requested a review from AlexKoff88 March 11, 2024 16:16
@alexsu52
Copy link
Contributor

Should NPU config also contain this?

            "attributes": {
                "scales": "unified"
            },

Maybe @alexsu52 and @AlexKoff88 can answer this question.

Good question. Scale unification should be applied only for operation from CPU hardware config to align behavior between CPU and NPU in INT8 quantization. What affects other precision, we should not change their behavior.

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.

Do you have results of the conformance test for NPU config?

Please think about what test needs to be added to check the equality of the cpu, gpu and npu configs for int8.

nncf/common/hardware/configs/cpu.json Show resolved Hide resolved
@KodiaqQ
Copy link
Collaborator Author

KodiaqQ commented Mar 13, 2024

Do you have results of the conformance test for NPU config?

Please think about what test needs to be added to check the equality of the cpu, gpu and npu configs for int8.

We have no tests for NPU in the conformance as well. I haven't run these tests.

@KodiaqQ KodiaqQ requested a review from alexsu52 March 14, 2024 12:43
"15 /nncf_model_output_1" [id=15, label="nncf_model_output_#15", style=filled, type=nncf_model_output];
"16 /nncf_model_output_2" [id=16, label="nncf_model_output_#16", style=filled, type=nncf_model_output];
"17 /nncf_model_output_3" [id=17, label="nncf_model_output_#17", style=filled, type=nncf_model_output];
"6 MultiBranchesModel/MaxPool2d[max_pool_b]/SymmetricQuantizer/symmetric_quantize_0" [color=green, id=6, label="AFQ_[B:8 M:S SGN:U PC:N]_#6_G0", style=filled, type=symmetric_quantize];
Copy link
Contributor

Choose a reason for hiding this comment

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

previously conv had asymmetric int8 activations
image

Currently NPU config have symmetric activations on the first place, that's why there're 2 FQ.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

The same for all requant tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Other references just changed assymetric to symmetric in 8bit activations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the clarification.
But why do we have an asymmetric quantizer after the relu layer?

@AlexKoff88
Copy link
Contributor

Just for the record, the main motivation for keeping the config is QAT for NPU which has some custom features such as W4A4 support.

@KodiaqQ KodiaqQ self-assigned this Mar 19, 2024
@github-actions github-actions bot added dependencies Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PTQ Pull requests that updates NNCF PTQ labels Mar 20, 2024
@KodiaqQ KodiaqQ marked this pull request as draft April 2, 2024 07:27
@KodiaqQ KodiaqQ force-pushed the nm/align_npu_with_cpu branch from b741e0b to 6dd4caa Compare April 2, 2024 08:00
@github-actions github-actions bot removed dependencies Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PTQ Pull requests that updates NNCF PTQ labels Apr 2, 2024
@KodiaqQ KodiaqQ requested a review from alexsu52 April 2, 2024 08:21
@KodiaqQ KodiaqQ marked this pull request as ready for review April 2, 2024 08:21
@KodiaqQ KodiaqQ force-pushed the nm/align_npu_with_cpu branch from 1e959af to 54c317e Compare April 16, 2024 17:08
@KodiaqQ KodiaqQ force-pushed the nm/align_npu_with_cpu branch from 54c317e to e5cfe06 Compare May 3, 2024 11:53
@github-actions github-actions bot added the NNCF PTQ Pull requests that updates NNCF PTQ label May 3, 2024
@@ -206,6 +206,7 @@ def __init__(
else:
self._preset = QuantizationPreset.PERFORMANCE

self._override_device()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexsu52, I've added overriding. Review, please.

@KodiaqQ
Copy link
Collaborator Author

KodiaqQ commented May 6, 2024

@alexsu52, please, review

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

@alexsu52 alexsu52 merged commit b611322 into openvinotoolkit:develop May 6, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NNCF Common Pull request that updates NNCF Common NNCF PT Pull requests that updates NNCF PyTorch NNCF PTQ Pull requests that updates NNCF PTQ NNCF TF Pull requests that updates NNCF TensorFlow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants