Roderick wu/tested four over six - #2903
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: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the 'Four Over Six' (4/6) adaptive block scaling observer (FourOverSixObserver) for NVFP4 quantization, along with an example script demonstrating its usage on Llama 3. The feedback focuses on improving the observer's robustness and correctness: specifically, caching the computed optimized scale in get_qparams to ensure idempotency on subsequent calls, clearing this cache and resetting state when new statistics are observed or the observer is reset, and adding explicit type annotations to the _FP8ScaleData256 Pydantic subclass to prevent validation issues.
| def update_statistics_from_observed(self, observed: torch.Tensor) -> None: | ||
| self.min_vals = torch.amin(observed, dim=(0, -1)) | ||
| self.max_vals = torch.amax(observed, dim=(0, -1)) | ||
| self._observed_blocks = observed.detach().clone() |
There was a problem hiding this comment.
When new statistics are observed, any previously cached scale becomes invalid and must be cleared. Additionally, we should implement a reset method to properly clear the cached scale and observed blocks when the observer is reset.
| def update_statistics_from_observed(self, observed: torch.Tensor) -> None: | |
| self.min_vals = torch.amin(observed, dim=(0, -1)) | |
| self.max_vals = torch.amax(observed, dim=(0, -1)) | |
| self._observed_blocks = observed.detach().clone() | |
| def update_statistics_from_observed(self, observed: torch.Tensor) -> None: | |
| self.min_vals = torch.amin(observed, dim=(0, -1)) | |
| self.max_vals = torch.amax(observed, dim=(0, -1)) | |
| self._observed_blocks = observed.detach().clone() | |
| if hasattr(self, "_cached_scale"): | |
| delattr(self, "_cached_scale") | |
| def reset(self): | |
| super().reset() | |
| self._observed_blocks = None | |
| if hasattr(self, "_cached_scale"): | |
| delattr(self, "_cached_scale") |
| if self._observed_blocks is not None: | ||
| scale = self._select_block_scales( | ||
| self._observed_blocks, scale_6, zero_point, global_scale | ||
| ) | ||
| self._observed_blocks = None | ||
| else: | ||
| scale = scale_6 |
There was a problem hiding this comment.
Currently, self._observed_blocks is set to None after the first call to get_qparams(). If get_qparams() is called again (which can happen during serialization, export, or evaluation), it will fall back to returning scale_6 (the standard NVFP4 scale) instead of the optimized 4/6 adaptive block scales.
To ensure idempotency and correctness, we should cache the computed optimized scale in self._cached_scale and return it on subsequent calls.
| if self._observed_blocks is not None: | |
| scale = self._select_block_scales( | |
| self._observed_blocks, scale_6, zero_point, global_scale | |
| ) | |
| self._observed_blocks = None | |
| else: | |
| scale = scale_6 | |
| if self._observed_blocks is not None: | |
| scale = self._select_block_scales( | |
| self._observed_blocks, scale_6, zero_point, global_scale | |
| ) | |
| self._cached_scale = scale | |
| self._observed_blocks = None | |
| elif hasattr(self, "_cached_scale"): | |
| scale = self._cached_scale | |
| else: | |
| scale = scale_6 |
There was a problem hiding this comment.
@krishnateja95
This looks to me like a real issue? But I'm not too sure about this one. Are there conditions where get_qparams() is called again?
This reverts commit eb8966a. put on different branch
|
The quality checks have failed. Please run |
d04e417 to
103013c
Compare
|
The quality checks have failed. Please run |
mergify
Bot
added
two-reviews
Merge Protections🔴 2 of 2 protections blocking · waiting on 👀 reviews
🔴 Require one maintainer reviewWaiting for any of
This rule is failing.All PRs must have at least one approving review from a maintainer before merging.
🔴 Require two reviewsWaiting for
This rule is failing.PRs labelled "two-reviews" must have at least two approving reviews before merging.
|
|
The quality checks have failed. Please run |
|
The quality checks have failed. Please run |
|
This pull request has merge conflicts that must be resolved before it can be |
kylesayrs
left a comment
kylesayrs
left a comment
There was a problem hiding this comment.
Looks correct to me, @Roderick-Wu please review and ping when ready to merge
2 participants
From vllm-project/compressed-tensors#776