-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Scope MoE expert LoRA detection to actual MLP projection targets #6849
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
Changes from 5 commits
c68e60e
c36e674
0f60ede
c6a611a
cdbb3dc
5aca14f
9d98b36
787d3b9
c8d58b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3507,8 +3507,25 @@ def _moe_target_set_from_string(target_modules: str) -> set[str]: | |
| return {target_modules} | ||
|
|
||
| is_regex = re.search(r"[*+?()[\]{}|\\^$]", target_modules) is not None | ||
| targets_mlp = "mlp" in target_modules or "ffn" in target_modules | ||
| if is_regex and "proj" in target_modules and targets_mlp: | ||
| # Key detection on the mlp/ffn/experts path segment (absent from an | ||
| # attention-only regex), never on q/k/v/o leaves alone. | ||
| targets_mlp_path = any( | ||
| tag in target_modules for tag in ("mlp", "ffn", "feed_forward", "experts") | ||
| ) | ||
| if not is_regex or not targets_mlp_path: | ||
| return set() | ||
| # Explicit expert leaves scope the target set to exactly those leaves. | ||
| named = {name for name in _MOE_BROAD_MLP_TARGETS if name in target_modules} | ||
| if named: | ||
| return named | ||
| # A generic projection under an mlp path (e.g. ".*mlp.*proj"): any proj | ||
| # occurrence that is not an attention leaf name. | ||
| if re.search(r"(?<![qkvo]_)(?<!out_)(?<!in_)proj", target_modules): | ||
| return set(_MOE_BROAD_MLP_TARGETS) | ||
| # The auto regex on fused-expert models lists only attention Linears as | ||
| # leaves; its mlp tag block is the remaining MLP-intent signal. A regex | ||
| # like "(mlp|self_attn).(q_proj|o_proj)" has neither and stays attention-only. | ||
| if "mlp|feed_forward|ffn|dense" in target_modules: | ||
| return set(_MOE_BROAD_MLP_TARGETS) | ||
|
Comment on lines
+3529
to
3530
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an explicit attention-only list is routed through Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Confirmed: an attention-only explicit list under family scoping (vision off) routes through get_peft_regex and the emitted regex carries the full mlp|feed_forward|ffn|dense component block while its leaf group is only q/k/v/o_proj, so the fallback trained the experts for a language-only request. The string alone cannot tell this apart from the fused-expert auto regex, so I now feed the caller's original leaf list into expert detection; only the auto (None/all-linear) path still relies on the regex. Attention-only lists no longer target experts, and fused-expert auto detection is unchanged. Added a regression test. |
||
|
|
||
| return set() | ||
|
|
||
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.
When
get_peft_regexemits the attention-only pattern described by this change, its path group can still containmlp|feed_forward|ffn|densewhile the leaf group is onlyq_proj|k_proj|v_proj|o_proj; the earliernamedand generic-projchecks correctly do not treat that as expert intent, but this fallback then returns all MoE targets solely from the path alias substring. Becauseget_moe_target_parametersonly sees the regex string and not the originalfinetune_mlp_modulesflag, attention-only runs on fused MoE models still silently attach LoRA togate_up_projanddown_projwhenever that canonical alias block is present.Useful? React with 👍 / 👎.
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.
get_peft_regex only appends the mlp tags (mlp|feed_forward|ffn|dense) to the path group when finetune_mlp_modules=True (peft_utils.py: regex_components += mlp_tags is guarded by that flag). An attention-only regex emits self_attn|attention|attn|mixer as its path group, so the tag block is never present. Confirmed empirically for both a fused-expert MoE and a per-expert-Linear MoE: the attention-only regex contains no feed_forward / tag block and get_moe_target_parameters returns None. Branch 3 only fires when mlp finetuning is on (fused-expert case), which is intended. No code change.