Skip to content

Guard _convert_peft_config_moe against regex-string target_modules#3232

Open
1fanwang wants to merge 1 commit into
huggingface:mainfrom
1fanwang:fix/target-modules-regex-moe-conversion
Open

Guard _convert_peft_config_moe against regex-string target_modules#3232
1fanwang wants to merge 1 commit into
huggingface:mainfrom
1fanwang:fix/target-modules-regex-moe-conversion

Conversation

@1fanwang

Copy link
Copy Markdown

Closes #3229.

_convert_peft_config_moe calls set(peft_config.target_modules or []) unconditionally. When target_modules is a regex string (e.g. ms-swift's get_multimodal_target_regex produces ^(thinker\.model(?=\.).*\.(q_proj|k_proj|v_proj|...))$ for Qwen3-Omni), set(str) splits the regex into individual characters, then PEFT's downstream module-matching fails with:

ValueError: Target modules {'^', '(', 'q', '_', ...} not found in the base model

The MoE conversion loop assumes module-name granularity — there's no sensible way to rewrite a regex into per-module mappings. The fix emits a UserWarning pointing at the v5 module names when target_modules is a str, and returns early. Callers using regex target_modules get a clear signal instead of a confusing downstream error.

Tests

Two parametrized tests in TestTransformersV5:

  • 3 regex strings from real configs (ms-swift Qwen3-Omni, a simpler ^.*q_proj$, an empty regex) — all warn + skip the conversion
  • 3 list-shaped target_modules continue to flow through normally

Both fail on main (the regex tests hit the bug path without warning); both pass with the fix.

`_convert_peft_config_moe` called `set(peft_config.target_modules or [])`
unconditionally. When a user passes `target_modules` as a regex string
(e.g. ms-swift's `get_multimodal_target_regex` emits something like
`^(thinker\.model(?=\.).*\.(q_proj|k_proj|...))$` for multimodal MoE
models such as Qwen3-Omni), the `set()` iterates the string character
by character. That produces a nonsense set of regex metacharacters and
letters and fails downstream with:

    ValueError: Target modules {'i', 'e', 'p', 'q', 't', '(', 'o', '|',
    '.', ')', 'h', '_', 'j', 'r', '^', '\\', '$', 'd', 'k', '?', 'v',
    'n', 'm', 'l', '=', '*'} not found in the base model.

The MoE conversion loop further down assumes module-name granularity,
so a regex pattern can't be remapped automatically. Detect the regex
case explicitly, emit a UserWarning that points users at the v5
module names, and return without touching the config.

Fixes huggingface#3229.
@BenjaminBossan

Copy link
Copy Markdown
Member

@1fanwang Thanks for providing a PR to fix the problem. In the future, please ask first if you can work on the issue (or instruct the agent to do that) because otherwise, this may create duplicate work. We may close PRs without inspection if they're already being worked on by someone else.

As for your solution: I wonder if we can use peft.tuners.tuners_utils.check_target_module_exists to "resolve" the actual target modules from the string and work based on that, instead of bailing. Did you explore that option?

@1fanwang

Copy link
Copy Markdown
Author

Apologies on the duplicate-work front — happy to wait for a green light before drafting next time.

On check_target_module_exists: I did look at it before settling on the bail. The blocker for using it here is that _convert_peft_config_moe only has peft_config and model_type, not the model — so it can't enumerate concrete module names against the regex (tuners_utils.py L1805: check_target_module_exists(config, key) takes a single key per call). The caller convert_peft_config_for_transformers does have model and could thread it through:

# convert_peft_config_for_transformers, transformers_weight_conversion.py L444
_convert_peft_config_moe(peft_config, model_type, model=model)

then inside _convert_peft_config_moe we'd walk model.named_modules(), run re.fullmatch(peft_config.target_modules, name) on each, and use the matching leaf-names as the candidate set for the old→new remap in target_module_mapping (L382-L385 in the current file).

Two things made me back off:

  1. It's a non-local change — every caller of _convert_peft_config_moe would need to pass the model, and the regex-resolved set is structurally different from the user-declared set (the user declared a pattern, we'd be writing a set of concrete matches back).
  2. The remap on L398-L399 sets peft_config.target_modules = remaining_target_modules — for a regex input, "remaining" doesn't have a clean meaning if some leaves matched and others didn't. We'd have to either widen target_modules to a list-of-leaves (changes user intent) or leave the regex untouched and only mutate target_parameters/rank_pattern/alpha_pattern.

If you'd prefer the resolve-via-model path I'm happy to switch — option (2) above (regex stays, only parameters/patterns updated) feels safer than bailing, but it's a bigger diff than this PR. Want me to take that direction in this PR or stack it as a follow-up after the bail lands?

@BenjaminBossan

Copy link
Copy Markdown
Member

On check_target_module_exists: I did look at it before settling on the bail. The blocker for using it here is that _convert_peft_config_moe only has peft_config and model_type, not the model — so it can't enumerate concrete module names against the regex (tuners_utils.py L1805: check_target_module_exists(config, key) takes a single key per call). The caller convert_peft_config_for_transformers does have model and could thread it through

Yes, passing the model directly (instead of model_type) would be the idea.

  1. It's a non-local change — every caller of _convert_peft_config_moe would need to pass the model

There is only one call site, you can consider this to be "private".

2. The remap on L398-L399 sets peft_config.target_modules = remaining_target_modules — for a regex input, "remaining" doesn't have a clean meaning if some leaves matched and others didn't.

I don't get what you mean by that. Can you give an example?

@BenjaminBossan

Copy link
Copy Markdown
Member

One more thing, if we allow strings here, we also need to take care of "all-linear", which is a special string that matches by type and not by regex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] peft 0.19 target_modules (str) use set

2 participants