Skip to content

[Common] Extract Functions to Get and Validate Group Sizes in Weights Compression - #4237

Merged
AlexanderDokuchaev merged 8 commits into
openvinotoolkit:developfrom
anzr299:an/wc_group_size_default
Oct 13, 2026
Merged

AlexanderDokuchaev merged 8 commits into
openvinotoolkit:developfrom
anzr299:an/wc_group_size_default

Conversation

@anzr299

@anzr299 anzr299 commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Changes

Rework the weights compression algorithm to make reusable getter for default and fixed group sizes and also for validating group sizes for different modes.

Reason for changes

To be reused by scoped config feature

Related tickets

CVS-180366
#4171

Tests

@anzr299
anzr299 requested a review from a team as a code owner September 25, 2026 14:17
Copilot AI lite review requested due to automatic review settings September 25, 2026 14:17
@github-actions github-actions Bot added the NNCF OpenVINO Pull requests that updates NNCF OpenVINO label Sep 25, 2026

This comment was marked as resolved.


mode: CompressWeightsMode = CompressWeightsMode.INT8_ASYM
group_size: int = -1
group_size: int | None = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

group_size is annotated as int | None, even though __post_init__ resolves it to an int.
But Mypy still treats it as optional elsewhere, for example:
channel_size // group_size a possible None operand`

mypy isn't turned on for this files yet, but refactoring like this will only complicate its adoption across the whole project.

Looking at the implementation, now i see that passing an internal config to the external API seems like a bad idea. We won't be able to change internal details later without breaking/changing the external API.

@anzr299 anzr299 Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That is a good point. We can still create a thin wrapper on top of WeightCompressionConfig to expose externally. it might be overcomplicating though.

@github-actions github-actions Bot removed the NNCF OpenVINO Pull requests that updates NNCF OpenVINO label Oct 13, 2026
@anzr299 anzr299 changed the title [Common] Make Group Size Default to None [Common] Extract Functions to Get and Validate Group Sizes in Weights Compression Oct 13, 2026
DEFAULT_GROUP_SIZE_FOR_NON_INT8_MODES = 128


def get_default_group_size(mode: CompressWeightsMode, group_size: int) -> int:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. If it's named get_default_group_size, semantically it should only take mode and use.
    The problem is that the function does more than its name implies, which is likely why the incorrect type annotation was added.
group_size or get_default_group_size(model)
# or 
if group_size is None:
   group_size = get_default_group_size(model)

It's ok if you keep as it, but fix group_size: int | None) and rename function to
resolve_group_size or get_group_size_or_default

And idea to the next PRs, move some function in seprate files, algorithms.py now contains more 1000 lines

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good point. I will move the check for group_size is None from the function to call site.
since it is better if the function does 1 small nuclear thing

I agree with moving some functions and will do it in a follow up PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The refactoring preserves existing behavior and is covered by existing integration tests.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@AlexanderDokuchaev
AlexanderDokuchaev merged commit 4b29953 into openvinotoolkit:develop Oct 13, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants