nncf icon indicating copy to clipboard operation
nncf copied to clipboard

[Good First Issue][NNCF]: Replace common tensor_statistics by experimental tensor_statistics

Open kshpv opened this issue 1 year ago • 23 comments

Context

Historically, NNCF has maintained two sets of tensor statistics: the old tensor statistics and the new tensor statistics. The new tensor statistics, located in the experimental/common/tensor_statistics directory, offer improved functionality and better performance. However, the codebase still contains references to the old tensor statistics, leading to redundancy and potential confusion.

What needs to be done?

  1. Search the codebase for all instances where the old tensor statistics are being used. This includes algorithms, tests, and any utility functions.
  2. Modify the identified algorithms to utilize the new tensor statistics from experimental/common/tensor_statistics.
  3. Update the corresponding tests to ensure they are compatible with the new tensor statistics.
  4. Replace the contents of common/tensor_statistics/collectors with the corresponding implementations from experimental/common/tensor_statistics.
  5. Once all references to the old tensor statistics have been updated, remove the experimental/common/tensor_statistics.
  6. Remove common/tensor_statistics/reduction if it is not needed anymore.

In the end, there should be no experimental/common/tensor_statistics.

Example Pull Requests

https://github.com/openvinotoolkit/nncf/pull/2117

Resources

Contact points

@kshpv

kshpv avatar Oct 28 '24 11:10 kshpv

.take

olegkkruglov avatar Oct 28 '24 11:10 olegkkruglov

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

github-actions[bot] avatar Oct 28 '24 11:10 github-actions[bot]

@kshpv So basically functionally replace everything in old files(common) with the new files(Experimental) and then delete the new files(Experimental) ?

AHB102 avatar Oct 28 '24 16:10 AHB102

@AHB102, you also need to be sure that all tests passed and no regression is introduced

kshpv avatar Oct 29 '24 09:10 kshpv

@kshpv Great! I've reviewed the example and will create a PR for a single-file change. We can go from there

AHB102 avatar Oct 29 '24 12:10 AHB102

@AHB102 I am really sorry, but @olegkkruglov already picked this GFI. I suggest you to pick another one. We have many things to do. I think we probably open new GFIs in the following days. If you are interested I can ping you. What do you say?

kshpv avatar Oct 29 '24 16:10 kshpv

@kshpv Yup no problem 😄

AHB102 avatar Oct 29 '24 16:10 AHB102

hey @olegkkruglov - do you need any support or you may not have a time to finish the task?

mlukasze avatar Nov 06 '24 07:11 mlukasze

hey, the work is in progress. sorry, I didn't have enough time to finish. I think I will be able to do it during this weekend, is it ok?

olegkkruglov avatar Nov 06 '24 18:11 olegkkruglov

of course :)

mlukasze avatar Nov 07 '24 05:11 mlukasze

Hi @AHB102 https://github.com/openvinotoolkit/nncf/issues/3120 - this is probably for you :)

kshpv avatar Nov 28 '24 14:11 kshpv

@kshpv By the time I looked, someone else had taken the issue. Maybe next time.

AHB102 avatar Nov 29 '24 02:11 AHB102

I'm unassigning the task due to assignee's inactivity. @kshpv could you please unassign the task? I don't have appropriate permissions in the NNCF issues.

p-wysocki avatar Mar 05 '25 12:03 p-wysocki

Do we need to merge PR https://github.com/openvinotoolkit/nncf/pull/3106/ before working on this issue?

shumaari avatar Mar 05 '25 15:03 shumaari

Do we need to merge PR #3106 before working on this issue?

If you want, you can continue working on this issue. In this case, you should open your PR, creating a branch from PR #3106. You can then rebase your branch to develop and respond to reviewers' comments. After you open your PR, I will close PR #3106 due to lack of activity.

alexsu52 avatar Mar 06 '25 09:03 alexsu52

.take

darshil929 avatar Mar 07 '25 15:03 darshil929

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

github-actions[bot] avatar Mar 07 '25 15:03 github-actions[bot]

Thanks for being interested in this issue. It looks like this ticket is already assigned to a contributor. Please communicate with the assigned contributor to confirm the status of the issue.

github-actions[bot] avatar Mar 08 '25 07:03 github-actions[bot]

Thanks for being interested in this issue. It looks like this ticket is already assigned to a contributor. Please communicate with the assigned contributor to confirm the status of the issue.

github-actions[bot] avatar Mar 08 '25 07:03 github-actions[bot]

Hi is this open?

akshatsen123 avatar Mar 09 '25 21:03 akshatsen123

Hi @kshpv ,

I've been working on the tensor statistics migration issue assigned to me and wanted to check in to ensure I've understood the task correctly.

From my investigation, I see that NNCF currently maintains two sets of tensor statistics:

  1. The old implementation in nncf/common/tensor_statistics with files: aggregator.py, collectors.py, init.py, reduction.py, statistic_point.py, statistics.py, statistics_serializer.py, and statistics_validator.py
  2. The new implementation in nncf/experimental/common/tensor_statistics with fewer files: collectors.py, init.py, statistical_functions.py, and statistics.py

I've written a script to search for references to both implementations across the codebase to identify which files need to be updated.

My understanding of the migration task is:

  1. Update all algorithms, tests, and utility functions to use the new tensor statistics from the experimental implementation
  2. Replace the contents of nncf/common/tensor_statistics/collectors with the corresponding implementation from nncf/experimental/common/tensor_statistics/collectors
  3. Remove nncf/common/tensor_statistics/reduction if it's no longer needed
  4. Finally, remove the experimental implementation (nncf/experimental/common/tensor_statistics) once all references have been migrated

Since the new implementation has fewer files, I'm wondering how the functionality has been reorganized. Has the functionality from files like statistic_point.py, statistics_serializer.py, and statistics_validator.py been consolidated into the remaining files, or is some of it no longer needed?

I'd appreciate your guidance on whether my understanding is correct and if there's anything important I've missed or should be particularly careful about during this migration.

Thank you!

darshil929 avatar Mar 11 '25 16:03 darshil929

@darshil929 Please make note of this comment:

Do we need to merge PR #3106 before working on this issue?

If you want, you can continue working on this issue. In this case, you should open your PR, creating a branch from PR #3106. You can then rebase your branch to develop and respond to reviewers' comments. After you open your PR, I will close PR #3106 due to lack of activity.

shumaari avatar Mar 12 '25 03:03 shumaari

@shumaari I'll take a note of it, Thanks.

darshil929 avatar Mar 12 '25 05:03 darshil929