Skip to content

feat: add standard PPO training with GAE and value critic (Transformers/FSDP) - #256

Open
xxyyrr598 wants to merge 7 commits into
modelscope:mainfrom
xxyyrr598:add-ppo
Open

feat: add standard PPO training with GAE and value critic (Transformers/FSDP)#256
xxyyrr598 wants to merge 7 commits into
modelscope:mainfrom
xxyyrr598:add-ppo

Conversation

@xxyyrr598

@xxyyrr598 xxyyrr598 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

PR type

  • New Feature
  • Bug Fix
  • Document Updates
  • More Models or Datasets Support

PR information

Summary

This PR adds the first standard PPO + GAE training pipeline to Twinkle, on top of the existing GRPO/DPO RL stack. It introduces the missing PPO components — a token-level GAE advantage estimator, a full-parameter value critic, and a clipped value loss — and provides an end-to-end GSM8K cookbook with LoRA policy + full-parameter critic + vLLM sampler.

What's included

New components

  • cookbook/rl/ppo/ — end-to-end GSM8K PPO training script & shell launcher:
    • LoRA policy (FSDP) + full-parameter critic (FSDP) + vLLM sampler on separate GPU groups
    • Frozen base model as reference policy via forward_only(disable_lora=True) (no extra ref GPUs)
    • ppo_epochs mini-batch updates over fixed rollouts
  • src/twinkle/advantage/gae.pyGAEAdvantage, token-level GAE with:
    • terminal bootstrap = 0, padding masked out
    • per-token KL shaping reward (logp_old - logp_ref) with kl_coef
    • optional advantage normalization
  • src/twinkle/loss/value.pyPPOValueLoss, clipped value objective max((v-r)², (clip(v,v_old)-r)²)
  • src/twinkle/model/transformers/value_model.pyTransformersValueModel, causal-LM backbone with a hidden_size -> 1 value head replacing the LM head
  • src/twinkle/metric/ppo.pyPPOValueMetric (value/return mean, value clip ratio, explained variance)
  • PPOLoss(GRPOLoss) / PPOMetric(GRPOMetric) — semantic aliases reusing the existing clipped policy objective & ratio/clip statistics

Framework generalization (backward-compatible)

  • ModelOutput gains a values field; forward/forward_only can emit values when the loss declares require_values
  • processor packing, nccl_safe wrapper, and loss/base handle the new values tensor
  • New CLI args: --critic-model-gpus, --ppo-epochs, --gamma, --gae-lambda, --kl-coef, --normalize-advantages, --critic-learning-rate, --value-clip

Tests

  • tests/advantage/test_gae.py — single/multi-step GAE, terminal bootstrap, padding mask, normalization, sparse reward + KL shaping
  • tests/loss/test_ppo.pyPPOLoss parity with GRPOLoss, clipped/unclipped value loss, ragged token alignment
  • tests/model/test_value_model.py — value-head forward/backward, save/load
  • tests/cli/test_cli.py — new PPO CLI args

Docs

  • README / README_ZH cookbook table updated with the PPO entry

Results

explained variance KL Clip Ratio reward

Usage

cd cookbook/rl/ppo
bash ppo.sh   # default 12 GPUs: 4 policy + 4 critic + 4 sampler

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

del logits
if loss_require_values:
values = outputs['logits']
outputs['values'] = values.squeeze(-1) if values.shape[-1] == 1 else values

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

确认下这里是否要做shift移位

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

此处无需对 values 额外 shift。Twinkle 的 labels 已遵循 causal-LM 的预移位约定,logits[t] 直接与 labels[t] 对齐;Value Model 的 values[t] 也表示预测该 token action 前状态的 V(s_t)。PPO 中 logprob、value、reward 均按同一 labels mask 选取,GAE 所需的 V(s_{t+1}) 在 advantage 计算阶段通过 next value 获取,末位置 bootstrap 为 0。若在此处单独 shift values,会破坏该对齐关系。

Comment thread src/twinkle/infra/_ray/ray_helper.py Outdated
Comment on lines +74 to +90
# Auto-detect device resources for accelerators that Ray doesn't
# natively recognise (e.g. Ascend NPU). Ray auto-detects GPU via
# nvidia-smi, but has no built-in detection for NPU / other devices,
# so without explicit resources Ray nodes report 0 NPU and the
# subsequent ResourceManager assertion fails.
_non_cpu_types = {g.device_type.upper() for g in device_groups} - {'CPU'}
_resources: dict[str, float] = {}
if len(_non_cpu_types) == 1:
_dev_type = next(iter(_non_cpu_types))
if _dev_type == 'NPU':
try:
import torch
_npu_count = torch.npu.device_count() if hasattr(torch, 'npu') else 0
if _npu_count > 0:
_resources['NPU'] = float(_npu_count)
except Exception:
pass

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

单独抽象成一个方法比较好

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NPU 资源探测逻辑抽取为 RayHelper._get_ray_custom_resources,使 initialize() 只负责 Ray 初始化。
该方法仅在 device group 为 NPU 时通过 torch.npu.device_count() 注册 {"NPU": count};GPU 继续由 Ray 原生自动发现,因此不会引入额外的 GPU custom resource。对于 CPU、GPU 或当前不支持的混合设备组,方法返回空字典,保持原有行为。
这里的资源注册用于当前进程启动本地 Ray 的场景;若连接已启动的 Ray 集群,则仍需在各节点启动 Ray 时注册 NPU resource。

Comment thread src/twinkle/cli/cli.py Outdated
gkd_temperature: float = 1.0
gkd_topk: int = 64
router_replay_mode: Literal['disabled', 'R2', 'R3'] = 'disabled'
ppo_epochs: int = 4

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

是不是可以复用 TrainArgs.num_train_epochs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

已经修改成使用TrainArgs.num_train_epochs

Comment thread .gitattributes
@@ -0,0 +1 @@
* text=auto

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

这里是为什么

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

这个改动纯粹是行尾规范化,不影响任何运行逻辑,整个文件只有一行:

作用:

  • 匹配所有文件;
    text=auto 让 Git 自动区分文本/二进制文件。文本文件入库时统一按 LF 存储(无论提交者是什么平台),检出时再按当前平台转回(Windows 用 CRLF,Linux/macOS 用 LF);二进制文件完全不碰。
    为什么加:

我在 Windows/WSL 下开发,而 CI 跑在 Linux 上。不做规范化的话,只改一行代码就可能因为行尾(CRLF vs LF)变化导致整个文件显示成全量修改,污染 diff、干扰 review。
同时避免把 CRLF 意外提交进仓库,防止 Linux CI 上出现由 \r 引起的偶发失败(比如 shell/python 文件)。
它只影响 Git 对后续提交的行尾处理,不改变任何代码、依赖或行为。如果维护者认为这个 PR 不需要包含该规范化,我可以移除。

Comment thread src/twinkle/loss/grpo.py
return LossOutput(loss=loss, num_tokens=0)


class PPOLoss(GRPOLoss):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

不需要重写_aggregate_loss 吗

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

确认后,PPO 的 policy loss 更适合默认按所有有效 response token 直接求平均,即 token-mean。这样每个 action token 权重一致,也与 PPOValueLoss 当前的 token-level 聚合方式保持一致。
同时在 PPOLoss 中保留了 seq-mean-token-mean 模式,即先对每条 response 内的 token loss 求平均,再对 batch 求平均。该模式能保证不同长度的 response 具有相同总权重,但会使短 response 中单个 token 的权重更大。
目前这个选择仅用于 PPO,因此没有添加到公共 CLI 参数中,而是在 PPO cookbook 中显式指定 loss_agg_mode='token-mean'。等后续其他算法也需要统一选择聚合方式时,再考虑将其抽象为公共配置。两种聚合模式均已补充单元测试。

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.

3 participants