[trainer] feat: enable model engine based critic

[vermouth1992]

What does this PR do?

Due to fused worker design and ActorRolloutRef is a async Ray Actor, we have to temporarily implement train_mini_batch API for TrainingWorker and use it to implement update_actor and update_critic
In this PR, we

• Completely remove CriticWorker in engine and directly use TrainingWorker
• Implement e2e ppo process using model engine
• Results on char count task matches with legacy worker implementation
• Note that currently megatron backend for model engine value model is not supported until mbridge officially supports value model save/load

• orange: ppo with legacy worker
• blue: ppo with model engine worker
• white: grpo with model engine worker

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 refactors the critic to use the model engine-based TrainingWorker, removing the specialized CriticWorker. This is a significant and positive simplification of the architecture. The changes across the trainer, configurations, and tests are largely consistent with this goal. However, I've identified two critical bugs in verl/workers/engine_workers.py where a .cpu() call on a None value can occur, leading to worker crashes. I've also found a high-severity issue with a misleading assertion message that could hinder debugging. Addressing these points will improve the robustness of the new implementation.

[gemini-code-assist]

Potential AttributeError here. final_output can be None if self.engine.is_mp_src_rank_with_outputs() is False (line 285), which will cause None.cpu() to raise an error. The call to .cpu() should be conditional to handle cases where final_output is None.

Suggested change

	return final_output.cpu()
	return final_output.cpu() if final_output is not None else None

[gemini-code-assist]

Potential AttributeError here. final_output can be None if self.engine.is_mp_src_rank_with_outputs() is False (line 322), which will cause None.cpu() to raise an error. The call to .cpu() should be conditional to handle cases where final_output is None.

Suggested change

	return final_output.cpu()
	return final_output.cpu() if final_output is not None else None

[gemini-code-assist]

The assertion message here is incorrect due to what appears to be a copy-paste error. It references batch_size_per_dp and num_mini_batch, which are from the other if branch. This can be misleading when debugging. The message should reference mini_batch_size and self.engine.get_data_parallel_size().

Suggested change

	assert mini_batch_size % self.engine.get_data_parallel_size() == 0, (
	f"Got {batch_size_per_dp=} and {num_mini_batch=}"
	)
	assert mini_batch_size % self.engine.get_data_parallel_size() == 0, (
	f"Got {mini_batch_size=} and data_parallel_size={self.engine.get_data_parallel_size()}"
	)

[vermouth1992]

/gemini review

[gemini-code-assist]

Code Review

This pull request is a significant refactoring that enables the critic to use the model engine-based TrainingWorker, unifying worker implementations and simplifying the trainer logic. The introduction of train_mini_batch in TrainingWorker is a good move to centralize mini-batch handling. I've identified a critical bug in this new method related to handling asynchronous results, which could lead to a runtime error. Additionally, there's a high-severity issue with a metric key typo that would cause incorrect logging. I've provided code suggestions for both issues. Overall, this is a valuable structural improvement, and addressing these points will make it ready for merging.

[gemini-code-assist]

It appears there's a bug in how results from train_batch are processed. The train_batch method is decorated with blocking=False, which means it returns a future. The output_lst is therefore a list of futures. However, you are attempting to process this list of futures directly with tu.get without first resolving them (e.g., using .get() or ray.get()). This will likely cause a runtime error as tu.get expects a TensorDict, not a future. You should resolve the futures before processing their results.

Suggested change

	actor_output = [tu.get(output, "metrics") for output in output_lst]
	actor_output = [tu.get(output.get(), "metrics") for output in output_lst]

[gemini-code-assist]

There seems to be a typo in the metric key. You are renaming critic/mfu to perf/critic/actor. This is likely a copy-paste error from _update_actor and should probably be perf/mfu/critic. The current key perf/critic/actor is confusing and will conflict with the actor's MFU metric, leading to incorrect logging.

Suggested change

	output["perf/critic/actor"] = output.pop("critic/mfu")
	output["perf/mfu/critic"] = output.pop("critic/mfu")

[vermouth1992]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the PPO trainer to use a generic TrainingWorker for the critic model, instead of a specialized CriticWorker. This is a significant and positive architectural change that improves code reuse and maintainability. The core logic for handling mini-batch updates has been encapsulated within the TrainingWorker, simplifying the main trainer loop. The changes also include updates to tests to reflect the new API, and additions to model registries to support Llama-based token classification models for the critic. My review found one issue related to configuration handling that could lead to incorrect behavior.