Skip to content

Extend PyTorch RTN weight quantization to MoE experts - #2584

Open
titaiwangms with Copilot wants to merge 26 commits into
mainfrom
copilot/resume-jambayk-moe-quant-extend-rtn-weight
Open

Extend PyTorch RTN weight quantization to MoE experts#2584
titaiwangms with Copilot wants to merge 26 commits into
mainfrom
copilot/resume-jambayk-moe-quant-extend-rtn-weight

Conversation

Copilot AI commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Extends Olive's native PyTorch RTN quantization (olive/common/quant/,
olive/passes/pytorch/rtn.py) to cover MoE fused-expert weights, in addition
to the existing nn.Linear / nn.Embedding support. Produces a standard HF
safetensors checkpoint with MoE experts already quantized, so downstream
consumers (Mobius / ORT GenAI ModelBuilder) don't need to run their own
inline quantization pass.

Design (carried over from jambayk/moe-quant, unchanged)

  • Storage-only MoE quantization. No QuantExperts module re-implementing
    per-architecture forwards (that design was considered and rejected — see
    the original design notes). Instead, a torch.Tensor wrapper subclass,
    QuantTensor (modelled on Quark's quark.qtensor.QTensor), replaces the
    quantized parameter in-place. Host model forwards run unchanged.
  • Uniform by parameter, not by layer type. Target selection
    (selection.py) treats every nn.Parameter the same way regardless of
    rank — 2D linear/embedding weights and 3D fused-expert weights both flow
    through the same WeightQuantizer, which quantizes along the last dim
    regardless of rank.
  • Mobius owns ONNX export of MoE experts. Olive never attempts
    torch.onnx.export on a 3D QuantTensor; this is an explicit, permanent
    design decision, documented in-code and via onnxruntime/mobius#427.

What changed in this PR on top of jambayk/moe-quant

  1. selection.py (new) — single generator (iter_quant_targets)
    unifying target selection for both the HF quantizer and RTN/GPTQ passes.
    Adds MoE-aware routing detection (_collect_experts,
    _layers_missing_experts, _config_indicates_moe) with fail-closed
    behavior: if the config looks like an MoE architecture but the experts
    subtree can't be resolved, quantization refuses to proceed rather than
    silently skipping the expert weights.
  2. wrapper.pyLayerWrapper.get_experts() / get_router()
    accessors, generalizing the existing per-layer-type accessor pattern to
    MoE sub-modules.
  3. Round-1 remediation (8 items from the first adversarial review pass)
    — assorted correctness/robustness fixes to selection, patterns, and
    QuantTensor construction.
  4. Round-2 remediation (4 items):
    • patterns.py: reject nested-group alternation ((a|b) inside a
      repeated group) at any nesting depth — closes a ReDoS bypass of the
      skip-pattern regex safety check. Docstrings demoted from "prevents
      ReDoS" to "best-effort UX check, not a security boundary" (decision:
      no regex third-party dependency added).
    • tensor.py / hf_utils.py / state_dict.py: explicit
      is_placeholder flag threaded through QuantTensor's lifecycle so
      init-style ops (zero_, normal_, ...) only no-op on real
      placeholders and raise otherwise (previously any QuantTensor
      silently no-op'd on these ops, which could mask real bugs).
    • tensor.py: reject rank>1 boolean-mask indexing instead of
      misclassifying it as a safe leading-dim integer index.
    • selection.py / defaults.yaml: rewrite _config_indicates_moe to
      reuse the existing resolve_alias() nested-config mechanism (already
      used for HF I/O config resolution) plus a bounded sub-config sweep —
      fixes DBRX-style nested MoE config detection (ffn_config.moe_num_experts).

All changes verified: 314 tests passing (test/common/quant/,
test/passes/pytorch/test_rtn.py), lintrunner clean.

Known follow-ups (tracked separately, not blocking this PR)

Testing

  • pytest test/common/quant/ test/passes/pytorch/test_rtn.py — 314 passed.
  • lintrunner — clean.
  • Full forward-parity tests (test_forward_parity.py) included, comparing
    quantized vs. unquantized model outputs for both 2D and 3D (MoE) targets.

Copilot AI and others added 14 commits May 15, 2026 22:01
…ent)

This commit checkpoints the in-progress MoE quantization work before a
larger refactor that deletes QuantLinear/QuantEmbedding in favour of
storing every quantized weight (2D linear, 2D embedding, 3D MoE experts)
as a QuantTensor nn.Parameter on the original host module.

Included so far:
- New olive/common/quant/patterns.py for re: prefix matching in
  modules_to_not_convert / overrides.
- New olive/common/quant/tensor.py with QuantTensor wrapper subclass
  (_make_wrapper_subclass + __torch_function__ + __torch_dispatch__),
  supporting 2D and 3D layouts.
- LayerWrapper.get_experts() / get_router() accessors.
- 3D quantize helpers in olive/common/quant/utils.py.
- moe field on OliveHfQuantizationConfig.
- _process_model_before_weight_loading skips ModuleList(Expert) subtrees
  when moe=False, fixing a latent silent-quantization bug for
  Mixtral / PhiMoE / Qwen2/3-MoE.
- Fused-3D MoE support in prepare_model / finalize via QuantTensor
  parameters; current save layout uses _qweight buffer suffixes — to be
  replaced in the upcoming refactor with the canonical
  <param>.qweight/.scales/.qzeros layout.
- ModelBuilder raises NotImplementedError for Olive-quantized MoE
  checkpoints (Mobius is the intended consumer).
- Test additions:
  test/common/quant/test_patterns.py, test/common/quant/test_tensor.py,
  TestOliveHfQuantizerMoE / TestRegexOverrides in test_hf_utils.py,
  test/passes/pytorch/test_quant_utils.py for flatten helper,
  test_olive_quantized_model_raises_for_moe in test_model_builder.py.
- 294 tests pass; lintrunner clean (--skip PYLINT).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch Olive's native quantization representation to a single design:
every quantized weight is an nn.Parameter(QuantTensor) on the original
host nn.Linear / nn.Embedding / fused-3D experts module, with sibling
<pname>_qweight / _scales / _qzeros buffers aliasing the QuantTensor's
inner tensors.

Save: a state-dict hook drops the QuantTensor parameter entry; the
buffers already carry the data (plain Tensors, safetensors-friendly).
Load: HF's loader fills the buffers natively via dotted paths; a
post-load helper re-binds the QuantTensor inner refs to the freshly
loaded buffer storage.

QuantLinear / QuantEmbedding (olive/common/quant/nn.py) are kept only
as ONNX-exportable wrappers used by make_export_compatible_quant; they
are no longer the runtime representation.

* New olive/common/quant/state_dict.py with install_quant_tensor_param
  and refresh_quant_tensor_refs helpers.
* OliveHfQuantizer rewritten for the new layout (placeholder install
  before weight load + ref refresh after).
* finalize() in passes/pytorch/quant_utils.py installs QuantTensor
  params via install_quant_tensor_param (replaces the old
  flatten_quant_tensor_params helper).
* prepare_model skips modules whose weight is already a QuantTensor,
  so composing multiple Rtn passes on top of a partially quantized
  model works.
* make_export_compatible_quant detects nn.Linear / nn.Embedding whose
  weight is a QuantTensor and swaps them with QuantLinear /
  QuantEmbedding wrappers before any model dtype casting, preserving
  the existing com.microsoft::MatMulNBits /
  com.microsoft::GatherBlockQuantized symbolic export path.
* OliveQuantizedModel (model_builder.py) normalizes the new
  <dotted>.weight_qweight key layout back to the legacy
  <dotted>.qweight layout for the existing genai loader, and raises
  NotImplementedError for moe=True checkpoints.
* Tests updated to assert against QuantTensor weight instead of
  isinstance(module, QuantLinear); legacy tie_quant_modules tests
  removed; new install_quant_tensor_param test suite added.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er to N-D

* Remove olive/common/quant/nn.py (QuantModule, QuantLinear,
  QuantEmbedding) entirely. The only purpose of those modules was
  ONNX export, which is now handled by reusing the existing
  QuantLinearNbit from olive/common/hf/quant.py and a new
  parallel QuantEmbeddingNbit (com.microsoft::GatherBlockQuantized
  symbolic) in the same file.
* Add QuantLinearNbit.from_quant_tensor / QuantEmbeddingNbit.from_quant_tensor
  factories so make_export_compatible_quant can swap any nn.Linear /
  nn.Embedding whose weight is a QuantTensor into the export wrappers.
* Generalize WeightQuantizer (get_num_groups, get_qparam_shape,
  find_qparams, quantize, dequantize, _reshape_tensor) and
  pack_to_uint8 / unpack_from_uint8 to operate on any N-D tensor;
  quantization is always along the last dim, leading dims are
  preserved.
* Drop quantize_along_leading_dim / pack_to_uint8_along_last /
  unpack_from_uint8_along_last and the explicit 3D leading-dim loops
  in QuantTensor.from_float and _dequantize.
* Delete test/common/quant/test_nn.py; add N-D tests for the
  generalized quantizer + pack helpers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of pre-walking the safetensors dict to rewrite
``<dotted>.weight_qweight`` -> ``<dotted>.qweight``, derive the
destination attribute name inside ``set_tensor`` once we already
know ``submodule`` is a ``QuantizedTensorModule``. Strip any of the
known Olive buffer suffixes (``QWEIGHT_SUFFIX``, ``SCALES_SUFFIX``,
``QZEROS_SUFFIX`` from ``olive.common.quant.state_dict``) from the
last path component to produce the bare ``qweight`` / ``scales`` /
``qzeros`` attribute that the genai ``QuantizedTensorModule``
expects.

Also drops internal dev-iteration version labels from comments and
docstrings in olive/common/quant and olive/passes/onnx.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both QuantTensor's 2D layout and QuantLinearNbit's MatMulNBits
buffer layout pack the quantization axis as uint8 with the same
in-byte order (low nibble = elem[2j], high nibble = elem[2j+1] for
4-bit, etc.). They differ only in qweight rank: QuantTensor uses
(out, in / pack_factor), QuantLinearNbit uses
(out, n_blocks, blob_size) where n_blocks * blob_size ==
in / pack_factor. So the conversion is a pure reshape; the previous
unpack -> .t() -> from_tensors round-trip is unnecessary.

scales and qzeros buffer shapes also match exactly between the two
layouts, so they are copied as-is. For symmetric weights
(QuantTensor.qzeros is None) we fill the QuantLinearNbit.qzeros
buffer with the packed midq pattern that the contrib op expects.

Verified numerically: F.linear via QuantTensor and the
dequantize-from-buffers path through QuantLinearNbit produce
bit-identical outputs across {4,8} bits, {symmetric, asymmetric},
{groupwise, per-channel}.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The ORT contrib MatMulNBits / GatherBlockQuantized ops treat a missing
zero-points input as midq for unsigned quantization, matching Olive's
symmetric-quantization convention. Drop the synthetic packed-midq
buffer that was previously emitted for symmetric weights and instead
omit the input entirely:

* QuantLinearNbit gains a has_qzeros flag (default True for back-compat);
  pack/from_tensors/from_quant_tensor pass through None as needed.
* QuantLinearTorchFunction (TorchScript + dynamo) skips the qzeros input
  when None, inserting an empty placeholder only when g_idx must be
  positionally aligned.
* QuantEmbeddingTorchFunction.symbolic gains the missing dynamo arg
  exposed by the new symmetric-embedding export path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
g_idx alongside a missing qzeros is not a real combination in Olive
(GPTQ always produces qzeros), so skip the empty-tensor placeholder
and just omit qzeros from the input list entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace duplicated model walks in hf_utils._process_model_before_weight_loading
  and quant_utils.prepare_model with a shared iter_quant_targets helper that
  returns a list of (module, dotted_name, param_name, shape, dtype, device,
  kind) entries. Selection rules (lm_head/embeds/moe category flags, skip
  patterns, extra_skip_modules, already-quantized) live in one place.
- QuantLinearNbit/QuantEmbeddingNbit: raise instead of synthesising a
  placeholder when g_idx is supplied alongside symmetric quantization.
- tie_quant_word_embeddings: require both input and output embeddings to
  already be QuantTensor-backed with matching shape/dtype before tying.
- Fix CodeQL mismatched-assignment false positives in QuantTensor dispatch
  (index args directly), fix ruff D205/D401/PLW0108/A002 warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Neither attribute is read anywhere — the post-walk loop that produced
the literal skip-name list was a leftover from before the refactor.
The configured patterns already live on quantization_config.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop tagging modules with quant_info / quant_info_3d and stop branching
on 2D vs 3D in the quantization passes. The quantizer already operates
along the last dim regardless of rank, so a single iteration over
parameters that carry a quant_info attribute is enough.

- QuantTarget slims to (module, module_name, pname, full_name) with a
  .param property; the caller reads shape/dtype/device from the
  parameter directly. No more 'kind' field.
- prepare_model writes target.param.quant_info in one pass — both 2D
  linear/embedding weights and fused experts parameters use the same
  code path. The quant_info_3d dict-stash on experts modules is gone.
- finalize iterates every parameter that has quant_info, calls
  QuantTensor.from_float (already rank-generic), and installs in place.
- GPTQ and AutoClip read module.weight.quant_info; module discovery
  uses hasattr(module.weight, 'quant_info') instead of a module-level
  attribute.
- HF placeholder install pulls shape/dtype/device off target.param and
  the placeholder builder is now rank-generic.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The dataclass had four fields and a one-line .param property, used by
two callers. A plain tuple is shorter, matches how the layerwise
quantization loop already iterates over (module, pname, param, info)
tuples, and removes the unused module_name field and dead
for_each_target helper.

QuantTarget remains as a type alias for the public signature.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Filter fused-MoE params to 2D/3D ranks in iter_quant_targets so a
  1D bias-like parameter fails at selection time instead of much later
  in finalize.
* refresh_quant_tensor_refs: also check isinstance(param.data,
  QuantTensor) for forward-compat with future torch versions that may
  not return the underlying subclass from nn.Parameter().
* OliveHfQuantizationConfig: replace bare '# pylint: disable' with the
  specific super-init-not-called rule; use output.get(k) in to_dict.
* finalize: log a warning when moe=True that the resulting checkpoint
  isn't directly ONNX-exportable via the Olive conversion pass — it
  must be consumed by an MoE-aware model builder.
* Add regression test that _module_weight_has_quant_info ignores
  nn.LayerNorm / nn.Conv2d / unmarked nn.Linear (defends GPTQ/AutoClip
  discovery against future drift).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- __torch_dispatch__ clone/contiguous now forwards extra args/kwargs.
- iter_quant_targets skips ALL nn.Embedding when embeds=False (positional
  / token-type embeddings like GPT-2 wpe are no longer silently quantized).
- WeightQuantizer assertion message: 2/4/8-bit (was 4/8-bit).
- tie_quant_word_embeddings: mark dst aliased buffers non-persistent so
  safetensors save emits one copy of qweight/scales/qzeros.
- finalize: group selected params by host module so each module's
  to(device)/to(cpu) cycle runs once for MoE experts modules carrying
  multiple 3D weight params.
- state_dict: add ensure_state_dict_hooks(model) defensive walk that
  installs the save hook on every host module that owns a QuantTensor
  parameter (idempotent).
- Add test_forward_parity.py: bit-exact eager parity for full models
  (embedding + linears) and fused 3D MoE forwards, plus end-to-end
  ONNX export -> onnxruntime numerical parity for Olive-quantized
  nn.Linear via make_export_compatible_quant.
- Enable pylint by adding file-level protected-access disables on the
  files that intentionally touch nn.Module._parameters.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

copilot added 5 commits July 21, 2026 23:30
…main

Resolve conflicts in quant_utils.py by combining the param-level
iter_quant_targets walk with main's QKV-aware override renormalization
and QuantTensor-based already-quantized detection. Adapt kquant.py and
its tests to the storage-only (param-level quant_info) API.
Copilot AI changed the title [WIP] Extend RTN weight quantization to MoE experts Extend PyTorch RTN weight quantization to MoE experts Jul 22, 2026
Copilot AI requested a review from titaiwangms July 22, 2026 00:08
@titaiwangms
titaiwangms requested a review from Copilot July 22, 2026 16:31

Copilot AI 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.

Pull request overview

This PR extends Olive’s native PyTorch RTN-style weight quantization to support Mixture-of-Experts (MoE) expert weights by moving from module-swapping (QuantLinear/QuantEmbedding) to a parameter-level, storage-only representation (QuantTensor) that can also handle fused 3D expert parameters. It also centralizes quantization target selection and pattern matching, updates ONNX export/model-builder integration boundaries, and adds extensive regression and parity tests.

Changes:

  • Introduces QuantTensor + state-dict helpers to quantize weights (including fused 3D MoE expert tensors) without swapping parent modules, and removes olive.common.quant.nn.
  • Adds centralized quantization target selection (iter_quant_targets) and pattern matching helpers (re: regex support with safety validation; insertion-order override precedence).
  • Updates PyTorch passes (RTN/GPTQ/KQuant/AutoClip), ONNX ModelBuilder behavior, documentation, and adds broad test coverage (selection, tensor behavior, regex safety, parity, and model-builder rejection for MoE).

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/passes/pytorch/test_rtn.py Updates RTN tests to assert quantization via QuantTensor-backed weights instead of QuantLinear/QuantEmbedding.
test/passes/pytorch/test_quant_utils.py Adjusts quant-utils tests for parameter-level quant_info and adds new state-dict helper coverage.
test/passes/pytorch/test_kquant.py Updates KQuant tests to use QuantTensor checks and bit assertions.
test/passes/pytorch/test_gptq.py Updates GPTQ tests to validate QuantTensor-backed weights and composition behavior.
test/passes/onnx/test_model_builder.py Adds coverage ensuring ModelBuilder rejects Olive-quantized MoE checkpoints.
test/common/quant/test_utils.py Extends quant utils tests to validate N-D quantization and pack/unpack helpers.
test/common/quant/test_tensor.py New tests for QuantTensor 2D/3D behavior, indexing, movement guards, and ONNX-export guards.
test/common/quant/test_selection.py New tests for iter_quant_targets selection rules including MoE gating, fail-closed behavior, and already-quantized skipping.
test/common/quant/test_patterns.py New tests for override/skip matching semantics and regex safety validation.
test/common/quant/test_nn.py Removes tests for deprecated QuantLinear/QuantEmbedding module wrappers.
test/common/quant/test_hf_utils.py Updates HF quantizer tests for QuantTensor-based layout, adds MoE and regex-override coverage.
test/common/quant/test_forward_parity.py New numerical parity tests (eager vs dense reference) plus ONNX export parity for the export-compatible wrappers.
olive/passes/pytorch/rtn.py Enables MoE support in RTN pass config via allow_moe=True.
olive/passes/pytorch/quant_utils.py Refactors quantization to parameter-level selection, adds MoE config/options, and installs QuantTensor params during finalize.
olive/passes/pytorch/kquant.py Adjusts KQuant to use weight-level quant_info and shared _module_weight_has_quant_info.
olive/passes/pytorch/gptq.py Migrates GPTQ calibration/processing to store metadata on module.weight.quant_info.
olive/passes/pytorch/autoclip.py Migrates AutoClip input caching and processing to use module.weight.quant_info.
olive/passes/onnx/model_builder.py Rejects Olive-quantized MoE checkpoints; adds suffix mapping so Olive’s *_qweight/*_scales/*_qzeros buffers load into expected ModelBuilder attributes.
olive/common/quant/utils.py Generalizes quantization and packing utilities to N-D tensors (quantize/pack along last dim).
olive/common/quant/tensor.py New QuantTensor tensor-subclass implementing storage-only quantization with dispatch, 3D MoE indexing behavior, and ONNX/movement guards.
olive/common/quant/state_dict.py New state-dict utilities for installing QuantTensor parameters + buffer aliases and refreshing references after load.
olive/common/quant/selection.py New shared quantization target selection logic with MoE-aware rules and fail-closed behavior.
olive/common/quant/patterns.py New pattern matching helpers for overrides/skip patterns with re: support and regex safety validation.
olive/common/quant/nn.py Removes deprecated QuantLinear/QuantEmbedding module implementations.
olive/common/quant/hf_utils.py Updates HF quantization config and quantizer implementation to use QuantTensor placeholders + state-dict refresh; adds regex overrides.
olive/common/hf/wrapper.py Adds MoE experts/router conventions and accessors on LayerWrapper.
olive/common/hf/quant.py Adds export-compatible wrappers creation from QuantTensor and improves symmetric handling (optional qzeros) for contrib ops.
docs/source/features/quantization.md Documents PyTorch Native RTN, MoE flag behavior, regex semantics/safety, precedence rules, and migration away from QuantLinear/QuantEmbedding.

Comment thread olive/common/quant/hf_utils.py Outdated
Comment thread olive/common/quant/hf_utils.py Outdated
Comment thread olive/common/quant/selection.py

Copilot AI 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.

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

olive/common/quant/hf_utils.py:80

  • The overrides docstring says "longest pattern wins", but get_qlinear_init_args uses match_override(...) which is insertion-order / first-match-wins. This is misleading and contradicts the behavior documented elsewhere in this PR (and tested in test_patterns.py).
        overrides: Per-module overrides for quantization parameters.
            Keys use **literal equality** matching by default; entries
            prefixed with ``re:`` use ``re.fullmatch``. Among matching
            keys, the longest pattern wins (ties broken lexically).

olive/common/quant/selection.py:103

  • The docstring states quantize_embeds=False only skips the input embedding module, but the implementation currently skips all nn.Embedding modules when quantize_embeds is false. The docstring should match the actual selection semantics (and the pass config description: "input embeddings").
    * ``quantize_lm_head=False`` skips the output embedding module.
    * ``quantize_embeds=False`` skips the input embedding module.
    * ``quantize_moe=False`` skips every ``nn.Module`` under any

olive/common/quant/selection.py:176

  • When quantize_embeds=True, the current logic will quantize every nn.Embedding in the model (positional/token-type/etc.), not just the input embeddings. This contradicts the config/CLI description ("quantize the input embeddings") and can unintentionally change model behavior. Consider restricting embedding quantization to model.get_input_embeddings() when available.
        if isinstance(module, (nn.Linear, nn.Embedding)):
            if isinstance(module, nn.Embedding) and not quantize_embeds:
                continue
            if _is_skipped(module, name):
                continue

@titaiwangms
titaiwangms marked this pull request as ready for review July 23, 2026 20:55
@titaiwangms

Copy link
Copy Markdown
Contributor

Review team synthesis (5 parallel reviewers: readability / code / critical / deep / integration)

This PR was reviewed against the acceptance criteria in #2583. Several claims were independently verified/reproduced against the actual diff before being reported here. Overall: not yet mergeable — two Critical findings have concrete repros, plus several Major gaps in the OOM guard, legacy-checkpoint compatibility, and required tracked-issue/test coverage.

🔴 Critical

1. Override precedence flips across to_dict() serialization (spec deviation + load-time correctness bug)
hf_utils.py::to_dict() calls sort_layers_by_name(overrides), which reorders overrides into name-sorted order. The finalized rule (issue #2583 §6, and patterns.py::match_override's own docstring) is insertion-order, first-match-wins — explicitly not longest-pattern or sorted. The reload path (_process_model_before_weight_loadingget_qlinear_init_argsmatch_override) does depend on this order to pick bits/group_size for the placeholder QuantTensor.

Reproduced independently:

insertion order: ['re:.*\.w1' -> bits 8, 'model.layers.0.mlp.experts.0.w1' -> bits 6]
pre-serialize winner bits  = 8   (regex first, per insertion-order rule) ✔ correct
serialized overrides order = ['model.layers.0.mlp.experts.0.w1', 're:.*\.w1']  (sorted!)
post-roundtrip winner bits = 6   ← precedence FLIPPED

This means quantizing and saving a checkpoint, then reloading it, can silently resolve overlapping overrides differently — producing a shape/metadata mismatch or silently wrong dequantization on reload. Existing AC #4 regression tests only exercise the in-memory path, never a to_dict → reconstruct round-trip.
Fix: stop sorting overrides in to_dict() (preserve insertion order), and add a serialize→reload precedence regression test.

2. Real ReDoS bypass in the "safe" regex validator
_assert_regex_safe in patterns.py rejects nested unbounded quantifiers like (a+)+, but a pattern where the inner group is bounded and only the outer quantifier is unbounded — e.g. (a{1,2})+$ — passes validation while still causing catastrophic backtracking. Reproduced directly:

pattern = re.compile(r'(a{1,2})+$')
20 chars: 1.3ms
25 chars: 14.6ms
28 chars: 61ms
30 chars: 160ms   (clearly exponential)

A user-supplied re: override/skip pattern of this shape can hang model loading. The validator needs to also reject "quantified group whose body itself contains any repetition (bounded or unbounded) or alternation," not only nested-unbounded bodies.

🟠 Major

3. Eager-mode OOM guard does not hold for unregistered ops
The PR description claims "other 3D indexing raises instead of fully dequantizing" as an OOM guard, but this only holds under torch.onnx.is_in_onnx_export(). In plain eager mode, an unregistered op like torch.index_select(qt, 0, ids) falls through __torch_dispatch__ to to_dense(), fully materializing the 3D expert tensor — and the returned object is a malformed QuantTensor missing bits/qweight, causing a subsequent AttributeError on any further use. This should either raise consistently (in and out of export), or the OOM-guard claim in the description should be scoped down to export-time only.

4. Legitimate MoE routing index patterns are rejected
_indexes_leading_dim_only in tensor.py is overly restrictive: it rejects tuple-form indices like w[expert_ids, :, :] and rank->1 tensor indices (common for top-k routing), forcing them onto the slow/error path even though they only index the leading (expert) dimension. This will break real top-k MoE routing code that uses these forms.

5. Old QuantLinear/QuantEmbedding-era checkpoints cannot be reloaded
Verified directly in state_dict.py/hf_utils.py: there is no migration for legacy bare qweight/scales/qzeros buffer names (from the deleted classes) to the new <pname>_qweight convention on Olive's own HF-checkpoint reload path. This contradicts the PR's stated compatibility claim ("state dicts still load"). Note: this is a different code path from model_builder.py's OliveQuantizedModel, which does correctly migrate key names for ModelBuilder consumption — that path is fine; the gap is specifically in Olive's own HF model reload.

6. Fail-closed MoE detection has a partial-discovery gap
The fail-closed check in selection.py only fires when expert_modules is entirely empty. A hybrid model where some layers resolve their experts subtree and others don't will bypass the guard: with moe=False, nn.Linears in the unresolved subtree still get quantized; with moe=True, unresolved fused weights silently stay at full precision either way.

7. GptqRtn(moe=True, embeds=True) composability is not tested end-to-end
The current regression test emulates GPTQ output by hand-installing QuantTensor placeholders and asserting iter_quant_targets skips them — it never runs the actual Gptq pass followed by the actual Rtn pass. Per #2583's "Composability with Gptq" section, this needs a true two-pass test verifying Linear layers keep GPTQ quantization while MoE experts/embeds get RTN-quantized without conflict (including the save/reload round-trip).

8. Required Mobius tracked issue not filed
#2583's acceptance criteria explicitly require a tracked issue (not a prose mention) documenting the preprocess_olive_weights 3D-mis-reshape risk. The PR description only states "a tracked issue must be filed" with no link — this AC item appears unmet.

9. Non-MoE key-migration path in model_builder.py has no regression test
The set_tensor suffix-migration logic (weight_qweightqweight, etc.) that keeps existing Linear/Embedding ModelBuilder checkpoints loading is untested — only the moe=True-rejection path has a test. Per #2583 this test bullet was explicitly required.

🟡 Minor

  • Stale/contradictory docstrings: hf_utils.py's OliveHfQuantizationConfig/get_qlinear_init_args docstrings still say "the longest pattern wins (ties broken lexically)," directly contradicting the shipped and documented first-match/insertion-order rule. Please sync with patterns.py's docstring and quantization.md.
  • 2-bit RTN quantization cannot currently be exported to ONNX (QuantLinearNbit only supports 4/8-bit) — should be documented or rejected earlier with a clear error.
  • The compiled-regex cache (@cache in patterns.py) is unbounded for the process lifetime; consider a bounded cache for long-lived services.
  • olive/passes/pytorch/kquant.py (+ its test) is modified but not listed in Resume jambayk/moe-quant: extend RTN weight quantization to MoE experts #2583's enumerated in-scope file list — the change looks like a necessary mechanical ripple, but should be acknowledged explicitly rather than silently included.
  • _assert_regex_safe validates lazily (only when a pattern is first matched against some module name); an invalid re: pattern that never matches anything is never validated. Consider validating all re: keys eagerly at config construction.

✅ What's solid

  • N-D quantization math generalization is clean and verified: test_quantizer_3d_matches_2d_per_slice proves the 3D path is bit-identical to independently quantizing each expert slice across bits∈{2,4,8} and group_size∈{-1,16,32}.
  • The gpt-oss 2D-bias mis-quantization bug is correctly fixed (dim() == 3 requirement) with a dedicated regression test.
  • Fail-closed ordering is correct where it does fire (raises before any parameter is mutated).
  • No remaining production references to the deleted QuantLinear/QuantEmbedding classes anywhere in the repo (verified via full-repo search).
  • The central ONNX-export rejection path (_maybe_dense routing everything unregistered through a guard) correctly closes the "silent partial export" gap for movement ops and non-fast-path indexing under export.

Recommendation: address the two Critical items (override-serialization precedence, ReDoS bypass) and the eager-mode OOM guard / legacy-checkpoint-load gaps before merge; file the Mobius issue and add the two missing regression tests (model_builder non-MoE migration, true two-pass Gptq→Rtn composition) to close out the remaining acceptance criteria.

… fixes

Addresses adversarial-review findings on the MoE weight-level quantization
work (QuantTensor storage-only design):

Round-1 (8 items, per PR #2584 review):
- Various correctness/robustness fixes to selection, patterns, and
  QuantTensor construction found in the first review pass.

Round-2 (4 items, per remediation_plan_v2.md):
- R2-1: patterns.py — reject nested-group alternation (`(a|b)` inside a
  repeated group) at any nesting depth, not just top-level. Regex safety
  check docstrings demoted from "prevents ReDoS" to "best-effort UX check,
  not a security boundary" per explicit decision not to add the `regex`
  third-party dependency (Option A only).
- R2-2: tensor.py/hf_utils.py/state_dict.py — thread an explicit
  `is_placeholder` flag through QuantTensor's lifecycle so init-style ops
  (`zero_`, `normal_`, etc.) only no-op on real placeholders, and raise on
  any other QuantTensor (previously any QuantTensor silently no-op'd,
  masking real bugs).
- R2-3: tensor.py — reject rank>1 boolean-mask indexing instead of
  misclassifying it as a safe leading-dim integer index.
- R2-4: selection.py/defaults.yaml — rewrite `_config_indicates_moe` to
  reuse the existing `resolve_alias()` nested-config mechanism plus a
  bounded sub-config sweep, fixing DBRX-style nested MoE config detection.

Known unresolved issues (found in round-3 review, NOT fixed in this
commit — see PR description for details and rationale):
- uint8-dtype tensor indices are still misclassified as safe integer
  indices (same bug class as R2-3, different dtype).
- `copy_()` does not propagate/clear `is_placeholder`.
- `refresh_quant_tensor_refs` clears `is_placeholder` unconditionally,
  not gated on an actual data load completing.
- Arbitrary-rank integer indexing (added for top-k MoE routing) can
  produce a QuantTensor that can't be dequantized or re-indexed.
- A third ReDoS bypass via `(?#...)` inline-comment regex syntax.

All changes verified: 314 tests passing in test/common/quant and
test/passes/pytorch/test_rtn, lintrunner clean.
titaiwangms and others added 2 commits July 31, 2026 00:41
transformers>=5.x defaults save_pretrained(save_original_format=True),
which for Mixtral-family MoE architectures round-trips the on-disk
state dict through a legacy per-expert nn.Linear-shaped layout
(splitting the fused-3D experts.gate_up_proj/down_proj into
experts.{i}.w1/w2/w3.weight and back). That reshape/(un)fuse machinery
assumes plain float weight tensors and silently drops the trailing
group-size dimension of our quantized _scales/_qzeros buffers, which
crashes real forward() calls on the reloaded model.

Request the new non-legacy on-disk format (save_original_format=False)
when supported, so the fused-3D quantized buffers round-trip byte-for-
byte as-is. Falls back to the default for older transformers versions
that don't accept this kwarg (they also predate the legacy-format
conversion machinery, so there's nothing to opt out of).

Add test_rtn_moe_real_forward_after_reload, a regression test that
quantizes a real MoE model, saves via the actual pass output, reloads
from disk, and calls the model's real forward() -- asserting no crash,
no NaN/Inf, and that the fused-3D scales buffer keeps its group-size
dimension. Verified this test fails at the exact corrupted-shape
assertion without the fix, and passes with it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 85549b60-0fb9-4d65-a4e8-7a8995939d68
Two related silent-data-integrity bugs in the placeholder lifecycle:

1. copy_() did not propagate/clear is_placeholder. Copying real data
   into a placeholder QuantTensor left it flagged as a placeholder, so
   a later in-place initializer could still silently no-op and discard
   the just-copied real data. Fix: mirror the source's is_placeholder
   state after copy_.

2. refresh_quant_tensor_refs() unconditionally cleared is_placeholder
   for every QuantTensor parameter in the model, even though it is
   invoked once for the whole model with no per-parameter signal about
   whether that parameter's checkpoint key was actually present. A
   parameter with a missing key keeps its original placeholder buffer
   objects untouched, so we can detect an actual load per-parameter by
   comparing buffer object identity: only clear is_placeholder if at
   least one buffer object was actually swapped by the loader.

Added regression tests for both fixes plus the missing-key case in
test_tensor.py.
Item 1: treat torch.uint8 tensor indices as legacy boolean masks in
_is_bool_index, matching PyTorch's own semantics for that dtype (same
bug class as the earlier boolean-mask fix, different dtype variant).

Item 3 (strengthened): refresh_quant_tensor_refs now accepts an optional
checkpoint_keys set and, when provided, uses exact full-dotted-key
membership as the authoritative is_placeholder determination instead of
the weaker buffer-identity heuristic (which in-place .copy_() loaders can
fool). OliveHfQuantizer now captures checkpoint_files (a kwarg HF's
preprocess_model already passes to _process_model_before_weight_loading)
via a new _read_checkpoint_keys() helper that reads .safetensors headers
through safe_open().keys(), and forwards it to
_process_model_after_weight_loading.

Item 4: QuantTensor.__getitem__ now rejects rank>=2 integer-tensor
indices (e.g. an un-flattened (tokens, k) top-k routing tensor) instead
of silently producing a >3D QuantTensor that can never be dequantized or
re-indexed. Callers needing a multi-dim batch of expert ids should
flatten to 1-D first and reshape the dense output afterward; this
restriction (Option A) is chosen over generalizing arbitrary-rank support
(Option B) because there is no validated caller or design for relaxing
_maybe_dense's rank-based OOM guard, and no real consumer uses rank>=2
indexing today (confirmed via repo-wide search; even GPTQModel's
reference MoE calibration uses per-expert scalar indexing, not batched
rank>=2 gather).

Item 5: documented as a known, deferred issue rather than fixed. Found a
concrete working ReDoS-scanner bypass via `(?#...)` inline-comment regex
syntax (the 3rd consecutive bypass of this blacklist-enumeration check).
Per discussion, this is not treated as a security vulnerability under
Olive's current trust model -- re: patterns are trusted, user-authored
config running in the user's own process, not adversarial input crossing
a trust boundary. Added a NOTE/TODO in patterns.py recording the bypass
mechanism, the decision not to patch it now, and a sketched
runtime-timeout-based alternative to revisit if this config path is ever
exposed to untrusted input.

Tests: 317 tests in test/common/quant/ pass (up from 306), plus 44 tests
across test/passes/pytorch/test_rtn.py, test_gptq.py, test_kquant.py.
lintrunner clean.
@titaiwangms
titaiwangms requested a review from jambayk August 1, 2026 00:42
- Fix Critical: tied lm_head/embed_tokens embeddings corrupted after
  checkpoint reload (refresh_quant_tensor_refs rewritten to dedupe
  shared QuantTensor objects, pick one canonical source site, and
  alias all hosting modules' buffers back to it).
- Fix Major: refresh_quant_tensor_refs silently left placeholder
  (zero) weights when a checkpoint was missing expected keys; now
  fails closed with a RuntimeError, requiring ALL mandatory buffers
  (qweight+scales, +qzeros if asymmetric) to show complete load
  evidence (AND logic, not OR) to avoid false negatives on partial
  buffer loads.
- Fix Major: ModelBuilder ignored regex `overrides` for per-layer
  bits/group_size, now resolved via match_override.
- Fix Major: QuantEmbeddingNbit had no ORT block_size validation
  (GatherBlockQuantized requires power-of-2, >=16); added
  _validate_onnx_block_size to both QuantEmbeddingNbit and
  QuantLinearNbit.
- Fix Major: QuantEmbeddingNbit.from_quant_tensor scales/qzeros
  shape mismatch; now reshaped like QuantLinearNbit.
- Add torch.equal override for QuantTensor: transformers 5.4's
  tie_weights() calls torch.equal on tied meta-device params before
  Olive's postprocess_model hook runs, which previously crashed.

Found via a full-PR review pass (readability, code, critical, deep,
integration reviewers + qa-tester) requested to confirm mergeability.
Fixed across two rounds: a comprehensive fix for all findings, then a
targeted fix for a partial-buffer false-negative in the fail-closed
check that the round-1 targeted re-review (code + critical reviewers)
caught.

448 tests passing (test/common/quant, test/common/hf/test_quant.py,
test/passes/pytorch/test_rtn.py, test/passes/pytorch/test_gptq.py,
test/passes/pytorch/test_kquant.py, test/passes/pytorch/test_quant_utils.py,
test/passes/pytorch/test_autoclip.py, test/passes/onnx/test_model_builder.py),
lintrunner clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 85549b60-0fb9-4d65-a4e8-7a8995939d68
@titaiwangms

Copy link
Copy Markdown
Contributor

Full-PR review pass (readability, code, critical, deep, integration reviewers + QA)

Per request, ran the full 6-agent review team over the entire PR diff (not just the recent commits), to confirm mergeability. This surfaced pre-existing bugs unrelated to #2598's original 5 items:

1 Critical

  • Tied lm_head/embed_tokens embeddings got corrupted after checkpoint save→reload. Root cause: refresh_quant_tensor_refs visited every module hosting the shared QuantTensor and did last-write-wins re-binding, picking up a stale placeholder object instead of the reloaded one. Verified end-to-end (diff was ~2.26e18 before fix).

4 Major

  • refresh_quant_tensor_refs silently left placeholder (zero) weights if a checkpoint was missing expected keys instead of failing. Fixed to fail closed with a RuntimeError, requiring all mandatory buffers (qweight+scales, +qzeros if asymmetric) to individually show complete load evidence — an initial round-1 fix used OR logic across buffers, which a targeted re-review (code + critical reviewers) caught as a false negative on partial buffer loads; fixed in round 2 with AND logic.
  • ModelBuilder ignored regex overrides for per-layer bits/group_size (only did a plain dict lookup). Fixed to use match_override.
  • QuantEmbeddingNbit had no ORT block_size validation; GatherBlockQuantized requires power-of-2 and >=16. Added validation to both QuantEmbeddingNbit and QuantLinearNbit.
  • QuantEmbeddingNbit.from_quant_tensor had a scales/qzeros shape mismatch; fixed to reshape like the Linear equivalent.

Also fixed: a torch.equal override for QuantTensortransformers 5.4's tie_weights() calls torch.equal on tied meta-device params before Olive's postprocess_model hook runs, which previously crashed.

All fixed across two rounds (comprehensive fix, then a targeted fix for the partial-buffer false negative found in re-review), each verified with fail-before/pass-after tests. Added 2 new test files (test/common/hf/test_quant.py, test/common/quant/test_state_dict.py) plus a real end-to-end tied-embedding save→reload regression test in test/passes/pytorch/test_rtn.py.

Current status: 448 tests passing across the affected test modules, lintrunner clean, working tree clean. Commits 3ba5b683 (item 1/4-strengthening/5-doc from #2598) and 6baed50e (this review's fixes) pushed.

Two Minor findings remain deferred as non-blocking follow-ups (not filed as issues yet): torch.equal override implements encoding-equality rather than value-equality for quantized tensors; named_modules(remove_duplicate=True) could theoretically hide a dual-registered module in an edge case not currently exercised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants