Fixed to dump the ignored_scope in quantization more gracefully #2578

Viditagarwal7479 · 2024-03-15T17:55:50Z

Changes

In the case of dumping parameters of IgnoredScope in quantization:

The field validate isn't dumped
If the values corresponding to the rest of the fields are empty ([]) they aren't dumped
In case the IgnoredScope is passed blank, then only <ignored_scope value="[]"> is dumped

Related tickets

Ticket: 129593
Fixes Issue: #2564

Tests

The test case, tests/openvino/native/quantization/test_quantization_pipeline.py::test_meta_information has been modified to validate the new changes. To protect code from regressions.

codecov · 2024-03-15T18:30:08Z

Codecov Report

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

Project coverage is 33.14%. Comparing base (12f4720) to head (7845e42).
Report is 60 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #2578       +/-   ##
============================================
- Coverage    91.18%   33.14%   -58.05%     
============================================
  Files          492      493        +1     
  Lines        45096    45551      +455     
============================================
- Hits         41123    15099    -26024     
- Misses        3973    30452    +26479

Files	Coverage Δ
nncf/openvino/rt_info.py	`0.00% <0.00%> (-86.67%)`	⬇️

... and 315 files with indirect coverage changes

Flag	Coverage Δ
COMMON	`43.98% <ø> (+0.05%)`	⬆️
ONNX	`?`
OPENVINO	`?`
TENSORFLOW	`30.05% <0.00%> (+0.18%)`	⬆️
TORCH	`?`

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

Components	Coverage Δ
common	`85.62% <ø> (-8.15%)`	⬇️
torch	`0.01% <ø> (-93.59%)`	⬇️
tensorflow	`93.74% <ø> (-0.01%)`	⬇️
onnx	`0.00% <ø> (-93.06%)`	⬇️
openvino	`0.00% <0.00%> (-94.07%)`	⬇️
ptq	`28.00% <ø> (-62.22%)`	⬇️

Viditagarwal7479 · 2024-03-16T23:03:14Z

Hello @ljaljushkin, a gentle reminder to review the PR.

ljaljushkin

Thanks for the contribution!
Please address the comments

tests/openvino/native/quantization/test_quantization_pipeline.py

nncf/openvino/rt_info.py

…alue

ljaljushkin · 2024-03-20T09:24:45Z

@Viditagarwal7479 please fix the test to pass CI

https://github.com/openvinotoolkit/nncf/actions/runs/8344527642/job/22869598068?pr=2578

>           assert quantized_model.get_rt_info(rt_path) == str(value)
E           RuntimeError: Check 'rt_info.find(name) != rt_info.end()' failed at src/core/include/openvino/core/model.hpp:502:
E           Cannot get runtime attribute. Path to runtime attribute is incorrect.

tests/openvino/native/quantization/test_quantization_pipeline.py:108: RuntimeError

tests/openvino/native/quantization/test_quantization_pipeline.py

…ignored scope dump

Viditagarwal7479 · 2024-03-20T12:10:29Z

@Viditagarwal7479 please fix the test to pass CI

https://github.com/openvinotoolkit/nncf/actions/runs/8344527642/job/22869598068?pr=2578

>           assert quantized_model.get_rt_info(rt_path) == str(value)
E           RuntimeError: Check 'rt_info.find(name) != rt_info.end()' failed at src/core/include/openvino/core/model.hpp:502:
E           Cannot get runtime attribute. Path to runtime attribute is incorrect.

tests/openvino/native/quantization/test_quantization_pipeline.py:108: RuntimeError

While making modifications through the model.set_rt_info we are making changes in the output of model.get_rt_info so, I guess we need to update this test case accordingly to facilitate the changes.

tests/openvino/native/quantization/test_quantization_pipeline.py

Viditagarwal7479 · 2024-03-20T18:55:29Z

The pre-commit-linters / pre-commit (pull_request) is failing because of this unknown reason:

Run actions/setup-python@v3
Version 3.8.10 was not found in the local cache
Error: read ECONNRESET

AlexanderDokuchaev · 2024-03-20T20:20:40Z

@Viditagarwal7479 rerun the job helped, tests passed.

Viditagarwal7479 · 2024-03-21T01:08:26Z

It's showing "1 review requesting changes by reviewers with write access" but I have incorporated all the requested changes so far.

tests/openvino/native/quantization/test_quantization_pipeline.py

nncf/openvino/rt_info.py

ljaljushkin · 2024-04-15T13:27:09Z

@Viditagarwal7479, the last commit was a month ago. There's still one unresolved comment. Are you going to resolve it? Do you need a help? Please inform us if you do not plan to continue working on this task. Thanks!

Viditagarwal7479 · 2024-04-15T17:48:59Z

@Viditagarwal7479, the last commit was a month ago. There's still one unresolved comment. Are you going to resolve it? Do you need a help? Please inform us if you do not plan to continue working on this task. Thanks!

Yes, I need help in determining the parameters for subgraph in IgnoredScope for a given model. I looked at the PR you mentioned, but that PR wasn't of much help in resolving this, one thing that can be of help is if we also use the same model used in the PR, then as requested, we can include a subgraph based test case for this PR.

ljaljushkin · 2024-04-17T11:27:12Z

@Viditagarwal7479, the last commit was a month ago. There's still one unresolved comment. Are you going to resolve it? Do you need a help? Please inform us if you do not plan to continue working on this task. Thanks!

Yes, I need help in determining the parameters for subgraph in IgnoredScope for a given model. I looked at the PR you mentioned, but that PR wasn't of much help in resolving this, one thing that can be of help is if we also use the same model used in the PR, then as requested, we can include a subgraph based test case for this PR.

Firstly, I suggest using not quantized model, since the ignored scope defines nodes that should not be quantized.
If they are already quantized, this attribute does not make sense and do nothing.

For instance, you can consider WeightsModel.

To not quantize all layers in the subgraph marked red, you can specify the names of input and output nodes, as follows.

IgnoredScope(
    subgraphs=[
        nncf.Subgraph(
            inputs=[
                "MatMul_1",
            ],
            outputs=[
                "MatMul"
            ],
        )
    ],
),

Hope this example, makes things clear.

ljaljushkin · 2024-04-25T09:43:17Z

@Viditagarwal7479 one week left since my last comment
Please inform us if you do not plan to continue working on this task. Thanks!

Viditagarwal7479 · 2024-04-26T17:06:16Z

@Viditagarwal7479 one week left since my last comment Please inform us if you do not plan to continue working on this task. Thanks!

Thank you so much, @ljaljushkin, for helping me (in fact, spoon-feeding on how to add the subgraph-based test case).

Through the diagram, you showed it's clear, but just to make sure, for any model whose subgraph we want to ignore in the quantised model, we just have to mention the input node and the output node. Through this, the nodes that are taking the input as the value passed from the mentioned input node and whose output is, in a way, being passed to the mentioned output node get removed from the quantised model. Is my interpretation correct?

Sorry, for the delay from my end in making the minor changes. If my above interpretation is correct so should we make some more guides related to how to specify the subgraph in the IgnoredScope maybe just by adding similar subgraph based test cases or a tutorial kind of stuff?

ljaljushkin · 2024-04-26T17:18:27Z

@Viditagarwal7479 one week left since my last comment Please inform us if you do not plan to continue working on this task. Thanks!

Thank you so much, @ljaljushkin, for helping me (in fact, spoon-feeding on how to add the subgraph-based test case).

Through the diagram, you showed it's clear, but just to make sure, for any model whose subgraph we want to ignore in the quantised model, we just have to mention the input node and the output node. Through this, the nodes that are taking the input as the value passed from the mentioned input node and whose output is, in a way, being passed to the mentioned output node get removed from the quantised model. Is my interpretation correct?

Sorry, for the delay from my end in making the minor changes. If my above interpretation is correct so should we make some more guides related to how to specify the subgraph in the IgnoredScope maybe just by adding similar subgraph based test cases or a tutorial kind of stuff?

Yes, your interpretation is correct. More guidance about defining subgraph in the IgnoredScope would be definitely useful.
Feel free to add it in another PR. Probably, just updating docstring with some examples for IgnoredScope class can be enough.
You can visualize my example using symbols and tabs:

  |                  |     
input1        input2
   \               /
    node...
        |
       ...
        |
     output1

Viditagarwal7479 · 2024-04-26T18:27:52Z

@Viditagarwal7479 one week left since my last comment Please inform us if you do not plan to continue working on this task. Thanks!

Thank you so much, @ljaljushkin, for helping me (in fact, spoon-feeding on how to add the subgraph-based test case).
Through the diagram, you showed it's clear, but just to make sure, for any model whose subgraph we want to ignore in the quantised model, we just have to mention the input node and the output node. Through this, the nodes that are taking the input as the value passed from the mentioned input node and whose output is, in a way, being passed to the mentioned output node get removed from the quantised model. Is my interpretation correct?
Sorry, for the delay from my end in making the minor changes. If my above interpretation is correct so should we make some more guides related to how to specify the subgraph in the IgnoredScope maybe just by adding similar subgraph based test cases or a tutorial kind of stuff?

Yes, your interpretation is correct. More guidance about defining subgraph in the IgnoredScope would be definitely useful. Feel free to add it in another PR. Probably, just updating docstring with some examples for IgnoredScope class can be enough. You can visualize my example using symbols and tabs:
  |                  |     
input1        input2
   \               /
    node...
        |
       ...
        |
     output1

Thank you for the confirmation. I will soon (2-3 days) create a PR corresponding to this.

Viditagarwal7479 added 2 commits March 15, 2024 23:09

added dumping of ignored scope parameters gracefully

852ff84

added tests for dumping of ignored scope parameters gracefully

876e2f0

Viditagarwal7479 requested a review from a team as a code owner March 15, 2024 17:55

github-actions bot added the NNCF OpenVINO Pull requests that updates NNCF OpenVINO label Mar 15, 2024

Merge branch 'openvinotoolkit:develop' into dump_ignored_scope

6d61ee5

ljaljushkin requested changes Mar 18, 2024

View reviewed changes

tests/openvino/native/quantization/test_quantization_pipeline.py Outdated Show resolved Hide resolved

nncf/openvino/rt_info.py Outdated Show resolved Hide resolved

Viditagarwal7479 added 2 commits March 19, 2024 18:54

while dumping IgnoreScope fixed to not change original IgnoredScope v…

4cd78a5

…alue

added new test to compare IgnoredScope parameter after model is dumped

8c3a5d2

Viditagarwal7479 requested a review from ljaljushkin March 19, 2024 14:11

ljaljushkin requested changes Mar 20, 2024

View reviewed changes

updated the way parameters were passed and evalutated while checking …

206db77

…ignored scope dump

Viditagarwal7479 requested a review from ljaljushkin March 20, 2024 12:32

ljaljushkin requested changes Mar 20, 2024

View reviewed changes

Viditagarwal7479 added 2 commits March 20, 2024 21:11

fixed typos in ignored scope dump test case

dd9d01a

Merge branch 'openvinotoolkit:develop' into dump_ignored_scope

3024c88

Viditagarwal7479 requested a review from ljaljushkin March 20, 2024 15:47

ljaljushkin reviewed Mar 20, 2024

View reviewed changes

tests/openvino/native/quantization/test_quantization_pipeline.py Outdated Show resolved Hide resolved

fixed linting issues

8ae9d68

Viditagarwal7479 requested a review from ljaljushkin March 20, 2024 16:17

Viditagarwal7479 added 2 commits March 20, 2024 22:59

fixed a concating of path while saving dummy model

517afbf

updated the test_meta_information based on changes in dump_parameter

95a529e

ljaljushkin approved these changes Mar 21, 2024

View reviewed changes

ljaljushkin requested a review from andrey-churkin March 21, 2024 14:47

andrey-churkin requested changes Mar 21, 2024

View reviewed changes

reformatted how keys with empty value are removed

00a5e3e

Viditagarwal7479 added 2 commits April 26, 2024 22:19

changed model to WeightsModel and added subgraph based test case

51f8de0

linted files

7845e42

Viditagarwal7479 requested a review from ljaljushkin April 26, 2024 16:54

Viditagarwal7479 requested a review from andrey-churkin April 26, 2024 18:28

ljaljushkin approved these changes Apr 26, 2024

View reviewed changes

ljaljushkin assigned andrey-churkin Apr 26, 2024

andrey-churkin approved these changes May 2, 2024

View reviewed changes

ljaljushkin merged commit 3586e58 into openvinotoolkit:develop May 3, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed to dump the ignored_scope in quantization more gracefully #2578

Fixed to dump the ignored_scope in quantization more gracefully #2578

Viditagarwal7479 commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

Viditagarwal7479 commented Mar 16, 2024

ljaljushkin left a comment

ljaljushkin commented Mar 20, 2024

Viditagarwal7479 commented Mar 20, 2024

Viditagarwal7479 commented Mar 20, 2024

AlexanderDokuchaev commented Mar 20, 2024

Viditagarwal7479 commented Mar 21, 2024

ljaljushkin commented Apr 15, 2024

Viditagarwal7479 commented Apr 15, 2024

ljaljushkin commented Apr 17, 2024

ljaljushkin commented Apr 25, 2024

Viditagarwal7479 commented Apr 26, 2024

ljaljushkin commented Apr 26, 2024

Viditagarwal7479 commented Apr 26, 2024

Fixed to dump the ignored_scope in quantization more gracefully #2578

Fixed to dump the ignored_scope in quantization more gracefully #2578

Conversation

Viditagarwal7479 commented Mar 15, 2024 • edited Loading

Changes

Related tickets

Tests

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

Viditagarwal7479 commented Mar 16, 2024

ljaljushkin left a comment

Choose a reason for hiding this comment

ljaljushkin commented Mar 20, 2024

Viditagarwal7479 commented Mar 20, 2024

Viditagarwal7479 commented Mar 20, 2024

AlexanderDokuchaev commented Mar 20, 2024

Viditagarwal7479 commented Mar 21, 2024

ljaljushkin commented Apr 15, 2024

Viditagarwal7479 commented Apr 15, 2024

ljaljushkin commented Apr 17, 2024

ljaljushkin commented Apr 25, 2024

Viditagarwal7479 commented Apr 26, 2024

ljaljushkin commented Apr 26, 2024

Viditagarwal7479 commented Apr 26, 2024

Viditagarwal7479 commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading