Add --muon-use-syrk option for Triton SYRK kernel in Newton-Schulz - #6381
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
0a052d8 to
a7703f2
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
Replace the inspect.signature probe of newton_schulz_tp with a version check, following the get_*_version / is_*_min_version pattern already used for TE, mamba_ssm, causal_conv1d and flashinfer. The SYRK kernel landed in emerging_optimizers 0.3.0. Raise instead of warning when --muon-use-syrk is requested against an older release. Silently falling back to the standard GEMM made a performance flag look like it had taken effect when it had not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
f2963ae to
a5e91d4
Compare
|
Addressed the review feedback and rebased onto latest @claude's two items:
Correction to the version number. The gate is Note |
|
/claude review |
There was a problem hiding this comment.
Light review. Wiring looks correct — I traced muon_use_syrk -> use_syrk through _kwargs_from_config, and confirmed the second _kwargs_from_config(TensorParallelAdaptiveMuon, "adaptive_muon", config) in _adaptive_muon_config_to_kwargs does not clobber it (neither adaptive_muon_use_syrk nor use_syrk exists on the config, so the key is absent from the update dict). Two test-coverage gaps:
1. The positive path — the actual behavior change — is untested.
Both new tests only exercise the rejection branch. Nothing asserts that use_syrk=True actually reaches newton_schulz_tp, which is the entire point of the flag. Combined with the rev = "v0.2.0" pin, the ns_kwargs forwarding in scaled_orthogonalize_fn (megatron/core/optimizer/emerging_optimizers.py:221-227) will never execute in CI — a typo like {"syrk": True} would pass every test in this PR.
Suggest a test that patches the gate open and captures the kwargs:
@pytest.mark.parametrize("use_syrk,expected", [(True, True), (False, None)])
def test_muon_use_syrk_forwarded_to_newton_schulz(monkeypatch, use_syrk, expected):
"""use_syrk=True must reach newton_schulz_tp; use_syrk=False must not pass the kwarg."""
import megatron.core.optimizer.emerging_optimizers as eo_module
monkeypatch.setattr(eo_module, "is_emerging_optimizers_min_version", lambda _v: True)
captured = {}
def fake_newton_schulz_tp(grad, **kwargs):
captured.update(kwargs)
return grad
monkeypatch.setattr(eo_module, "newton_schulz_tp", fake_newton_schulz_tp)
model = torch.nn.Linear(60, 30, bias=False, dtype=torch.float32, device="cuda")
optimizer = TensorParallelMuon(
params=[model.weight], lr=0.01, pg_collection=None, tp_mode="duplicated",
use_syrk=use_syrk,
)
model(torch.randn(8, 60, device="cuda")).sum().backward()
optimizer.step()
assert captured.get("use_syrk") is expectedThe (False, None) case also pins the "do not forward the kwarg at all when disabled" contract that the new comment above ns_kwargs describes.
2. The new megatron/core/utils.py helpers have zero coverage.
get_emerging_optimizers_version() / is_emerging_optimizers_min_version() are never exercised — both new tests monkeypatch is_emerging_optimizers_min_version out of emerging_optimizers.py entirely, and that whole test file is pytest.mark.skipif-ed on LTS and when the package is missing. So the check_equality=False branch and the hyphenated-distribution-name fallback (version("emerging-optimizers")) are unverified.
tests/unit_tests/test_utils.py:57 already has the right shape for this (test_is_flashinfer_min_version), and putting it there gets it out from under the LTS skip:
@pytest.mark.skipif(not util.HAVE_PACKAGING, reason="packaging is not installed")
@pytest.mark.parametrize("check_equality", [True, False])
def test_is_emerging_optimizers_min_version(check_equality):
from packaging.version import Version as PkgVersion
with patch.object(util, "get_emerging_optimizers_version", return_value=PkgVersion("0.4.0")):
assert util.is_emerging_optimizers_min_version("0.4.0.dev0", check_equality=check_equality) is True
assert util.is_emerging_optimizers_min_version("0.5.0", check_equality=check_equality) is False
assert (
util.is_emerging_optimizers_min_version("0.4.0", check_equality=check_equality)
is check_equality
)Nothing blocking beyond that — raising rather than silently falling back is the right call, and the get_*_version helper matches the existing is_mamba_min_version / is_causal_conv1d_min_version pattern (including letting the ImportError propagate when the package is absent).
|
/ok to test a5e91d4 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31429558540 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31438323017 |
…VIDIA#6381) Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Upstream already ships --muon-use-syrk (via newton_schulz_tp, NVIDIA#6381); the cherry-picked wiring commit brought a second dataclass field of the same name, which dataclasses resolve silently last-wins. Keep one field with the layer-sharded docstring, note the TP entry point's emerging-optimizers >= 0.4.0 floor, and generalize the CLI help: the flag now covers every Muon mode, not just layer sharding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: wanyingw <wanyingw@nvidia.com>
Upstream already ships --muon-use-syrk (via newton_schulz_tp, NVIDIA#6381); the cherry-picked wiring commit brought a second dataclass field of the same name, which dataclasses resolve silently last-wins. Keep one field with the layer-sharded docstring, note the TP entry point's emerging-optimizers >= 0.4.0 floor, and generalize the CLI help: the flag now covers every Muon mode, not just layer sharding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: wanyingw <wanyingw@nvidia.com>
Upstream already ships --muon-use-syrk (via newton_schulz_tp, NVIDIA#6381); the cherry-picked wiring commit brought a second dataclass field of the same name, which dataclasses resolve silently last-wins. Keep one field with the layer-sharded docstring, note the TP entry point's emerging-optimizers >= 0.4.0 floor, and generalize the CLI help: the flag now covers every Muon mode, not just layer sharding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: wanyingw <wanyingw@nvidia.com>
Summary
Adds an opt-in
--muon-use-syrkflag that routes the Gram-matrix computation inside Newton-Schulz through the Triton SYRK kernel inemerging_optimizers. The Gram matrixX @ X.Tis symmetric, so SYRK computes roughly half the FLOPs of the equivalent GEMM.Off by default, so behavior is unchanged unless the flag is set.
Wiring
OptimizerConfig.muon_use_syrk(defaultFalse).--muon-use-syrkin_add_regularization_args.use_syrkparameter onTensorParallelMuon.__init__andTensorParallelAdaptiveMuon.__init__.No plumbing code is needed between the config and the optimizer:
_kwargs_from_configalready mapsmuon_<name>config fields onto matching__init__parameters, somuon_use_syrkreachesuse_syrkautomatically.Version compatibility
newton_schulz_tp— the only Newton-Schulz entry point Megatron calls — gained theuse_syrkkwarg in emerging_optimizers 0.4.0:newton_schulz_tpacceptsuse_syrkNote that
use_syrkappears on the non-TPnewton_schulzas far back as v0.2.0, so the presence of the name inmuon_utils.pyis not a reliable signal; only the TP variant matters here.The check is a version gate built on a new
get_emerging_optimizers_version()/is_emerging_optimizers_min_version()pair inmegatron/core/utils.py, following the existingis_te_min_version/is_mamba_min_version/is_causal_conv1d_min_versionhelpers. The minimum is spelled0.4.0.dev0so pre-release builds of that line are accepted, matching how the TransformerEngine minimums are written elsewhere in the tree.Requesting
--muon-use-syrkagainst an older release raisesValueErrorat optimizer construction rather than warning and silently falling back. A perf flag that appears to take effect but doesn't is worse than a hard failure, and the fallback made it impossible to tell from the logs whether SYRK was actually running.Dependency pin
pyproject.tomlcurrently pinsemerging_optimizersatrev = "v0.2.0", so--muon-use-syrkwill raise under the locked dependency until that pin moves to v0.4.0. This is intentional for now: the flag is opt-in and aimed at runs on newer builds, and bumping the pin pulls in everything else that changed across 0.2.0 → 0.4.0 plus auv.lockregeneration, which belongs in its own PR.