[NNCF]: Optimized memory footprint by removing redundant collected statistics #2563

AdiKsOnDev · 2024-03-08T12:26:50Z

Changes

Made changes in:

nncf/common/tensor_statistics/statistic_point.py
nncf/quantization/algorithms/pipeline.py

Focus on the way I used newly created remove_statistic_point() inside pipeline.py, to see if it's up to the expectations

Reason for changes

Save space by removing "unused" statistic points associated with an algorithm

Related tickets

120377

Tests

tests/common/test_statistic_points.py was added:

Test removing associated statistical points
Test removing from an empty container

Closes #2557

TODO: Integrate the method into the pipeline

In .build/ folder as well

codecov · 2024-03-08T13:01:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.32%. Comparing base (8ad77dc) to head (aefd170).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #2563       +/-   ##
============================================
- Coverage    77.91%   52.32%   -25.59%     
============================================
  Files          494      494               
  Lines        45387    45387               
============================================
- Hits         35363    23749    -11614     
- Misses       10024    21638    +11614

Files	Coverage Δ
nncf/common/tensor_statistics/statistic_point.py	`93.10% <100.00%> (+1.61%)`	⬆️
nncf/quantization/algorithms/algorithm.py	`100.00% <100.00%> (ø)`
nncf/quantization/algorithms/pipeline.py	`93.50% <100.00%> (-1.17%)`	⬇️

... and 276 files with indirect coverage changes

Flag	Coverage Δ
COMMON	`44.22% <77.77%> (?)`
ONNX	`34.65% <100.00%> (?)`
TENSORFLOW	`30.11% <22.22%> (+<0.01%)`	⬆️
TORCH	`?`

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

Components	Coverage Δ
common	`87.60% <100.00%> (-0.72%)`	⬇️
torch	`32.25% <ø> (-61.24%)`	⬇️
tensorflow	`93.74% <ø> (ø)`
onnx	`93.07% <ø> (+93.07%)`	⬆️
openvino	`0.00% <ø> (-25.77%)`	⬇️
ptq	`48.47% <100.00%> (-4.58%)`	⬇️

kshpv

Thank you for looking into the issue!
General recommendation: I suggest beginning with Test-Driven Development (TDD) by initially crafting a test case.

Add multiple statistical points.
Subsequently remove some of them/all. Don't forget about edge cases.
Concluding with a final verification to ensure the function operates as intended.

nncf/common/tensor_statistics/statistic_point.py

Co-authored-by: Aleksei Kashapov <[email protected]>

AdiKsOnDev · 2024-03-08T18:31:05Z

@kshpv Are any actions expected from me any further?

kshpv · 2024-03-08T18:38:21Z

@kshpv Is any action expected from me any further?

Could you add a test on this functionality? You can create a file test_statistic_points.py with a test and put it in tests/common/

Also, it would be beneficial if you provide the memory usage before and after your changes for some of NNCF examples

Thank you!

AdiKsOnDev · 2024-03-08T18:43:58Z

Yes, absolutely! I shall request a re-review for another PR tomorrow (hopefully) with a test for this and I'll give the results of a benchmark before/after the change

Thank you for responding so quickly!

Forgot to refer to the re-newed algorithm_name parameter

TODO: Fix the initialization of target_node

AdiKsOnDev · 2024-03-09T12:20:15Z

Benchmark Examples

@kshpv

Before

After

AdiKsOnDev · 2024-03-11T13:35:21Z

@KodiaqQ Are any actions expected from me any further?

nikita-malininn · 2024-03-11T13:40:25Z

Benchmark Examples

@kshpv

Before

...

After

...

Hi, @AdiKsOnDev. Thank you for your contribution.
I wonder to understand, why the performance of the model was decreased after the changes?

AdiKsOnDev · 2024-03-11T13:44:01Z

Benchmark Examples

@kshpv

Before

...

After

...

Hi, @AdiKsOnDev. Thank you for your contribution.
I wonder to understand, why the performance of the model was decreased after the changes?

Hi, can it be that in the remove_statistical_points() the loop that's doing all the work is taking too long?

nncf/common/tensor_statistics/statistic_point.py

remove_statistic_point() is now integrated in the pipeline

kshpv · 2024-03-28T07:50:06Z

@AdiKsOnDev Hi!
Are you going to continue work on this?
I think a fixing the precommit is the last part that you need to do to finish the PR.

AdiKsOnDev · 2024-03-28T08:09:19Z

@AdiKsOnDev Hi!
Are you going to continue work on this?
I think a fixing the precommit is the last part that you need to do to finish the PR.

Hello! I remember this, don't worry I am still extremely eager to finish, but I have a huge workload on me right now. I shall continue working this weekend

Very sorry for the silence

kshpv · 2024-03-28T08:45:40Z

@AdiKsOnDev Hi!
Are you going to continue work on this?
I think a fixing the precommit is the last part that you need to do to finish the PR.

Hello! I remember this, don't worry I am still extremely eager to finish, but I have a huge workload on me right now. I shall continue working this weekend

Very sorry for the silence

Ok Then!
If you have any question, don't hesistate to ask :)

AdiKsOnDev · 2024-03-31T15:23:04Z

@kshpv @KodiaqQ For some reason (that is unknown to me, lol) algorithm in the loop provided below is not able to call the .algorithm_key() I made. (Throws TypeError: 'str' object is not callable)
HOWEVER, I am able to use algorithm._algorithm_key BUT it ends up failing the following test (Along with many others):

FAILED tests/onnx/quantization/test_bias_correction.py::TestONNXBCAlgorithm::test_update_bias[MultipleConvTestModel-ref_biases0] - KeyError: '/Relu_1'

This test only passes when I just pass the algorithm object itself, but in that case there is no point in .remove_statiistic_points() because it doesn't end up removing the algorithm (I think)

for algorithm in pipeline_step[:-1]:
    current_model = algorithm.apply(current_model, current_graph, step_statistics)
    current_graph = NNCFGraphFactory.create(current_model)
    step_statistics.remove_statistic_points(algorithm)
                                                                                       
current_model = pipeline_step[-1].apply(current_model, current_graph, step_statistics)
step_statistics.remove_statistic_points(pipeline_step[-1])

AdiKsOnDev · 2024-04-01T08:55:57Z

@kshpv Following up on the above^

AdiKsOnDev · 2024-04-02T09:23:45Z

@KodiaqQ

kshpv · 2024-04-02T09:49:54Z

@kshpv @KodiaqQ For some reason (that is unknown to me, lol) algorithm in the loop provided below is not able to call the .algorithm_key() I made. (Throws TypeError: 'str' object is not callable) HOWEVER, I am able to use algorithm._algorithm_key BUT it ends up failing the following test (Along with many others):
FAILED tests/onnx/quantization/test_bias_correction.py::TestONNXBCAlgorithm::test_update_bias[MultipleConvTestModel-ref_biases0] - KeyError: '/Relu_1'
This test only passes when I just pass the algorithm object itself, but in that case there is no point in .remove_statiistic_points() because it doesn't end up removing the algorithm (I think)
for algorithm in pipeline_step[:-1]:
    current_model = algorithm.apply(current_model, current_graph, step_statistics)
    current_graph = NNCFGraphFactory.create(current_model)
    step_statistics.remove_statistic_points(algorithm)
                                                                                       
current_model = pipeline_step[-1].apply(current_model, current_graph, step_statistics)
step_statistics.remove_statistic_points(pipeline_step[-1])

algorithm_key is decorated method by property decorator. So syntaxis to get a property value is the following:
algorithm.algorithm_key

Please, fix this and then it looks like it will work

AdiKsOnDev · 2024-04-02T10:46:17Z

@kshpv @KodiaqQ For some reason (that is unknown to me, lol) algorithm in the loop provided below is not able to call the .algorithm_key() I made. (Throws TypeError: 'str' object is not callable) HOWEVER, I am able to use algorithm._algorithm_key BUT it ends up failing the following test (Along with many others):
FAILED tests/onnx/quantization/test_bias_correction.py::TestONNXBCAlgorithm::test_update_bias[MultipleConvTestModel-ref_biases0] - KeyError: '/Relu_1'
This test only passes when I just pass the algorithm object itself, but in that case there is no point in .remove_statiistic_points() because it doesn't end up removing the algorithm (I think)
for algorithm in pipeline_step[:-1]:
    current_model = algorithm.apply(current_model, current_graph, step_statistics)
    current_graph = NNCFGraphFactory.create(current_model)
    step_statistics.remove_statistic_points(algorithm)
                                                                                       
current_model = pipeline_step[-1].apply(current_model, current_graph, step_statistics)
step_statistics.remove_statistic_points(pipeline_step[-1])
algorithm_key is decorated method by property decorator. So syntaxis to get a property value is the following: algorithm.algorithm_key

Please, fix this and then it looks like it will work

@kshpv I've tried this, but I still get this error:

FAILED tests/onnx/quantization/test_bias_correction.py::TestONNXBCAlgorithm::test_update_bias[MultipleConvTestModel-ref_biases0] - KeyError: '/Relu_1'

AdiKsOnDev · 2024-04-02T11:30:26Z

@kshpv UPDATE On the above:
I think it was just a local error, you can approve the git workflows now, let's see if it runs properly on the cloud (Should run fine)

AdiKsOnDev · 2024-04-02T13:16:08Z

@kshpv Nope, fails here as well :/ Any ideas?

kshpv · 2024-04-03T07:48:12Z

Hello @AdiKsOnDev!
Could you rebase on the lateset develop branch? It seems that the recent PRs should fix your red precommit issue.

kshpv

Precommit is green. Code looks okay. Please, apply last comments and I approve

nncf/quantization/algorithms/algorithm.py

tests/common/test_statistic_points.py

Forgot to remove this change earlier

AdiKsOnDev · 2024-04-06T10:22:57Z

Hello once again! Just wanted to confirm, is this PR ready for being merged or is something more expected from my end?
If the work is done here, I'd like to start working on some other issue while this one is getting merged

Thanks

kshpv · 2024-04-08T07:02:10Z

Hello @AdiKsOnDev!
I conducted some experiments with your changes to assess their impact on memory consumption during post-training quantization. Unfortunately, for the current pipeline (MinMax + FastBiasCorrection/BiasCorrection), these adjustments do not seem to have any significant influence. This lack of impact stems from the fact that the statistics are already optimized, making these changes negligible.

So, I am not sure about merging these changes yet.

Initially, I had high hopes that this PR would significantly reduce memory usage. However, after experimenting and crunching the numbers, it has become apparent that the impact is small.

I am disappointed that things did not pan out as I hoped, but I want to express my gratitude for your hard work. I hope you gain some experience from it, and I encourage you to keep contributing to the enhancement of NNCF.

AdiKsOnDev · 2024-04-08T07:05:35Z

Yes, the experience was very interesting so it's perfectly fine :P
Thanks for the collaboration either way!

kshpv · 2024-04-08T07:17:37Z

So, thank you @AdiKsOnDev again! I am closing the PR

AdiKsOnDev added 2 commits March 8, 2024 15:10

feat: Added a method for removing stat_point

b84acfd

TODO: Integrate the method into the pipeline

feat: Integrated remove_statistic_point() to the pipelines

3a85e8f

In .build/ folder as well

AdiKsOnDev requested a review from a team as a code owner March 8, 2024 12:26

github-actions bot added NNCF Common Pull request that updates NNCF Common NNCF PTQ Pull requests that updates NNCF PTQ labels Mar 8, 2024

alexsu52 requested a review from kshpv March 8, 2024 12:31

kshpv reviewed Mar 8, 2024

View reviewed changes

nncf/common/tensor_statistics/statistic_point.py Outdated Show resolved Hide resolved

nncf/common/tensor_statistics/statistic_point.py Outdated Show resolved Hide resolved

kshpv reviewed Mar 8, 2024

View reviewed changes

nncf/common/tensor_statistics/statistic_point.py Outdated Show resolved Hide resolved

AdiKsOnDev and others added 2 commits March 8, 2024 21:33

refactor: Rename the parameter name

42260a4

Co-authored-by: Aleksei Kashapov <[email protected]>

fix: Removed unnecessary list conversion

e4c6755

github-actions bot removed the NNCF PTQ Pull requests that updates NNCF PTQ label Mar 8, 2024

AdiKsOnDev added 5 commits March 9, 2024 13:27

fix: Wrong parameter usage

c83ce10

Forgot to refer to the re-newed algorithm_name parameter

refactor: Refactored the code to match PEP rules

5c7bdaa

CI/CD: Tests for the new feature

4368f49

TODO: Fix the initialization of target_node

fix: Made a copy of the self.data dictionary

856ce28

refactor: Pre-Commit reformatting

ef4bb4a

AdiKsOnDev requested a review from kshpv March 9, 2024 12:22

alexsu52 requested a review from nikita-malininn March 11, 2024 06:02

nikita-malininn reviewed Mar 11, 2024

View reviewed changes

nncf/common/tensor_statistics/statistic_point.py Outdated Show resolved Hide resolved

AdiKsOnDev added 2 commits March 11, 2024 20:13

fix: Returned the deleted line

13356ca

remove_statistic_point() is now integrated in the pipeline

fix: Typo in the method's name

3131d48

github-actions bot added the NNCF PTQ Pull requests that updates NNCF PTQ label Mar 11, 2024

AdiKsOnDev added 4 commits March 31, 2024 15:59

fix: Compare alg name instead of Object itself

5f32d93

fix: Iterating through items of collectors

3bf0fef

fix: Should work now

56fa9e6

fix: Type 'str' not callable

427f680

fix: @Property deleted

80b6afc

AdiKsOnDev and others added 2 commits April 3, 2024 14:22

Merge branch 'openvinotoolkit:develop' into develop

7fa02d4

git: Sync fork

495bd4f

kshpv requested changes Apr 3, 2024

View reviewed changes

nncf/quantization/algorithms/algorithm.py Show resolved Hide resolved

tests/common/test_statistic_points.py Outdated Show resolved Hide resolved

AdiKsOnDev added 2 commits April 3, 2024 16:12

fix: Returned property decorator

a0a0c5f

Forgot to remove this change earlier

refactor: Formatted & Cleaned the code

aefd170

kshpv self-requested a review April 3, 2024 13:31

kshpv approved these changes Apr 3, 2024

View reviewed changes

andrey-churkin approved these changes Apr 3, 2024

View reviewed changes

kshpv closed this Apr 8, 2024

kshpv mentioned this pull request Apr 8, 2024

[Good First Issue][NNCF]: Optimize memory footprint by removing redundant collected statistics #2557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NNCF]: Optimized memory footprint by removing redundant collected statistics #2563

[NNCF]: Optimized memory footprint by removing redundant collected statistics #2563

AdiKsOnDev commented Mar 8, 2024 •

edited

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

kshpv left a comment •

edited

Loading

AdiKsOnDev commented Mar 8, 2024 •

edited

Loading

kshpv commented Mar 8, 2024 •

edited

Loading

AdiKsOnDev commented Mar 8, 2024

AdiKsOnDev commented Mar 9, 2024

AdiKsOnDev commented Mar 11, 2024

nikita-malininn commented Mar 11, 2024

Benchmark Examples

Before

After

AdiKsOnDev commented Mar 11, 2024

Benchmark Examples

Before

After

kshpv commented Mar 28, 2024

AdiKsOnDev commented Mar 28, 2024

kshpv commented Mar 28, 2024

AdiKsOnDev commented Mar 31, 2024 •

edited

Loading

AdiKsOnDev commented Apr 1, 2024

AdiKsOnDev commented Apr 2, 2024

kshpv commented Apr 2, 2024 •

edited

Loading

AdiKsOnDev commented Apr 2, 2024 •

edited

Loading

AdiKsOnDev commented Apr 2, 2024

AdiKsOnDev commented Apr 2, 2024

kshpv commented Apr 3, 2024

kshpv left a comment

AdiKsOnDev commented Apr 6, 2024

kshpv commented Apr 8, 2024

AdiKsOnDev commented Apr 8, 2024

kshpv commented Apr 8, 2024

[NNCF]: Optimized memory footprint by removing redundant collected statistics #2563

[NNCF]: Optimized memory footprint by removing redundant collected statistics #2563

Conversation

AdiKsOnDev commented Mar 8, 2024 • edited Loading

Changes

Reason for changes

Related tickets

Tests

codecov bot commented Mar 8, 2024 • edited Loading

Codecov Report

kshpv left a comment • edited Loading

Choose a reason for hiding this comment

AdiKsOnDev commented Mar 8, 2024 • edited Loading

kshpv commented Mar 8, 2024 • edited Loading

AdiKsOnDev commented Mar 8, 2024

AdiKsOnDev commented Mar 9, 2024

Benchmark Examples

Before

After

AdiKsOnDev commented Mar 11, 2024

nikita-malininn commented Mar 11, 2024

Benchmark Examples

Before

After

AdiKsOnDev commented Mar 11, 2024

Benchmark Examples

Before

After

kshpv commented Mar 28, 2024

AdiKsOnDev commented Mar 28, 2024

kshpv commented Mar 28, 2024

AdiKsOnDev commented Mar 31, 2024 • edited Loading

AdiKsOnDev commented Apr 1, 2024

AdiKsOnDev commented Apr 2, 2024

kshpv commented Apr 2, 2024 • edited Loading

AdiKsOnDev commented Apr 2, 2024 • edited Loading

AdiKsOnDev commented Apr 2, 2024

AdiKsOnDev commented Apr 2, 2024

kshpv commented Apr 3, 2024

kshpv left a comment

Choose a reason for hiding this comment

AdiKsOnDev commented Apr 6, 2024

kshpv commented Apr 8, 2024

AdiKsOnDev commented Apr 8, 2024

kshpv commented Apr 8, 2024

AdiKsOnDev commented Mar 8, 2024 •

edited

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

kshpv left a comment •

edited

Loading

AdiKsOnDev commented Mar 8, 2024 •

edited

Loading

kshpv commented Mar 8, 2024 •

edited

Loading

AdiKsOnDev commented Mar 31, 2024 •

edited

Loading

kshpv commented Apr 2, 2024 •

edited

Loading

AdiKsOnDev commented Apr 2, 2024 •

edited

Loading