[Dev] Add SiTU-GLU through Transformer Engine - #6673
Conversation
c4f2ae5 to
59e5675
Compare
59e5675 to
75e87cb
Compare
75e87cb to
5770c65
Compare
yuzhongw-nvidia
left a comment
There was a problem hiding this comment.
LGTM. Thanks for your outstanding work!
5770c65 to
4edf750
Compare
|
/ok to test 4edf750 |
|
/claude review |
| if use_glu_fusion and self.activation_recompute: | ||
| return _unsupported( | ||
| "Transformer Engine scaled GLUs do not support activation recompute" | ||
| ) |
There was a problem hiding this comment.
This gate is broader than SiTU-GLU and looks like an unintended regression for the existing SwiGLU/quick-GEGLU paths.
use_glu_fusion is true for F.silu and quick_gelu as well, so any run that combines --use-transformer-engine-op-fuser with --recompute-modules [... moe_act] now reports unsupported. Since __init__ does assert self._is_fused_impl_supported() when use_transformer_engine_op_fuser is set (experts.py:277-280), that turns into a hard failure for configurations that work on main today.
It also contradicts _make_fused_ops a few lines below, which still computes activation_recompute_in_mlp and forwards it to ScaledSwiGLU / ScaledClampedQGeGLU / ScaledSReLU when TE exposes the parameter — that plumbing becomes dead code.
Only ScaledSiTUGLU is constructed without an activation_recompute_in_mlp kwarg, so scoping the check to SiTU-GLU keeps the new activation safe without changing existing behavior:
| if use_glu_fusion and self.activation_recompute: | |
| return _unsupported( | |
| "Transformer Engine scaled GLUs do not support activation recompute" | |
| ) | |
| if self.config.activation_func is situlu and self.activation_recompute: | |
| return _unsupported( | |
| "Transformer Engine ScaledSiTUGLU does not support activation recompute" | |
| ) |
There was a problem hiding this comment.
Fixed in 74aafeb52. The recompute guard is now scoped only to situlu/ScaledSiTUGLU. Existing SwiGLU and quick-GEGLU paths retain their prior activation-recompute plumbing, with a parametrized regression test that sets activation_recompute=True for both. The final focused suites pass on dev (41 tests) and main (60 tests).
| @jit_fuser | ||
| def situlu(x: torch.Tensor, beta1: float = 4.0, beta2: float = 25.0) -> torch.Tensor: |
There was a problem hiding this comment.
jit_fuser resolves to torch.compile on torch >= 2.2 (megatron/core/jit.py:19-21), so situlu is a compiled callable. Two consequences worth checking:
-
Every other function in this file is unary;
situluis the first one taking extra float args. The three call sites pass betas positionally (situlu(x, beta1, beta2)), whichtorch.compilehandles, but the default values4.0/25.0become recompile-triggering guards rather than free constants — worth confirming a config with non-default betas doesn't cause a recompile per call. -
Identity comparisons (
self.activation_func is situlu,config.activation_func == situlu) are load-bearing throughout this PR. They compare against the decorated wrapper, which is consistent as long as nothing re-bindssituluafterdisable_jit_fuser()/enable_jit_fuser()runs.megatron/core/jit.pyrebinds the module-globaljit_fuser, not previously-decorated functions, so this holds — just flagging that the whole feature's dispatch rests on it.
There was a problem hiding this comment.
Confirmed. jit_fuser decorates situlu once, and toggling the module-global fuser does not rebind that callable, so the identity-based dispatch remains stable. The beta values are configuration invariants: each call site reuses the same pair for the lifetime of a config/module; a different config may compile a separate specialization, but the values do not vary per invocation. Non-default betas remain covered by the configuration/op-selection tests, so no code change was needed for this observation.
There was a problem hiding this comment.
Light review. Nice, thorough PR — validation coverage and the config gating are good. Two inline comments (one likely regression), plus the notes below.
1. --situ-glu unconditionally forces use_te_activation_func=True
megatron/training/arguments.py:2248-2252 and the duplicate in megatron/training/argument_utils.py:431-435:
if use_situ_glu:
kw_args['activation_func'] = situlu
kw_args['gated_linear_unit'] = True
kw_args['use_te_activation_func'] = True
kw_args['bias_activation_fusion'] = FalseThis makes the PyTorch reference path unreachable from the CLI, even though the PR description says "other paths use the PyTorch reference" and test_transformer_config_accepts_pytorch_situ_glu_fallback explicitly covers use_te_activation_func=False. It also silently overrides use_te_activation_func for anyone who set it via --transformer-impl local style configs, and gpt_layer_specs.py:237-238 raises AssertionError("use_te_activation_func not compatible with using kitchen") — so --situ-glu plus kitchen becomes an unexplained assertion rather than a clear message.
Consider not forcing the flag, and letting the existing TransformerConfig.__post_init__ validation handle the rest:
if use_situ_glu:
kw_args['activation_func'] = situlu
kw_args['gated_linear_unit'] = True
kw_args['bias_activation_fusion'] = False2. Checkpoint converters in tools/checkpoint/ are not covered
megatron/training/checkpointing.py was updated for the FSDP-DTensor path, but the tools/checkpoint/ converters still key on md.swiglu alone:
tools/checkpoint/loader_base.py:446—md.swiglu = self.margs.swiglutools/checkpoint/loader_base.py:321,337— chunksmlp_l0_weight/mlp_l0_biasinto W/V halves onlyif self.md.swiglutools/checkpoint/saver_base.py:539,573— the matching re-concat
A SiTU-GLU checkpoint has the same doubled FC1 layout, so converting one through these paths will take the non-gated branch and produce a silently wrong mlp l0 weight. The PR description says "Extend checkpoint conversion ... to treat SiTU-GLU as a gated GLU," which is done for checkpointing.py but not here. Suggested fix in loader_base.py:
md.swiglu = self.margs.swiglu or getattr(self.margs, "situ_glu", False)That keeps all downstream md.swiglu branches (loader + saver) correct without touching them individually.
3. Pre-existing bug reinforced in TEActivationOp
Not introduced here, but the new elif is now guarded behind it. megatron/core/extensions/transformer_engine.py:481-486:
if config.activation_func == F.silu:
layer_type = te.pytorch.ops.SwiGLU
elif config.activation_func == F.gelu:
layer_type = te.pytorch.ops.GEGLU
elif config.activation_func == F.silu: # <-- duplicate; should be F.relu -> ReGLU
layer_type = te.pytorch.ops.ReGLU
elif config.activation_func is situlu:The third branch repeats F.silu so ReGLU is dead (same duplication in the non-gated else block at line 497-498 for ReLU). The situlu branch is fine because it comes after, but the error message this PR edits now advertises ReGLU support that cannot be reached. Fixing the two F.silu typos to F.relu would be a small, contained cleanup if you want it in scope.
4. Test coverage note
Unit coverage for the op selection and config validation is solid. Two gaps:
- No functional test.
tests/functional_tests/test_cases/moe/gpt3_mcore_te_tp1_pp1_te_4experts_groupedGEMM_op_fuser/is the natural template (it already setsNVTE_CUTEDSL_FUSED_GROUPED_MLP: 1and--use-transformer-engine-op-fuser: true); a sibling case swapping--swiglu: truefor--situ-glu: truewould exercise the routed-expert path end to end. Understandable to defer while the TE dependency (TransformerEngine#3402) is unmerged — worth a note in the PR body so it isn't lost. test_situlu_reference_matches_kimi_bf16_precision_and_backwardasserts withrtol=0, atol=0against an expression rebuilt from the same ops in the same order. That pins the FP32-intermediate/BF16-output contract, which is the stated intent, but it will not catch a formula error since the reference mirrors the implementation. A handful of hand-computed expected values at a few points (including large-magnitude inputs where thebeta2soft-cap saturates) would make it a real correctness check against Equation 12.
Signed-off-by: Harry Zhou <hhanyu@nvidia.com>
4edf750 to
74aafeb
Compare
|
Addressed the current review findings on both SiTU-GLU PRs in
I kept the CLI's TE selection intentional: stock SiTU-GLU paths require the TE operators, while Final focused GPU suites on NVIDIA B300: dev 41 passed; main 60 passed. |
| state_dict = raw_state_dict.copy() | ||
| handle_fp8_extra_state_case(state_dict["model"]) | ||
| if args.swiglu: | ||
| if args.swiglu or getattr(args, "situ_glu", False): |
There was a problem hiding this comment.
--use-checkpoint-args does not currently restore situ_glu. load_args_from_checkpoint() restores squared_relu and swiglu, but never calls _set_arg("situ_glu", force=True). As a result, loading a SiTU-GLU checkpoint with --use-checkpoint-args reconstructs it as a non-gated MLP unless the user also repeats --situ-glu, which leads to an FC1 shape mismatch. Could we restore situ_glu from the checkpoint args as well?
What does this PR do?
Add Kimi K3 SiTU-GLU as a global FFN activation on
dev.Paired main-target PR: #6674
Activation
For gate projection
G, up projectionU, and K3 defaultsbeta1=4,beta2=25:Reference: Kimi K3 technical report, Figure 4 and Equation 12.
Design
--situ-glu, with--moe-use-situ-gluas an alias, and apply it consistently to dense, routed-expert, and shared-expert FFNs.situ_glu_beta1=4andsitu_glu_beta2=25toTransformerConfig, with validation that SiTU-GLU is gated, unclamped, uses zero GLU linear offset, and has finite positive beta values.situluas the correct PyTorch pointwise reference and configuration marker. It serves non-TE/custom module paths until PyTorch provides a dedicatedtorch.nn.functional.situlu-style operation; unaryF.siluis not equivalent.transformer_engine.pytorch.ops.SiTUGLUfor ordinary dense, sequential-expert, and ordinary shared-expert paths.transformer_engine.pytorch.ops.ScaledSiTUGLUfor fused grouped dense, routed-expert, and shared-expert paths.The required TE interface is available in NVIDIA/TransformerEngine#3402.
With that TE interface, an older cuDNN frontend without SiTU parameters uses TE's unfused
GroupedLinear -> ScaledSiTUGLU -> GroupedLinearoperation sequence. A SiTU-capable cuDNN frontend enables fused grouped GEMM + SiTU-GLU without an MCore-side backend check.Test results
Validated on NVIDIA B300 with NVIDIA/TransformerEngine#3402 and cuDNN Frontend develop through NVIDIA/cudnn-frontend#670, using CuTe DSL 4.6.2:
ScaledSiTUGLU: passed;ScaledSiTUGLU, with the fused cuDNN/CuTe grouped-MLP operation asserted: passed;ScaledSiTUGLU, with the fused Hadamard forward operation asserted: passed;git diff --check, Python compilation, and Ruff on all changed source and test files: passed;Fused-shared NVFP4 currently reaches an existing TE FC1 weight-scale reshape failure that reproduces with an equivalent SwiGLU control, so this PR does not claim that combination. MXFP8 dSiTU validation used the cuDNN frontend correction included in NVIDIA/cudnn-frontend#670; the published cuDNN frontend 1.27.0 package tested here does not yet contain that correction.
Scope and dependencies
This PR depends on NVIDIA/TransformerEngine#3402 for TE-backed SiTU-GLU functionality. The cuDNN frontend support is optional for correctness and required only for fused block-scaled grouped execution. A functional test is deferred until that TE interface is available in Megatron-LM CI; the focused MCore unit and GPU suites cover the current integration boundary.
This PR is independent of Quantile Balancing and #6637.
Issue tracking
For PRs from open-source community contributors:
Linked issue: TODO — link a Megatron-LM feature-request issue before marking this draft ready for review.
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.