Skip to content

Hejian/mor - #1

Open
HJSang wants to merge 7 commits into
mainfrom
hejian/mor
Open

Hejian/mor#1
HJSang wants to merge 7 commits into
mainfrom
hejian/mor

Conversation

@HJSang

@HJSang HJSang commented Oct 2, 2025

Copy link
Copy Markdown
Owner

What does this PR do?

Add concise overview of what this PR aims to achieve or accomplish. Reference related GitHub issues and PRs that help with the review.

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.

Summary by CodeRabbit

  • New Features

    • Mix actor and fixed-policy rollouts with a configurable allocation schedule.
    • Add rollout-only (fixed policy) worker support and automatic importance-weighted PPO using mixed samples.
    • New configuration blocks for rollout_only and rollout_allocation_schedule.
    • Example script updated with MLflow logging, offload options, and rollout settings.
  • Improvements

    • Added metrics and debug logging for rollout policy choices and timing.
    • Safer default config handling and broader compute_log_prob support.
  • Documentation

    • New design doc explaining mixture of rollout models and usage.
  • Chores

    • Updated dependencies, .gitignore entries, and a subproject reference.

@coderabbitai

coderabbitai Bot commented Oct 2, 2025

Copy link
Copy Markdown

Walkthrough

Adds a rollout-only fixed-policy pathway and a rollout allocation schedule to PPO training. Wires a new RolloutOnly role, worker registration, and rollout selection into the trainer. Extends configs, implements schedule classes, updates FSDP log-prob handling, and adds extensive debug logging. Includes new documentation, example script updates, and minor repo hygiene changes.

Changes

Cohort / File(s) Summary
Design Docs
docs/design/mixture_rollout_models.md
New design doc describing mixture of rollout models, importance-weighted PPO, config schema, worker roles, and training integration.
Configs
verl/trainer/config/ppo_trainer.yaml
Adds rollout_only and rollout_allocation_schedule blocks with fields for fixed-policy sampling and policy selection schedule.
Trainer Orchestration
verl/trainer/main_ppo.py, verl/trainer/ppo/ray_trainer.py
Introduces RolloutOnly worker registration, rollout-only config derivation, schedule-driven rollout selection, fixed/actor rollout generation paths, worker init, and logging/metrics propagation.
Rollout Allocation Schedule
verl/trainer/ppo/rollout_allocation_schedule.py
New module with abstract RolloutAllocationSchedule and Constant/Linear/Exponential/Step implementations; factory function to create from config.
Worker Logic
verl/workers/fsdp_workers.py, verl/workers/actor/dp_actor.py
FSDP compute_log_prob handles actor vs rollout-only paths; dp_actor uses safe config.get defaults and grad clip defaulting.
Rewards Debugging
verl/trainer/ppo/reward.py, verl/utils/reward_score/__init__.py, verl/utils/reward_score/gsm8k.py
Adds timing and diagnostic prints across reward loading and scoring; no API changes.
Example Script
examples/grpo_trainer/run_qwen3-8b.sh
Switches logging to MLflow, updates model paths, enables offload, toggles KL loss, adds rollout_only and schedule params, adjusts trainer args.
Submodule/Deps/Misc
sglang, test_requirements.txt, .gitignore
Updates sglang commit pointer; adds test deps (datasets, transformers, torch_memory_saver, sglang); ignores workspace/ and openconnect.json.
Enums/Utils
verl/trainer/ppo/utils.py
Adds Role.RolloutOnly enum member for fixed-policy rollout.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Trainer as PPOTrainer
  participant Schedule as RolloutAllocationSchedule
  participant ActorW as Actor Worker(s)
  participant FixedW as RolloutOnly Worker
  participant PPO as PPO Update

  User->>Trainer: start_training()
  loop per training step
    Trainer->>Schedule: get_policy_choice(step)
    Schedule-->>Trainer: "actor" | "fixed"
    alt choice == "actor"
      Trainer->>ActorW: generate_rollouts(request)
      ActorW-->>Trainer: batch(meta.policy_type="current")
    else choice == "fixed"
      Trainer->>FixedW: generate_rollouts(request)
      FixedW-->>Trainer: batch(meta.policy_type="reference")
    end
    note over Trainer,PPO: old_log_probs computed using policy_type metadata
    Trainer->>PPO: update with mixed samples (importance in ratio)
    PPO-->>Trainer: metrics
  end
Loading
sequenceDiagram
  autonumber
  participant Runner as TaskRunner
  participant Pool as ResourcePoolMgr
  participant Registry as Ray
  participant FixedW as RolloutOnly Worker

  Runner->>Pool: init_resource_pool_mgr(config)
  alt rollout_only configured
    Runner->>Registry: register(FixedW, role=RolloutOnly)
    Registry-->>Runner: handle
  else not configured
    note over Runner: skip RolloutOnly registration
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

A rabbit taps the training drum, thump-thump with tidy zeal,
One paw picks actor paths, the other fixed-as-steel.
Schedules whisper “now switch lanes,” the gradients hum along,
Log-probs line in careful rows, importance keeping strong.
With hops through rolls and metrics bright—
We ship this mix by moonlit night. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title “Hejian/mor” is simply the branch name and does not convey any information about the introduction of mixture rollout model support, scheduling, or importance sampling enhancements in PPO training. It fails to summarize the primary change in a clear and concise manner. As such, it does not meet the requirement for a descriptive pull request title. Please rename the pull request to a concise, descriptive title that highlights the main change, for example “Add mixture rollout model support with importance-sampling schedule for PPO training.”
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch hejian/mor

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
verl/trainer/ppo/reward.py (1)

112-177: Remove the hot-path print spam

compute_reward/load_reward_manager now emit >10 print() calls on every invocation. These run on the critical reward loop (often thousands of times per training step) and will hammer stdout, tank throughput, and drown operator logs. Please gate this behind the module logger (e.g., logger.debug) or a configurable flag so the default path stays quiet.

verl/utils/reward_score/__init__.py (1)

43-130: Don't dump per-sample scores with print

default_compute_score is called for every reward evaluation. The new unconditional print()s (including raw result payloads) will spew megabytes of logs and slow the trainer materially. Please switch to the module logger at debug level (or otherwise guard the output) so normal runs stay performant.

verl/utils/reward_score/gsm8k.py (1)

24-115: Replace GSM8K hot-path prints with proper logging

extract_solution / compute_score now print multiple lines for every sample. GSM8K scoring runs in tight inner loops, so this will flood stdout and slow training considerably. Please route these through the module logger at debug (or behind a flag) instead of unconditional print().

examples/grpo_trainer/run_qwen3-8b.sh (1)

5-50: Ensure bash execution and preserve forwarded args

You introduced source and left the script without a bash shebang, so ./run_qwen3-8b.sh now fails on systems where /bin/sh is dash. The updated tail still uses bare $@, which will re-split arguments. Please add #!/usr/bin/env bash at the top and change $@"$@" on the final line.

verl/trainer/ppo/ray_trainer.py (1)

487-504: Guard against “fixed” policy when no rollout-only worker exists.

create_rollout_allocation_schedule({}) yields a 50/50 constant schedule, so existing configs (with no rollout_only worker) will eventually hit the "fixed" branch and _generate_fixed_rollouts raises the “worker not initialized” ValueError. Please short-circuit _generate_rollouts (or adjust the schedule) so that when self.rollout_only_wg is None, we always fall back to the actor path instead of crashing.

🧹 Nitpick comments (1)
docs/design/mixture_rollout_models.md (1)

11-13: Add language hints to fenced code blocks.

markdownlint is flagging the bare code fences; please annotate them (e.g., ```math for the equation snippet, ```text for the directory tree) to keep our docs lint‑clean.

Also applies to: 262-274

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f50e5c2 and 359e326.

📒 Files selected for processing (14)
  • .gitmodules (1 hunks)
  • docs/design/mixture_rollout_models.md (1 hunks)
  • examples/grpo_trainer/run_qwen3-8b.sh (3 hunks)
  • run.sh (1 hunks)
  • sglang (1 hunks)
  • verl/trainer/config/ppo_trainer.yaml (1 hunks)
  • verl/trainer/main_ppo.py (3 hunks)
  • verl/trainer/ppo/ray_trainer.py (12 hunks)
  • verl/trainer/ppo/reward.py (6 hunks)
  • verl/trainer/ppo/rollout_allocation_schedule.py (1 hunks)
  • verl/trainer/ppo/utils.py (1 hunks)
  • verl/utils/reward_score/__init__.py (4 hunks)
  • verl/utils/reward_score/gsm8k.py (2 hunks)
  • verl/workers/fsdp_workers.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
verl/trainer/main_ppo.py (1)
verl/trainer/ppo/utils.py (1)
  • Role (26-38)
verl/trainer/ppo/ray_trainer.py (4)
verl/trainer/ppo/rollout_allocation_schedule.py (6)
  • create_rollout_allocation_schedule (141-176)
  • get_policy_choice (32-41)
  • get_policy_choice (56-59)
  • get_policy_choice (83-88)
  • get_policy_choice (103-107)
  • get_policy_choice (127-138)
verl/protocol.py (6)
  • get (1151-1158)
  • DataProto (329-1109)
  • from_single_dict (487-500)
  • repeat (913-955)
  • pop (687-718)
  • union (747-764)
verl/workers/fsdp_workers.py (2)
  • generate_sequences (890-936)
  • compute_log_prob (940-1002)
verl/single_controller/ray/base.py (1)
  • RayClassWithInitArgs (182-259)
verl/utils/reward_score/__init__.py (9)
verl/utils/reward_score/gsm8k.py (1)
  • compute_score (76-115)
recipe/genrm_remote/reward_function.py (1)
  • compute_score (80-96)
verl/utils/reward_score/math_reward.py (1)
  • compute_score (17-28)
verl/utils/reward_score/sandbox_fusion/__init__.py (1)
  • compute_score (28-121)
verl/utils/reward_score/prime_math/__init__.py (1)
  • compute_score (389-411)
verl/utils/reward_score/geo3k.py (1)
  • compute_score (33-36)
verl/utils/reward_score/math_dapo.py (1)
  • compute_score (242-272)
verl/utils/reward_score/search_r1_like_qa_em.py (1)
  • compute_score (96-128)
verl/utils/reward_score/prime_code/__init__.py (1)
  • compute_score (21-73)
verl/workers/fsdp_workers.py (2)
verl/utils/fsdp_utils.py (2)
  • load_fsdp_model_to_gpu (178-194)
  • fsdp_version (399-405)
verl/workers/actor/dp_actor.py (2)
  • compute_log_prob (297-356)
  • DataParallelPPOActor (48-495)
verl/trainer/ppo/reward.py (3)
verl/workers/reward_manager/registry.py (1)
  • get_reward_manager_cls (43-55)
verl/experimental/agent_loop/agent_loop.py (1)
  • compute_score (332-356)
verl/utils/reward_score/sandbox_fusion/__init__.py (1)
  • compute_score (28-121)
verl/utils/reward_score/gsm8k.py (3)
examples/data_preprocess/gsm8k_multiturn_w_interaction.py (1)
  • extract_solution (29-34)
examples/data_preprocess/gsm8k.py (1)
  • extract_solution (27-32)
examples/data_preprocess/gsm8k_multiturn_w_tool.py (1)
  • extract_solution (29-34)
🪛 GitHub Actions: sanity
verl/trainer/config/ppo_trainer.yaml

[error] 311-340: YAML documentation format check failed: Missing blank lines and inline comments detected in top block rules (e.g., around rollout_only, temperature, top_p, top_k, rollout_allocation_schedule, alpha, alpha_0, beta, max_alpha, gamma, switch_steps, initial_policy). Please fix according to the documented format rules in the top block of this file.

🪛 markdownlint-cli2 (0.18.1)
docs/design/mixture_rollout_models.md

11-11: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


262-262: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 Shellcheck (0.11.0)
examples/grpo_trainer/run_qwen3-8b.sh

[error] 50-50: Double quote array expansions to avoid re-splitting elements.

(SC2068)

run.sh

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


[error] 50-50: Double quote array expansions to avoid re-splitting elements.

(SC2068)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: pre_commit_for_ppo (3.12)
  • GitHub Check: pre-commit (3.12)
🔇 Additional comments (1)
sglang (1)

1-1: Submodule pointer looks good

Pinned SHA addition is straightforward and poses no issues.

Comment thread run.sh Outdated
Comment on lines +1 to +50
# Tested successfully on the hiyouga/verl:ngc-th2.6.0-cu126-vllm0.8.4-flashinfer0.2.2-cxx11abi0 image.
# It outperforms the Qwen2 7B base model by two percentage points on the test set of GSM8K.

set -x
source /home/jobuser/mldev-scripts/setup_mlflow_hf.sh

python -m verl.trainer.main_ppo \
algorithm.adv_estimator=grpo \
data.train_files=/shared/public/sharing/hsang/data/train.parquet \
data.val_files=/shared/public/sharing/hsang/data/test.parquet \
data.train_batch_size=4 \
data.max_prompt_length=512 \
data.max_response_length=1024 \
data.filter_overlong_prompts=True \
data.truncation='error' \
actor_rollout_ref.model.path=/shared/public/elr-models/Qwen/Qwen3-4B/9e1b55c76f4b5bf0d14d37da8010110060f512e0 \
actor_rollout_ref.actor.optim.lr=1e-6 \
actor_rollout_ref.model.use_remove_padding=True \
actor_rollout_ref.actor.ppo_mini_batch_size=1 \
actor_rollout_ref.actor.ppo_micro_batch_size_per_gpu=1 \
actor_rollout_ref.actor.use_kl_loss=False \
actor_rollout_ref.actor.kl_loss_coef=0.001 \
actor_rollout_ref.actor.kl_loss_type=low_var_kl \
actor_rollout_ref.actor.entropy_coeff=0 \
actor_rollout_ref.model.enable_gradient_checkpointing=True \
actor_rollout_ref.actor.fsdp_config.param_offload=True \
actor_rollout_ref.actor.fsdp_config.optimizer_offload=True \
actor_rollout_ref.rollout.log_prob_micro_batch_size_per_gpu=1 \
actor_rollout_ref.rollout.tensor_model_parallel_size=4 \
actor_rollout_ref.rollout.name=sglang \
actor_rollout_ref.rollout.gpu_memory_utilization=0.4 \
actor_rollout_ref.rollout.n=8 \
actor_rollout_ref.ref.log_prob_micro_batch_size_per_gpu=1 \
actor_rollout_ref.ref.fsdp_config.param_offload=True \
algorithm.use_kl_in_reward=False \
trainer.critic_warmup=0 \
trainer.logger='["console","mlflow"]' \
trainer.project_name=${MLFLOW_PROJECT_NAME} \
trainer.experiment_name='qwen3_4b_qwen3_8b' \
trainer.n_gpus_per_node=8 \
trainer.nnodes=1 \
trainer.save_freq=20 \
trainer.test_freq=5 \
trainer.total_epochs=15 \
rollout_only.model_path=/shared/public/elr-models/Qwen/Qwen3-8B/9c925d64d72725edaf899c6cb9c377fd0709d9c5 \
rollout_only.temperature=1.0 \
rollout_only.top_p=1.0 \
rollout_only.top_k=-1 \
rollout_allocation_schedule.type=constant \
rollout_allocation_schedule.alpha=0.5 $@ No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add a bash shebang and quote $@

This script uses source (a bash builtin) but has no shebang, so running it as ./run.sh falls back to /bin/sh where source is undefined. It also appends $@ without quotes, letting spaces/globs in forwarded args get mangled. Please add a #!/usr/bin/env bash header and change the tail to "$@" so the script runs reliably.

🧰 Tools
🪛 Shellcheck (0.11.0)

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


[error] 50-50: Double quote array expansions to avoid re-splitting elements.

(SC2068)

🤖 Prompt for AI Agents
In run.sh lines 1-50: the script relies on bash-specific builtin `source` and
forwards positional args unquoted; add a bash shebang line at the top
(#!/usr/bin/env bash) so the script executes with bash when run as ./run.sh, and
update the final argument forwarding from $@ to "$@" to preserve spaces and
prevent glob expansion.

Comment on lines +311 to +340
rollout_only:
# Model path for fixed policy (only difference from actor_rollout)
model_path: null

# Sampling parameters (only differences from actor_rollout)
temperature: 1.0
top_p: 0.9
top_k: 50

# All other parameters are inherited from actor_rollout_ref.rollout

# config for rollout allocation schedule
rollout_allocation_schedule:
# Schedule type: "constant", "linear", "exponential", "step"
type: "constant"

# Constant schedule parameters
alpha: 0.5 # Probability of using actor policy

# Linear schedule parameters
alpha_0: 0.1 # Initial alpha value
beta: 0.01 # Linear increase rate per step
max_alpha: 1.0 # Maximum alpha value

# Exponential schedule parameters
gamma: 0.1 # Exponential decay rate

# Step schedule parameters
switch_steps: [100, 500, 1000] # Steps at which to switch policies
initial_policy: "fixed" # Initial policy to use

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix YAML layout to satisfy style checks

The newly added rollout_only and rollout_allocation_schedule blocks violate the file’s documented rules (blank line between fields, comments on lines above fields), which is why the sanity pipeline failed. Please move comments onto their own lines and insert the required blank lines so CI passes.

🧰 Tools
🪛 GitHub Actions: sanity

[error] 311-340: YAML documentation format check failed: Missing blank lines and inline comments detected in top block rules (e.g., around rollout_only, temperature, top_p, top_k, rollout_allocation_schedule, alpha, alpha_0, beta, max_alpha, gamma, switch_steps, initial_policy). Please fix according to the documented format rules in the top block of this file.

🤖 Prompt for AI Agents
In verl/trainer/config/ppo_trainer.yaml around lines 311 to 340, the
rollout_only and rollout_allocation_schedule blocks contain inline comments and
missing blank lines that violate the style rules; move all comments to their own
lines immediately above the field they describe (do not keep inline comments),
and insert a single blank line between top-level fields and between logical
groups inside those blocks so each field/comment pair is separated by a blank
line and no comments appear on the same line as a key.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
verl/trainer/ppo/ray_trainer.py (1)

1087-1427: Consider making debug logging configurable.

The extensive debug logging added throughout the training loop (with prefixes like [TRAINING_LOOP], [ACTOR_ROLLOUT], etc.) is helpful for development but may cause:

  • Performance overhead from frequent I/O
  • Log clutter in production deployments
  • Difficulty finding important messages

Consider one of these approaches:

  1. Add a config flag to control debug verbosity:
if self.config.trainer.get("debug_logging", False):
    print(f"[TRAINING_LOOP] ...")
  1. Use proper logging levels:
import logging
logger = logging.getLogger(__name__)
logger.debug(f"[TRAINING_LOOP] ...")  # Only shown if log level is DEBUG
  1. Remove most debug prints before production release, keeping only critical status messages.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 359e326 and fbd7c37.

📒 Files selected for processing (2)
  • test_requirements.txt (1 hunks)
  • verl/trainer/ppo/ray_trainer.py (16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
verl/trainer/ppo/ray_trainer.py (4)
verl/trainer/ppo/rollout_allocation_schedule.py (6)
  • create_rollout_allocation_schedule (141-176)
  • get_policy_choice (32-41)
  • get_policy_choice (56-59)
  • get_policy_choice (83-88)
  • get_policy_choice (103-107)
  • get_policy_choice (127-138)
verl/protocol.py (6)
  • get (1151-1158)
  • DataProto (329-1109)
  • from_single_dict (487-500)
  • repeat (913-955)
  • pop (687-718)
  • union (747-764)
verl/workers/fsdp_workers.py (1)
  • compute_log_prob (940-1002)
verl/single_controller/ray/base.py (1)
  • RayClassWithInitArgs (182-259)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: pre_commit_for_ppo (3.12)
  • GitHub Check: pre-commit (3.12)
🔇 Additional comments (8)
verl/trainer/ppo/ray_trainer.py (8)

341-342: LGTM!

The rollout allocation schedule is properly initialized in __init__ and will be available for use throughout the training loop.


420-428: LGTM!

The method correctly delegates to the factory function with appropriate defaults. The debug logging will help with troubleshooting schedule configuration.


430-451: LGTM!

The rollout-only config is correctly derived from actor_rollout_ref with appropriate overrides. Forcing calculate_log_probs = True is essential for importance sampling when using fixed policy samples.


454-498: LGTM with note on debug logging.

The rollout generation methods correctly:

  • Consult the allocation schedule to choose between actor and fixed policies
  • Mark policy_type in meta_info for downstream processing
  • Handle both async and sync rollout modes
  • Include guards against uninitialized rollout-only worker

The extensive debug logging is helpful during development. Consider reducing verbosity for production deployments to avoid log clutter.


792-810: LGTM!

The rollout-only worker initialization follows the same pattern as other worker types and includes appropriate guards for backward compatibility when rollout-only is not configured.

Also applies to: 856-864


1122-1134: LGTM!

The training loop correctly integrates the rollout generation scheduling:

  • Replaces direct generation with schedule-aware _generate_rollouts
  • Extracts and logs policy_type for monitoring
  • Handles timing metadata properly

1198-1222: LGTM!

The old log-prob computation correctly implements importance sampling:

  • Uses fixed policy log-probs when samples come from fixed policy
  • Uses actor log-probs for actor samples (standard PPO)
  • Includes proper guards and defaults for backward compatibility

This is essential for correct importance sampling when mixing policies.


1296-1316: LGTM!

The actor update correctly handles both policy types:

  • Always updates actor (importance sampling is already handled via log-prob computation)
  • Logs policy_type for monitoring and debugging
  • Maintains backward compatibility with default policy_type

Comment thread test_requirements.txt
@@ -0,0 +1,4 @@
datasets==4.0.0
transformers==4.56.2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Pin targets a nonexistent transformers release.

transformers==4.56.2 hasn’t been published (latest on PyPI is still ≈4.45.x), so any install of this test requirements file will fail. Please retarget an existing release or relax the pin to a valid range before merging.

🤖 Prompt for AI Agents
In test_requirements.txt around lines 2 to 2, the pin "transformers==4.56.2"
points to a nonexistent release and will break installs; update the requirement
to a valid release or a permissive range (for example use a published version
like "transformers==4.45.0" or a range such as "transformers>=4.45,<4.57" or
"~=4.45.0") so pip can resolve and install the package successfully.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
verl/workers/actor/dp_actor.py (1)

279-283: Consider caching the grad_clip value.

The expression self.config.get("grad_clip", 1.0) is repeated three times. Consider caching it in a local variable to improve readability and reduce redundancy.

Apply this diff to cache the value:

     def _optimizer_step(self):
+        grad_clip_value = self.config.get("grad_clip", 1.0)
 
         if isinstance(self.actor_module, FSDP):
-            grad_norm = self.actor_module.clip_grad_norm_(max_norm=self.config.get("grad_clip", 1.0))
+            grad_norm = self.actor_module.clip_grad_norm_(max_norm=grad_clip_value)
         elif isinstance(self.actor_module, FSDPModule):
-            grad_norm = fsdp2_clip_grad_norm_(self.actor_module.parameters(), max_norm=self.config.get("grad_clip", 1.0))
+            grad_norm = fsdp2_clip_grad_norm_(self.actor_module.parameters(), max_norm=grad_clip_value)
         else:
-            grad_norm = torch.nn.utils.clip_grad_norm_(self.actor_module.parameters(), max_norm=self.config.get("grad_clip", 1.0))
+            grad_norm = torch.nn.utils.clip_grad_norm_(self.actor_module.parameters(), max_norm=grad_clip_value)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fbd7c37 and 79e049e.

📒 Files selected for processing (2)
  • .gitignore (1 hunks)
  • verl/workers/actor/dp_actor.py (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
verl/workers/actor/dp_actor.py (4)
verl/base_config.py (1)
  • get (40-53)
verl/utils/torch_functional.py (1)
  • entropy_from_logits (145-149)
verl/workers/critic/dp_critic.py (1)
  • _optimizer_step (134-150)
verl/utils/fsdp_utils.py (1)
  • fsdp2_clip_grad_norm_ (553-566)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: pre_commit_for_ppo (3.12)
  • GitHub Check: pre-commit (3.12)
  • GitHub Check: type-coverage-check
🔇 Additional comments (2)
verl/workers/actor/dp_actor.py (2)

71-74: LGTM! Proper defensive config access.

The use of config.get() with sensible defaults prevents AttributeErrors when keys are missing and aligns with the broader config access pattern across the codebase.


199-199: LGTM! Consistent entropy checkpointing config access.

Both occurrences properly use config.get() with a False default, maintaining consistency across the rmpad and non-rmpad code paths.

Also applies to: 268-268


def _optimizer_step(self):
assert self.config.grad_clip is not None
assert self.config.get("grad_clip", 1.0) is not None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove the now-meaningless assertion.

The assertion assert self.config.get("grad_clip", 1.0) is not None will always pass because config.get() returns 1.0 when the key is missing, and 1.0 is not None. This makes the assertion redundant.

Apply this diff to remove the assertion:

-    def _optimizer_step(self):
-        assert self.config.get("grad_clip", 1.0) is not None
-
+    def _optimizer_step(self):
         if isinstance(self.actor_module, FSDP):
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert self.config.get("grad_clip", 1.0) is not None
def _optimizer_step(self):
if isinstance(self.actor_module, FSDP):
# …rest of the original logic…
🤖 Prompt for AI Agents
In verl/workers/actor/dp_actor.py around line 276, the assertion assert
self.config.get("grad_clip", 1.0) is not None is redundant because get(..., 1.0)
always returns a non-None default; remove that assertion line. If the original
intent was to ensure the key exists or to validate the value, replace the
assertion with an explicit check (e.g., if "grad_clip" not in self.config: raise
KeyError(...) or validate its numeric range) otherwise simply delete the
assertion to clean up the code.

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.

1 participant