Skip to content

[WIP][Quantization] Update default observer to be MSE #300

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shanjiaz
Copy link

@shanjiaz shanjiaz commented Apr 15, 2025

Description
This PR addresses the following issue. Update such that the MSE is used as the default observer as opposed to MinMax.

Testing
Ran examples/quantization_w4a16 and inspected the observer. See QuantizationArgs:

weights=QuantizationArgs(num_bits=4, type='int', symmetric=True, group_size=128, strategy='group', block_structure=None, dynamic=False, actorder=None, observer='mse', observer_kwargs={})

More details:
Screenshot 2025-04-15 at 3 50 43 PM
Concern
@anmarques @eldarkurtic Wanted to reach out to confirm if this is what we wanted : )

rahul-tuli
rahul-tuli previously approved these changes Apr 15, 2025
Copy link
Member

@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.

Let's go!

Copy link
Contributor

@brian-dellabetta brian-dellabetta left a comment

Choose a reason for hiding this comment

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

This seems like a pretty significant one-line change, that would affect pretty much every quantization pathway. Do we always want MSE over MinMax? Should we start a CHANGELOG so users are aware of internal changes like this?

@kylesayrs
Copy link
Contributor

@brian-dellabetta : @dalistarh has validated that the MSE observer is better across the board (within reason). Logging changes is another topic outside of the scope of this PR I believe

Copy link
Contributor

@kylesayrs kylesayrs left a comment

Choose a reason for hiding this comment

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

Since observers are only used by LLM Compressor, this change is safe to make without affecting existing checkpoints.

Just FYI for the future, this is not the case for actorder, as the checkpoints do not write the actorder explicitly, so modifying the CT checkpoint default will modify existing checkpoints. If we want to modify the actorder default, it will likely require adding an actorder field to GPTQModifier and resolving that with existing QuantizationArgs.

But side note aside, looks great!

@shanjiaz
Copy link
Author

Since observers are only used by LLM Compressor, this change is safe to make without affecting existing checkpoints.

Just FYI for the future, this is not the case for actorder, as the checkpoints do not write the actorder explicitly, so modifying the CT checkpoint default will modify existing checkpoints. If we want to modify the actorder default, it will likely require adding an actorder field to GPTQModifier and resolving that with existing QuantizationArgs.

But side note aside, looks great!

Thanks Kyle! Will keep that in mind.

@dsikka
Copy link
Collaborator

dsikka commented Apr 15, 2025

This seems like a pretty significant one-line change, that would affect pretty much every quantization pathway. Do we always want MSE over MinMax? Should we start a CHANGELOG so users are aware of internal changes like this?

We should run through our lm-eval testing after this change lands. Ive already spoken to Helen about running the test

Copy link
Collaborator

@dsikka dsikka left a comment

Choose a reason for hiding this comment

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

We likely need to do two sets of validations before landing

  1. Run through our lm-eval tests to ensure no regression
  2. Validate timings/potential slowdowns.

We can connect on how to set this up @shanjiaz

@shanjiaz shanjiaz changed the title [Quantization] Update default observer to be MSE [WIP][Quantization] Update default observer to be MSE Apr 15, 2025
@shanjiaz shanjiaz marked this pull request as draft April 15, 2025 22:43
@shanjiaz
Copy link
Author

Test results available here.

Copy link
Contributor

@brian-dellabetta brian-dellabetta left a comment

Choose a reason for hiding this comment

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

First PR! 🥳 🥇

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.

5 participants