[AutoEP]Fix optimizer and replaced MOE parameter mismatch - #8377
Conversation
Signed-off-by: pengdurice <pengduhit@gmail.com>
Signed-off-by: pengdurice <pengduhit@gmail.com>
Signed-off-by: pengdurice <pengduhit@gmail.com>
Signed-off-by: pengdurice <pengduhit@gmail.com>
Signed-off-by: pengdurice <pengduhit@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc3c948431
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| import hashlib | ||
| import logging | ||
| from collections import defaultdict, OrderedDict, deque | ||
| from collections import Counter, defaultdict, OrderedDict, deque |
There was a problem hiding this comment.
Add the required Signed-off-by trailer
This non-merge commit has no Signed-off-by trailer, so it violates the repository's mandatory commit policy and may fail DCO/CI validation. Recreate the commit with --signoff using the configured Git identity.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| unoptimized = 0 | ||
| for param in missing: | ||
| sources = replacement_sources.get(id(param), ()) | ||
| source_groups = sorted({group_of_source[id(s)] for s in sources if id(s) in group_of_source}) |
There was a problem hiding this comment.
Reject partially optimized grouped expert sources
When module_list storage packs several local experts into one replacement tensor and the client optimizer contains only some of those source weights, this filtering produces one nonempty group and assigns the entire grouped tensor there. The previously excluded experts then receive gradients and optimizer updates, silently defeating selective-expert fine-tuning; detect sources missing from all groups and reject this partially represented mapping rather than treating it as unambiguous.
Useful? React with 👍 / 👎.
| missing = [p for p in missing if id(p) in placement] | ||
|
|
||
| for group in param_groups: | ||
| group["params"] = [p for p in group["params"] if id(p) in live] |
There was a problem hiding this comment.
Preserve unrelated external optimizer parameters
When AutoEP performs any replacement under ZeRO-0 and a valid caller-supplied optimizer also owns a parameter outside model.parameters()—for example, an auxiliary trainable loss parameter—this filter removes that parameter merely because it is not in the module tree; the following state cleanup also discards its optimizer state. Restrict removal to parameters known to belong to discarded AutoEP modules instead of every non-model parameter.
Useful? React with 👍 / 👎.
Signed-off-by: pengdurice <pengduhit@gmail.com>
Signed-off-by: Masahiro Tanaka <tanaka.masahiro@gmail.com>
tohtana
left a comment
There was a problem hiding this comment.
Hi @pengdurice, thank you for the fix!
The direction looks good, and I left a few comments about some cornor cases.
I opened a PR to clarify the issues and share suggested fix: pengdurice#1
Feel free to refer to it when you think it is useful.
…optimizer-remap Fix AutoEP optimizer remap edge cases
tohtana
left a comment
There was a problem hiding this comment.
Thank you for accepting the PR. This looks good to me.
Fix: AutoEP silently drops expert parameters from a caller-supplied optimizer
Problem
_configure_expert_parallel(engine.py) replaces every MoE module, and it runs before_configure_optimizer. Nothing remaps optimizer param groups in between —set_optimizer_flagsonly sets Muon flags.torch.optim.Optimizer.__init__materialises its argument eagerly (param_groups = list(params)),so an optimizer the caller built from
model.parameters()keeps hard references to the discardedexpert tensors, while the live
GroupedExpertsweights belong to no param group.This is the path HF Trainer and Accelerate take whenever the DeepSpeed config declares no
optimizerblock (transformers/integrations/deepspeed.py:optimizer = trainer.create_optimizer()in the
elsebranch).Two symptoms, same cause:
zero.Init(zero3_init_flag: true)zero.InitAttributeError: 'Parameter' object has no attribute 'partition_numel'from_create_fp16_sub_groups(stage3.py), because the stale params were never ZeRO-converted.Measured on a 2-layer / 4-expert model, ZeRO-3,
autoep_size=2, one step at lr=0.1:The same run with the optimizer declared in
ds_configmoves every tensor by1.005e-01.Fix
_remap_client_optimizer_after_module_replacement, called immediately after_configure_expert_parallel:DummyOptim/ config-built paths return early);so per-group hyper-parameters (e.g. a weight-decay split) survive;
optimizer.stateentries, which Adam keys on the parameter object;assignment would silently mis-apply weight decay.
Tests
New
tests/unit/v1/moe/test_autoep_client_optimizer.py,world_size = 2:test_client_optimizer_covers_replacement_parameters— every trainable param is in a param group;replacement actually happened; no optimized param is detached from the module.
test_client_optimizer_updates_expert_weights— after one real step, no parameter isbit-identical. This is the test that catches the silent freeze.
test_client_optimizer_preserves_param_group_hyperparameters— with decay / no-decay groups,replacement params land in the right group and both
weight_decayvalues survive.3 passed with the fix; 3 failed with
engine.pyreverted and everything else identical.The tests use bf16 rather than the shared
mixed_precision_config()helper: fp16 carries a lossscaler that skips the first optimizer step on overflow, which would leave every parameter
unchanged and make the update test vacuous.
Scope
ZeRO-3 only. A caller-supplied optimizer with MoE on ZeRO-1/2 additionally requires param groups
marked
{"moe": True}(stage_1_and_2.py:780,bf16_optimizer.py:128) — a separate pre-existingrequirement, not addressed here.
Why this was not caught
Every existing AutoEP test supplies the optimizer through
ds_config(
"optimizer": {"type": "Adam"}inmake_autoep_config), so DeepSpeed builds it afterreplacement and the client-optimizer path is never exercised.