[perf] feat: add range tag to start/stop profile; clean actor_rollout_ref.profiler by davidmlw · Pull Request #2456 · verl-project/verl

davidmlw · 2025-07-10T08:48:07Z

What does this PR do?

I found the cost of workers start/stop profile is not negligible, there are big gap between steps which is annoying. So I add range tag to them, making it clear.

Another change, I realize that actor_rollout_ref needs only one profiler config, and needn't redundant for each role.

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.

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.

gemini-code-assist

Code Review

This pull request introduces two main changes: adding range tags to profiler start/stop calls to measure their overhead, and refactoring the profiler configuration for actor_rollout_ref to use a single configuration instead of per-role configurations.

The changes in the configuration files (.yaml and .sh) and worker implementations (fsdp_workers.py, megatron_workers.py) are well-aligned with the goal of simplifying profiler configuration and look correct.

The new profiling timer logic in dapo_ray_trainer.py and ray_trainer.py achieves the goal of measuring the start/stop overhead. However, I've identified a high-severity maintainability issue due to code duplication in both files. The logic to determine which worker groups to profile is repeated for both starting and stopping the profilers. This could lead to bugs in the future if new worker groups are added. I've left comments with suggestions for refactoring to improve maintainability.

gemini-code-assist · 2025-07-10T08:49:55Z

+                    if do_profile:
+                        self.actor_rollout_wg.start_profile()
+                        if self.use_reference_policy:
+                            self.ref_policy_wg.start_profile()
+                        if self.use_critic:
+                            self.critic_wg.start_profile()
+                        if self.use_rm:
+                            self.rm_wg.start_profile()


This block of code for starting profilers is nearly identical to the block for stopping them later in this method (lines 305-312). This duplication makes the code harder to maintain and prone to errors. For instance, if a new worker group is added, a developer might forget to update both sections, leading to inconsistent profiling.

To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I recommend refactoring this logic. You could create a list of worker groups to profile at the beginning of the loop iteration and then iterate over this list for both starting and stopping the profilers.

Here is a conceptual example:

# At the start of the loop iteration wgs_to_profile = [] if do_profile: wgs_to_profile.append(self.actor_rollout_wg) if self.use_reference_policy: wgs_to_profile.append(self.ref_policy_wg) if self.use_critic: wgs_to_profile.append(self.critic_wg) if self.use_rm: wgs_to_profile.append(self.rm_wg) # To start profiling with marked_timer("start_profile", timing_raw): for wg in wgs_to_profile: wg.start_profile() # To stop profiling (later in the loop) with marked_timer("stop_profile", timing_raw): for wg in wgs_to_profile: wg.stop_profile()

This would centralize the logic for selecting which workers to profile. A similar issue exists in verl/trainer/ppo/ray_trainer.py.

gemini-code-assist · 2025-07-10T08:49:55Z

+                    if do_profile:
+                        self.actor_rollout_wg.start_profile()
+                        if self.use_reference_policy:
+                            self.ref_policy_wg.start_profile()
+                        if self.use_critic:
+                            self.critic_wg.start_profile()
+                        if self.use_rm:
+                            self.rm_wg.start_profile()


This block of code for starting profilers is nearly identical to the block for stopping them later in this method (lines 1165-1172). This duplication makes the code harder to maintain and prone to errors. For instance, if a new worker group is added, a developer might forget to update both sections, leading to inconsistent profiling.

To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I recommend refactoring this logic. You could create a list of worker groups to profile at the beginning of the loop iteration and then iterate over this list for both starting and stopping the profilers.

Here is a conceptual example:

# At the start of the loop iteration wgs_to_profile = [] if do_profile: wgs_to_profile.append(self.actor_rollout_wg) if self.use_reference_policy: wgs_to_profile.append(self.ref_policy_wg) if self.use_critic: wgs_to_profile.append(self.critic_wg) if self.use_rm: wgs_to_profile.append(self.rm_wg) # To start profiling with marked_timer("start_profile", timing_raw): for wg in wgs_to_profile: wg.start_profile() # To stop profiling (later in the loop) with marked_timer("stop_profile", timing_raw): for wg in wgs_to_profile: wg.stop_profile()

This would centralize the logic for selecting which workers to profile. A similar issue exists in recipe/dapo/dapo_ray_trainer.py.

eric-haibin-lin · 2025-07-10T18:43:07Z

let's add the config changes to #2270

ETOgaosion · 2025-07-11T02:12:32Z

Thanks for contribution! Remember to add changes to #2270 ~ , and maybe add some docs?

By the way, can current profiling system be able to profile SGLang?

davidmlw · 2025-07-11T02:37:51Z

Thanks for contribution! Remember to add changes to #2270 ~ , and maybe add some docs?

By the way, can current profiling system be able to profile SGLang?

Yes, I just use it get SGLang timeline.
A little difference to vLLM SPMD. Nsys will dump all the subprocesses from the first process it hooked. So Nsys traces both SGLang and trainer and show the timeline.

eric-haibin-lin · 2025-07-13T23:33:40Z

-                        self.rm_wg.start_profile()
+                with marked_timer("start_profile", timing_raw):
+                    if do_profile:
+                        self.actor_rollout_wg.start_profile(role="e2e", profile_step=self.global_steps)


@davidmlw could you wrap these code logic in self._start_profile() and self._stop_profile() so fit() is shorter?

…_ref.profiler (verl-project#2456) ### What does this PR do? I found the cost of workers start/stop profile is not negligible, there are big gap between steps which is annoying. So I add range tag to them, making it clear. Another change, I realize that `actor_rollout_ref` needs only one `profiler` config, and needn't redundant for each role. ### 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. ### 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: ... - [X] 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).

davidmlw requested review from PeterSH6, eric-haibin-lin, tongyx361 and vermouth1992 as code owners July 10, 2025 08:48

gemini-code-assist Bot reviewed Jul 10, 2025

View reviewed changes

add range tag to start/stop profile; clean actor_rollout_ref.profiler

7a2dfe5

davidmlw force-pushed the bench branch from ad19732 to 7a2dfe5 Compare July 10, 2025 10:00

davidmlw added 2 commits July 10, 2025 18:31

fix /test_legacy_config_on_cpu.py; fix a typo

83a7e9f

fix test_nvtx_profile.py

1d991c9

ETOgaosion approved these changes Jul 11, 2025

View reviewed changes

ETOgaosion merged commit 1dfc135 into verl-project:main Jul 11, 2025

eric-haibin-lin reviewed Jul 13, 2025

View reviewed changes

Navigation Menu

Repository navigation

[perf] feat: add range tag to start/stop profile; clean actor_rollout_ref.profiler - #2456

[perf] feat: add range tag to start/stop profile; clean actor_rollout_ref.profiler#2456
ETOgaosion merged 3 commits into
verl-project:mainfrom
davidmlw:bench

davidmlw commented Jul 10, 2025

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Jul 10, 2025

Uh oh!

gemini-code-assist Bot Jul 10, 2025

Uh oh!

eric-haibin-lin commented Jul 10, 2025

Uh oh!

ETOgaosion commented Jul 11, 2025 •

edited

Loading

Uh oh!

davidmlw commented Jul 11, 2025

Uh oh!

eric-haibin-lin Jul 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

davidmlw commented Jul 10, 2025

What does this PR do?

Checklist Before Starting

Test

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 Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

eric-haibin-lin commented Jul 10, 2025

Uh oh!

ETOgaosion commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidmlw commented Jul 11, 2025

Uh oh!

eric-haibin-lin Jul 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ETOgaosion commented Jul 11, 2025 •

edited

Loading