moe: the shared-experts-fusion decision is a per-runner value the loader installs - #33889
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21e986251a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ch-wan
left a comment
There was a problem hiding this comment.
Summary
The per-runner MoE flag design (ACTIVE + speculative twin, bracketed by speculative_moe_backend_context, both-ways record_shared_experts_fusion_decision, forward-time read refusal) is sound and matches the series goal of finishing config-mutation closeout. Most model gates and layer reads migrate cleanly, and the new isolation tests cover the intended contracts. However, the prior codex finding on GLM-4 NextN is a real regression: glm4_moe_nextn.py was not migrated with glm4_moe_lite_nextn.py, so top-level weight-loader metadata can still read config intent while the decoder bakes the ACTIVE decision — a fused/unfused mismatch after target auto-disable.
Issue counts by severity
- bugs: 1
- suggestions: 2
- nits: 1
Issues outside the diff
These findings reference lines that are not present in the diff and could not be posted as inline comments:
-
[bug] python/sglang/srt/models/glm4_moe_nextn.py:145 --
Glm4MoeForCausalLMNextNstill sets top-levelself.num_fused_shared_expertsfromget_exec().moe.disable_shared_experts_fusion(config bag / user intent). This PR stops writing auto-disable decisions to the bag and only records them on the ACTIVE moe flag. The decoder path (Glm4MoeDecoderLayer) is updated tois_shared_experts_fusion_disabled(), andglm4_moe_lite_nextn.pywas fixed the same way, but the non-lite NextN twin was not. After a target gate auto-disables fusion (EP, quant, capability, deepep, …), bag intent can remain False while ACTIVE is True: the NextN decoder builds unfused (shared_expertsmodule, no fused expert slot), thenload_weights(viaGlm4MoeForCausalLM.load_weightswhenself.num_fused_shared_experts > 0) remapsmlp.shared_experts.*→mlp.experts.{n_routed}.*as if fused. That is a real weight-load / structure mismatch for GLM-4.5/4.6/4.7 MTP drafts.- Suggestion: Mirror
glm4_moe_lite_nextn.py: setself.num_fused_shared_expertsfromis_shared_experts_fusion_disabled()(construction-time ACTIVE), notget_exec(). Prefer also callingdetermine_num_fused_shared_expertsbefore buildingGlm4MoeModelNextN(Deepseek NextN pattern) so a draft can refine its own decision underspeculative_moe_backend_contextrather than only inheriting the target's ACTIVE value.
- Suggestion: Mirror
-
[suggestion] python/sglang/srt/arg_groups/overrides.py:305 --
declare_load_time_overrideis deleted, butdraft_model_load_scope/_DRAFT_LOAD_SCOPESremain as scaffolding whose only consumer was that API.ModelRunner._draft_declaration_scope(and its docstring) still wraps every load in the now-empty scope. Dead code and stale docs increase the chance someone reintroduces bag-scoped load-time overrides against the new design.- Suggestion: Remove
draft_model_load_scope,_DRAFT_LOAD_SCOPES, and_draft_declaration_scope(or reduce them to a no-op with an explicit "retired" comment and a follow-up), and update the skill/docstring references that still mentiondeclare_load_time_overrideas a live path.
- Suggestion: Remove
-
[nit] python/sglang/srt/models/glm_ocr_nextn.py:142 -- Same bag-intent read as Issue 1 (
get_exec().moe.disable_shared_experts_fusionfor top-levelnum_fused_shared_experts). ParentGlmOcrload paths do not appear to remap shared experts from this field, so this is likely inert metadata rather than a weight-load bug. Still inconsistent with the new per-runner flag model if anything later trusts the attribute.- Suggestion: Switch to
is_shared_experts_fusion_disabled()for consistency, or drop the field if unused.
- Suggestion: Switch to
04a5ca6 to
6cd2cf8
Compare
21e9862 to
ab1e187
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab1e187f7e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ch-wan
left a comment
There was a problem hiding this comment.
Re-review (updated head ab1e187f7e3e)
All four prior findings on older head 21e9862 are fixed:
- bug
glm4_moe_nextn.pybag-intent / no gate before decoder — fixed (runsdetermine_num_fused_shared_experts()and setsnum_fused_shared_expertsbefore model build) - suggestion leftover
draft_model_load_scope/_DRAFT_LOAD_SCOPES— fixed (gone withdeclare_load_time_override) - suggestion lite NextN no fusion gate before decoder — fixed (
determine_num_fused_shared_experts(...NextN)before model) - nit
glm_ocr_nextn.pybag-intent — fixed (is_shared_experts_fusion_disabled())
The ACTIVE + speculative leaf design (record_shared_experts_fusion_decision, speculative_moe_backend_context, forward-time refusal, GDN MTP hybrid refusal, draft update_model_fields isolation) looks correct and well covered by the isolation tests.
Issue counts by severity
- bugs: 0
- suggestions: 0
- nits: 0
Verdict: LGTM on the updated head. Prior blocking issue is resolved.
6cd2cf8 to
be4effd
Compare
be4effd to
05bccd9
Compare
7f1b8e7 to
3cee4c0
Compare
05bccd9 to
7803414
Compare
3d25e75 to
647f118
Compare
7803414 to
bb4387e
Compare
647f118 to
93607b4
Compare
bb4387e to
9824202
Compare
93607b4 to
90d55c0
Compare
…der installs
A draft's construction used to rewrite the process config record. Three
writers, three shapes:
**1. The shared-experts-fusion decision.** `declare_load_time_override` wrote it
to the target's bags, and an MTP/nextn draft IS a DeepSeek/GLM/Qwen3.5/MiniMax
model, so a draft whose checkpoint differs from the target's (quantization)
corrupted the target's record. This is ancestral behavior: the pre-refactor code
wrote the shared `ServerArgs` object.
The decision is per runner, and it is now made in one place — the loader's
single model-instantiation point, before any layer exists:
_initialize_model() # the only place a model class is built
install_shared_experts_fusion_decision(model_class, hf_config, quant_config)
model_class(**kwargs) # the layers read the installed value
- `flags.moe.disable_shared_experts_fusion` carries it, in the
`runner_backend` / `speculative_runner_backend` shape:
`initialize_moe_config` seeds both leaves with the user's intent, and
`draft_model_build_scope` — entered by every draft build — routes the draft's
answer onto the speculative leaf as well, so a flags dump afterwards shows
both runners' decisions. The accessor refuses forward-time reads: a forward
reads what its build baked in (`num_fused_shared_experts` on the layer), and
during a draft's build the ACTIVE leaf holds the draft's value.
- Each family that can auto-disable exposes its conditions as
`shared_experts_fusion_disable_reason(hf_config, quant_config)`, answering
from the config and quantization it is handed — no instance, no layers. That
is where the conditions already lived, and the loader is the last point that
still knows both the checkpoint's quantization and whether this runner is a
draft. `determine_num_fused_shared_experts` shrinks to reading the installed
decision, keeping each family's asserts and logs. A family without a gate
follows the user's intent.
- The architecture a class fuses for becomes a class attribute
(`fused_shared_experts_architecture`) instead of an argument threaded through
the call: the NextN drafts and GLM's DSA variant override it, which also
removes the `super()` hop in `GlmMoeDsaForCausalLM`.
- qwen3_5 needed care: its auto-disable is ROCm-only and reads the *text*
config, while the loader instantiates the VL entry classes, so the gate
resolves the text config itself and is attached to all four classes the loader
may build — otherwise the ROCm auto-disable would have silently stopped firing.
The bag leaf is the user's pristine intent again, consistent with the
kv_cache_dtype ruling, and with the last caller gone (the device-driven sm80
dtype fallback records directly through `get_context().override`)
`declare_load_time_override` is deleted.
**2. The v2 draft path re-ran the hybrid wrapper's process-level
initialization** (the SM100 GDN prefill default + the process-global linear-attn
kernel dispatch) that the v1 draft factory deliberately skips.
`hybrid_gdn_config` now refuses an all-softmax GDN MTP draft (their MTP models
force `full_attention_interval=1`) the same way `mamba2_config` refuses
NemotronH's — a STANDALONE GDN draft keeps its linear layers and the hybrid path.
**3. A draft weight update recorded the draft's `model_path`/`load_format` as
the process's**; the record is target-only now (the draft's own state is on the
runner).
`draft_model_build_scope` deliberately does not swap `runner_backend`: that is
`speculative_moe_backend_context`'s job and has to bracket the draft's whole
lifecycle, which dflash/dspark/standalone do not do — a construction-only swap
would build and execute the draft under different backends.
Tests: the draft-isolation cases drive the installer with stand-in model classes
(one that always disables, one with no gate), and
`test_shared_experts_fusion_gates.py` pins each family's branch table — the only
safety net for the families that cannot be run on one box, where a wrong answer
is a silently wrong weight remap rather than a crash.
90d55c0 to
e996f26
Compare
What
A draft's construction used to rewrite the process config record. Two writers, two shapes:
1. The shared-experts-fusion decision becomes a per-runner value the loader installs.
declare_load_time_overridewrote the fusion decision to the target's bags — and an MTP/nextn draft IS a DeepSeek/GLM/Qwen3.5/MiniMax model, so a draft whose checkpoint differs from the target's (quantization) corrupted the target's record. This is ancestral behavior: the pre-refactor code wrote the sharedServerArgsobject.The decision is per runner, and it is now made in one place — the loader's single model-instantiation point, before any layer exists:
flags.moe.disable_shared_experts_fusioncarries it, in therunner_backend/speculative_runner_backendshape:initialize_moe_configseeds both leaves with the user's intent, anddraft_model_build_scope— entered by every draft build — routes the draft's answer onto the speculative leaf as well, so a flags dump afterwards shows both runners' decisions. The accessor refuses forward-time reads: a forward reads what its build baked in (num_fused_shared_expertson the layer), and during a draft's build the ACTIVE leaf holds the draft's value.Each family that can auto-disable exposes its conditions as
shared_experts_fusion_disable_reason(hf_config, quant_config), answering from the config and quantization it is handed — no instance, no layers. That is where the conditions already lived, and the loader is the last point that still knows both the checkpoint's quantization and whether this runner is a draft.determine_num_fused_shared_expertsshrinks to reading the installed decision, keeping each family's asserts and logs. A family without a gate (qwen2_moe) follows the user's intent.The architecture a class fuses for becomes a class attribute (
fused_shared_experts_architecture) instead of an argument threaded through the call: the NextN drafts and GLM's DSA variant override it, which also removes thesuper()hop inGlmMoeDsaForCausalLM.A wrapper answers with the config it hands its nested body. The loader asks the class it instantiates, which for several archs is a wrapper around a DeepSeek/Qwen3.5 body — and those conditions used to be evaluated during that nested construction. Each wrapper therefore delegates with the config (and quantization) its constructor passes down, derived by a function the constructor shares so the two cannot drift apart:
KimiK25ForConditionalGenerationDeepseekV3ForCausalLMtext_config; encoder-only →NoneKimiVLForConditionalGenerationDeepseekV2ForCausalLMtext_configdeep-copied witharchitecturesrewritten, exactly as the constructor doesDotsVLMForCausalLMDeepseekV2ForCausalLMlanguage_configDeepseekVL2ForCausalLMDeepseekV2ForCausalLMlanguage_config, no quantization (the construction passes none); non-MLA builds the dense model →NoneDeepseekOCRForCausalLMDeepseekV2/V3ForCausalLMtext_config, MoE branches only (OCR2 / non-MLA →None)PixtralForConditionalGenerationMistralLarge3ForCausalLMtext_configwhen it isdeepseek_v3; the GQA branch builds dense Mistral →NoneMiniCPMV,MiniCPMV4_6ForConditionalGenerationQwen3_5ForCausalLMtext_configQwen3_5ForCausalLMMTPQwen3_5ForCausalLMtext_config+ the MTP module's own quantization normalization (_mtp_quant_config, shared with the constructor)qwen3_5_text.Qwen3_5{,Moe}ForCausalLMbody_clsqwen3_5 needed particular care twice: its auto-disable is ROCm-only and reads the text config while the loader instantiates the VL entry classes, and
qwen3_5_text.pydefines its own classes with the same names as the multimodal ones — the registry resolves text-only checkpoints to those, so attaching the gate to the multimodal classes alone would have silently stopped the ROCm auto-disable.The bag leaf is the user's pristine intent again, consistent with the kv_cache_dtype ruling, and with the last caller gone (the device-driven sm80 dtype fallback, identical for every runner in the process, records directly through
get_context().override)declare_load_time_overrideis deleted.draft_model_build_scopedeliberately does not swaprunner_backend: that isspeculative_moe_backend_context's job and has to bracket the draft's whole lifecycle, which dflash/dspark/standalone do not do — a construction-only swap would build and execute the draft under different backends.2. A draft weight update recorded the draft's
model_path/load_formatas the process's (today the scheduler feeds both runners the same path, so the write was same-value redundant); the record is target-only now.Verification
test_shared_experts_fusion_gates.pypins every family's gate branch table and which config / quantization each wrapper hands its nested body — handing over the top-level config is the failure mode, since it answers for the wrong checkpoint.test_fusion_gate_coverage.pywalks the model registry and fails, naming the arch, if any entry class can reach a fusion-gated family without answering for it; the gated-class set is resolved dynamically so a wrapper around an inherited gate (DeepseekV3ForCausalLM) counts. Reverse-verified by removing a gate and confirming the arch is named.test_draft_construction_isolation.pydrives the installer with stand-in model classes and covers flag seeding, draft-scope routing and restore, forward-time-read refusal, and the weight-update record.--ep-size 2: two disable messages, target and draft — the loader-installed decision matches what the per-family evaluation produced) and Qwen3-Next-80B-FP8 + NEXTN (accept length 2.20). The Qwen3.5 rejection-sampling case (test_eagle_reject_sampling.py) launches with the same KV pool layout as the base commit, checked against a side-by-side run.CI States
Latest PR Test (Base): 🚫 Run #31242283228
Latest PR Test (Extra): 🚫 Run #31242283198