[BREAKING][misc] feat: Abstract optimizer #3656
Merged
Conversation
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
EduardDurech
requested review from
PeterSH6,
ZihengJiang,
eric-haibin-lin,
tongyx361,
vermouth1992 and
zhaochenyang20
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a flexible optimizer abstraction, allowing users to specify any optimizer via configuration. This is a great enhancement for modularity. My review focuses on the implementation of the new build_optimizer function. I've identified a critical issue where the argument handling for the dynamically loaded optimizer is not robust and can lead to runtime TypeError exceptions. My suggestion involves using Python's inspect module to build the arguments dictionary safely, ensuring only valid parameters are passed to the optimizer's constructor. This will make the implementation more generic and prevent unexpected crashes.
EduardDurech
force-pushed
the
feat/optim_abstract
branch
from
098c2c8 to
e9cfd78
Compare
|
@EduardDurech Please resolve conflicts with main branch. |
|
@wuxibin89 I overwrote the PR #3692 as extra parameters should now be defined in ci is unrelated to PR, tests passed #cf4cc6a6c60b2a21b1765825b83158ae6bea101b cpu_unit_tests
sgl
e2e_ascend |
7 tasks
7 tasks
vermouth1992
pushed a commit
that referenced
this pull request
Oct 13, 2025
…heduler_type (#3739) ### What does this PR do? > Rename `warmup_style` in FSDPOptimizerConfig to `lr_scheduler_type` to align with Hugging Face Trainer API。 The following pull request is for refactoring the optimizer, however, the naming issue persists. #3656 ### Checklist Before Starting - [x] Search for similar PRs. Paste at least one query link here: ... - [x] Format the PR title as `[{modules}] {type}: {description}` (This will be checked by the CI) - `{modules}` include `fsdp`, `megatron`, `sglang`, `vllm`, `rollout`, `trainer`, `ci`, `training_utils`, `recipe`, `hardware`, `deployment`, `ray`, `worker`, `single_controller`, `misc`, `perf`, `model`, `algo`, `env`, `tool`, `ckpt`, `doc`, `data` - 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 > For changes that can not be tested by CI (e.g., algorithm implementation, new model support), validate by experiment(s) and show results like training curve plots, evaluation results, etc. ### API and Usage Example > Demonstrate how the API changes if any, and provide usage example(s) if possible. ```python # Add code snippet or script demonstrating how to use this ``` ### Design & Code Changes > Demonstrate the high-level design if this PR is complex, and list the specific changes. ### Checklist Before Submitting > [!IMPORTANT] > Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review. - [x] Read the [Contribute Guide](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md). - [x] Apply [pre-commit checks](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md#code-linting-and-formatting): `pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always` - [x] Add / Update [the documentation](https://github.com/volcengine/verl/tree/main/docs). - [x] Add unit or end-to-end test(s) to [the CI workflow](https://github.com/volcengine/verl/tree/main/.github/workflows) to cover all the code. If not feasible, explain why: ... - [ ] Once your PR is ready for CI, send a message in [the `ci-request` channel](https://verl-project.slack.com/archives/C091TCESWB1) in [the `verl` Slack workspace](https://join.slack.com/t/verl-project/shared_invite/zt-3855yhg8g-CTkqXu~hKojPCmo7k_yXTQ). (If not accessible, please try [the Feishu group (飞书群)](https://applink.larkoffice.com/client/chat/chatter/add_by_link?link_token=772jd4f1-cd91-441e-a820-498c6614126a).) --------- Co-authored-by: weiqi.li <[email protected]>
|
@wuxibin89 @vermouth1992 could we get this merge, it's very annoying to have to resolve merge conflicts every other day because of other PRs, I don't have time to maintain a single PR for weeks |
|
Let me know when you guys are ready to merge please, I cannot waste an hour each time to fix merge conflicts and then it doesn't get merged |
masoudhashemi
pushed a commit
to masoudhashemi/verl
that referenced
this pull request
Oct 19, 2025
…heduler_type (volcengine#3739) ### What does this PR do? > Rename `warmup_style` in FSDPOptimizerConfig to `lr_scheduler_type` to align with Hugging Face Trainer API。 The following pull request is for refactoring the optimizer, however, the naming issue persists. volcengine#3656 ### Checklist Before Starting - [x] Search for similar PRs. Paste at least one query link here: ... - [x] Format the PR title as `[{modules}] {type}: {description}` (This will be checked by the CI) - `{modules}` include `fsdp`, `megatron`, `sglang`, `vllm`, `rollout`, `trainer`, `ci`, `training_utils`, `recipe`, `hardware`, `deployment`, `ray`, `worker`, `single_controller`, `misc`, `perf`, `model`, `algo`, `env`, `tool`, `ckpt`, `doc`, `data` - 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 > For changes that can not be tested by CI (e.g., algorithm implementation, new model support), validate by experiment(s) and show results like training curve plots, evaluation results, etc. ### API and Usage Example > Demonstrate how the API changes if any, and provide usage example(s) if possible. ```python # Add code snippet or script demonstrating how to use this ``` ### Design & Code Changes > Demonstrate the high-level design if this PR is complex, and list the specific changes. ### Checklist Before Submitting > [!IMPORTANT] > Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review. - [x] Read the [Contribute Guide](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md). - [x] Apply [pre-commit checks](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md#code-linting-and-formatting): `pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always` - [x] Add / Update [the documentation](https://github.com/volcengine/verl/tree/main/docs). - [x] Add unit or end-to-end test(s) to [the CI workflow](https://github.com/volcengine/verl/tree/main/.github/workflows) to cover all the code. If not feasible, explain why: ... - [ ] Once your PR is ready for CI, send a message in [the `ci-request` channel](https://verl-project.slack.com/archives/C091TCESWB1) in [the `verl` Slack workspace](https://join.slack.com/t/verl-project/shared_invite/zt-3855yhg8g-CTkqXu~hKojPCmo7k_yXTQ). (If not accessible, please try [the Feishu group (飞书群)](https://applink.larkoffice.com/client/chat/chatter/add_by_link?link_token=772jd4f1-cd91-441e-a820-498c6614126a).) --------- Co-authored-by: weiqi.li <[email protected]>
EduardDurech
force-pushed
the
feat/optim_abstract
branch
from
930b779 to
70aa8fc
Compare
vermouth1992
approved these changes
Oct 23, 2025
sunnweiwei
pushed a commit
to sunnweiwei/verl
that referenced
this pull request
Oct 23, 2025
Abstract optimizer so can be used with whatever module and method a user
wants, should be backwards compatible as default is `torch.optim.AdamW`,
adds
`{actor_rollout_ref.actor,critic}.optim.{optimizer,optimizer_impl,override_optimizer_config}`
```yaml
# Default
optimizer_impl: torch.optim
optimizer: AdamW
```
```yaml
# Example
optimizer_impl: torchao.optim
optimizer: _AdamW
override_optimizer_config:
bf16_stochastic_round: true
```
**Important**: fsdp_sft_trainer optim aligned with FSDP optim
`optim.warmup_steps_ratio`->`optim.lr_warmup_steps_ratio`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Abstract optimizer so can be used with whatever module and method a user wants, should be backwards compatible as default is
torch.optim.AdamW, adds{actor_rollout_ref.actor,critic}.optim.{optimizer,optimizer_impl,override_optimizer_config}Important: fsdp_sft_trainer optim aligned with FSDP optim
optim.warmup_steps_ratio->optim.lr_warmup_steps_ratio