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

Ignored scope matching working with multiple NNCFGraphs #2723

Merged
merged 32 commits into from
Jun 18, 2024

Conversation

kshpv
Copy link
Collaborator

@kshpv kshpv commented Jun 6, 2024

Changes

Add support of IgnoredScope matching many NNCFGraphs.
Change unmatched error logging when strict=True:
  before: the first unmatched rule is logged
  after: all unmatched rules are logged.
IgnoredScope validation for OpenVINO models with IF operation was updated with new changes - the validation is put before running the quantization pipeline.

Reason for changes

IgnoredScope validation failure for a OV model with IF operation.

Related tickets

135110

Tests

TBD

@github-actions github-actions bot added NNCF Common Pull request that updates NNCF Common NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PTQ Pull requests that updates NNCF PTQ labels Jun 6, 2024
@github-actions github-actions bot added NNCF PT Pull requests that updates NNCF PyTorch NNCF ONNX Pull requests that updates NNCF ONNX labels Jun 6, 2024
@kshpv kshpv changed the title Ignored scope extension for working with multiple NNCFGraphs Ignored scope matching working with multiple NNCFGraphs Jun 6, 2024
@kshpv kshpv marked this pull request as ready for review June 6, 2024 13:52
@kshpv kshpv requested a review from a team as a code owner June 6, 2024 13:52
nncf/onnx/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/openvino/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/quantization/quantize_model.py Show resolved Hide resolved
nncf/openvino/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/openvino/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/openvino/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/openvino/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/scopes.py Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
@github-actions github-actions bot removed NNCF PT Pull requests that updates NNCF PyTorch NNCF ONNX Pull requests that updates NNCF ONNX labels Jun 10, 2024
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.67%. Comparing base (85b3263) to head (1c88dee).
Report is 4 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2723      +/-   ##
===========================================
- Coverage    91.18%   90.67%   -0.51%     
===========================================
  Files          483      483              
  Lines        46363    46443      +80     
===========================================
- Hits         42274    42111     -163     
- Misses        4089     4332     +243     
Files Coverage Δ
nncf/common/graph/graph.py 96.56% <100.00%> (+0.01%) ⬆️
nncf/openvino/quantization/quantize_ifmodel.py 100.00% <100.00%> (ø)
nncf/quantization/quantize_model.py 80.82% <75.00%> (+0.26%) ⬆️
nncf/scopes.py 95.65% <98.07%> (+4.08%) ⬆️
nncf/openvino/quantization/quantize_model.py 65.03% <90.47%> (+3.43%) ⬆️

... and 23 files with indirect coverage changes

Flag Coverage Δ
COMMON 41.93% <62.50%> (-0.10%) ⬇️
ONNX 34.18% <55.95%> (-0.01%) ⬇️
OPENVINO 40.96% <95.23%> (+0.11%) ⬆️
TENSORFLOW 29.39% <13.09%> (-0.04%) ⬇️
TORCH 64.84% <53.57%> (-0.59%) ⬇️

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

Components Coverage Δ
common 93.55% <100.00%> (+<0.01%) ⬆️
torch 92.00% <ø> (-1.65%) ⬇️
tensorflow 93.26% <ø> (ø)
onnx 93.06% <ø> (ø)
openvino 94.48% <91.66%> (+<0.01%) ⬆️
ptq 90.50% <75.00%> (+0.06%) ⬆️

@KodiaqQ KodiaqQ requested review from alexsu52 and KodiaqQ and removed request for KodiaqQ June 10, 2024 13:53
@alexsu52 alexsu52 requested a review from KodiaqQ June 12, 2024 12:26
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.

Please provide the output for ignored scope before and after this PR, because this PR changes logging for the ignored scope.

nncf/openvino/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/openvino/quantization/quantize_model.py Outdated Show resolved Hide resolved
nncf/scopes.py Outdated Show resolved Hide resolved
@kshpv
Copy link
Collaborator Author

kshpv commented Jun 13, 2024

For the https://github.com/openvinotoolkit/nncf/blob/develop/examples/post_training_quantization/openvino/yolov8/main.py with the following ignored_scope:

ignored_scope=nncf.IgnoredScope(
            patterns=[".bad_pattern"],
            names=["bad_name", "/model.22/Concat"],
            types=["Subtract", "bad_type"],
            subgraphs=[
                nncf.Subgraph(
                    inputs=["/model.22/Concat", "/model.22/Concat_1", "/model.22/Concat_2"],
                    outputs=["output0/sink_port_0"],
                ),
                nncf.Subgraph(
                    inputs=["/model.22/Concat"],
                    outputs=["/model.22/Concat"],
                ),
            ],
        ),

The resulted logs are obtained:
Old log:
image
New log:
image

@alexsu52
Copy link
Contributor

For the https://github.com/openvinotoolkit/nncf/blob/develop/examples/post_training_quantization/openvino/yolov8/main.py with the following ignored_scope:

ignored_scope=nncf.IgnoredScope(
            patterns=[".bad_pattern"],
            names=["bad_name", "/model.22/Concat"],
            types=["Subtract", "bad_type"],
            subgraphs=[
                nncf.Subgraph(
                    inputs=["/model.22/Concat", "/model.22/Concat_1", "/model.22/Concat_2"],
                    outputs=["output0/sink_port_0"],
                ),
                nncf.Subgraph(
                    inputs=["/model.22/Concat"],
                    outputs=["/model.22/Concat"],
                ),
            ],
        ),

The resulted logs are obtained: Old log: image New log: image

I have found original_graph.dot in the message. It looks like is not valid. I would suggest to remove it for PTQ.

@kshpv
Copy link
Collaborator Author

kshpv commented Jun 13, 2024

I have found original_graph.dot in the message. It looks like is not valid. I would suggest to remove it for PTQ.

Updated:
image

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

@KodiaqQ
Copy link
Collaborator

KodiaqQ commented Jun 18, 2024

@kshpv, can we merge it? Were all fixes included?

@KodiaqQ KodiaqQ merged commit 5384965 into openvinotoolkit:develop Jun 18, 2024
12 checks passed
@kshpv kshpv mentioned this pull request Jul 25, 2024
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 OpenVINO Pull requests that updates NNCF OpenVINO NNCF PTQ Pull requests that updates NNCF PTQ
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants