feat(megatron-bridge): adopt TEGroupedMLP per-expert quantizers (#1550) for W4A16 NVFP4 four_over_six - #2072
feat(megatron-bridge): adopt TEGroupedMLP per-expert quantizers (#1550) for W4A16 NVFP4 four_over_six#2072yueshen2016 wants to merge 18 commits into
Conversation
Give each fused expert in a TEGroupedLinear its own weight quantizer via a
GroupedQuantizer (an nn.ModuleList surfaced as weight_quantizer.{i}), so
per-expert amax is independent instead of expert-0's shared across all.
Includes the amax-preserving restore fix (Issue 1): modelopt_post_restore
keeps the loaded MSE/static-calibrated (and QAD-frozen) amax and only
re-max_calibrates a quantizer whose loaded amax is shape-incompatible with
its weight (a genuine TP/EP change), detected via a fake-quant dry-run.
Adds an opt-in torch.compile path for the per-expert quantize loop
(MODELOPT_TEGROUPED_COMPILE_WEIGHT_LOOP=1; default stays eager).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
_QuantMegatronTEGroupedLinear inherited the base sharded_state_dict, which
emitted per-expert amax under local keys weight_quantizer.{0..num_local-1}
with no expert offset, so every EP rank wrote identical keys and torch_dist
dedup kept only one rank's experts (EP16: 128 -> 8). Override it to emit each
per-expert amax with global_expert_idx = ep_rank*num_gemms + gemm_idx
(mirroring MCore _sharded_state_dict_grouped), so all num_global_experts
persist and reshard at any EP. Fixes both the collapsed save and the EP>1
first-load mis-map that corrupted static-NVFP4 QAD.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Export each fused expert with its own qformat/scales by swapping in that
expert's weight{i} and TensorQuantizer, instead of applying weight0's scales
to every expert. Combined with the EP-gather path (local_expert_indices +
all_gather_object across the EP group, collective-safe missing-key check) so
EP>1 exports gather all global experts; EP=1 reduces to the plain per-expert loop.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Add GPU tests asserting per-expert amax independence, sharded_state_dict global expert identity, and the opt-in compile path; plus the changelog entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Introduce module-level _ANY_QUANTIZER = (TensorQuantizer, SequentialQuantizer, GroupedQuantizer) and use it at the two "is this child any quantizer" sites, instead of repeating the three-way isinstance union. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Introduce AnyQuantizer = (TensorQuantizer, SequentialQuantizer, GroupedQuantizer) in quantization.nn and route the leaf/container isinstance checks through it, replacing the local _ANY_QUANTIZER tuple and the (TensorQuantizer, SequentialQuantizer)-only checks that skipped GroupedQuantizer: - model_calib: MoE amax completeness + DP/EP amax sync - vllm_fakequant_hf: weight-quantizer-disabled export guard Also: - representative_weight_quantizer: handle a singular GroupedQuantizer (TEGroupedLinear fused experts) by returning its first expert, so the exported hf_quant_config qformat/scales are correct instead of missing. - Drop GroupedQuantizer.get_modelopt_state; the container needs no serialized meta state. - Fix mypy in the per-expert export path (narrowing asserts) and drop the now-unused NVFP4StaticQuantizer import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
…king change Add a 0.47 New Features entry for per-expert weight quantization of TEGroupedMLP (GroupedQuantizer, one amax per expert) plus the torch.compile opt-in, and a Backward Breaking Changes note that pre-0.47 quantized TEGroupedMLP checkpoints are incompatible with 0.47's per-expert amax layout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
…review fixes Add QuantizeConfig.te_per_expert_quantizers (default False = legacy single shared weight quantizer per TEGroupedLinear); True installs a GroupedQuantizer with one quantizer per fused expert. Plumbed via convert_to_quantized_model to the env var the TE plugin reads; _setup and the per-expert methods fall back to the single shared quantizer when off. Adds a parametrized toggle unit test (validated on GPU). Also addresses PR review comments: - transformer_engine post_restore: only a genuine amax/weight shape mismatch falls through to the max|W| recompute; CUDA/OOM/other errors re-raise, and any recompute now warns instead of silently discarding MSE/static/QAD amax. - vllm_fakequant_hf disable loop: handle GroupedQuantizer so the widened _check_all_weight_quantizers_disabled(AnyQuantizer) check passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Remove the te_per_expert_quantizers opt-in flag and always give each fused expert of a TEGroupedLinear its own weight quantizer (a GroupedQuantizer with one TensorQuantizer per expert, independent amax). TP>1 works for dynamic quant (no stored amax to shard), so there is no reason to keep the legacy single-shared-quantizer path. - config.py: drop the QuantizeConfig.te_per_expert_quantizers field - conversion.py: drop the config->env bridge (and now-unused import os) - transformer_engine.py: drop _PER_EXPERT_QUANTIZER_ENV and the _te_per_expert_quantizers_enabled() gate; _setup unconditionally installs the per-expert GroupedQuantizer - test: replace the parametrized toggle test with test_te_grouped_per_expert_quantizer_default (GPU-validated: 1 passed) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Add an explicit @pytest.mark.timeout(90) to test_te_grouped_real_compile_weight_quantizer_loop so a runaway inductor recompile is bounded tighter than the 120s gpu_megatron group default, while keeping headroom over a cold first-compile so CI doesn't flake. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
…xpert export Static-NVFP4 output_layer (lm_head) amax was dropped on save and never re-applied on restore, so TE-spec PTQ->HF export crashed on the lm_head with "Weight quantizer does not have attribute amax". Two-sided fix (ported from the LOCAL cherry-new-loss-qad branch), plus export-path robustness. - quantization/plugins/megatron.py: register modelopt get/set_extra_state for EVERY QuantModule incl. output_layer (old is_enabled gate ran pre-replacement so it always skipped output_layer). EP-downsize fallback rewrites only the expert index after weight_quantizer (preserves SequentialQuantizer suffixes). - opt/plugins/mcore_dist_checkpointing.py: after load_state_dict, explicitly call set_extra_state for modules with modelopt callbacks. - quantization/plugins/transformer_engine.py: modelopt_post_restore skips the CUDA-only fp4 dry-run when the weight is on CPU (export loads on CPU). - export/unified_export_megatron.py: assert weight_quantizer is GroupedQuantizer or None; warn on TP/EP-mismatch clamp; revert temporary export amax in finally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
After 57fe963 registered get/set_extra_state on every QuantModule (incl. output_layer), quant_module_get_extra_state still always returned a non-empty modelopt_quantizer_state (it iterates all TensorQuantizers, including disabled ones). An *unquantized* output_layer therefore emitted a non-empty _extra_state, and Megatron-Bridge's save_megatron_model -> GPTModel.sharded_state_dict (which asserts output_layer._extra_state is empty) failed with "Boolean value of Tensor with more than one value is ambiguous". Gate quant_module_get_extra_state to return {} when the module has no enabled TensorQuantizer and is not a compressed RealQuantLinear; the aggregator then returns None, satisfying Megatron's assert, while a genuinely quantized lm_head still saves its amax. Fixes tests/examples/megatron_bridge/test_quantize_export.py::test_quantize_and_export Signed-off-by: Jennifer Chen <jennifchen@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.rst
…QAD support
Also fixes a ModelOpt bug that affects ANY model with an untied lm_head quantized through
Megatron-Bridge, not just Nemotron: the output layer was silently exported as BF16.
An untied `lm_head` (`output_layer`) was silently exported as BF16 instead of NVFP4
whenever the model was built with Megatron-Bridge, even though the recipe enabled
`*output_layer*weight_quantizer`. The same recipe under Megatron-LM quantized it
correctly, so this was not a configuration problem.
`_MegatronParallelLinear.sharded_state_dict()` special-cases `output_layer` and asks
`megatron.training.get_args()` whether embeddings are untied. Megatron-Bridge has no
global args store, so the call raises and the handler falls back to "tied", taking the
early return that drops all quantizer state. Fixing that alone is not sufficient: the
dist-checkpoint loader silently skips any checkpoint key the model does not advertise,
and `sharded_state_dict()` can only advertise a buffer that already exists, so the
calibrated scales in the checkpoint had nowhere to land.
* `_resolve_output_layer_untied()` reads `share_embeddings_and_output_weights` off the
model, which Megatron-Core carries under both frameworks, and records it on the
config so `sharded_state_dict()` can consult it. `get_args()` remains the fallback,
so Megatron-LM behavior is unchanged, and an unknown result still means "tied".
* Materialize missing weight-quantizer scale buffers before the load plan is built.
`_amax` must be allocated flat as `[numel // block, 1]`: `_process_quantizer_amax`
exposes it to the checkpoint as an `[out_features, blocks]` view over the same
storage, so the loader writes straight through. Allocating the viewed shape loads
successfully but leaves the wrong in-memory rank, which then breaks the exporter's
scale math. `_global_amax` is registered directly because its property lives on
`StaticBlockScaleQuantizer` and the module is still a plain `TensorQuantizer` here.
The export example now fails loudly instead of quietly emitting BF16 when an enabled
NVFP4 weight quantizer is missing either scale, naming the module and the attribute.
Static-block NVFP4 needs both; an `_amax`-only check let a half-restored quantizer
through, which failed much later inside `NVFP4QTensor.quantize` where `scale * scale_2`
broadcasts `[N, 1]` against `[N]`. `MODELOPT_ALLOW_UNCALIBRATED_NVFP4=1` restores the
previous behavior, reported rather than silent.
Also included: Megatron-Bridge PTQ/QAD enablement for Nemotron-style hybrid MoE models
(gate the non-grouped MoE spec to the quantized student so the BF16 teacher is built
with its natural spec, SFT-masked distillation, student initialization from a Megatron
checkpoint, calibration random offset).
Verified end to end against a Megatron-LM-produced reference: `lm_head.weight` is now
`U8 [131072, 1344]` with `weight_scale` and `weight_scale_2`, 18487 keys and 72
excluded modules, matching the reference exactly (previously BF16 `[131072, 2688]`,
18485 keys, 73 excluded with `lm_head` among them).
Two Megatron-Bridge compatibility shims were removed after being shown unnecessary:
a `DistillationProvider.to_cfg_dict` monkeypatch (a 5-iteration distillation run trains
and checkpoints cleanly without it) and an `InferenceCudaGraphScope` enum stub added for
a Megatron-LM-PTQ import path that is not used (zero occurrences across a full
PTQ/QAD/export run).
Signed-off-by: James Shen <yueshen@nvidia.com>
The two example scripts no longer hard-code model-shape assumptions. MoE expert grouping is a
`--grouped_experts` flag on both `quantize.py` and the exporter, defaulting to non-grouped so
existing behavior is unchanged; per-block NVFP4 requires non-grouped because TEGroupedLinear can
only represent a per-tensor scale, while per-tensor recipes can now opt into faster grouped GEMM.
The exporter reads `mtp_num_layers` from the checkpoint's run_config.yaml instead of assuming 0,
and `quantize.py` now warns when it drops MTP heads, matching prune_minitron.py. Expert grouping is
deliberately NOT derived from run_config.yaml: a MambaModelProvider sets the layout via
mamba_stack_spec, so a non-grouped checkpoint still records `moe_grouped_gemm: true` and trusting it
would build a mismatched model.
Signed-off-by: James Shen <yueshen@nvidia.com>
Review fixes: the export guard no longer excludes StaticBlockScaleQuantizer, which is the class that
owns `_global_amax` -- excluding it skipped exactly the case the guard exists to catch -- and it now
filters on `is_enabled` to match the message it prints. The scale-buffer materialization checks
`in_features % block_size`, matching the `view(weight.shape[0], -1)` performed later rather than
total element count, and warns instead of silently leaving the buffers unallocated.
Signed-off-by: James Shen <yueshen@nvidia.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Signed-off-by: James Shen <yueshen@nvidia.com> # Conflicts: # examples/megatron_bridge/distill.py # modelopt/torch/quantization/plugins/megatron.py
…ar setup PR #1550 builds the new per-expert GroupedQuantizer in _QuantTEGroupedLinear._setup() from `self.default_quant_desc_weight`, but that attribute does not resolve on the class: _ParallelLinear derives from QuantModule, not _QuantLinear, and _ParallelLinear._setup() itself references the class attribute directly (see plugins/custom.py). Constructing any grouped-experts model therefore fails with AttributeError: QuantTEColumnParallelGroupedLinear object has no attribute default_quant_desc_weight Use _QuantLinear.default_quant_desc_weight, matching what _ParallelLinear._setup() already does for the non-grouped path. Verified on Nemotron-Nano-3 W4A16 NVFP4 four_over_six with TEGroupedMLP: PTQ (6382 quantizers) -> QAD (200 iters, logits-KD 3.37e-2 -> 1.91e-2) -> HF export (18487 keys; routed-expert scales stay per-block, e.g. [1856, 116] with block 16) -> compressed-tensors -> served on stock vLLM 0.26.0 at TP=2. Signed-off-by: James Shen <yueshen@nvidia.com>
📝 WalkthroughWalkthroughThe change adds independent per-expert ChangesPer-expert quantization workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Calibration
participant GroupedQuantizer
participant TEGroupedMLP
participant CheckpointExporter
Calibration->>GroupedQuantizer: collect and synchronize per-expert amax
GroupedQuantizer->>TEGroupedMLP: quantize each expert weight
TEGroupedMLP->>CheckpointExporter: expose per-expert scales and expert identities
CheckpointExporter-->>GroupedQuantizer: restore grouped quantizer state
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelopt/torch/export/plugins/vllm_fakequant_hf.py (1)
632-639: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFlatten nested
SequentialQuantizermembers before saving rotation state.
GroupedQuantizeracceptsSequentialQuantizermembers, but_rotateexists only on theirTensorQuantizerleaves. Accessingsub._rotateraisesAttributeError, and restoring the container would not restore the leaves. Iterate over the leaves when recording, disabling, and restoring rotation state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/torch/export/plugins/vllm_fakequant_hf.py` around lines 632 - 639, Update the quantizer handling around SequentialQuantizer and GroupedQuantizer to flatten nested SequentialQuantizer members to their TensorQuantizer leaves before recording rotation state. Record each leaf’s _rotate value, disable rotation on each leaf, and ensure restoration targets those same leaves rather than container quantizers.
🧹 Nitpick comments (5)
examples/megatron_bridge/distill.py (3)
348-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFunction-scope imports without a justification comment in
examples/megatron_bridge/distill.pyandmodelopt/torch/distill/plugins/megatron.py. Both new imports sit inside a function body with no stated reason. The coding guidelines allow a local import only for a circular dependency, an optional dependency, or an unusually heavy import, and require a brief comment naming that reason.
examples/megatron_bridge/distill.py#L348-L348: movefrom modelopt.torch.nas.plugins.megatron import get_te_mamba_stack_specto the module header. It runs on every_build_model_providercall but only the branch at line 350 uses it.modelopt/torch/distill/plugins/megatron.py#L624-L627: keep thetensor_quantizerimport local if it breaks a circular dependency, and add a one-line comment naming that dependency. Otherwise move it to the module header.As per coding guidelines: "Keep imports at the top of Python source and test files; use local imports only for justified circular dependencies, optional dependencies, or unusually heavy imports, with a brief explanatory comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/megatron_bridge/distill.py` at line 348, Move get_te_mamba_stack_spec from the local scope of _build_model_provider in examples/megatron_bridge/distill.py to the module header. In modelopt/torch/distill/plugins/megatron.py, move the tensor_quantizer import to the module header unless it is required to avoid a circular dependency; if it must remain local, add a brief comment identifying that dependency.Source: Coding guidelines
65-73: 📐 Maintainability & Code Quality | 🔵 TrivialTODO tracked: replace the
DistillationProvider.providepatch.The TODO states that this class-level patch must go away once Megatron-Bridge exposes a student-initialization hook. The registry is keyed by
id(distill_provider), so the workaround also depends on the provider object staying alive for the process lifetime.Do you want me to open an issue to track the upstream hook and the removal of this patch?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/megatron_bridge/distill.py` around lines 65 - 73, Track the upstream hook request as an issue, documenting that the class-level DistillationProvider.provide patch and _MEGATRON_STUDENT_CKPT_PATHS registry should be removed once Megatron-Bridge exposes student initialization before distillation conversion.
365-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead
FORCE_NO_PER_TOKEN_LOSSonce.The same environment variable is read in two places with inverted conditions. The two settings must stay consistent:
calculate_per_token_lossandaverage_in_collectiveare a matched pair. A future edit to one site can silently desynchronize them.Compute the value once in
mainand use it at both sites.♻️ Proposed fix
def main(args: argparse.Namespace): + # SFT with CP>1 needs per-token loss on the provider and average_in_collective=False on DDP. + # Read the override once so the two settings cannot desynchronize. + per_token_loss = args.sft and os.environ.get("FORCE_NO_PER_TOKEN_LOSS", "0") != "1" checkpoint_dir = os.path.join(args.output_dir, "checkpoints")if args.sft: - # Finetuning (SFT) with context parallel (CP>1) requires per-token loss so the - # response loss-mask reduces correctly across the CP ranks. - provider.calculate_per_token_loss = os.environ.get("FORCE_NO_PER_TOKEN_LOSS", "0") != "1" + # Finetuning (SFT) with context parallel (CP>1) requires per-token loss so the + # response loss-mask reduces correctly across the CP ranks. + provider.calculate_per_token_loss = per_token_loss- # Finetuning (SFT) with CP>1 requires per-token loss (set on the provider) and - # average_in_collective=False (the per-token loss is summed, not averaged, in the collective). - average_in_collective=(not args.sft) or os.environ.get("FORCE_NO_PER_TOKEN_LOSS", "0") == "1", + # The per-token loss is summed, not averaged, in the collective. + average_in_collective=not per_token_loss,Also applies to: 484-486
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/megatron_bridge/distill.py` around lines 365 - 368, Read FORCE_NO_PER_TOKEN_LOSS once in main, store the resulting boolean, and reuse it when assigning both provider.calculate_per_token_loss and the corresponding average_in_collective setting. Remove the second direct environment lookup while preserving the existing inverted relationship between these matched settings.modelopt/torch/distill/plugins/megatron.py (1)
644-655: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse rank-aware logging for the promotion messages.
Both messages run on every rank, so a large job emits one copy per rank.
setup_distillation_configin this same file already guardslogger.infowith a rank check at the tensor-and-context-parallel rank. Apply the same guard here, or useprint_rank_0/warn_rank_0.♻️ Proposed fix
if amax is None: # Uncalibrated: leave it alone rather than silently changing precision. - logger.warning(f"NVFP4 weight quantizer {name} has no _amax; not promoted.") + if parallel_state.get_tensor_and_context_parallel_rank() == 0: + logger.warning(f"NVFP4 weight quantizer {name} has no _amax; not promoted.") n_skipped += 1 continue- if n_promoted or n_skipped: + if (n_promoted or n_skipped) and parallel_state.get_tensor_and_context_parallel_rank() == 0: logger.info(As per coding guidelines: "Develop distributed code with rank-aware logging such as
print_rank_0orwarn_rank_0".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/torch/distill/plugins/megatron.py` around lines 644 - 655, Update the NVFP4 promotion logging near StaticBlockScaleQuantizer.from_tensor_quantizer so both the missing-_amax warning and promotion summary are emitted only from the tensor-and-context-parallel rank, using the existing rank-check pattern from setup_distillation_config or the available print_rank_0/warn_rank_0 helpers.Source: Coding guidelines
modelopt/torch/utils/dataset_utils.py (1)
765-765: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
random_offsetin the docstring Args list.
get_dataset_dataloaderis a public API and every other parameter is documented. Add an entry forrandom_offsetdescribing the extra sequence-length of headroom and the dropped leading tokens.As per coding guidelines: "Document public and higher-level APIs with docstrings, including examples when useful".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/torch/utils/dataset_utils.py` at line 765, Update the docstring Args section for get_dataset_dataloader to document random_offset, describing that it enables extra sequence-length headroom and drops the corresponding leading tokens. Keep the documentation consistent with the existing parameter entries.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/megatron_bridge/distill.py`:
- Around line 311-314: Resolve the conflict between the validation and fallback
around the argument-parsing logic: either remove the
hf_export_path/student_hf_model ValueError so the existing student_hf_path
fallback remains usable for exports, or retain the validation and update the
--student_hf_model help text to explicitly state it is required with
--hf_export_path. Ensure the chosen behavior is consistent and the fallback is
not misleading.
- Line 348: Move the get_te_mamba_stack_spec import from _build_model_provider
to the module-level imports in examples/megatron_bridge/distill.py, unless
modelopt.torch.nas.plugins.megatron is intentionally optional; in that case,
retain the local import and add a brief comment explaining the
optional-dependency justification.
- Around line 192-198: Update the --sft argument help text in
parser.add_argument to name FinetuningDatasetConfig instead of
GPTSFTDatasetConfig, matching the configuration class constructed by the SFT
branch while preserving the rest of the description.
In `@examples/megatron_bridge/export_quantized_megatron_to_hf.py`:
- Around line 136-142: Update the resolved configuration construction to treat
explicit null values in model_cfg as missing, so defaults such as
mtp_num_layers=0 are preserved. Move the isinstance(model_cfg, dict) check
outside the comprehension, and only read keys from model_cfg when it is a
dictionary; otherwise retain defaults unchanged.
- Around line 125-127: Update the run_config resolution near run_config so it
uses the checkpoint iteration/path selected by the loader, including tracker,
ckpt_step, direct iteration, or checkpoint state metadata. Remove the
lexicographic sorted(...)[0] selection and derive run_config.yaml from the same
resolved checkpoint location used for loading. Preserve the root-level fallback
only when the loader resolves the root checkpoint itself.
In `@modelopt/torch/distill/plugins/megatron.py`:
- Around line 630-634: Update the quantizer scan near the loop over named
modules to traverse only the student model rather than self, preventing teacher
submodules from being promoted. Preserve the existing TensorQuantizer and
StaticBlockScaleQuantizer filtering and promotion behavior for student weight
quantizers.
In `@modelopt/torch/export/unified_export_megatron.py`:
- Around line 1062-1065: Update the assertion message in the grouped_wq
validation to reference pre-0.46 single-quantizer checkpoints instead of
pre-0.47, matching the documented per-expert layout break and re-quantization
release. Leave the assertion condition and type reporting unchanged.
- Around line 1147-1149: Update the grouped-expert handling around
_get_quantized_state and _record_excluded_module so unquantized experts are
added to exclude_modules when qformat is None. For each expert, format the
unformatted prefix with its global_id, append ".", and record that resulting
module name, while preserving existing behavior for quantized experts.
- Around line 1142-1145: Restrict the temporary amax assignment in the
weight-quantizer handling to instances of TensorQuantizer, preserving the
existing _amax and enabled checks. Import TensorQuantizer alongside
GroupedQuantizer, and ensure only TensorQuantizer objects are appended to
temp_amax_wqs so delegated SequentialQuantizer members are not modified or
reset.
In `@modelopt/torch/quantization/nn/modules/tensor_quantizer.py`:
- Around line 1870-1879: Update GroupedQuantizer.__init__ to reject an empty
quantizers argument by raising ValueError before representative access can
occur, while preserving the existing member-type validation. Add a CPU
regression test that verifies constructing GroupedQuantizer without members
raises ValueError.
In `@modelopt/torch/quantization/plugins/custom.py`:
- Around line 143-155: Update _has_complete_static_nvfp4_weight_state and the
surrounding restore path to inspect every member of a GroupedQuantizer against
its matching expert weight, rather than only member zero; ensure recalibration
and restoration preserve each member’s distinct amax state and prevent later
expert processing from skipping invalid members. Add a regression test covering
at least two experts with different amax states, while preserving backward
compatibility for serialized ModeloptBaseConfig and QuantizeConfig objects.
In `@modelopt/torch/quantization/plugins/megatron.py`:
- Around line 241-253: Restrict the missing-state skip in the quantizer
restoration flow to names matching the per-expert `weight_quantizer.<i>` pattern
handled by the fallback in `quantizer_substate`. For non-expert names such as
`input_quantizer` and `output_quantizer`, preserve the previous loud failure
behavior instead of continuing with default properties.
In `@modelopt/torch/utils/dataset_utils.py`:
- Around line 715-718: Update the random_offset handling in
get_megatron_calibration_dataloader to derive the leading-token offset from a
fixed, shared seed rather than the unseeded global random.randint call. Reuse
the existing deterministic seed approach used by the multi-source shuffle so
every rank produces the identical token_stream and runs remain reproducible.
In `@tests/gpu_megatron/torch/quantization/plugins/test_megatron.py`:
- Around line 1026-1027: Deep-copy quant_cfg independently before each
mtq.quantize call in the affected test and in
_test_te_grouped_sharded_state_dict_global_expert_identity_helper, so neither
invocation nor later parametrized tests share a potentially mutated
configuration. Follow the existing copy.deepcopy pattern used by the nearby
quantization call sites.
---
Outside diff comments:
In `@modelopt/torch/export/plugins/vllm_fakequant_hf.py`:
- Around line 632-639: Update the quantizer handling around SequentialQuantizer
and GroupedQuantizer to flatten nested SequentialQuantizer members to their
TensorQuantizer leaves before recording rotation state. Record each leaf’s
_rotate value, disable rotation on each leaf, and ensure restoration targets
those same leaves rather than container quantizers.
---
Nitpick comments:
In `@examples/megatron_bridge/distill.py`:
- Line 348: Move get_te_mamba_stack_spec from the local scope of
_build_model_provider in examples/megatron_bridge/distill.py to the module
header. In modelopt/torch/distill/plugins/megatron.py, move the tensor_quantizer
import to the module header unless it is required to avoid a circular
dependency; if it must remain local, add a brief comment identifying that
dependency.
- Around line 65-73: Track the upstream hook request as an issue, documenting
that the class-level DistillationProvider.provide patch and
_MEGATRON_STUDENT_CKPT_PATHS registry should be removed once Megatron-Bridge
exposes student initialization before distillation conversion.
- Around line 365-368: Read FORCE_NO_PER_TOKEN_LOSS once in main, store the
resulting boolean, and reuse it when assigning both
provider.calculate_per_token_loss and the corresponding average_in_collective
setting. Remove the second direct environment lookup while preserving the
existing inverted relationship between these matched settings.
In `@modelopt/torch/distill/plugins/megatron.py`:
- Around line 644-655: Update the NVFP4 promotion logging near
StaticBlockScaleQuantizer.from_tensor_quantizer so both the missing-_amax
warning and promotion summary are emitted only from the
tensor-and-context-parallel rank, using the existing rank-check pattern from
setup_distillation_config or the available print_rank_0/warn_rank_0 helpers.
In `@modelopt/torch/utils/dataset_utils.py`:
- Line 765: Update the docstring Args section for get_dataset_dataloader to
document random_offset, describing that it enables extra sequence-length
headroom and drops the corresponding leading tokens. Keep the documentation
consistent with the existing parameter entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a8ec06a9-7a9f-42e4-9786-0b06303a155c
📒 Files selected for processing (20)
CHANGELOG.rstexamples/megatron_bridge/distill.pyexamples/megatron_bridge/export_quantized_megatron_to_hf.pyexamples/megatron_bridge/quantize.pymodelopt/torch/distill/plugins/megatron.pymodelopt/torch/export/plugins/vllm_fakequant_hf.pymodelopt/torch/export/unified_export_megatron.pymodelopt/torch/opt/plugins/mcore_dist_checkpointing.pymodelopt/torch/quantization/config.pymodelopt/torch/quantization/model_calib.pymodelopt/torch/quantization/nn/__init__.pymodelopt/torch/quantization/nn/modules/tensor_quantizer.pymodelopt/torch/quantization/plugins/custom.pymodelopt/torch/quantization/plugins/megatron.pymodelopt/torch/quantization/plugins/transformer_engine.pymodelopt/torch/quantization/utils/core_utils.pymodelopt/torch/utils/dataset_utils.pymodelopt/torch/utils/plugins/megatron_calibration.pytests/gpu_megatron/torch/quantization/plugins/test_megatron.pytests/unit/torch/quantization/test_tensor_quantizer_cpu.py
| parser.add_argument( | ||
| "--sft", | ||
| action="store_true", | ||
| help="SFT-masked distillation: read raw prompt-completion jsonl from --sft_dataset_root and " | ||
| "mask the loss to the completion (assistant response) tokens. Uses GPTSFTDatasetConfig + the " | ||
| "real (HuggingFace) tokenizer instead of the pretraining GPTDataset + NullTokenizer.", | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Help text names a config class the code does not use.
The --sft help says GPTSFTDatasetConfig, but the SFT branch at line 426 builds a FinetuningDatasetConfig. Use the actual class name so users can match the flag to the code.
📝 Proposed fix
help="SFT-masked distillation: read raw prompt-completion jsonl from --sft_dataset_root and "
"mask the loss to the completion (assistant response) tokens. Uses GPTSFTDatasetConfig + the "
+ "real (HuggingFace) tokenizer instead of the pretraining GPTDataset + NullTokenizer.",Replace with:
help="SFT-masked distillation: read raw prompt-completion jsonl from --sft_dataset_root and "
"mask the loss to the completion (assistant response) tokens. Uses FinetuningDatasetConfig + "
"the real (HuggingFace) tokenizer instead of the pretraining GPTDataset + NullTokenizer.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/megatron_bridge/distill.py` around lines 192 - 198, Update the --sft
argument help text in parser.add_argument to name FinetuningDatasetConfig
instead of GPTSFTDatasetConfig, matching the configuration class constructed by
the SFT branch while preserving the rest of the description.
| if args.hf_export_path and not args.student_hf_model: | ||
| raise ValueError("Must provide --student_hf_model if --hf_export_path is provided.") | ||
| if args.student_hf_model is None: | ||
| args.student_hf_model = args.student_hf_path |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The new check makes the existing --student_hf_model fallback unreachable for export.
Line 311 rejects --hf_export_path without --student_hf_model. Line 313 then defaults student_hf_model to student_hf_path. The default can never apply to an export run, because line 311 already raised. Users who previously relied on the fallback for export now get an error.
Choose one behavior. Either keep the fallback and drop the check, or keep the check and state in the --student_hf_model help that it is required with --hf_export_path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/megatron_bridge/distill.py` around lines 311 - 314, Resolve the
conflict between the validation and fallback around the argument-parsing logic:
either remove the hf_export_path/student_hf_model ValueError so the existing
student_hf_path fallback remains usable for exports, or retain the validation
and update the --student_hf_model help text to explicitly state it is required
with --hf_export_path. Ensure the chosen behavior is consistent and the fallback
is not misleading.
| # For a hybrid Mamba provider the layer SPEC must be rebuilt with moe_grouped_gemm=False -- | ||
| # setting the flag alone does not propagate. Mirror modelopt's load_mbridge_model_from_hf. | ||
| provider.mtp_num_layers = 0 | ||
| from modelopt.torch.nas.plugins.megatron import get_te_mamba_stack_spec |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move get_te_mamba_stack_spec to the top-level imports.
This import sits inside _build_model_provider with no justification comment, and it runs on every provider build even though only the branch at line 350 uses it. The coding guidelines allow a local import only for a circular dependency, an optional dependency, or an unusually heavy import, and require a brief comment naming the reason.
If modelopt.torch.nas.plugins.megatron is an optional dependency here, keep the local import and add the comment. Otherwise move it to the module header.
♻️ Proposed fix
provider.mtp_num_layers = 0
- from modelopt.torch.nas.plugins.megatron import get_te_mamba_stack_spec
-
if quantized and args.student_nongrouped_experts:Add at the module header instead:
from modelopt.torch.nas.plugins.megatron import get_te_mamba_stack_specAs per coding guidelines: "Keep imports at the top of Python source and test files; use local imports only for justified circular dependencies, optional dependencies, or unusually heavy imports, with a brief explanatory comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/megatron_bridge/distill.py` at line 348, Move the
get_te_mamba_stack_spec import from _build_model_provider to the module-level
imports in examples/megatron_bridge/distill.py, unless
modelopt.torch.nas.plugins.megatron is intentionally optional; in that case,
retain the local import and add a brief comment explaining the
optional-dependency justification.
Source: Coding guidelines
| run_config = next(iter(sorted(pathlib.Path(megatron_path).glob("*/run_config.yaml"))), None) | ||
| if run_config is None: | ||
| run_config = pathlib.Path(megatron_path) / "run_config.yaml" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the iter_* resolution used by the Megatron checkpoint loader.
set -euo pipefail
fd -t f 'mbridge.py' modelopt | xargs -r rg -n -C 5 '_get_modelopt_checkpoint_path|iter_'
rg -n -C 5 'def _get_modelopt_checkpoint_path' --glob '*.py'Repository: NVIDIA/Model-Optimizer
Length of output: 2401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exporter ---'
cat -n examples/megatron_bridge/export_quantized_megatron_to_hf.py | sed -n '95,145p'
printf '%s\n' '--- loader call sites and definitions ---'
rg -n -C 12 'load_modelopt_megatron_checkpoint|_get_modelopt_checkpoint_path|def _get_modelopt_checkpoint_path' \
modelopt examples --glob '*.py'
printf '%s\n' '--- iteration directory fixtures/usages ---'
rg -n -C 4 'iter_[0-9]+|run_config\.yaml' modelopt examples --glob '*.py' --glob '*.yaml'Repository: NVIDIA/Model-Optimizer
Length of output: 29061
🌐 Web query:
Megatron-Bridge _get_modelopt_checkpoint_path source iter_ latest checkpoint
💡 Result:
The function _get_modelopt_checkpoint_path is a utility in the Megatron-Bridge library (specifically within bridge.training.post_training.checkpointing) used to determine the correct path for ModelOpt operations [1][2][3]. This function effectively handles iteration directories by checking the provided checkpoint_path for the presence of ModelOpt states [1][4]. It specifically looks for modelopt_state directories either within specific iteration subdirectories (which follow the iter_* naming convention) or within the root checkpoint directory [1][2]. In the context of Megatron-LM and Megatron-Bridge checkpointing: - Megatron-Bridge typically uses a base directory structure where iteration-specific checkpoints are stored in iter_N/ subdirectories [5][6]. - When determining the "latest" checkpoint, Megatron Bridge relies on a tracker file named latest_train_state.pt located in the root directory [5][7]. - The _get_modelopt_checkpoint_path utility abstracts the path resolution to ensure ModelOpt-specific data is correctly identified and retrieved, regardless of whether it is stored at the root or within an iteration-specific folder [1][3]. For users needing to load a specific checkpoint iteration, the ckpt_step parameter is used to override the default tracker behavior and target a specific iter_N folder directly [5][7]. If you are performing manual conversion or export tasks, logic similar to _get_modelopt_checkpoint_path often involves scanning the checkpoint_path for iter_* directories, sorting them numerically, and selecting the highest index to identify the latest iteration [8].
Citations:
- 1: https://docs.nvidia.com/nemo/megatron-bridge/latest/apidocs/bridge/bridge.training.post_training.checkpointing.html
- 2: https://docs.nvidia.com/nemo/megatron-bridge/0.5.0/apidocs/bridge/bridge.training.post_training.checkpointing.html
- 3: https://docs.nvidia.com/nemo/megatron-bridge/nightly/apidocs/bridge/bridge.training.post_training.checkpointing.html
- 4: https://docs.nvidia.com/nemo/megatron-bridge/0.4.0/apidocs/bridge/bridge.training.post_training.checkpointing.html
- 5: https://docs.nvidia.com/nemo/megatron-bridge/latest/training/checkpointing.html
- 6: https://docs.nvidia.com/nemo/megatron-bridge/0.4.1/training/checkpointing.html
- 7: https://docs.nvidia.com/nemo/megatron-bridge/nightly/training/checkpointing.html
- 8: https://github.com/NVIDIA-NeMo/Megatron-Bridge/blob/main/examples/conversion/convert_checkpoints.py
🌐 Web query:
site:github.com/NVIDIA-NeMo/Megatron-Bridge "_get_modelopt_checkpoint_path"
💡 Result:
The term get_modelopt_checkpoint_path does not appear to be a standard, publicly documented public API function within the NVIDIA Model Optimizer (ModelOpt) or the NVIDIA NeMo/Megatron-Bridge libraries. Based on an analysis of the NVIDIA NeMo/Megatron-Bridge repository [1][2][3], interactions with ModelOpt checkpoints typically involve: 1. Manual Directory Specification: Scripts generally expect users to provide a path to the checkpoint directory directly via arguments (e.g., --megatron-save-path or --pretrained-checkpoint) [3][4][5]. 2. Internal Logic: The codebase contains logic within internal utility modules (such as src/megatron/bridge/training/post_training/checkpointing.py) that handles the identification and selection of ModelOpt checkpoints [6]. This logic often involves checking for specific configuration files like run_config.yaml or iterating through subdirectories labeled iter [1][2]. 3. Model Configuration: Quantization configurations are typically handled through functions like get_modelopt_torch_quantization_config [4] or by defining model specifications using tools like get_gpt_modelopt_spec [7]. If you encountered this name in a specific codebase, it is likely an internal helper function or a custom utility specific to that project rather than a public function provided by NVIDIA ModelOpt. You may wish to search for the definition of this function within your local project files to understand its specific implementation and usage.
Citations:
- 1: https://github.com/NVIDIA-NeMo/Megatron-Bridge/blob/main/examples/conversion/convert_checkpoints.py
- 2: https://github.com/NVIDIA-NeMo/Megatron-Bridge/blob/main/src/megatron/bridge/models/conversion/auto_bridge.py
- 3: https://github.com/NVIDIA-NeMo/Megatron-Bridge/tree/main/examples/quantization
- 4: https://github.com/NVIDIA-NeMo/Megatron-Bridge/blob/main/examples/quantization/quantize.py
- 5: [OMNIML-2910] ModelOpt QAT support NVIDIA-NeMo/Megatron-Bridge#873
- 6: NVIDIA-NeMo/Megatron-Bridge@190e6e4
- 7: https://github.com/NVIDIA-NeMo/Megatron-Bridge/blob/main/src/megatron/bridge/models/gpt_provider.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/NVIDIA-NeMo/Megatron-Bridge/main/src/megatron/bridge/training/post_training/checkpointing.py'
curl -fsSL "$url" | rg -n -C 20 '_get_modelopt_checkpoint_path|iter_|latest_train_state|run_config'Repository: NVIDIA/Model-Optimizer
Length of output: 5010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/NVIDIA-NeMo/Megatron-Bridge/main/src/megatron/bridge/training/checkpointing.py'
curl -fsSL "$url" | rg -n -C 25 '_resolve_checkpoint_iteration|_DIRECT_ITERATION_DIR_SENTINEL|latest_train_state|ckpt_step'
printf '%s\n' '--- exporter path construction ---'
cat -n examples/megatron_bridge/export_quantized_megatron_to_hf.py | sed -n '145,180p'
rg -n -C 8 'megatron_path|checkpoint_export_paths' examples/megatron_bridge/export_quantized_megatron_to_hf.pyRepository: NVIDIA/Model-Optimizer
Length of output: 28745
Resolve run_config.yaml from the checkpoint iteration selected by the loader. sorted(...)[0] selects the first lexicographic match, while the loader uses the tracker, ckpt_step, direct iteration paths, or checkpoint state metadata. Do not replace this with reverse sorting; reuse the loader’s resolution logic so mtp_num_layers matches the loaded checkpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/megatron_bridge/export_quantized_megatron_to_hf.py` around lines 125
- 127, Update the run_config resolution near run_config so it uses the
checkpoint iteration/path selected by the loader, including tracker, ckpt_step,
direct iteration, or checkpoint state metadata. Remove the lexicographic
sorted(...)[0] selection and derive run_config.yaml from the same resolved
checkpoint location used for loading. Preserve the root-level fallback only when
the loader resolves the root checkpoint itself.
| model_cfg = cfg.get("model") if isinstance(cfg.get("model"), dict) else cfg | ||
| resolved = { | ||
| key: model_cfg.get(key, default) | ||
| for key, default in defaults.items() | ||
| if isinstance(model_cfg, dict) | ||
| } | ||
| resolved = {**defaults, **resolved} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
An explicit null in the YAML defeats the default.
If run_config.yaml contains mtp_num_layers: null, model_cfg.get(key, default) returns None, and the following merge keeps None because resolved overrides defaults. The provider then receives mtp_num_layers=None instead of 0.
The if isinstance(model_cfg, dict) filter is also loop-invariant. Move it out of the comprehension.
♻️ Proposed fix
model_cfg = cfg.get("model") if isinstance(cfg.get("model"), dict) else cfg
- resolved = {
- key: model_cfg.get(key, default)
- for key, default in defaults.items()
- if isinstance(model_cfg, dict)
- }
- resolved = {**defaults, **resolved}
+ resolved = dict(defaults)
+ if isinstance(model_cfg, dict):
+ for key, default in defaults.items():
+ value = model_cfg.get(key)
+ resolved[key] = default if value is None else value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model_cfg = cfg.get("model") if isinstance(cfg.get("model"), dict) else cfg | |
| resolved = { | |
| key: model_cfg.get(key, default) | |
| for key, default in defaults.items() | |
| if isinstance(model_cfg, dict) | |
| } | |
| resolved = {**defaults, **resolved} | |
| model_cfg = cfg.get("model") if isinstance(cfg.get("model"), dict) else cfg | |
| resolved = dict(defaults) | |
| if isinstance(model_cfg, dict): | |
| for key, default in defaults.items(): | |
| value = model_cfg.get(key) | |
| resolved[key] = default if value is None else value |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/megatron_bridge/export_quantized_megatron_to_hf.py` around lines 136
- 142, Update the resolved configuration construction to treat explicit null
values in model_cfg as missing, so defaults such as mtp_num_layers=0 are
preserved. Move the isinstance(model_cfg, dict) check outside the comprehension,
and only read keys from model_cfg when it is a dictionary; otherwise retain
defaults unchanged.
| def __init__(self, *quantizers: "TensorQuantizer | SequentialQuantizer"): | ||
| """Initialize GroupedQuantizer module.""" | ||
| super().__init__(quantizers) | ||
| assert all(isinstance(q, (TensorQuantizer, SequentialQuantizer)) for q in self), ( | ||
| "All quantizers must be a TensorQuantizer or SequentialQuantizer." | ||
| ) | ||
|
|
||
| def forward(self, inputs): | ||
| """Apply the representative quantizer for single-weight compatibility paths.""" | ||
| return self[0](inputs) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject empty GroupedQuantizer instances.
__init__ accepts zero members, but forward and representative property access use self[0]. An empty container fails later with IndexError. Raise ValueError during construction and add a CPU regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelopt/torch/quantization/nn/modules/tensor_quantizer.py` around lines 1870
- 1879, Update GroupedQuantizer.__init__ to reject an empty quantizers argument
by raising ValueError before representative access can occur, while preserving
the existing member-type validation. Add a CPU regression test that verifies
constructing GroupedQuantizer without members raises ValueError.
| quantizer = ( | ||
| quantizer[0] | ||
| if isinstance(quantizer, (SequentialQuantizer, GroupedQuantizer)) | ||
| else quantizer | ||
| ) | ||
| return hasattr(quantizer, name) | ||
|
|
||
| def _has_complete_static_nvfp4_weight_state(quantizer, weight): | ||
| quantizer = quantizer[0] if isinstance(quantizer, SequentialQuantizer) else quantizer | ||
| quantizer = ( | ||
| quantizer[0] | ||
| if isinstance(quantizer, (SequentialQuantizer, GroupedQuantizer)) | ||
| else quantizer | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve each grouped weight quantizer during restore.
_QuantTEGroupedLinear.modelopt_post_restore calls this generic restore path. The path checks only member zero. If member zero requires recalibration, GroupedQuantizer.reset_amax() clears every member, but max_calibrate(grouped, ...) forwards through only member zero. The later per-expert loop then skips members with _amax is None.
Restore and validate each grouped weight quantizer against its matching expert weight. Add a checkpoint restore regression test with at least two experts and different amax states.
As per coding guidelines, “Preserve backward compatibility for serialized Pydantic-based ModelOpt configuration and checkpoint objects such as ModeloptBaseConfig and QuantizeConfig.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelopt/torch/quantization/plugins/custom.py` around lines 143 - 155, Update
_has_complete_static_nvfp4_weight_state and the surrounding restore path to
inspect every member of a GroupedQuantizer against its matching expert weight,
rather than only member zero; ensure recalibration and restoration preserve each
member’s distinct amax state and prevent later expert processing from skipping
invalid members. Add a regression test covering at least two experts with
different amax states, while preserving backward compatibility for serialized
ModeloptBaseConfig and QuantizeConfig objects.
Source: Coding guidelines
| quantizer_substate = quantizer_state.get(name) | ||
| if quantizer_substate is None: | ||
| # Per-expert quantizers ("weight_quantizer.<i>") are saved per EP rank, so a | ||
| # module loaded at smaller EP (e.g. EP1 export from an EP16 ckpt) has more | ||
| # experts than the saved state. Per-expert properties are uniform across | ||
| # experts (amax rides separately as globally-indexed sharded tensors), so | ||
| # fall back to expert 0's state. Rewrite only the expert index right after | ||
| # weight_quantizer, preserving deeper suffixes (e.g. a SequentialQuantizer level | ||
| # weight_quantizer.<i>.<lvl>); non-expert names are left unchanged. | ||
| fallback = re.sub(r"(weight_quantizer)\.\d+", r"\1.0", name) | ||
| quantizer_substate = quantizer_state.get(fallback) | ||
| if quantizer_substate is None: | ||
| continue |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restrict the silent skip to per-expert quantizer names.
The comment justifies the fallback for weight_quantizer.<i> names only. The continue at Line 253 applies to every quantizer name. A missing input_quantizer or output_quantizer entry, which previously failed loudly, is now skipped without any signal, and the module restores partially with default quantizer properties.
Keep the fallback for per-expert names and keep a loud failure for the rest.
🐛 Proposed fix
quantizer_substate = quantizer_state.get(name)
if quantizer_substate is None:
# Per-expert quantizers ("weight_quantizer.<i>") are saved per EP rank, so a
# module loaded at smaller EP (e.g. EP1 export from an EP16 ckpt) has more
# experts than the saved state. Per-expert properties are uniform across
# experts (amax rides separately as globally-indexed sharded tensors), so
# fall back to expert 0's state. Rewrite only the expert index right after
# weight_quantizer, preserving deeper suffixes (e.g. a SequentialQuantizer level
# weight_quantizer.<i>.<lvl>); non-expert names are left unchanged.
fallback = re.sub(r"(weight_quantizer)\.\d+", r"\1.0", name)
+ if fallback == name:
+ raise KeyError(
+ f"Quantizer state for {name} is missing from the checkpoint."
+ )
quantizer_substate = quantizer_state.get(fallback)
if quantizer_substate is None:
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| quantizer_substate = quantizer_state.get(name) | |
| if quantizer_substate is None: | |
| # Per-expert quantizers ("weight_quantizer.<i>") are saved per EP rank, so a | |
| # module loaded at smaller EP (e.g. EP1 export from an EP16 ckpt) has more | |
| # experts than the saved state. Per-expert properties are uniform across | |
| # experts (amax rides separately as globally-indexed sharded tensors), so | |
| # fall back to expert 0's state. Rewrite only the expert index right after | |
| # weight_quantizer, preserving deeper suffixes (e.g. a SequentialQuantizer level | |
| # weight_quantizer.<i>.<lvl>); non-expert names are left unchanged. | |
| fallback = re.sub(r"(weight_quantizer)\.\d+", r"\1.0", name) | |
| quantizer_substate = quantizer_state.get(fallback) | |
| if quantizer_substate is None: | |
| continue | |
| quantizer_substate = quantizer_state.get(name) | |
| if quantizer_substate is None: | |
| # Per-expert quantizers ("weight_quantizer.<i>") are saved per EP rank, so a | |
| # module loaded at smaller EP (e.g. EP1 export from an EP16 ckpt) has more | |
| # experts than the saved state. Per-expert properties are uniform across | |
| # experts (amax rides separately as globally-indexed sharded tensors), so | |
| # fall back to expert 0's state. Rewrite only the expert index right after | |
| # weight_quantizer, preserving deeper suffixes (e.g. a SequentialQuantizer level | |
| # weight_quantizer.<i>.<lvl>); non-expert names are left unchanged. | |
| fallback = re.sub(r"(weight_quantizer)\.\d+", r"\1.0", name) | |
| if fallback == name: | |
| raise KeyError( | |
| f"Quantizer state for {name} is missing from the checkpoint." | |
| ) | |
| quantizer_substate = quantizer_state.get(fallback) | |
| if quantizer_substate is None: | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelopt/torch/quantization/plugins/megatron.py` around lines 241 - 253,
Restrict the missing-state skip in the quantizer restoration flow to names
matching the per-expert `weight_quantizer.<i>` pattern handled by the fallback
in `quantizer_substate`. For non-expert names such as `input_quantizer` and
`output_quantizer`, preserve the previous loud failure behavior instead of
continuing with default properties.
| if random_offset: | ||
| max_off = min(seq_length, max(0, len(token_stream) - num_rows * seq_length)) | ||
| if max_off > 0: | ||
| token_stream = token_stream[random.randint(0, max_off):] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Seed the offset RNG so ranks agree and runs reproduce.
random.randint uses the unseeded global RNG. get_megatron_calibration_dataloader builds this dataset independently on every rank and then shards it with a DistributedSampler that assumes an identical dataset on all ranks. With random_offset=True, each rank drops a different number of leading tokens, so the same sampler index maps to different rows on different ranks. The calibration set then contains duplicated and missing windows, and the run is not reproducible.
Derive the offset from a fixed seed, as the multi-source shuffle at line 870 already does.
🛠️ Proposed fix
if random_offset:
max_off = min(seq_length, max(0, len(token_stream) - num_rows * seq_length))
if max_off > 0:
- token_stream = token_stream[random.randint(0, max_off):]
+ # Fixed seed: every rank must derive the same offset, otherwise the
+ # DistributedSampler shards a different token grid per rank.
+ token_stream = token_stream[random.Random(0).randint(0, max_off) :]Note: the avoid-random-python static-analysis hint on this line is a false positive. This offset is calibration sampling, not a security value.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if random_offset: | |
| max_off = min(seq_length, max(0, len(token_stream) - num_rows * seq_length)) | |
| if max_off > 0: | |
| token_stream = token_stream[random.randint(0, max_off):] | |
| if random_offset: | |
| max_off = min(seq_length, max(0, len(token_stream) - num_rows * seq_length)) | |
| if max_off > 0: | |
| # Fixed seed: every rank must derive the same offset, otherwise the | |
| # DistributedSampler shards a different token grid per rank. | |
| token_stream = token_stream[random.Random(0).randint(0, max_off) :] |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 717-717: use secrets package over random package
Context: random.randint(0, max_off)
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelopt/torch/utils/dataset_utils.py` around lines 715 - 718, Update the
random_offset handling in get_megatron_calibration_dataloader to derive the
leading-token offset from a fixed, shared seed rather than the unseeded global
random.randint call. Reuse the existing deterministic seed approach used by the
multi-source shuffle so every rank produces the identical token_stream and runs
remain reproducible.
Source: Linters/SAST tools
| mtq.quantize(te_grouped, quant_cfg, forward) | ||
| mtq.quantize(sequential, quant_cfg, forward) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Deep-copy quant_cfg before each mtq.quantize call.
quant_cfg is a module-level constant supplied by @pytest.mark.parametrize (mtq.FP8_DEFAULT_CFG / mtq.NVFP4_DEFAULT_CFG). The other call sites in this file pass copy.deepcopy(...) for that reason (Line 749, Line 817, Line 884). Here the same object is passed twice and is shared with every other parametrized test in the session. If mtq.quantize mutates the config, the second call and later tests observe the mutation.
The same applies to Line 1129 in _test_te_grouped_sharded_state_dict_global_expert_identity_helper.
🐛 Proposed fix
- mtq.quantize(te_grouped, quant_cfg, forward)
- mtq.quantize(sequential, quant_cfg, forward)
+ mtq.quantize(te_grouped, copy.deepcopy(quant_cfg), forward)
+ mtq.quantize(sequential, copy.deepcopy(quant_cfg), forward)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/gpu_megatron/torch/quantization/plugins/test_megatron.py` around lines
1026 - 1027, Deep-copy quant_cfg independently before each mtq.quantize call in
the affected test and in
_test_te_grouped_sharded_state_dict_global_expert_identity_helper, so neither
invocation nor later parametrized tests share a potentially mutated
configuration. Follow the existing copy.deepcopy pattern used by the nearby
quantization call sites.
| ^^^^^^^^^^^^^^^^^ | ||
|
|
||
| - Add the ``nvfp4_act_headroom`` calibration algorithm for NVFP4 **activation** global scales. Plain ``max`` sets a tensor's global scale from the largest per-block amax seen during calibration, leaving no room above it, so any activation larger than the calibration max saturates. ``nvfp4_act_headroom`` instead anchors the global scale to a low percentile of the per-block amax distribution, ``amax = max(rho * anchor, upper)``, placing the calibrated blocks in the lower part of the FP8 block-scale range and leaving the rest as headroom. ``upper`` is the top of the range the scale commits to representing and defaults to the 99.99th percentile rather than the literal maximum: chasing a lone freak block would drag the global scale up until every other block's FP8 block scale falls below subnormal and flushes to zero, so the rarest blocks are clipped instead. Set ``upper_percentile=100`` to use the literal observed max, which guarantees no calibration data is clipped. The calibrator warns when the per-block range is too wide for ``rho`` to clear any headroom. Tunable via ``anchor_percentile`` (default 1), ``upper_percentile`` (default 99.99) and ``rho`` (default 16384). Applies only to NVFP4 dynamic-block input quantizers. Weight scales are an orthogonal axis, selected by a nested ``weight_scale_algorithm`` (``max`` by default, or ``mse`` / ``local_hessian`` with that algorithm's own options), so one recipe can combine a weight calibration with this activation policy in a single pass. ``SequentialQuantizer`` activation quantizers are not supported and raise. Ships ``modelopt_recipes/general/ptq/nvfp4_act_headroom-kv_fp8_cast.yaml``, which mirrors ``nvfp4_default-kv_fp8_cast`` (dynamic NVFP4 W4A4 plus FP8 KV-cache cast) with only the calibration algorithm swapped, so it exports a standard NVFP4 checkpoint. | ||
| - Add per-expert weight quantization for Transformer Engine ``TEGroupedMLP`` (fused MoE experts): each expert now has its own ``weight_quantizer`` (a ``GroupedQuantizer`` holding one ``TensorQuantizer`` per expert) with an independent ``amax``, instead of a single shared ``amax`` across all experts. Applies to ``mtq.quantize`` calibration, HF / Megatron export, and QAD. |
There was a problem hiding this comment.
this is a copy from my PR #1550, you can remove this as you will rebase on my later
| if shape_ok: | ||
| continue # loaded amax is valid -> keep it, do NOT recompute | ||
| # Recompute is lossy for static recipes; never do it silently. | ||
| warnings.warn( |
There was a problem hiding this comment.
please remove any duplicate code between this PR and #1550 as you will rebase on top of it
What does this PR do ?
Type of change: Bug fix + integration validation
Overview: Adopts the per-expert
TEGroupedMLPquantization from #1550 for theNemotron-Nano-3 W4A16 NVFP4
four_over_sixrecipe, and fixes one bug that blocks it.Until now this recipe had to force
SequentialMLP(non-grouped experts), becauseTEGroupedLinear._process_quantizer_amaxassertedv.numel() == 1— i.e. per-tensor only —while
four_over_sixis static per-block NVFP4 whose_amaxhas one entry per block.With #1550 that restriction is gone, so the grouped path now works and
SequentialMLPis nolonger required for this recipe.
The fix
_QuantTEGroupedLinear._setup()builds the new per-expertGroupedQuantizerfromself.default_quant_desc_weight, but that attribute does not resolve:_ParallelLinearderivesfrom
QuantModule, not_QuantLinear, and_ParallelLinear._setup()itself references the classattribute directly (
plugins/custom.py). Building any grouped-experts model fails with:Changed to
_QuantLinear.default_quant_desc_weight, matching the non-grouped path.Validation — grouped vs non-grouped, end to end
Nemotron-Nano-3, W4A16 NVFP4
four_over_six, TEGroupedMLP, 4x GB200:seq_load_balancing_losssteady ~1.013All enabled NVFP4 weight quantizers have calibrated scaleslm_head.weight_packed U8 [131072, 1344], 72 exclusionsThe grouped export is structurally indistinguishable from the SequentialMLP one:
W4A16_NVFP4,num_bits=4,block=16,dynamic=False,ignore=72experts.0.up_proj.weight_scale [1856, 168]anddown_proj.weight_scale [2688, 116](168 = 2688/16, 116 = 1856/16)
Accuracy of the grouped path vs SequentialMLP has not been measured yet; only structural
equivalence and serving are verified here.
Stacking
This branch is stacked and should merge after both:
jennifchen/te_per_expert) — the per-expertGroupedQuantizerthis builds onlm_headquantization fix for Megatron-BridgeThe diff against
maintherefore includes commits from both. Our own commits are:72d3cb5e7— from feat(megatron-bridge): Nemotron-Nano-3 W4A16 NVFP4 four_over_six PTQ/QAD support #2055c53ee3041— merge of Support per expert weight quantizer in TEGroupedMLP #1550, resolving two conflicts (below)5f2f3de3c— thedefault_quant_desc_weightfixMerge conflict resolutions
quantization/plugins/megatron.py— Support per expert weight quantizer in TEGroupedMLP #1550 registers ModelOpt extra-state callbacks for everyQuantModule, which subsumes the narroweroutput_layerspecial-case from feat(megatron-bridge): Nemotron-Nano-3 W4A16 NVFP4 four_over_six PTQ/QAD support #2055. Took Support per expert weight quantizer in TEGroupedMLP #1550'sversion; the
_resolve_output_layer_untiedhelper and thesharded_state_dictbuffermaterialisation from feat(megatron-bridge): Nemotron-Nano-3 W4A16 NVFP4 four_over_six PTQ/QAD support #2055 are unaffected and still used.
examples/megatron_bridge/distill.py— kept both validation blocks, and took Support per expert weight quantizer in TEGroupedMLP #1550'smost_recent_k=args.checkpoint_keep_last. Note its default is 5; checkpoints here are ~413 GBeach, so pass
--checkpoint_keep_last 2on constrained filesystems.Usage
Testing
Full PTQ -> QAD -> export -> compressed-tensors -> vLLM serving on Nemotron-Nano-3, as tabulated
above. The
_QuantTEGroupedLinearchange is exercised by every grouped-experts run; without it,_setup()raises before any model can be built.Before your PR is "Ready for review"
--grouped_experts; the default remains non-grouped.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes