Repository navigation
[OMNIML-5899] Add IQ unified checkpoint export - #2444
hychiang-git wants to merge 32 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
|
/claude review |
There was a problem hiding this comment.
Claude review — IQ unified checkpoint export
Scope: full review (trigger comment carried no scoping instructions). 11 changed files (+343/-13); reviewed all of modelopt/torch/export/ (6 files) and all 4 test files, plus traced the new symbols into modelopt/torch/quantization/ggml/, layerwise_export.py, and quantization/utils/core_utils.py for callers. Only docs/source/deployment/3_unified_hf.rst was skimmed rather than reviewed in depth.
Findings: CRITICAL: 0 · IMPORTANT: 2 · SUGGESTION: 5
IMPORTANT
- TP>1 guard misses the second Megatron entry point (
unified_export_megatron.py:316-324). TheNotImplementedErrorsits insave_pretrained, butsave_pretrained_extra_modulesreadsself.extra_state_dict, which reaches_get_iq_weight_statethrough the same_name_remappingchain. An EAGLE/Medusa + IQ export at TP>1 therefore packs each rank's partial weight into GGML blocks and writes it with no error. Moving the check into_get_iq_weight_statecovers every entry point and cannot be bypassed by a future one. - Fused-expert amax fallback now fires for every IQ projection (
moe_utils.py:218region). IQ quantizers are amax-free by construction, so the 'was not calibrated (amax missing or zero) … Consider using more calibration data' fallback is unconditionally true on this newly-enabled path. Because the message embeds the expert index, Python's duplicate-warning filter does not collapse it — a 128-expert model emits roughly 384 bogus warnings per MoE layer, and export writes a meaningless_amaxonto each quantizer. Gate the block on the format needing an amax.
SUGGESTION (non-blocking, details inline)
"weight_shape"added toweight_suffixesinpostprocess_state_dictis dead — this design deliberately emits no shape companion, and a test asserts its absence.- Block geometry (
50/74/1.5625/2.3125/256) is hardcoded in three places whileIQ1_S_BLOCK_BYTES, theEFFECTIVE_BITSconstants, andGGML_BLOCK_SIZEalready exist inquantization/ggml; also, the GGML-specific keys sit inside the compressed-tensorsweightsargs rather than at the scheme level where this file'sNVFP4_SVDprecedent puts non-schema keys. - Megatron's hand-built fallback
quantization_configomitsgroup_size/packing/block_payload_bytesfor IQ, so the same format gets differenthf_quant_config.jsonmetadata depending on whethercombined_layer_config_dictwas populated. - The qformat-to-packer dispatch is duplicated three times, and export ignores
backend_extra_args['search_impl']— harmless today (only'auto'is accepted), but it means export is only guaranteed to match calibration by there being a single implementation. - Adding IQ to
FUSION_FREE_FORMATSalso opts it intolayerwise_export.SUPPORTED_FORMATS. That path traces as functional, but it is neither claimed in the PR body nor tested.
What checked out
- The scale-free representation is coherent end to end: packed shape
[*logical[:-1], logical[-1]//256, payload_bytes]makes the logical shape unambiguously recoverable, and the Megatron test'sassert_close(dequantize(packed), weight_quantizer(weight))verifies export bytes match what fake-quant produced — the property that actually matters here. - All seven weight-writing branches in
unified_export_megatron.py(_name_remapping, gated MLP, expert shards, qkv slicing, GatedDeltaNet, and both merge paths) are patched, each with the IQ branch correctly ahead ofweight_scale is None— without that ordering the weights would have silently exported as raw BF16. The GatedDeltaNetkeep_bf16interaction is handled. get_quantization_format's stringnum_bitsbranch is safe:representative_weight_quantizerunwrapsGroupedQuantizerto aTensorQuantizer, so.backendalways resolves, and the branch sits after theSequentialQuantizerhandling.- The
moe_utilsbuffer-vs-parameter fix is necessary and correct — assigning a plain tensor toproj.weightwould have become an ordinary attribute and dropped the packed payload fromstate_dict()entirely. - Mixed-precision config generation works for IQ via
_quant_algo_to_group_config, and_postprocess_single_tensorpasses theuint8weight through untouched.
Risk: moderate. No correctness bug in the packing or metadata for the paths the PR claims. Finding 1 is a real silent-corruption hole, but only on the EAGLE/Medusa + IQ + TP>1 combination; finding 2 is a usability regression rather than a wrong checkpoint. Backward compatibility looks clean — every change is behind a new-format branch, no existing signature, default, or modelopt_state key moves. Since GPU CI has not run these paths, the Megatron coverage is still unproven in practice.
756d86d to
28a6e1b
Compare
|
Addressed all seven review findings in
Local validation: 133 focused tests passed, and all pre-commit hooks passed. GPU-only coverage remains for CI. |
|
/claude review |
There was a problem hiding this comment.
Claude review — IQ unified checkpoint export (round 2, 28a6e1bb)
Scope: full review (trigger comment carried no scoping instructions). 12 changed files (+554/-16); reviewed all 6 modelopt/torch/export/ files and all 5 test files, plus re-traced into modelopt/torch/quantization/ggml/ (__init__.py, common.validate_weight, iq1_s, iq2_xs) and quantization/utils/core_utils.representative_weight_quantizer. docs/source/deployment/3_unified_hf.rst read but not deeply reviewed.
Findings: CRITICAL: 2 · IMPORTANT: 1 · SUGGESTION: 1
Prior round: all seven addressed, verified
- TP>1 guard now sits in
_get_iq_weight_state(unified_export_megatron.py:1163), so it coverssave_pretrained_extra_modules/ EAGLE-Medusa too, plus a direct unit test — resolved. - Fused-expert
amaxfallback is gated onis_iqat both the per-channel slice and the calibration fallback (moe_utils.py:172,:201), with a test asserting no warning and no_amaxcreation — resolved. "weight_shape"removed from_BASE_SKIP_KEYS; grep confirms nothing undermodelopt/torch/emits that key, so the removal is inert — resolved.- Format metadata centralized in
IQ_FORMAT_SPECS/iq_format_spec, sourced from theggmlconstants; packing dispatch centralized in_pack_iq_weight— resolved. _validate_iq_quantizer_configrejects non-autosearch_impl, mirroring theiq1_s.py:310/iq2_xs.py:311fake-quant check, with a test — resolved.- Megatron fallback
quantization_confignow mergesgroup_size/block_payload_bytes/packing(:406) — resolved. - Layerwise coverage added (
test_iq2_layerwise_export_matches_whole_model_export) — resolved.
I did not re-raise the config.json group-config metadata suggestion from last round: with quant_method: "modelopt" and quant_algo: "IQ1_S" both present in the emitted quantization_config, a consumer has a discriminator and won't mistake it for a plain compressed-tensors int1 scheme, so leaving packing/block_payload_bytes to hf_quant_config.json is a defensible call.
New this round
CRITICAL — Megatron packed-expert paths pack along the wrong axis (_pack_name_remapping:1920, _pack_name_remapping_gpt_oss:2033)
Both helpers stack per-expert weights to [E, out, in], transpose(-2, -1) into the HF [E, in, out] layout (and, for GPT-OSS, additionally interleave the gate/up halves along the last dim), and only then call _get_iq_weight_state. quantize_iq1_s/quantize_iq2_xs always reshape to (-1, 256), i.e. they block along the last dimension — which after the transpose is out, not the in reduction axis that iq*_fake_quant blocked along during calibration (block_sizes={-1: 256}).
So each 256-weight super-block covers a completely different element set than the one whose d and local scales were fitted. dequantize_iq*(exported) does not reproduce weight_quantizer(weight) — the same invariant the new dense test (test_megatron_name_remapping_exports_iq_payload) correctly asserts, and which no MoE test covers. It is also not GGML-interpretable (GGML blocks run along the GEMM reduction axis), and it can hard-fail validate_weight, whose divisibility-by-256 check applies to the post-transpose last dim (hidden_size for linear_fc2, 2 * ffn_hidden_size_per_partition for linear_fc1) rather than to in.
MoE Megatron export at TP=1 is inside the scope the PR body claims, and nothing guards it, so this currently produces a silently wrong checkpoint. Simplest correct move for this PR: raise NotImplementedError for IQ in both pack helpers, matching how TP>1 is handled, and define the packed-expert layout separately.
IMPORTANT — device-memory spike from keep_weight_device on the expert fan-in paths (:1113)
The flag is right for the single-module paths, but the two pack helpers call _get_quantized_state once per local expert and accumulate, so IQ now holds all num_local_experts bf16 weights on the accelerator and allocates two more E × out × in device copies (stack, then transpose().contiguous()) while the model's own expert weights are still live. Every other format does that fan-in on CPU. Multi-GB transient per layer on a 128-expert model, immediately before packing allocates the payload.
SUGGESTION — effective_bits in IQ_FORMAT_SPECS has no reader (details inline).
What checked out
- The scale-free representation is coherent for the paths the dense tests cover: packed shape
[*logical[:-1], logical[-1]//256, payload_bytes]makes the logical shape unambiguously recoverable given the divisibility precondition, and the Megatron test verifiesdequantize(packed) == weight_quantizer(weight)rather than just shapes/dtypes — the property that actually matters. _validate_iq_quantizer_configcomposes correctly with all three quantizer layouts:representative_weight_quantizerunwrapsGroupedQuantizer(TEGroupedLinear) and the pluralweight_quantizersModuleList(_QuantFusedExperts), and theisinstance(..., TensorQuantizer)check correctly rejects aSequentialQuantizerrather than silently readingnum_bitsoff the wrong object.- The
moe_utilsbuffer-vs-parameter fix is necessary and correct: after_export_quantized_weightswapswrapper.weightfor auint8buffer, a plainproj.weight = tensorassignment would have become an ordinary attribute and dropped the payload fromstate_dict()entirely. The new test pins exactly that. get_quantization_format's string-num_bitsbranch is safe — it sits after theSequentialQuantizerhandling, and thebackend != "ggml"rejection prevents an IQ name reaching a non-GGML packer.- HF-side
_export_quantized_weight:weight_name != "weight"is rejected up front, so the hardcodedregister_buffer("weight", ...)cannot diverge fromweight_name;delattrbeforeregister_bufferis the right order for aParameter→ buffer swap. - Config generation:
process_layer_quant_configproduces the full IQ metadata for the uniform case,_quant_algo_to_group_confighandles both the uniform and theMIXED_PRECISIONper-group case (quant_algo.lower()correctly matching the upper-case"IQ1_S"stored inquantized_layers), and the Megatron fallback path now agrees with it. - Backward compatibility is clean: every change is behind a new-format branch or a defaulted keyword (
keep_weight_device=False); no existing signature, default, ormodelopt_statekey moves.quant_format.py's new top-level import ofmodelopt.torch.quantization.ggmlis import-safe — the CUDA extension is loaded lazily insidequantize_iq*, not at package import. - All seven
unified_export_megatronweight-writing branches place the IQ check ahead ofweight_scale is None; without that ordering the weights would have exported as raw BF16. Thekeep_bf16interaction in the GatedDeltaNet path is handled.
Risk: moderate-to-high for MoE Megatron, low elsewhere. The dense HF and dense Megatron paths look correct and are now tested end-to-end on the property that matters. The packed-expert paths were wired in mechanically alongside them and are wrong in a way that produces a loadable-but-incorrect checkpoint with no error — that is the one thing to fix before merge. GPU and Megatron CI still have not run any of this.
28a6e1b to
0098670
Compare
|
Addressed all four round-two findings in
Current local regression: 87 tests passed, and all pre-commit hooks passed. Megatron GPU coverage remains for CI. |
|
/claude review |
There was a problem hiding this comment.
Claude review — IQ unified checkpoint export (round 3, 00986707)
Scope: full review (trigger comment carried no scoping instructions). 12 changed files (+584/-14); reviewed all 6 modelopt/torch/export/ files and all 5 test files, plus re-traced into modelopt/torch/quantization/ggml/ (__init__, common.validate_weight/validate_packed_weights, iq1_s.quantize_iq1_s, iq2_xs), export/plugins/mcore_custom.py, export/plugins/mcore_llama.py, and tests/gpu_megatron/conftest.py. docs/source/deployment/3_unified_hf.rst was read and its block-layout arithmetic checked, but not deeply reviewed.
Findings: CRITICAL: 0 · IMPORTANT: 1 · SUGGESTION: 1
Round-two findings: all four addressed, verified
- Packed-expert axis bug — both
_pack_name_remapping(:1882) and_pack_name_remapping_gpt_oss(:1956) now raiseNotImplementedErrorfor IQ. The raise sits after_get_quantized_statefor the first expert but beforeweight_list.append,torch.stack,transpose, the GPT-OSS interleave,_record_layer_quant_config, and anyself._state_dictwrite — so it fires on iteration 0 with no partial mutation, which the new test pins viaassert exporter._state_dict == {}. Resolved. - Device-memory spike on expert fan-in — the same early raise removes the accumulate-then-stack path entirely for IQ. I separately confirmed the remaining
keep_weight_device=Trueconsumer, the TEGroupedLinear expert loop (:1455–:1500), holds at most one expert weight on the accelerator per iteration:_get_iq_weight_statepacks and returns.detach().cpu()inside the shard loop, and thename_to_value["weight"]device copy is popped immediately. Resolved. - Dense TP=1 Megatron still supported — confirmed; only the two pack helpers and TP>1 are gated.
- Unused
effective_bits— removed fromIQ_FORMAT_SPECS; each remaining key (quant_algo,num_bits,group_size,block_payload_bytes,packing) has a reader inprocess_layer_quant_config,_quant_algo_to_group_config, or the Megatron fallback. Resolved.
I also re-verified the seven round-one fixes still hold (TP guard in _get_iq_weight_state, is_iq amax gating in moe_utils, weight_shape removal from _BASE_SKIP_KEYS, centralized IQ_FORMAT_SPECS/_pack_iq_weight, search_impl rejection, Megatron fallback metadata, layerwise coverage).
New this round
IMPORTANT — test_megatron_name_remapping_exports_iq_payload errors instead of asserting (test_unified_export_megatron.py:113)
The test runs in the main pytest process and never patches get_tensor_model_parallel_world_size, which _get_iq_weight_state now calls unconditionally. tests/gpu_megatron/conftest.py only initializes model-parallel state inside spawned dist_workers, so there is no TP group and MCore asserts tensor model parallel group is not initialized. The two sibling TP tests you added patch this exact symbol for the same reason.
This is the only test that checks dequantize(packed) == weight_quantizer(weight) — the invariant that actually proves the Megatron dense path encodes along the calibrated axis. It will error in CI rather than validate the round-trip, and the shape/dtype/layer_config_dict assertions after it never run. One-line fix in the inline comment.
SUGGESTION — _validate_iq_quantizer_config does not reject an enabled input_quantizer/pre_quant_scale, and both IQ write paths return before activation metadata is emitted, so an IQ-weights + quantized-activations config would export a silently weight-only checkpoint (details inline).
What checked out
- Block axis is preserved on every enabled path. I enumerated all seven weight-writing branches and confirmed each hands
_get_iq_weight_statea tensor whose last dim is still the reduction axis:_name_remappingwrites[out, in]unchanged; the gated-MLP split (:1289), the TEGroupedLinear gated shard split (:1483), and the GatedDeltaNet projections (:1778) all slice dim 0;_qkv_slicingreshapes to[qkv_dim, head_size, hidden_size]and_takerestores[-1, hidden_size]. A grep fortranspose/permute/viewin the exporter turns up layout changes only in the two now-guarded pack helpers and_merge_nvfp4_expert_scales. Soquantize_iq*'sreshape(-1, 256)blocks along the same axisblock_sizes={-1: 256}calibrated on, andvalidate_weight's divisibility check applies to the real reduction dim. - The HF fused-MoE path is also axis-correct, which matters because it is enabled rather than guarded:
first_projis[E, 2*expert_dim, hidden], sofirst_proj[idx, :expert_dim, :]anddown[idx]both keepinlast — consistent with what the 3-D fake-quant blocked along. _pack_name_remapping'stranspose=Falsemode has no caller (mcore_llama.py:72/:76are the only twoPackNameRemappingsites, neither passes it), so the unconditional IQ raise there rejects nothing that would have been correct.search_implvalidation matches calibration exactly —extra_args.get("search_impl", extra_args.get("iq_search_impl", "auto"))is identical to the fake-quant checks atiq1_s.py:309andiq2_xs.py:310, including the legacy alias, so export cannot accept an impl the quantizer rejected.moe_utilsbuffer-vs-parameter fix is necessary and correct: after_export_quantized_weightswapswrapper.weightfor auint8buffer,proj.weight = tensorwould have become a plain attribute and dropped the payload fromstate_dict(). The new test pins exactly that.- Config generation is self-consistent across all three producers —
process_layer_quant_config(uniform +MIXED_PRECISIONper-group), the Megatron fallback at:406, and_quant_algo_to_group_config.quant_algo.lower() in IQ_FORMATScorrectly matches the upper-case"IQ1_S"stored inquantized_layers,iq_format_speclowercases its argument, and the emittedquant_method: "modelopt"plusquant_algo: "IQ1_S"give a consumer a discriminator so the weights-only int group is not mistaken for plain compressed-tensors int1. postprocess_state_dictpasses the packed weight through untouched —"weight"matches no_BASE_SKIP_KEYSentry, so it takes the earlycontinue, and_maybe_squeeze_scaleis scale-keyed.- Docs arithmetic is right: 2 + 64 + 8 = 74 bytes, 32 groups / 16 shared local scales = 2 groups per scale, 2^9 = 512 codebook rows, 74*8/256 = 2.3125 bits, and the recovery rule
[*shape[:-2], shape[-2]*256]inverts the packed shape given the divisibility precondition. - Import safety and backward compat:
quant_format.py's new top-levelmodelopt.torch.quantization.ggmlimport is safe (the CUDA extension loads lazily insidequantize_iq*), andconvert_hf_config.py's new dependency on it only reaches consumers already inside the torch export stack. Every behavioral change is behind a new-format branch or a defaulted keyword (keep_weight_device=False); no existing signature, default, ormodelopt_statekey moves.
Risk: low. The correctness picture is materially better than round two — the axis hazard is now gated on both packed-expert helpers with no partial-write window, and I could not find another enabled path where the exported blocks diverge from what fake-quant produced. The one remaining item is test-side but consequential: the single test proving the Megatron round-trip cannot run as written, so after fixing it please confirm Megatron CI actually goes green on this file rather than green-by-collection.
🤖 Generated with Claude Code
0098670 to
ea560c0
Compare
|
Addressed both round-three findings in
Validation: 109/109 targeted export and IQ tests passed, and all changed-file pre-commit hooks passed. Megatron runtime coverage remains for GPU CI. |
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
3c474fe to
d9be3bb
Compare
|
/claude review |
There was a problem hiding this comment.
Claude review — IQ unified checkpoint export (head d9be3bb0, "Collect all IQ export preflight failures")
Findings: CRITICAL: 0 · IMPORTANT: 1 · SUGGESTION: 1
Scope
Bare /claude review, so a full pass. 22 changed files (+1724/-39). All 10 changed modelopt/torch/export/ files reviewed hunk-by-hunk with surrounding context; CHANGELOG.rst, docs/source/deployment/3_unified_hf.rst, and the modelopt_recipes/ diffs read in full; test diffs read from the diff (GPU / Megatron suites not executed). Traced into modelopt/torch/quantization/ggml/ (common.validate_weight / validate_packed_weights, iq1_s._quantize_iq1_s_packed / dequantize_iq1_s), quantization/utils/core_utils.py (representative_weight_quantizer, weight_attr_names, quantizer_attr_names, is_quantized_linear), nn/modules/tensor_quantizer.GroupedQuantizer, and export/plugins/mcore_custom.py + mcore_llama.py + mcore_gptoss.py for the packed-expert rule names.
Findings
| Severity | Location | Finding |
|---|---|---|
| IMPORTANT | quant_utils.py:195 |
The HF-side preflight collects only shape errors. _iq_export_quantizer_config_errors — the model-wide config collector this PR adds — is wired only into the Megatron guard, so search_impl != "auto", unknown backend_extra_args, and enabled activation quantizers / a live pre_quant_scale still raise mid-walk from inside _pack_iq_weight. Whole-model HF export leaves layers 0..N-1 already replaced by uint8 payloads in place; layerwise export leaves already-finished layer shards written under layerwise.export_dir. One-line fix using the collector that already exists. |
| SUGGESTION | quant_utils.py:133 |
The GroupedQuantizer branch yields a weight<index> name, so quantizer_attr_names derives weight0_input_quantizer — an attribute a TEGroupedLinear does not have. The weight-only activation guard is a silent no-op for grouped experts in preflight, while pack time (weight_name="weight") does catch it. |
Prior-round finding: addressed, verified
The round-6 suggestion — the packed-expert NotImplementedError in _pack_name_remapping / _pack_name_remapping_gpt_oss being the one non-collective IQ guard — is now closed. _custom_mapping_to_lambda stamps _modelopt_export_func_name onto the wrapped rule (lambda to named def, closure semantics unchanged), _model_has_packed_expert_iq_quantizer reduces self.rules to linear_fc1 / linear_fc2 for the PackNameRemapping / PackNameRemappingGPT entries in mcore_llama.py:72-79 and mcore_gptoss.py:41-48, and matches only modules with local_experts in their path — so a dense mlp.linear_fc1 cannot false-positive and a MoE arch whose rules use plain NameRemapping yields an empty set. self.rules = self.all_rules[self.arch] (:207, re-bound at :221 / :232 for Medusa) keeps the scan arch-scoped. The flag is folded into the existing MAX all-reduce, so every rank raises together before layer_state_dicts materialization.
What checked out this round
- Packing contract.
_pack_iq_weight'sexpected_shapestill matches_quantize_iq1_s_packed's ownpacked_shapeexactly, andIQ1_S_BLOCK_SIZE/IQ2_XS_BLOCK_SIZEboth resolve toGGML_BLOCK_SIZE = 256, so the documented[*shape[:-2], shape[-2] * 256]recovery rule agrees withvalidate_packed_weights. TheRuntimeErrorfor a shape mismatch sits outside theexcept (TypeError, ValueError)attribution handler, so it is not relabeled;validate_weight'sTypeError(non-float weight) andValueError(empty / non-divisible / non-finite) both keep their type throughraise type(exc)(...). - Preflight axis.
weight.shape[-1] % group_sizeremains the packed axis for every path that packs:_gated_mlp_slicing(weight[:ffn_hidden_size]),_qkv_slicing(_take(...).reshape(-1, hidden_size)),_gated_delta_net_slicing(torch.split(..., dim=0)),_grouped_mlp_slicing(weight[:half]), HF fused experts (first_proj[idx, :expert_dim, :],down[idx]). The two transposing paths are the ones rejected. - No unguarded weight write. All seven
_get_quantized_statecall sites accounted for: five have anif qformat in IQ_FORMATSbranch, two raise. Every state-dict weight write at:1342,:1398-1399,:1610,:1893is inside anelsereachable only for non-IQ, and_gated_delta_net_slicing'skeep_bf16projections.cpu()the split view explicitly now that the source stays on device. keep_weight_devicealiasing._get_weight_bias(keep_weight_device=True)can returnmodule.weightitself whendtypealready matches, but every IQ consumer either packs into a freshuint8tensor (.detach().cpu()) or.cpu()-copies thekeep_bf16slice, and_name_remapping'sweight = weight + 1.0is out-of-place — so no state-dict entry aliases a live parameter.- EP gather. IQ payloads reach
all_gather_objectthrough the existingtorch.save-bytes round trip (:1647-1666) that was added precisely because picklinguint8quantized tensors trips onUntypedStorage, so the new format needs no extra handling there. - Config emission.
_get_quantization_format(self.model)isget_quantization_format(model)with no dependence onlayer_config_dict, so hoisting it aboveself.layer_state_dictsinsave_pretrainedis behavior-preserving.convert_hf_quant_config_format's newIQ_FORMATSbranch reuses_quant_algo_to_group_config, andprocess_layer_quant_configemits the same four keys for theMIXED_PRECISIONpath. - Backward compatibility.
module_name,describe_as,keep_weight_deviceare all optional with prior behavior as the default;VllmFqGPTModelExporteracceptsdescribe_asfor signature compatibility and opts out via_packs_iq_weights = False. No mode registration,modelopt_stateschema, or public__init__.pychange. - Docs. The 50-byte IQ1_S (2 + 32 + 16) and 74-byte IQ2_XS (2 + 64 + 8) field breakdowns match the llama.cpp block definitions, and 1.5625 / 2.3125 bits per weight are the correct
bytes * 8 / 256. Themodelopt_recipes/ptq.mdrewrite now states the divisibility requirement for packed export rather than the old "export added separately".
Risk
Low. Every IQ path is gated on num_bits in IQ_FORMATS, and no shared branch changes behavior for existing formats. Unsupported configurations fail loudly with a qualified weight label. The one blocking item is not a wrong-checkpoint risk — it is that the "reject before we mutate anything" invariant the rest of this PR establishes is only enforced for shapes on the HF side, not for quantizer configs, and the layerwise variant of that gap can leave partial shards on disk. Residual coverage risk is unchanged: the Megatron and layerwise IQ suites still need GPU CI as the real gate.
🤖 Generated with Claude Code
Signed-off-by: Hung-Yueh Chiang <hungyuehc@nvidia.com>
|
/claude review |
| return f"{qualified_name}: nonstandard weight attributes are not supported" | ||
|
|
||
| weight = getattr(module, weight_name, None) | ||
| if not isinstance(weight, torch.Tensor): |
There was a problem hiding this comment.
[SUGGESTION] The shape preflight silently passes when the weight attribute it synthesized does not resolve to a tensor, which is the one case where the guard's "fail before any mutation" contract quietly stops holding.
For the GroupedQuantizer branch the weight name is derived, not observed: _iq_export_weights builds f"weight{index}" over range(max(num_gemms, len(grouped_quantizer))) (:137-138). If num_gemms overshoots the materialized parameters, or a future grouped-linear variant names its per-GEMM parameters anything other than weight0..weightN-1, getattr(module, weight_name, None) returns None, this early return None fires, and the divisibility check for that weight never runs. The config check still runs (it never touches the weight), so nothing looks wrong — the model passes preflight and then _get_iq_weight_state → _pack_iq_weight raises mid-walk, after earlier modules have already been packed in place. That is exactly the failure mode _validate_iq_export_support exists to prevent, so the silent skip is worth making visible.
The weight_attr_names branch is unaffected (those names come from named_parameters, so the attribute always exists) — only the synthesized grouped names can miss.
Suggested fix: keep the return None for the observed-name path, but have the grouped branch of _iq_export_weights skip indices whose parameter does not exist rather than yielding them, and log at debug level when a grouped module yields fewer weights than max(num_gemms, len(grouped_quantizer)). A cheap alternative that costs nothing at runtime is to distinguish "no such attribute" from "not a tensor" here and return an error string for the former, since a grouped module that reports num_gemms = N but has no weightK is a layout ModelOpt cannot pack anyway.
| def _model_has_packed_expert_iq_quantizer(self) -> bool: | ||
| """Return whether this rank would route an IQ expert through a packed-expert rule.""" | ||
| packed_projection_names = { | ||
| rule_name.rsplit(".", 1)[-1] | ||
| for rule_name, rule in getattr(self, "rules", {}).items() | ||
| if getattr(rule, "_modelopt_export_func_name", None) | ||
| in {"pack_name_remapping", "pack_name_remapping_gpt_oss"} | ||
| } | ||
| if not packed_projection_names: | ||
| return False | ||
|
|
||
| for module_name, module in self.model.named_modules(): | ||
| path = module_name.split(".") | ||
| if ( | ||
| "local_experts" in path | ||
| and path[-1] in packed_projection_names | ||
| and self._model_has_iq_quantizer(module) | ||
| ): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
[SUGGESTION] The packed-expert guard re-derives the dispatch decision through two indirect signals — a private marker attribute stuffed onto the rule closure, plus a module-name heuristic — when the export walk already has an authoritative flag for it.
The actual dispatch at :817 is if not self.rules.get("use_packed_local_experts", False): — that single boolean, not the rule's func_name, is what decides whether local_experts.linear_fc1 is called with the whole local_experts list (packed) or with one expert.linear_fc1 at a time. The guard here instead reconstructs the answer from _modelopt_export_func_name on apply_mapping and then matches "local_experts" in path and path[-1] in packed_projection_names. Today the two agree — every arch with PackNameRemapping{,GPT} on local_experts.* also sets use_packed_local_experts: True (mcore_llama.py:71,188, mcore_gptoss.py:40,66) — but they are independent tables, so a future arch that sets the flag while pointing the rule at a differently-named func, or one that packs experts stored under a container not literally named local_experts, would leave the guard silently returning False. The consequence is not a wrong checkpoint (_pack_name_remapping still raises at :1996/:2070), but the raise would then be non-collective and mid-walk, which is precisely the hang this collective preflight was added to avoid.
Reading the flag directly removes the marker plumbing added to _custom_mapping_to_lambda (:1122-1128) and keeps the guard tied to the same table the dispatch reads:
def _model_has_packed_expert_iq_quantizer(self) -> bool:
"""Return whether this rank would route an IQ expert through a packed-expert rule."""
if not getattr(self, "rules", {}).get("use_packed_local_experts", False):
return False
return any(
"local_experts" in module_name.split(".") and self._model_has_iq_quantizer(module)
for module_name, module in self.model.named_modules()
)If you prefer to keep the marker (it does document which projections pack, which the flag does not), consider asserting the two agree — i.e. that packed_projection_names is non-empty exactly when use_packed_local_experts is set — so a future divergence surfaces as a loud error rather than as a guard that stops guarding.
There was a problem hiding this comment.
Claude review — IQ unified checkpoint export (ea612a87)
Findings: CRITICAL: 0 · IMPORTANT: 0 · SUGGESTION: 2 → approving; neither suggestion blocks.
Scope. Full pass (no scoping instructions in the trigger). 22 changed files (+1797/-39): all 11 changed modelopt/torch/export/ files read hunk-by-hunk with surrounding context, plus CHANGELOG, docs/source/deployment/3_unified_hf.rst, and the modelopt_recipes/ changes. Traced outward into quantization/utils/core_utils.py (weight_attr_names, representative_weight_quantizer, quantizer_attr_names), tensor_quantizer.GroupedQuantizer, ggml/common.cached_reconstruction, the plugins/mcore_custom.py / mcore_llama.py / mcore_gptoss.py mapping tables, and the shipped configs/numerics/iq1_s.yaml plus configs/ptq/presets/model/iq1_s.yaml. GPU-only suites were not executed.
Note: the local base tip has moved ahead of GitHub's merge base, so git diff also surfaces an iq2_xs.py:_encode_blocks line this PR does not own. Excluded — a three-way merge keeps the base version.
The two suggestions (details inline):
_iq_export_weight_shape_errorsilently skips a weight it cannot resolve. Reachable only via theGroupedQuantizerbranch, where the name is synthesized asweight<index>overmax(num_gemms, len(grouped_quantizer))instead of observed fromnamed_parameters. Anum_gemmsovershoot or a differently-named grouped parameter passes preflight, then fails inside_pack_iq_weightmid-walk — the exact mutate-then-raise ordering this guard exists to prevent._model_has_packed_expert_iq_quantizerre-derives a decision the walk already owns, via a private_modelopt_export_func_namemarker on the rule closure plus alocal_experts-in-path heuristic, while the dispatch at:817keys offuse_packed_local_expertsinself.rules. They agree today; if they diverge the guard degrades to the non-collective raise at:1996/:2070— back to the mid-walk NCCL-hang shape it was added to fix.
What I verified:
- Packing contract / block axis.
expected_shapeis sourced from the IQ1_S and IQ2_XS block-size/byte constants viaIQ_FORMAT_SPECS, so the layout cannot drift from the encoder. I independently enumerated all 7_get_quantized_statecallers in the Megatron file (:1323,:1370,:1560,:1680,:1842,:1993,:2067) — every one has an IQ branch ahead ofelif weight_scale is None, so no site can write raw BF16 under an IQquant_algo. Every split feeding a packer is along a leading dim, soweight.shape[-1](what preflight checks) is the axis the packer blocks on. The two layout-changing helpers are the rejected ones. - HF fused experts. Confirmed the wrapper normalizes to
[E, out, in](fused_dim0 = first_proj.shape[1]), sofirst_proj[idx, :expert_dim, :]anddown[idx]keep the reduction dim last — matching the 3-D fake quant and matching what preflight checks on the unsplit parameter. Bothis_iqgates suppress the shared-weight_scale_2and per-projection amax fallbacks, so no bogus_amaxand no per-expert warning storm. - Preflight across all four quantizer layouts that
weight_attr_names/representative_weight_quantizersupport: standardweight; singular custom attr (BMM-style → correctly rejected as nonstandard, consistent with_export_quantized_weight's own raise); plural_QuantFusedExpertslists (allowed — rebuilt as standard per-expert wrappers);GroupedQuantizer(allowed; it is annn.ModuleList, not aTensorQuantizer, hence its own branch). The legacy_first_proj_attrsentinel fallback resolves to a quantizer whose sibling input quantizer does exist. - Weight-only enforcement cannot false-positive. The preset's
base_disable_allwildcard covers*output_quantizertoo, and KV quantizers are*_bmm_quantizeron attention, so IQ plus FP8 KV cache still exports. Thebackend_extra_argsunknown-key check matches the recipe exactly (onlysearch_implis set), and the non-autorejection mirrors the calibration-side check. - Config emission agrees across all three producers (
process_layer_quant_config, the Megatron fallback at:493,_quant_algo_to_group_config). The IQ check inget_quantization_formatsits ahead of the 4-bit and 8-bit branches, so an 8-bitinput_quantizercannot mislabel an IQ linear asint8_sq; the earlierSequentialQuantizerbranch cannot swallow it either. The weight-onlyconfig_groupsentry matches theW4A16_AWQ/W8A16precedent. - State-dict hygiene.
_reconstruction_cacheis a plain dict attribute, not a buffer, so a quantizer left attached after the IQ early return cannot leak a cached tensor into the checkpoint._maybe_squeeze_scaleis scale-key-gated, so a 3-D payload with leading dim 1 is not squeezed (which would break the documented shape recovery). No blanket dtype cast over the state dict;requires_grad=Falsemeans a later.to(bf16)skips the uint8 payload. - Distributed / mode-state.
_collective_iq_export_flagsMAX-reduces on a backend-appropriate device and handles the peer-owns-the-bad-shape case; the TP check inside_get_iq_weight_statebackstops directstate_dictcallers;_packs_iq_weights = Falsekeeps the vLLM fake-quant exporter out. No mode registration, nomodelopt_stateschema change, no public API change.quant_format.py's new import is the in-treeggmlpackage (no optional extra, no cycle, CUDA extension still lazy), so noimport_plugin()gate is needed. FUSION_FREE_FORMATS: checked all three consumers — suppresses the multi-module fusion path, suppresses the per-layer forward (which is what keeps a uint8 weight out of a GEMM), and opts IQ intolayerwise_export.SUPPORTED_FORMATS.- Backward compat: every new parameter is optional and defaults to prior behavior (
module_name,describe_as,keep_weight_device=False). Hoisting_get_quantization_formatabovelayer_state_dictsis order-safe (pure inspection, no collective). Theapply_mappingrewrite is behaviorally identical to the previous lambda. - Prior-round items hold: the module-local
deepcopyimport fixes the test-patch scope; the CHANGELOG layerwise bullet now lists IQ1_S and IQ2_XS; theoutput_quantizerguard, theIQ_FORMATSunification across write branches, and removal of the deadnn.Parameterisinstance fork are all present. Docs arithmetic re-derived: 2+32+16 = 50 and 2+64+8 = 74 bytes; 11-bit IQ1_S index gives 2048 rows, 9-bit IQ2_XS gives 512; 1.5625 and 2.3125 bits per weight; the documented recovery rule inverts the packed shape exactly.
Risk: low. Additive throughout — every IQ path is gated on the format set, and no shared branch changes behavior for an existing format. Unsupported configurations (TP greater than 1, packed experts, nonstandard weight attrs, last dim not divisible by 256, a non-auto search impl, any activation quantization or live pre_quant_scale) fail loudly with a qualified weight label, and the shape preflight is wired into all four packing entry points so a rejection leaves the model unmutated.
Residual risk is coverage, not correctness: per the PR body the Megatron and layerwise IQ paths were not run locally, so GPU CI is the real gate — please confirm tests/gpu_megatron/torch/export/test_unified_export_megatron.py and tests/gpu/torch/export/test_layerwise_export.py go green on hardware rather than green-by-collection.
🤖 Generated with Claude Code
What does this PR do?
Type of change: new feature
This is the export PR in a three-PR series. It:
<module>.weightas a shapeduint8payload in a unified Hugging Face checkpoint, without separatepacked_weightsorweight_shapekeys;Megatron export rejects tensor parallelism greater than one with a collective early error until cross-rank packing is defined. The guard scans every enabled tensor quantizer, including mixed-format models. Packed Megatron expert layouts also raise an early error because their HF transpose moves the 256-value block axis; dense tensor-parallel-size-1 Megatron export remains supported.
Review follow-up:
amaxfallback warnings and mutations for fused IQ experts;weight_shapemetadata from older checkpoints;Further review follow-up:
TypeErrorandValueErrorpacking failures while preserving their exception type;Usage
The exported weight shape is
[*logical_shape[:-1], logical_shape[-1] // 256, payload_bytes], wherepayload_bytesis 50 for IQ1_S and 74 for IQ2_XS.Testing
Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/A — no copied code or new PIP dependency.Additional Information
mainafter [OMNIML-5899] Add IQ1_S and IQ2_XS GGML quantization #2443 merges.