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

Implement FracBitsQuantizationBuilder and Controller #1234

Merged

Conversation

vinnamkim
Copy link
Contributor

@vinnamkim vinnamkim commented Jul 22, 2022

Changes

  • Implement Builder and Controller
  • Add and test ModelSizeCompressionLoss

Reason for changes

  • To implement the initial version of FracBitsQuantizationBuilder, Controller, and Loss.

Related tickets

87888, 87889

Tests

Related unit tests added

@vinnamkim vinnamkim requested a review from a team as a code owner July 22, 2022 08:24
@github-actions github-actions bot added experimental NNCF PT Pull requests that updates NNCF PyTorch labels Jul 22, 2022
@vinnamkim vinnamkim force-pushed the impl-fracbits-controller branch from e731ccb to 3a7d4e2 Compare July 22, 2022 10:15
 - Implement Builder and Controller
 - Add and test ModelSizeCompressionLoss

Signed-off-by: Kim, Vinnam <[email protected]>
@vinnamkim vinnamkim force-pushed the impl-fracbits-controller branch from 3a7d4e2 to 757e6a4 Compare July 22, 2022 10:17
@vinnamkim vinnamkim added the ready for reviews This PR is ready for reviews label Jul 22, 2022
Signed-off-by: Kim, Vinnam <[email protected]>
Signed-off-by: Kim, Vinnam <[email protected]>
@vinnamkim vinnamkim requested a review from wonjuleee July 25, 2022 07:46
@github-actions github-actions bot added the NNCF Common Pull request that updates NNCF Common label Jul 25, 2022
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.

@vinnamkim, thanks for your contribution! I have major comments/questions about this PR and #1231. Could you share what results you have achieved with the FracBits quantization algorithm? What user cases are you going to cover by this algorithm? What is the benefit?

In accordance with the contribution guide for NNCF. The PRs with fully implemented features are only approved. All research must be done on forks.

cc' @AlexKoff88

Signed-off-by: Kim, Vinnam <[email protected]>
@vinnamkim
Copy link
Contributor Author

vinnamkim commented Jul 26, 2022

@vinnamkim, thanks for your contribution! I have major comments/questions about this PR and #1231. Could you share what results you have achieved with the FracBits quantization algorithm? What user cases are you going to cover by this algorithm? What is the benefit?

First of all, as far as I know this item is agreed with Lee, Wonju and Kozlov, Alexander on seeing results after quick implementation. I'm working now to obtain the experiment results on MobileNetV2, and etc... Basically, it can extend user choice for QAT algorithms. Currently, we has been provided only two algorithm: HaWQ and AutoQ which has an additional exploration stage aside QAT. Fracbits provides fully differential training for bit-widths, which is completely different from HaWQ and AutoQ.

Therefore, this algorithm's benefit is exactly same for HaWQ and AutoQ. On top of that, I think the mixed precision will be more important in the future. This is because the model size of the vision models that NNCF has dealt with so far was so small if we compared to Transformers (BERT-Large ~1.4G). However, Transformers becomes the major player in AI scene and I believe there is demand from users to compress their deployment model size. We all know that commercial hardware acceleration for NN is supported up to 8bit, and OV runtime doesn't allow to save model weights on the mixed precision. But, I think it's easy to implement at the software level.

In accordance with the contribution guide for NNCF. The PRs with fully implemented features are only approved. All research must be done on forks.

I understand that this was against the rules. How about make a branching strategy to use a feature branch. We can update and review code in a fast cycle on the feature branch. If all work ends in that feature branch, we can try to merge that feature branch into the develop branch. This is because trying to merge large PRs at once should be avoided in terms of software development. What do you think?

@alexsu52
Copy link
Contributor

alexsu52 commented Jul 26, 2022

merge that feature branch into the develop branch. This is because trying to merge large PRs at once should be avoided in terms of software development. What do you think?

Hi, @vinnamkim. Thank you for your response! I have no concerns about researching in the mixed precision area, in particular, implementing FracBits quantization algorithm. My concern is that the PR must meet the contribution guide and be justified in order to be accepted for merging with develop.

The fork strategy is a default strategy for NNCF and uses in many repositories under the OpenVINO umbrella. If you do not like the fork strategy, I think you can create a branch in the NNCF repository to implement the research feature.

nncf/experimental/torch/fracbits/loss.py Outdated Show resolved Hide resolved
nncf/experimental/torch/fracbits/loss.py Show resolved Hide resolved
nncf/experimental/torch/fracbits/loss.py Outdated Show resolved Hide resolved
tests/torch/experimental/fracbits/test_builder.py Outdated Show resolved Hide resolved
@wonjuleee
Copy link
Contributor

wonjuleee commented Jul 27, 2022

merge that feature branch into the develop branch. This is because trying to merge large PRs at once should be avoided in terms of software development. What do you think?

Hi, @vinnamkim. Thank you for your response! I have no concerns about researching in the mixed precision area, in particular, implementing FracBits quantization algorithm. My concern is that the PR must meet the contribution guide and be justified in order to be accepted for merging with develop.

The fork strategy is a default strategy for NNCF and uses in many repositories under the OpenVINO umbrella. If you do not like the fork strategy, I think you can create a branch in the NNCF repository to implement the research feature.

Hi @alexsu52, FracBits feature should be merged into the develop branch in a viewpoint of "implement a recently published compression algorithm" in the contribution guide. If only the fully implemented feature including example and recipe can be merged into the develop, @vinnamkim, could you work on your forked branch and make PRs to implement all related to FracBits by separating PRs (a small PR must be preferred) and merge this branch to the develop in the upstream repository.
@alexsu52, is this okay to you?

@vinnamkim vinnamkim changed the base branch from develop to feature/fracbits July 27, 2022 04:57
@vinnamkim
Copy link
Contributor Author

vinnamkim commented Jul 27, 2022

I changed this PR's target branch to feature/fracbits and made a revert PR #1237. We can continue the review process for FracBits by uploading PR to feature/fracbits branch. When all works are done, we can make an another PR to merge feature/fracbits into develop branch.

@alexsu52
Copy link
Contributor

Hi, @wonjuleee. Thanks for your comment. I totally agree with you.

Copy link
Contributor

@wonjuleee wonjuleee left a comment

Choose a reason for hiding this comment

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

LGTM.
@vinnamkim, will you make a new PR for FracBits algorithm implementation?
Is this okay to merge this first?

@vinnamkim
Copy link
Contributor Author

vinnamkim commented Jul 28, 2022

LGTM. @vinnamkim, will you make a new PR for FracBits algorithm implementation? Is this okay to merge this first?

Yes, some remaining works may be uploaded as PRs targeting to feature/fracbits branch. If all works is done, we can try to merge feature/fracbits to develop branch.

@vinnamkim
Copy link
Contributor Author

merge that feature branch into the develop branch. This is because trying to merge large PRs at once should be avoided in terms of software development. What do you think?

Hi, @vinnamkim. Thank you for your response! I have no concerns about researching in the mixed precision area, in particular, implementing FracBits quantization algorithm. My concern is that the PR must meet the contribution guide and be justified in order to be accepted for merging with develop.

The fork strategy is a default strategy for NNCF and uses in many repositories under the OpenVINO umbrella. If you do not like the fork strategy, I think you can create a branch in the NNCF repository to implement the research feature.

@alexsu52
I see that your major claim for the change request is that this PR targets to develop branch. Because the target branch for this PR is changed, I think that we can merge it now.

@vinnamkim vinnamkim merged commit 8909446 into openvinotoolkit:feature/fracbits Jul 28, 2022
@alexsu52 alexsu52 self-requested a review July 28, 2022 05:26
vinnamkim added a commit to vinnamkim/nncf that referenced this pull request Oct 4, 2022
…#1234)

* Implement FracBitsQuantizationBuilder and Controller

 - Implement Builder and Controller
 - Add and test ModelSizeCompressionLoss

Signed-off-by: Kim, Vinnam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental NNCF Common Pull request that updates NNCF Common NNCF PT Pull requests that updates NNCF PyTorch ready for reviews This PR is ready for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants