-
Notifications
You must be signed in to change notification settings - Fork 588
[Fix] Calibrate non-decoder modules during layerwise quantization #2339
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| import time | ||
| import warnings | ||
| from collections.abc import Callable, Mapping, Sequence | ||
| from contextlib import ExitStack | ||
| from functools import partial | ||
| from typing import Any, TypeAlias | ||
|
|
||
|
|
@@ -34,6 +35,7 @@ | |
| from modelopt.torch.quantization.utils.layerwise_calib import ( | ||
| LayerActivationCollector, | ||
| _CheckpointState, | ||
| _hide_modules_from_traversal, | ||
| _reconcile_export_with_resume, | ||
| ) | ||
| from modelopt.torch.utils import print_rank_0, warn_rank_0 | ||
|
|
@@ -2084,6 +2086,20 @@ def layerwise_calibrate( | |
| "Layerwise calibration requires a model with identifiable transformer layers." | ||
| ) | ||
|
|
||
| decoder_owned_ids = {id(module) for layer in transformer_layers for module in layer.modules()} | ||
| has_enabled_outside_quantizer = any( | ||
| isinstance(module, TensorQuantizer) | ||
| and module.is_enabled | ||
| and id(module) not in decoder_owned_ids | ||
| for module in model.modules() | ||
| ) | ||
|
|
||
| if export_dir is not None and has_enabled_outside_quantizer: | ||
| raise ValueError( | ||
| "Layerwise export does not support enabled quantizers outside transformer layers. " | ||
| "Calibrate without export_dir, then export the completed model separately." | ||
| ) | ||
|
Comment on lines
+2089
to
+2101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMPORTANT Performance] The gate is Why it matters: for a weight-only recipe that enables This file already has the precise predicate for the forward question — Suggested shape: keep the if has_enabled_outside_quantizer:
...
with _hide_modules_from_traversal(model, transformer_layers):
extra_kwargs = dict(calib_kwargs)
if calib_func is max_calibrate:
# Outside quantizers may be weight-only / dynamic / MX; let max_calibrate
# skip the (full-model, full-dataset) forward when no activation stats are needed.
extra_kwargs.setdefault("skip_forward_without_activation_calib", True)
...At minimum, please make the
Comment on lines
+2097
to
+2101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION] The error message tells the user to drop That needs |
||
|
|
||
| num_layers = len(transformer_layers) | ||
| print_rank_0(f"Layerwise calibration: Found {num_layers} transformer layers") | ||
|
|
||
|
|
@@ -2205,6 +2221,27 @@ def _layer_forward_loop(m, _inputs=layer_inputs): | |
| if ckpt: | ||
| ckpt.full_restore(transformer_layers, model) | ||
|
|
||
| if has_enabled_outside_quantizer: | ||
| if any(device == "disk" for device in getattr(model, "hf_device_map", {}).values()): | ||
|
realAsma marked this conversation as resolved.
|
||
| warn_rank_0( | ||
| "Layerwise calibration found enabled quantizers outside transformer layers. " | ||
| "The required full-model calibration pass may be slow because disk-offloaded " | ||
| "decoder weights can be streamed for every batch." | ||
| ) | ||
|
|
||
| with _hide_modules_from_traversal(model, transformer_layers): | ||
|
Comment on lines
+2224
to
+2232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION] Today the common paths are safe: HF layerwise is effectively single-process, and Cheap insurance: all-reduce the flag (logical OR / max) across the model's parallel groups before branching, so every rank takes the same path. A comment stating the single-process/replica-symmetry assumption would also be enough if you'd rather not add the collective. |
||
| if qdq_from_prev: | ||
| calib_func(model, forward_loop, **calib_kwargs) | ||
| else: | ||
| with ExitStack() as stack: | ||
|
realAsma marked this conversation as resolved.
|
||
| for layer in transformer_layers: | ||
| stack.enter_context( | ||
| set_quantizer_by_cfg_context( | ||
| layer, [{"quantizer_name": "*", "enable": False}] | ||
| ) | ||
| ) | ||
| calib_func(model, forward_loop, **calib_kwargs) | ||
|
|
||
| if exporter is not None: | ||
| exporter.finalize() | ||
| print_rank_0(f"Layerwise export: wrote quantized checkpoint to {export_dir}") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| import os | ||
| import shutil | ||
| from collections import deque | ||
| from contextlib import contextmanager | ||
| from dataclasses import dataclass, field | ||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
|
|
@@ -42,7 +43,7 @@ | |
| ) | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Callable | ||
| from collections.abc import Callable, Sequence | ||
|
|
||
| from modelopt.torch.opt.searcher import ForwardLoop | ||
|
|
||
|
|
@@ -101,6 +102,48 @@ def forward(self, *args, **kwargs): | |
| ) | ||
|
|
||
|
|
||
| class _ForwardOnlyLayer(nn.Module): | ||
| """Hide a layer from module traversal while preserving its forward execution.""" | ||
|
|
||
| _PROXY_BLOCKLIST = _SkipLayer._PROXY_BLOCKLIST | ||
|
|
||
| def __init__(self, original: nn.Module): | ||
| super().__init__() | ||
| object.__setattr__(self, "_original", original) | ||
|
|
||
| def __getattr__(self, name: str): | ||
| try: | ||
| return super().__getattr__(name) | ||
| except AttributeError: | ||
| if name in self._PROXY_BLOCKLIST: | ||
| raise | ||
| return getattr(object.__getattribute__(self, "_original"), name) | ||
|
|
||
| def forward(self, *args, **kwargs): | ||
| return self._original(*args, **kwargs) | ||
|
|
||
|
Comment on lines
+105
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION] Two small things on the proxy:
|
||
|
|
||
| @contextmanager | ||
| def _hide_modules_from_traversal(model: nn.Module, modules: Sequence[nn.Module]): | ||
| """Temporarily hide registered modules while retaining their forward behavior.""" | ||
| target_ids = {id(module) for module in modules} | ||
| slots = [ | ||
| (parent, child_name, child) | ||
| for parent in tuple(model.modules()) | ||
| for child_name, child in tuple(parent._modules.items()) | ||
| if child is not None and id(child) in target_ids | ||
| ] | ||
| proxies = {id(child): _ForwardOnlyLayer(child) for _, _, child in slots} | ||
|
|
||
| try: | ||
| for parent, child_name, child in slots: | ||
| parent._modules[child_name] = proxies[id(child)] | ||
| yield | ||
| finally: | ||
| for parent, child_name, child in slots: | ||
| parent._modules[child_name] = child | ||
|
|
||
|
|
||
| class LayerActivationCollector: | ||
| """Collects layer activations for layerwise (layer-by-layer) calibration. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.