Skip to content

[model, cfg] fix:Allow list of modules to be passed to Lora target_modules#5223

Open
thvasilo wants to merge 4 commits intoverl-project:mainfrom
thvasilo:fix-lora-target-module-list
Open

[model, cfg] fix:Allow list of modules to be passed to Lora target_modules#5223
thvasilo wants to merge 4 commits intoverl-project:mainfrom
thvasilo:fix-lora-target-module-list

Conversation

@thvasilo
Copy link
Contributor

@thvasilo thvasilo commented Feb 6, 2026

What does this PR do?

Fixes #5222

PeftConfig from PEFT supports passing a list of strings as target_modules.

However, target_modules is annotated as Optional[str] in verl/workers/config/model.py which leads to runtime errors when trying to override its default "all-linear" value with e.g. a list encoded as JSON string e.g. ["q_proj","v_proj"].

Checklist Before Starting

  • Search for similar PRs. Paste at least one query link here: ...
  • [c] Format the PR title as [{modules}] {type}: {description} (This will be checked by the CI)
    • {modules} include fsdp, megatron, veomni, sglang, vllm, rollout, trainer, ci, training_utils, recipe, hardware, deployment, ray, worker, single_controller, misc, perf, model, algo, env, tool, ckpt, doc, data, cfg, reward
    • If this PR involves multiple modules, separate them with , like [megatron, fsdp, doc]
    • {type} is in feat, fix, refactor, chore, test
    • If this PR breaks any API (CLI arguments, config, function signature, etc.), add [BREAKING] to the beginning of the title.
    • Example: [BREAKING][fsdp, megatron] feat: dynamic batching

Test

Added unit test that caught the error (allows replication if you switch the type back to Optional[str])

API and Usage Example

Demonstrate how the API changes if any, and provide usage example(s) if possible.

# Users should now be able to pass
python -m verl.trainer.main_ppo \
    actor_rollout_ref.model.target_modules='["k_proj","o_proj"]'

Design & Code Changes

I tried setting the type to Optional[str | list[str]] but was getting an OmegaConf error because it doesn't support unions in structured configs: omry/omegaconf#144

Instead I opted for Any and added type checks in __post_init__.

Checklist Before Submitting

Important

Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly addresses an issue where target_modules could not be a list of strings. The approach of changing the type to Any and adding validation in __post_init__ is a good solution for the OmegaConf limitation. My review includes two main points for improvement: first, to make the validation logic more robust by raising TypeError instead of using assert, as assertions can be disabled. Second, to add unit tests for this new validation logic, as it is currently not covered by the new tests. These changes will improve the correctness and reliability of the configuration handling.

thvasilo and others added 3 commits February 6, 2026 10:45
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@thvasilo thvasilo force-pushed the fix-lora-target-module-list branch from 4091e44 to dccbe4f Compare February 6, 2026 18:57
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.

Passing a list of modules to actor_rollout_ref.model.target_modules causes OmegaConf error

1 participant