Repository navigation
[OMNIML-6076] [2/3] Prepare a model's config for expert parallelism from its spec #3135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
shengliangxu
wants to merge
6
commits into
main
Choose a base branch
from
shengliangx/hfep-9a2-ep-spec-applier
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8f86e32
Prepare a model's config for expert parallelism from its spec
shengliangxu 35012df
Check every registered plan's styles, containers and config placement
shengliangxu 71fea4c
Run the spec registry checks without transformers again
shengliangxu bc36220
Say an MTP head the FSDP2 load cannot build rules it out too
shengliangxu 3a86e4a
Check that a config prepare returns in place of its own keeps the plan
shengliangxu 2b3dfc6
Honor a config's own plan before requiring the spec's nested config
shengliangxu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SUGGESTION] The enumeration of the empty cases omits the one a caller is most likely to misread: a spec with a full EP section whose plan ships with transformers.
qwen3_moe(requires_transformers="5.7", no plan — "qwen3_moe's ownbase_model_ep_planships in transformers") andminimax_m3_vl(requires_transformers="5.13", no plan) both have a spec, have an EP section, are notunsupported, and still return{}here. That is correct for the function as titled — the applier attaches nothing for them — but the docstring lists only three ways to get{}, and none of them covers these two.It matters because of the stated purpose: "what the plan decision reads at an EP degree of 1, to refuse an MoE left whole on every rank." For those two models this returns nothing, so the decision in [3/3] must fall back to the plan on the config (
_ep_config) rather than read{}as "not a routed MoE" — otherwise exactly the run this is meant to refuse slips through. One clause would close it:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the list is incomplete.
qwen3_moeandminimax_m3_vlhave an EP section with no plan of their own, and get{}here.The caller you're worried about already reads the config's own plan first. In #2746 [3/3], the decision looks for routed experts through
_ep_config(hf_config).base_model_ep_plan, which also finds a plan that ships as a config-class default. It falls back to_spec_ep_planonly when that finds none at an EP degree of 1. So neither model's{}is read as "not an MoE".This is docstring-only, so I'm postponing it to this PR's next code change, which will add the clause.