1686 Logic matching refactor #1687

ved1beta · 2025-07-28T17:09:57Z

SUMMARY:
Refactor
had some questions regarding this PR
also let me if i am on the right track
#1686

github-actions · 2025-07-28T17:10:05Z

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed.

gemini-code-assist

Summary of Changes

Hello @ved1beta, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the AWQModifier by replacing a custom utility function for accessing nested PyTorch model layers with Python's built-in operator.attrgetter. The primary goal is to simplify the codebase and align with standard library practices for attribute lookup, thereby improving maintainability.

Highlights

Refactor get_layer_by_name usage: Replaced direct calls to the custom utility function get_layer_by_name with operator.attrgetter for retrieving nested torch.nn.Module instances by their string names. This change was applied in the _set_resolved_mappings method and the get_lowest_common_parent function within src/llmcompressor/modifiers/awq/base.py.
Dependency cleanup: Removed the import of get_layer_by_name from llmcompressor.utils.pytorch.module in src/llmcompressor/modifiers/awq/base.py as it is no longer utilized after the refactor.
Code simplification and standard library adoption: Introduced the import of operator.attrgetter to leverage a standard Python library function for attribute access, leading to a more concise and potentially more robust way of navigating the model's module hierarchy.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request refactors the usage of get_layer_by_name in src/llmcompressor/modifiers/awq/base.py to use operator.attrgetter directly. This is a good simplification that removes an unnecessary layer of abstraction.

I've identified a critical issue where the new code could crash if a module name doesn't contain a dot, and I've provided a suggestion to fix it. This issue was also present in the previous implementation but is a good opportunity to address now.

Regarding your question in the pull request description, if get_layer_by_name is no longer used anywhere else in the project after this change, then you should proceed with removing the function definition from src/llmcompressor/utils/pytorch/module.py and its corresponding tests. This would complete the refactoring. I'd recommend a project-wide search for other usages before removing it.

gemini-code-assist · 2025-07-28T17:11:41Z

src/llmcompressor/modifiers/awq/base.py

@@ -323,7 +324,7 @@ def _set_resolved_mappings(self, model: Module) -> None:
                smooth_layer = smooth_layers[smooth_name]

                smooth_parent_name = ".".join(smooth_name.split(".")[:-1])
-                smooth_parent = get_layer_by_name(smooth_parent_name, model)
+                smooth_parent = attrgetter(smooth_parent_name)(model)


This line could raise an AttributeError if smooth_parent_name is an empty string. This can happen if smooth_name refers to a top-level module in model (i.e., its name does not contain any dots). In that case, smooth_name.split('.')[:-1] results in an empty list, and smooth_parent_name becomes "", which is not a valid input for attrgetter.

To prevent a crash, you should handle this case by assigning model as the parent when smooth_parent_name is empty, as this indicates smooth_name is a direct attribute of model.

Suggested change

smooth_parent = attrgetter(smooth_parent_name)(model)

smooth_parent = attrgetter(smooth_parent_name)(model) if smooth_parent_name else model

Yeah I'm a bit concerned this has the potential to error out as well. what if there's no parent?

…yers ->match_named_modules

kylesayrs

Looks good so far, thanks for your help with this

This exclude_internal_modules option is now always on by default, so there's no need to include it explicitly

src/llmcompressor/modifiers/awq/base.py

Co-authored-by: Kyle Sayers <[email protected]>

ved1beta · 2025-08-07T12:51:55Z

hey any update on this ?

brian-dellabetta

Hi @ved1beta , thanks for the contributions. I have some questions/changes requested before we have the ci/cd workflows run

brian-dellabetta · 2025-08-07T16:58:50Z

src/llmcompressor/transformers/compression/helpers.py

@@ -76,7 +76,7 @@ def infer_sparsity_structure_from_model(model: torch.nn.Module) -> Optional[str]
    # check for the common sparsity structures
    structures = {"2:4"}
    for sparsity_structure in structures:
-        linear_modules = get_linear_layers(model)
+        linear_modules = match_named_modules(model, linear=True)


is this right? I don't see a linear input to match_named_modules

brian-dellabetta · 2025-08-07T16:59:22Z

src/llmcompressor/modifiers/smoothquant/base.py

                    balance_layers = []
                    for balance_suffix in to_balance:
                        # find the submodule that matches the activation layer
-                        _, balance_layer = get_matching_layer(
+                        _, balance_layer =match_modules_set(


looks like you need to run formatting

brian-dellabetta · 2025-08-07T17:00:54Z

src/llmcompressor/modifiers/obcq/sgpt_base.py

@@ -149,11 +147,11 @@ def on_start(self, state: State, event: Event, **kwargs):
                layer_sparsity = self.sparsity[index]
            else:
                layer_sparsity = self.sparsity
-
-            for name, module in get_prunable_layers(layer).items():
+            prunable_targets = ["Linear", "Conv1d", "Conv2d", "Conv3d", "QATLinear", "QATConv2d", "QATConv3d", "Conv1D"]


we should move this to a helper function instead of having it here and on line 211. @kylesayrs should we add this to the compressed-tensors matching code?

brian-dellabetta · 2025-08-07T17:02:23Z

src/llmcompressor/modifiers/distillation/output/base.py

@@ -109,8 +109,8 @@ def on_finalize(self, state: State, **kwargs) -> bool:

        with summon_full_params_context(state.teacher_model, offload_to_cpu=True):
            for key, (student_wrapper, teacher_wrapper) in self.wrappers_.items():
-                set_layer(key, student_wrapper.layer, state.model)
-                set_layer(key, teacher_wrapper.layer, state.teacher_model)
+                Module.set_submodule(key, student_wrapper.layer, state.model)


we're sure we want to call a class method on Module here and not a method on an instance?

brian-dellabetta · 2025-08-07T17:03:21Z

src/llmcompressor/modifiers/awq/base.py

@@ -323,7 +324,7 @@ def _set_resolved_mappings(self, model: Module) -> None:
                smooth_layer = smooth_layers[smooth_name]

                smooth_parent_name = ".".join(smooth_name.split(".")[:-1])
-                smooth_parent = get_layer_by_name(smooth_parent_name, model)
+                smooth_parent = attrgetter(smooth_parent_name)(model)


Yeah I'm a bit concerned this has the potential to error out as well. what if there's no parent?

…to refactor_1686

brian-dellabetta

LGTM! Thanks!

get_layer_by_name refactor

63cb6e6

gemini-code-assist bot reviewed Jul 28, 2025

View reviewed changes

ved1beta added 5 commits July 29, 2025 00:15

get_layers_params refactor

06a1e71

set_layer -> Module.set_submodule

932daf5

removed get_param get_params

ba045bc

removed get_layer, get_layers -> match_named_modules

d495843

removed get_default_params and match_layers_params

6303210

ved1beta changed the title ~~get_layer_by_name refactor~~ 1686 refactor Jul 29, 2025

ved1beta and others added 3 commits July 29, 2025 22:53

removed get_quantizable,prunable,terminal_layers

3394c8c

get_matching_layer -> match_modules_set, match_modules, get_linear_la…

f012ae6

…yers ->match_named_modules

Merge branch 'main' into refactor_1686

a709fa6

ved1beta marked this pull request as ready for review July 29, 2025 17:54

kylesayrs reviewed Jul 29, 2025

View reviewed changes

src/llmcompressor/modifiers/awq/base.py Outdated Show resolved Hide resolved

Update src/llmcompressor/modifiers/awq/base.py

94d2b94

Co-authored-by: Kyle Sayers <[email protected]>

ved1beta requested a review from kylesayrs August 2, 2025 13:46

brian-dellabetta changed the title ~~1686 refactor~~ 1686 Logic matching refactor Aug 7, 2025

brian-dellabetta requested changes Aug 7, 2025

View reviewed changes

ved1beta added 3 commits August 8, 2025 17:35

required changes

e740de3

format

0a7a79f

Merge branch 'refactor_1686' of github.com:ved1beta/llm-compressor in…

f5740fe

…to refactor_1686

brian-dellabetta approved these changes Aug 8, 2025

View reviewed changes

	smooth_parent = attrgetter(smooth_parent_name)(model)
	smooth_parent = attrgetter(smooth_parent_name)(model) if smooth_parent_name else model

1686 Logic matching refactor #1687

Are you sure you want to change the base?

1686 Logic matching refactor #1687

Uh oh!

Conversation

ved1beta commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 28, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

ved1beta Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

brian-dellabetta Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

kylesayrs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ved1beta commented Aug 7, 2025

Uh oh!

brian-dellabetta left a comment

Choose a reason for hiding this comment

Uh oh!

brian-dellabetta Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

brian-dellabetta Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

brian-dellabetta Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

brian-dellabetta Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

brian-dellabetta Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

brian-dellabetta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ved1beta commented Jul 28, 2025 •

edited

Loading