Skip to content

[Bugfix] Validate expand >= 1.0 in MovingAverageMSEObserver - #3095

Merged
kylesayrs merged 1 commit into
vllm-project:mainfrom
winklemad:fix/mse-observer-expand-validation
Sep 8, 2026
Merged

kylesayrs merged 1 commit into
vllm-project:mainfrom
winklemad:fix/mse-observer-expand-validation

Conversation

@winklemad

Copy link
Copy Markdown
Contributor

Summary

MemorylessMSEObserver.__init__ validates expand >= 1.0, but the default "mse" observer, MovingAverageMSEObserver, is missing the same check and silently accepts expand < 1.0.

expand scales the initial MSE search range (min_val = amin * expand, max_val = amax * expand in _grid_search_mse), and the grid search can only shrink it from there (p = 1 - i / grid <= 1). So an expand < 1.0 starts the search below the observed min/max and can never grow to cover the data — every candidate range clips the calibration values, producing a degenerate result. That's exactly why MemorylessMSEObserver rejects it; MovingAverageMSEObserver should too.

Reproduce

from compressed_tensors.quantization import QuantizationArgs
from llmcompressor.observers.mse import MemorylessMSEObserver, MovingAverageMSEObserver

args = QuantizationArgs(num_bits=8, symmetric=True)

MemorylessMSEObserver(base_name="weight", args=args, expand=0.5)
# ValueError: expand value must be at least 1.0, got 0.5   ✅

MovingAverageMSEObserver(base_name="weight", args=args, expand=0.5)
# (no error) ❌ — silently accepted, degenerate search range

Changes

  • Add the expand >= 1.0 validation to MovingAverageMSEObserver.__init__, matching MemorylessMSEObserver.
  • Add a parametrized regression test asserting both MSE observers reject expand < 1.0 and accept expand >= 1.0.

Testing

tests/llmcompressor/observers/test_mse.py passes (the new test_mse_observer_rejects_expand_below_one covers both observers). ruff check / ruff format --check clean on the changed files. (Note: the pre-existing test_mse_observer_torch_compile fails locally on macOS due to a torch.compile backend issue unrelated to this change — it fails the same way on main.)

@coderabbitai

coderabbitai Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: be7305ce-835c-4c59-bf07-2dbb57daeca1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

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

@mergify

mergify Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Require one maintainer review

All PRs must have at least one approving review from a maintainer before merging.

  • #changes-requested-reviews-by = 0
  • any of:
    • approved-reviews-by=kylesayrs
    • approved-reviews-by=HDCharles
    • approved-reviews-by=brian-dellabetta
    • approved-reviews-by=dsikka
    • approved-reviews-by=yiliu30

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request adds a validation check to the MSE observer initialization to ensure that the expand parameter is at least 1.0, raising a ValueError otherwise. It also adds a unit test to verify this behavior for MemorylessMSEObserver and MovingAverageMSEObserver. The review feedback suggests including NVFP4ExpandedMSEObserver in the test suite as well, and recommends recreating the QuantizationArgs instance between assertions to prevent test pollution caused by in-place mutations.

@winklemad

Copy link
Copy Markdown
Contributor Author

This is ready for review. I don't have permission to add the ready label myself — could a maintainer add it to unblock the review gate? Thanks!

@Roderick-Wu Roderick-Wu left a comment

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.

good catch

@Roderick-Wu Roderick-Wu added the ready When a PR is ready for full CI testing before merge label Aug 31, 2026
MemorylessMSEObserver rejects expand < 1.0, but the default "mse"
observer (MovingAverageMSEObserver) was missing the same check and
silently accepted it. Since expand scales the initial MSE search range
(min/max * expand) and the grid search can only shrink it from there, an
expand < 1.0 starts below the observed range and can never cover the
data, so it must be rejected rather than produce a degenerate range.
Add the validation to match MemorylessMSEObserver and cover both
observers with a regression test.

Signed-off-by: Madan Kumar <[email protected]>
@winklemad
winklemad force-pushed the fix/mse-observer-expand-validation branch from 9737eaa to ac84ba6 Compare September 4, 2026 20:08
@kylesayrs
kylesayrs enabled auto-merge (squash) September 8, 2026 16:54
@kylesayrs
kylesayrs merged commit d784cbd into vllm-project:main Sep 8, 2026
8 checks passed
@winklemad

Copy link
Copy Markdown
Contributor Author

Thanks @Roderick-Wu @kylesayrs!

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 full CI testing before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants