[model] feat: add FSDP/Megatron critic worker with model engine #3439

vermouth1992 · 2025-09-10T12:54:20Z

What does this PR do?

As title
Add a test to compare the output of FSDP/Megatron engine with huggingface model

Checklist Before Starting

Search for similar PRs. Paste at least one query link here: ...
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.

# 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.

Read the Contribute Guide.
Apply pre-commit checks: pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always
Add / Update the documentation.
Add unit or end-to-end test(s) to the CI workflow 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 in the verl Slack workspace. (If not accessible, please try the Feishu group (飞书群).)

gemini-code-assist

Code Review

This pull request introduces an FSDP critic worker and refactors the engine creation logic for both actor and critic workers to be more modular. A new test for the critic worker is also added. The changes are generally positive, but I have identified two high-severity issues. One is a bug in the new test that prevents it from covering all specified strategies. The other is a potential correctness issue in the critic worker where a shared configuration object is modified in-place, which could lead to side effects. I have provided suggestions to fix both issues.

gemini-code-assist · 2025-09-10T12:56:17Z

tests/models/test_engine.py

+    path = os.path.expanduser("~/models/Skywork/Skywork-Reward-V2-Qwen3-0.6B")
+    model_config = HFModelConfig(path=path)
+
+    strategy = "fsdp"


The strategy parameter from pytest.mark.parametrize is being overwritten on this line. This will cause the test to run with strategy="fsdp" for all parameterized cases, and the fsdp2 case will not be tested. The if/elif/else block for strategy selection will also not function as intended. Please remove this line to allow the test to correctly use the parameterized strategy values.

verl/workers/roles/critic.py

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

vermouth1992 · 2025-09-12T11:14:00Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a significant feature: a critic worker with FSDP/Megatron support, complete with a model engine. The changes include a new CriticWorker, refactoring of engine implementations for better modularity using a registry, and the addition of a new test for the critic. The configuration has also been refactored to be more organized. Overall, the changes are well-structured. However, I've identified a critical issue in the CriticWorker concerning how it modifies the model architecture, which is brittle and overly restrictive. Additionally, the new tests use hardcoded paths, which harms portability and should be addressed.

verl/workers/roles/critic.py

gemini-code-assist · 2025-09-12T11:16:05Z

tests/models/test_engine.py

+def test_actor_engine(strategy):
    ray.init()

    path = os.path.expanduser("~/models/Qwen/Qwen2.5-0.5B-Instruct")


Hardcoding the model path using os.path.expanduser makes this test dependent on a specific local file system setup. This will cause the test to fail for other developers or in CI environments where ~/models/Qwen/Qwen2.5-0.5B-Instruct does not exist. For large, pre-existing models, consider making the path configurable via an environment variable or a test configuration file, and provide instructions for setting it up. This will improve the test's portability and reproducibility.

tests/models/test_engine.py

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…engine#3439) ### What does this PR do? - As title - Add a test to compare the output of FSDP/Megatron engine with huggingface model ### Checklist Before Starting - [ ] Search for similar PRs. Paste at least one query link here: ... - [ ] 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. - [ ] Read the [Contribute Guide](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md). - [ ] 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` - [ ] Add / Update [the documentation](https://github.com/volcengine/verl/tree/main/docs). - [ ] 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: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

xihuai18 · 2025-09-14T07:38:50Z

verl/workers/config/actor.py

    engine: BaseConfig = field(default_factory=BaseConfig)
    data_loader_seed = 1
-    n: int = 1  # must be override by sampling config
+    rollout_n: int = 1  # must be override by sampling config


Is this renaming complete in all references to the "n" parameter?

…engine#3439) ### What does this PR do? - As title - Add a test to compare the output of FSDP/Megatron engine with huggingface model ### Checklist Before Starting - [ ] Search for similar PRs. Paste at least one query link here: ... - [ ] 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. - [ ] Read the [Contribute Guide](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md). - [ ] 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` - [ ] Add / Update [the documentation](https://github.com/volcengine/verl/tree/main/docs). - [ ] 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: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

vermouth1992 added 3 commits September 10, 2025 14:34

update

cede2a8

update

011f972

fix precommit

e52577b

vermouth1992 requested review from ZihengJiang and eric-haibin-lin as code owners September 10, 2025 12:54

gemini-code-assist bot reviewed Sep 10, 2025

View reviewed changes

vermouth1992 and others added 9 commits September 10, 2025 21:00

Update verl/workers/roles/critic.py

fe2bd10

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

update

5a05aab

fix ci

a18eaf8

update

bf34975

use fake model to test

370f174

update

2524490

precommit

72429d3

fix ci

a8ab634

upgrade transformers

8523e57

vermouth1992 closed this Sep 12, 2025

vermouth1992 added 6 commits September 12, 2025 19:08

Merge branch 'main' into chi/dev/critic

6484b07

update

e22a94b

update

581baf9

update

4693ab1

support megatron critic

d4234d3

precommit

40f034c

vermouth1992 reopened this Sep 12, 2025

vermouth1992 requested review from PeterSH6 and tongyx361 as code owners September 12, 2025 11:12

vermouth1992 changed the title ~~[model] feat: add FSDP critic worker with model engine~~ [model] feat: add FSDP/Megatron critic worker with model engine Sep 12, 2025

gemini-code-assist bot reviewed Sep 12, 2025

View reviewed changes

vermouth1992 added 2 commits September 12, 2025 19:16

fix ci

5336ed0

fix ci

4e7e4f6

vermouth1992 and others added 7 commits September 12, 2025 19:22

Update verl/workers/roles/critic.py

8952548

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

precommit

d768b40

update

4e9537d

update

2c9f369

precommit

140e3e7

update

f13d88b

update

f049154

PeterSH6 approved these changes Sep 13, 2025

View reviewed changes

vermouth1992 merged commit 6e6fafd into volcengine:main Sep 13, 2025
59 of 60 checks passed

xihuai18 reviewed Sep 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[model] feat: add FSDP/Megatron critic worker with model engine #3439

[model] feat: add FSDP/Megatron critic worker with model engine #3439

Uh oh!

vermouth1992 commented Sep 10, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Sep 10, 2025

Uh oh!

Uh oh!

vermouth1992 commented Sep 12, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

gemini-code-assist bot Sep 12, 2025

Uh oh!

Uh oh!

Uh oh!

xihuai18 Sep 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[model] feat: add FSDP/Megatron critic worker with model engine #3439

[model] feat: add FSDP/Megatron critic worker with model engine #3439

Uh oh!

Conversation

vermouth1992 commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist Before Starting

Test

API and Usage Example

Design & Code Changes

Checklist Before Submitting

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vermouth1992 commented Sep 12, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xihuai18 Sep 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vermouth1992 commented Sep 10, 2025 •

edited

Loading