Repository navigation
[Bugfix] Validate expand >= 1.0 in MovingAverageMSEObserver - #3095
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
👋 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. |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Require one maintainer reviewAll PRs must have at least one approving review from a maintainer before merging.
|
There was a problem hiding this comment.
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.
|
This is ready for review. I don't have permission to add the |
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]>
9737eaa to
ac84ba6
Compare
|
Thanks @Roderick-Wu @kylesayrs! |
Summary
MemorylessMSEObserver.__init__validatesexpand >= 1.0, but the default"mse"observer,MovingAverageMSEObserver, is missing the same check and silently acceptsexpand < 1.0.expandscales the initial MSE search range (min_val = amin * expand,max_val = amax * expandin_grid_search_mse), and the grid search can only shrink it from there (p = 1 - i / grid <= 1). So anexpand < 1.0starts 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 whyMemorylessMSEObserverrejects it;MovingAverageMSEObservershould too.Reproduce
Changes
expand >= 1.0validation toMovingAverageMSEObserver.__init__, matchingMemorylessMSEObserver.expand < 1.0and acceptexpand >= 1.0.Testing
tests/llmcompressor/observers/test_mse.pypasses (the newtest_mse_observer_rejects_expand_below_onecovers both observers).ruff check/ruff format --checkclean on the changed files. (Note: the pre-existingtest_mse_observer_torch_compilefails locally on macOS due to atorch.compilebackend issue unrelated to this change — it fails the same way onmain.)