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

Extend disable_hooks to keep subsets #1023

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

kylesayrs
Copy link
Collaborator

@kylesayrs kylesayrs commented Jan 1, 2025

Purpose

  • Allow subsets of handles to remain active, as is needed in the case of wanda
def _get_activations(self, model, dataloader, nsamples=128):
  def save_acts(module, input, name):
      ...
  
  hooks = set(
      self.register_hook(mod, partial(save_acts, name=name), "forward_pre")
      for name, mod in self.model.named_modules()
      if isinstance(mod, torch.nn.Linear) and "lm_head" not in name
  )
  # in the future, if the user puts wanda after another modifier,
  # initialize will run after other modifiers have added hooks

  # want to disable hooks from other modifiers, but keep the ones we just added
  with HooksMixin.disable_hooks(keep=hooks):
      run_basic(model, dataloader)
  self.remove_hooks(hooks)

Prerequisites

Postrequisites

  • Layer compressor deprecation

Changes

  • Add a _HOOKS_KEEP_ENABLED class variable

Tests

  • Added test_disable_hooks_keep in tests

Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Copy link

github-actions bot commented Jan 1, 2025

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

Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
@kylesayrs kylesayrs self-assigned this Jan 1, 2025
@kylesayrs kylesayrs added the ready When a PR is ready for review label Jan 19, 2025
rahul-tuli
rahul-tuli previously approved these changes Jan 29, 2025
Copy link
Collaborator

@rahul-tuli rahul-tuli left a comment

Choose a reason for hiding this comment

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

The new changes LGTM! Great work @kylesayrs

src/llmcompressor/modifiers/utils/hooks.py Show resolved Hide resolved
@kylesayrs kylesayrs changed the base branch from main to kylesayrs/hooks-mixin-remove-subsets January 29, 2025 18:32
@kylesayrs kylesayrs changed the base branch from kylesayrs/hooks-mixin-remove-subsets to main January 29, 2025 18:34
@kylesayrs kylesayrs dismissed rahul-tuli’s stale review January 29, 2025 18:34

The base branch was changed.

mgoin pushed a commit that referenced this pull request Jan 29, 2025
## Purpose ##
* Allow subsets of hooks to be removed
* Not strictly needed but helps promote code clarity in the case of
wanda which adds and removes subsets of hooks at different times.

## Postrequisites ##
* #1023
* Layer compressor deprecation

## Changes ##
* Change the datatype of `_hooks` from `List` to `Set`
* Add `handles` argument to `HooksMixin.remove_hooks`

## Testing ##
* Added `test_remove_hooks_parameterized` test

---------

Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready When a PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants