-
-
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 7 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() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1629,6 +1629,16 @@ def get_peft_model( | |
| ) | ||
| else: | ||
| _audio_kwargs = {} | ||
| # Remember the caller's ORIGINAL explicit leaf list for MoE expert | ||
| # detection. When an explicit list is routed through get_peft_regex for | ||
| # family scoping below, the generated regex carries get_peft_regex's full | ||
| # "mlp|feed_forward|ffn|dense" component block even when the caller named | ||
| # only attention leaves (q/k/v/o_proj). Keying expert detection on that | ||
| # regex would train the experts for an attention-only request. The | ||
| # original list carries the true leaf intent, so use it for MoE detection; | ||
| # only the auto (None / "all-linear") path relies on the regex, whose mlp | ||
| # block is the sole remaining MLP-intent signal on fused-expert models. | ||
| _moe_detect_target = target_modules if type(target_modules) in (list, tuple) else None | ||
| if target_modules is None or target_modules == "all-linear": | ||
| target_modules = get_peft_regex( | ||
| model, | ||
|
|
@@ -1706,9 +1716,12 @@ def get_peft_model( | |
| loftq_config, lora_dropout, bias, init_lora_weights, model | ||
| ) | ||
|
|
||
| # Auto-detect MoE models and populate target_parameters for expert layers | ||
| # Auto-detect MoE models and populate target_parameters for expert layers. | ||
| # For an explicit leaf list use the ORIGINAL list, not the scoped regex, so | ||
| # attention-only requests do not train experts via get_peft_regex's mlp block. | ||
| if target_parameters is None: | ||
| target_parameters = get_moe_target_parameters(model, target_modules) | ||
| _moe_targets = _moe_detect_target if _moe_detect_target is not None else target_modules | ||
| target_parameters = get_moe_target_parameters(model, _moe_targets) | ||
|
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 projection list is routed through the family filters above, this switches MoE detection back to the unscoped original list. For example, with 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, this was a real gap. Fixed in 787d3b9: MoE expert detection now prefers the original leaf list only when both MLP and language families are still in scope. When finetune_mlp_modules or finetune_language_layers is False, the scoped regex (which already drops the experts) is honored instead of re-adding the original list's gate/up/down leaves. Factored into _select_moe_detection_targets with unit tests over the full matrix (frozen-mlp, vision-only, in-scope-mlp, attention-only). |
||
|
|
||
| if finetune_last_n_layers is not None and layers_to_transform is None: | ||
| _total_layers = _get_total_transformer_layers(model) | ||
|
|
||
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.