【Task.27】feat(algorithms): add GDPO multi-reward advantage estimator - #277
【Task.27】feat(algorithms): add GDPO multi-reward advantage estimator#277Men1scus wants to merge 22 commits into
Conversation
|
整体架构方向是正确的:算法名称、能力和三段实现已经集中到 AlgorithmSpec;CLI、reward、advantage、policy loss 和角色拓扑均由注册表驱动;没有发现 if algorithm == "gdpo";多 reward key、示例和文档也比较完整。现有算法的 kernel 没有被改写,相关 parity 测试提供了较好的回归保障。 不过,当前 GDPO 数值实现还有以下问题需要解决: [P1] Noise floor 会清零真实的 GDPO 信号,偏离公式combine_group (https://github.com/redai-infra/Relax/blob/53ad6534231523371c46368e88bb8cf893fd6510/relax/algorithms/rewards.py#L314-L321) 在 Σ_k w_k z_ik 之后增加了一个 all-or-nothing if max(abs(combined)) <= floor: 这不是 GDPO 的标准化与加权合并公式,而且当前测试已经证明它会把真实的非零结果清零:
这些 characterization tests 实际上是在固定已知公式偏差,而不是验证 GDPO 公式正确性。建议移除或重新设计该 heuristic:训练目标应严格保留有限、非退化的 Σwz,数值异常可以 fail-fast 或单独做 [P1] Reward 权重被提前量化为 float32,可能改变 Eq.7 的结果weight_tensor 当前使用 float32 (https://github.com/redai-infra/Relax/blob/53ad6534231523371c46368e88bb8cf893fd6510/relax/algorithms/rewards.py#L356-L383),而 reward component 可复现例子:
建议权重也保持 float64,至少到加权合并以及 batch whitening 完成后再转换训练所需 dtype,并补对应回归测试。 [P2] Float64 合并结果可能在隐式 float32 交接时溢出并静默归零Reward 侧只检查 combined 在 float64 中有限,但 advantage 侧会在 _as_reward_tensor 例如两个同向分量 [0,1,2]、权重 [3e38,3e38],reward 侧产生有限的约 ±5.9994e38,转 float32 后变成 ±inf,最终整批 advantage 被静默清零。 建议让 whitening 保持 float64,或在实际 dtype handoff 后检查 finite;非有限输入应该显式报错,不能解释成零方差。 测试与文档建议当前 755 个算法测试覆盖面很好,但 Megatron 接线测试仍主要依赖源码正则断言 另外,PR body 和 RFC #218 §2.2 (#218) 仍声称等权重下 (1,0) 与 (0,1) 这种恒和分量能被 GDPO 已验证通过的部分
修复上述公式与 dtype 问题并补回归后,这个 PR 的整体架构方案具备验收基础。 |
|
GDPO 的整体接线完成度较高:没有新增 if algorithm == "gdpo",多 reward key、缺失/非数值 reward、零方差和 reward collapse 均有覆盖;每个 reward 组内标准化、合并及 optimizer-batch 分段白化的正常输入路径也基本正确。focused CPU tests 共 755 passed,示例和中英文文档也比较完整。 当前仍建议 Request changes,原因如下:
另外建议补一个真正经过 loss.compute_advantages_and_returns -> registry -> gdpo 的 CPU/Gloo 测试。当前大部分分布式测试直接调用纯函数,backend 接线主要通过源码检查;这不足以锁住未来的 dispatch、DP segment 和 dtype 回归。 最后,RFC #218 (#218) 和 PR body 关于等权重 (1,0)/(0,1) 恒和 reward 的描述也需要修正:标准化后两列仍互为相反数,等权重 GDPO 同样会抵消为零。仓库 GDPO 专项文档已经写对,建议同步 RFC 和 PR body。 建议处理顺序:#276 rebase 最新 main 并注册化 RLOO → 合入 #276 → #277 rebase → 修复 GDPO 公式与 dtype 问题 → 重跑 CPU tests 和 pre-commit。 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a declarative algorithm registry (AlgorithmSpec / ALGORITHM_SPECS) to make --advantage-estimator a single source of truth across reward normalization, advantage computation, policy loss dispatch, and controller role wiring, and adds GDPO as a multi-reward advantage estimator with per-component group standardization plus per-batch whitening.
Changes:
- Adds an algorithm registry (
relax/algorithms/*) and migrates existing estimators to registry-driven dispatch (reward/advantage/policy loss/role mapping). - Implements GDPO end-to-end: reward-component extraction + decoupled group normalization (rollout side) and segmented batch whitening (advantage side, including distributed correctness).
- Adds extensive characterization, wiring, and distributed tests, plus runnable GDPO example and documentation updates.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| relax/algorithms/init.py | Exposes registry API while keeping top-level imports lightweight. |
| relax/algorithms/spec.py | Defines AlgorithmSpec and ALGORITHM_SPECS as the single source of truth. |
| relax/algorithms/advantages.py | Centralizes advantage estimators and shared dispatcher (incl. GDPO whitening + segmentation). |
| relax/algorithms/policy.py | Registry-driven policy loss dispatch adapters. |
| relax/algorithms/numerics.py | Shared numeric helpers/constants for whitening/collapse detection and distributed stats. |
| relax/algorithms/rewards.py | Registry-driven reward normalizers, including GDPO decoupled multi-reward normalization and shared signal/metric helpers. |
| relax/utils/utils.py | Replaces estimator-name whitelists with registry-driven reward normalization dispatch; fixes raw_reward scalar fallback behavior. |
| relax/utils/types.py | Adds Sample.get_reward_components() for multi-reward algorithms. |
| relax/utils/metrics/metric_utils.py | Gates RLOO diagnostics based on declared reward normalizer rather than estimator name. |
| relax/engine/filters/dynamic_sampling_filters.py | Makes the “nonzero std” filter algorithm-aware (component-aware for GDPO), and aligns labeling with shared helpers. |
| relax/components/advantages.py | Replaces local estimator branching with shared compute_advantages_and_returns() dispatch. |
| relax/backends/megatron/loss.py | Uses registry-driven advantage + policy loss dispatch; forwards DP group and per-mini segmentation metadata for GDPO. |
| relax/core/registry.py | Derives ALGOS role mapping from the algorithm registry; keeps sft separate. |
| relax/agentic/rollout.py | Aligns zero-std metrics with shared helpers and avoids estimator-name branching. |
| relax/distributed/ray/rollout.py | Aligns zero-std metrics with shared helpers and avoids estimator-name branching. |
| examples/gdpo/reward_gdpo.py | Provides a two-component example reward (correctness/format) with GSM8K label normalization. |
| examples/gdpo/run-qwen3-0.6B-1xgpu-gdpo.sh | Adds a runnable single-GPU GDPO training script wiring reward + keys/weights. |
| examples/gdpo/README.md | Documents GDPO behavior, constraints, and known numerical limits for the example. |
| examples/gdpo/init.py | Adds package marker for example module import paths. |
| examples/algorithms/README.md | Documents GDPO option and links to the GDPO example directory. |
| examples/generate_reward_model/post_process_genrm_swap.py | Updates “GRPO normalize” helper to be registry-driven and robust to new algorithms. |
| docs/zh/guide/configuration.md | Updates estimator options text to reflect registry-driven choices; includes GDPO. |
| docs/en/guide/configuration.md | Same as zh configuration update. |
| docs/zh/guide/adding-an-algorithm.md | Adds a guide describing how to extend algorithms via the registry. |
| docs/en/guide/adding-an-algorithm.md | English version of the “adding an algorithm” guide. |
| docs/zh/examples/algorithms.md | Adds GDPO documentation section, parameters, and constraints. |
| docs/en/examples/algorithms.md | English version of GDPO documentation section. |
| docs/.vitepress/config.mts | Adds navigation entries for the new “adding an algorithm” docs pages. |
| tests/algorithms/test_algorithm_registry.py | Validates registry contents, invariants, and identifier resolution. |
| tests/algorithms/test_reward_normalizers.py | Bitwise characterization tests to ensure refactor preserves legacy normalizer behavior. |
| tests/algorithms/test_post_process_rewards_dispatch.py | Ensures post_process_rewards dispatch is registry-driven and preserves raw/normalized behavior. |
| tests/algorithms/test_policy_loss_dispatch.py | Ensures policy loss selection and call sites are registry-driven and not name-branched. |
| tests/algorithms/test_advantage_estimators.py | Golden-value tests for extracted advantage estimators including GDPO whitening. |
| tests/algorithms/test_multi_reward_consumers.py | Ensures upstream consumers (filters/metrics) ask the correct algorithm-dependent “signal” question for GDPO. |
| tests/algorithms/test_gdpo_loss_wiring.py | End-to-end Megatron loss entry point wiring test for GDPO segmentation/whitening. |
| tests/algorithms/test_distributed_whitening.py | Real 2-process gloo tests validating distributed whitening + deadlock avoidance for segmentation mismatch/malformed metadata. |
| tests/algorithms/test_algos_roles.py | Ensures controller role mapping derives from the registry and matches critic needs. |
| tests/algorithms/test_example_reward_gdpo.py | Validates the shipped GDPO example reward and launch script wiring and doc consistency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| """ | ||
| try: | ||
| return group_carries_reward_signal(args, samples) | ||
| except (TypeError, ValueError) as exc: |
`--advantage-estimator` was interpreted independently in six places: role lookup, reward normalisation, the advantage formula, the policy loss, and two rounds of argument validation. Adding an algorithm meant finding all of them, and missing one failed late -- `reinforce_plus_plus` was accepted by argparse while absent from `ALGOS`, crashing in `controller.register_all_serve`. `AlgorithmSpec` states those facts once. It holds string identifiers rather than callables: the advantage formula runs in the `Advantages` Ray Serve deployment while the policy loss runs in the Megatron worker, and those two processes import different module subsets, so each resolves the name against its own table. That also keeps the module free of heavy imports, which is what lets the registry be tested on a CPU-only runner. Only fields with a consumer are included. Role topology comes from `needs_critic`, so PPO keeps its Critic and nothing hard-codes the name; `process_role` is untouched, since it selects the role *iteration order* and that is the controller's orchestration surface. Both advantage call sites now share one handler while keeping their real differences: the Megatron path passes `padded_total_lengths` (GAE slices CP shards at padded offsets, and passing nothing there reads the wrong token positions rather than raising), the Advantages deployment cannot compute it and passes nothing. RLOO (redai-studio#205), which landed on main after this branch opened, is migrated in rather than merged alongside: keeping either side of that conflict was wrong, since taking this branch drops RLOO and taking main restores the algorithm-name branches. Its reward stage becomes the `group_leave_one_out` normaliser, its unclipped objective a `POLICY_LOSS_FNS` entry, and its eleven startup constraints six capability fields. `requires_on_policy_updates` is one field for five of those knobs because they have one cause -- an objective with no importance-ratio correction cannot account for the policy having moved -- and the spec says so, including that RLOO is currently its only member. Two duplicate REINFORCE++ name sets in `loss.py` (advantage normalisation at main's 691, the loss reducer at 851) become `advantage_normalization`. They had to stay in step because token-global normalisation is only correct together with the mask-safe reducer, and nothing enforced that. The test that was supposed to catch them was blind: it banned `args.advantage_estimator in [` while the implementation wrote `in {`. It is now a regex over every spelling, with its own test, and reintroducing `in {` turns it red. No behaviour change is intended: the normalisers reproduce the previous arithmetic constant for constant, including the 1e-6 group epsilon and the `--disable-grpo-std-normalization` gate, and RLOO's reward output is compared against a transcription of main's inline branch rather than against the helper it shares. `apply_custom_config_overrides` re-runs every algorithm validator, not two of them. `validate_reward_side_kl`, `validate_update_schedule` and `validate_batch_shape` were split out of `validate_algorithm_args` because validation has a derivation order, and only two of the four were wired back into the override path -- so a YAML file could select rloo and then set `--kl-coef`, `--num-steps-per-rollout 4`, or a `global_batch_size` that breaks the one-update guarantee, with nothing objecting. `derive_global_batch_size` is extracted for the same reason: `validate_batch_shape` reads the value that derivation writes, and re-running the validator without it rejected a legitimate config (a YAML moving 4 steps to 1 got compared against the batch size derived from 4). The comment above the calls lists what this still does *not* cover -- six non-algorithm checks that run before the merge -- because closing that class means merging the YAML before validation, which is larger than this change. Provenance in `test_dispatch_parity_vs_main.py` was wrong and is corrected: the header named a main SHA that does not exist in the repository, `MAIN_SHA` held a third, unrelated commit, and the transcribed line numbers pointed at a July revision with no rloo in it. All of them now name main@4899b8f3a90489840a736897b4c341d87c6267cf and its actual lines; nothing between 98a7234 and that commit touched advantages.py, loss.py, utils.py or ppo_utils.py. Docs: the estimator table listed every registered algorithm except the one this branch adds, and the module tree named `numerics.py`, which does not exist here. Tests: 1580 passed, 323 skipped. The 2 failures + 2 errors are identical to main@4899b8f on this machine (no /dev/shm on macOS; one pre-existing reward_router failure), verified in a detached worktree at that commit. `pre-commit run --all-files` clean.
GDPO (arXiv 2601.05242) standardises each reward component within its prompt group before combining them. A group whose rollouts share the same summed reward but differ in their components -- (correct, badly formatted) and (wrong, well formatted) both sum to 1 -- carries no signal under GRPO and is discarded; GDPO keeps both components' signal. Three steps, split across the two stages that already exist: steps 1 and 2 (per-component group standardisation, weighted combine) run in the reward normaliser on the rollout side and collapse to one scalar per sample, so the TransferQueue schema is unchanged. Step 3 (batch whitening) runs in the advantage stage, where the data-parallel group exists. Step 3 is whitened per training batch, not per merged rollout: the caller concatenates `num_rollout_minis` batches before the advantage stage, and whitening across the join centres both against a shared mean -- 4 of 8 samples flipped sign in a hand-checked case, which is a different objective rather than a rounding difference. `_whiten_by_segment` splits them back using `mini_batch_sizes`, so Eq. 6 holds regardless of `num_rollout_minis`. The six capability flags added here all have GDPO as their consumer: `forbids_normalize_advantages` (step 3 already whitens per sequence), `requires_rewards_normalization`, `supports_fully_async=False` (that mode's single-replica service sees one slice and a slice of one sample whitens to zero -- silently), `uses_reward_components`, `min_group_size=2` (an unbiased group std is undefined for one sample), and `allows_reward_post_process_hooks` (both hooks return ahead of the normaliser and would skip steps 1 and 2). Numerics: GDPO_EPS is 1e-4, matching the reference implementation, and deliberately not the 1e-6 the GRPO path is frozen at. Collapse is detected by exact equality rather than a tolerance -- any relative tolerance wide enough to catch float error also discards the genuinely informative [10000, 10000.005, 10000.010]. `distributed_mean_std` is two-pass in float64; the one-pass form returns variance 0 for [1000.0, 1000.01, 1000.02, 1000.03]. Verified on a single H100: Ray Job succeeded, 4 optimizer steps, loss / grad_norm / advantages all finite, no NaN or Inf (RELAX_REVISION=3caa34cc4c4b8c2f0c9aa654ddb911e1a639d9a2). rollout/advantages averaging 0 is by construction; the non-zero pg_loss (0.18-0.34) and grad_norm (1.29-2.22) are what show the gradient is real. Tests: 1637 passed, same 2 failures + 2 errors as main@98a1274 on this machine.
… overflow Two failure modes that were silent rather than loud. Segmentation. `_whiten_by_segment` runs one collective per segment, and the comment claimed every rank would agree on the count because `num_rollout_minis` comes from the minibatch plan. Expected is not checked, and the consequence of being wrong is a deadlock: a rank expecting two segments and a rank expecting three both block on the third collective, with no traceback and no exit. The same applies to a rank whose `mini_batch_sizes` is malformed -- raising locally strands its peers inside the collective sequence, which is worse than not validating at all. Both verdicts now travel in one MAX all-reduce ahead of any segment, so every rank reads the same numbers and raises together. Tests cover unequal shards across ranks (a DP split balances tokens, not sample counts), mismatched counts, the `None`-versus-segmented case, and malformed metadata on one rank; each asserts that *both* ranks return, so a regression times out instead of passing quietly. Overflow. `extract_reward_components` checked `math.isfinite` on the float64 value, then cast to float32. A reward of 1e300 passes that check and becomes `inf`, which reads as a non-finite std one stage later, zeroes the batch, and lets the run finish cleanly having trained on nothing. Rewards above float32's range are now rejected at the boundary, where the message can name the reward function that produced them. 1e30 still passes: the bound is the representable range, not an opinion about reward magnitude. Tests: 1645 passed, same 2 failures + 2 errors as main@98a1274 here.
Three consumers sit ahead of the advantage stage and ask "did this prompt group carry signal?" -- the dynamic-sampling filter, and the zero-std metrics on both the agentic and the Ray rollout paths. All three answered it with the single scalar `--reward-key` selects. For a multi-reward algorithm that is the wrong question, and wrong in the direction that undoes the algorithm. A group of `(1, 0)` and `(0, 1)` rollouts has an identical summed reward on every sample, so the filter reads it as dead and drops it before training -- while GDPO would have extracted signal from both components. Those are precisely the groups the estimator exists to keep, so leaving this unfixed means the reward stage does its job and the sampler throws the result away. `group_carries_reward_signal` asks the question the configured algorithm actually needs: the `--reward-key` scalar for single-reward algorithms, every component for multi-reward ones (a group is dead only when all of them are flat). The three call sites now share it. Two incidental fixes that fall out of touching these lines: - The zero-std metrics gated on `advantage_estimator == "ppo"`, two more hard-coded algorithm names. They now read `needs_critic`, which is what the gate meant: value-based estimators do not compute group-relative rewards. Same set of algorithms, no behaviour change. - The filter used `std > 0` while the metrics used exact equality, for the same question. Unified on exact equality, matching `is_collapsed` and for the reason argued there. The two differ only for a group that is exactly flat yet whose computed standard deviation is not exactly 0. The `--dynamic-sampling-filter-path` warning now fires only for a *custom* filter; the built-in one is correct here and no longer warrants one. Also fixes a flake I introduced in the previous commit: the segmented whitening tests derived a fixed port from `hash(mode)`, which is salted per interpreter and collided with `test_gdn_cp_reassembly`'s dynamically chosen port. Both now ask the OS for a free port. Tests: 1653 passed, twice in a row; same 2 failures + 2 errors as main@98a1274 here.
A multi-CLI review found that the case this feature has been advertised on
since the RFC is mathematically impossible, and that several guards were
narrower than their own comments claimed.
**The motivation was wrong.** "(correct, badly formatted) and (wrong, well
formatted) both sum to 1, so GRPO flattens the group and GDPO does not" is
false in its second half. If the components sum to a constant then
`r2 = C - r1`, hence `std(r2) == std(r1)` and `z2 == -z1`, so equal weights
cancel to exactly zero -- the same answer GRPO gives. No choice of samples
changes this; only unequal weights break the tie.
What GDPO actually buys, verified numerically and now documented:
- *Relative strength between groups.* Per-group standardisation forces every
group to unit variance, so a group where one component varies and a group
where both vary come out identical under GRPO (both ±0.707). Standardising
per component makes the second twice the amplitude, and step 3 whitens
across the batch, so it survives (±0.548 vs ±1.095).
- *Scale disparity between components.* correctness in {0,1} summed with a
reward in the hundreds yields a sum whose variance is essentially the large
component's. With the two ranked oppositely, GRPO gives the wrong-but-long
responses the highest advantage; GDPO does not.
**The consumers encoded the wrong question too.** `group_carries_reward_signal`
returned True whenever any raw component varied, which disagrees with the
advantage actually produced whenever a weight mutes a component or two
components cancel -- it kept groups that then contribute no gradient. It now
computes steps 1 and 2 (cheap, per-group) and asks whether the result is
non-zero.
**Single-reward judgement is back to float32.** The previous commit compared
Python floats and described that as equivalent to the `std > 0` it replaced.
It is not: `[0.1 + 0.2, 0.3, ...]` survives in float64 and collapses on cast,
and a group of NaNs reads as varying. Both were kept and then contributed
nothing. Judged on a float32 tensor, `min == max` and `std == 0` agree on
every input, so this is now genuinely the old behaviour; non-finite groups are
dropped as they were.
**Overflow checks reached only the cast.** Individual rewards were bounded by
float32's range, but the mean, std and weighted sum are computed in float32
too: [3e38, 2e38, 1e38, 0] overflows to -inf and gets silently zeroed. Weights
had the same gap in both directions (1e300 casts to inf; [1e-50, 0] casts to
all-zero after passing the "not all zero" check). All three now raise.
**`_as_reward_tensor` leaked autograd.** `torch.tensor(x)` detached; `.to()`
returns the same object when dtype and device match, so a reward tensor with
`requires_grad=True` produced advantages carrying grad history. Detached.
Also, from the same review:
- `loss.py` chose the REINFORCE++ normalisation and mask-safe reducer by
comparing algorithm names -- the last *maths* decision still made that way,
and the one PR1 most conspicuously missed. Both now read a new
`advantage_normalization` field. `loss.py` has no algorithm-name comparisons
left.
- `apply_custom_config_overrides` re-ran only the spec validator, so a YAML
file could switch to `reinforce_plus_plus_baseline` and enable a reward hook
it forbids: the spec is deliberately silent there, and the frozen validator
never ran. It now runs both, in the main path's order.
- With that fixed, `reinforce_plus_plus{,_baseline}` can declare the four
constraints they were leaving at defaults. An undeclared field is not
neutral -- it asserts the default -- so the spec had been stating four false
things about them.
- Docs claimed the built-in filter judges by `--reward-key` (untrue since the
previous commit), and claimed exact scale invariance (the additive epsilon
makes it approximate). The test oracle used a relative collapse tolerance
while the implementation uses exact equality; binary rewards hid the
disagreement.
Tests: 1657 passed, same 2 failures + 2 errors as main@98a1274 here.
Both were flagged in review as tautologies, and both were. `test_both_paths_delegate_to_the_shared_estimator` asserted only that the import line exists. A file that imports the shared handler and then ignores it satisfies that -- which is what a botched revert looks like. It now asserts the call, plus the absence of any direct `ppo_utils` kernel in either path: if either grows its own `get_grpo_returns` again, the estimator is being chosen outside the registry. Verified by mutation: re-adding that import to loss.py turns the test red. `test_no_estimator_name_comparisons_remain` claimed nothing in arguments.py compares algorithm names while checking two literals -- and `_validate_reinforce_plus_plus_args` does compare names, deliberately, as a frozen contract. Widening it to the whole file would either fail or need an exception list that hides the next regression. It is now scoped to `validate_algorithm_args`, named accordingly, and reads the function through `ast.unparse` so the check sees code rather than the docstring's own description of the pattern it removed. Any comparison inside that function fails it now, not just two spellings. Tests: 1657 passed, same 2 failures + 2 errors as main@98a1274 here.
…lying Second review round. The first finding is a regression I introduced in this branch; the rest are gaps the previous round left. **`loss.py` became unimportable without a full Megatron install.** Importing `ROLLOUT_MINI_LOCAL_SAMPLE_COUNTS_KEY` from `relax.backends.megatron.data` pulls in `megatron.core.packed_seq_params` at module scope, and the CPU tests for `loss.py` stub `megatron.core` without that submodule. One import for one string cost two existing tests: `test_reinforce_plus_plus_wiring.py` and `test_reinforce_plus_plus_loss.py` went from 4 passed on main to 2 failed here. The full suite hid it -- run together, some earlier test installs a stub that makes the import succeed, so `pytest tests/` stayed green while `pytest tests/backends/megatron/test_reinforce_plus_plus_*.py` failed. CI that shards or selects tests would have caught what I did not. `loss.py` now spells the key out, with `test_loss_py_uses_the_canonical_mini_batch_key` comparing it against the definition as source text -- no import either way, and it works on a runner with no megatron at all. **PPO's spec claimed --fully-async support.** `validate_ppo_config` rejects both --fully-async and --hybrid for PPO, but the spec left the field at its default. This is the same class of error the previous commit fixed for the REINFORCE++ variants, in the one algorithm that commit did not touch. Declaring it also moves the failure from service registration to argument validation. **`advantage_normalization` accepted anything.** It is not a lookup key, so `_assert_spec_implementations_resolve` could not cover it -- and its failure mode is worse than a missing key: a typo does not raise, it selects the default branch in `loss.py` and trains with different maths. Now validated against its two legal values. **Step 1 arithmetic moved to float64**, matching `distributed_mean_std` and for the same reason. It also happens to fix an overflow the review found: components like [3e38, 2e38, 1e38, 0] overflowed while computing their own mean in float32 and tripped the combined-value guard even when their weight was zero. In float64 they standardise normally and the zero weight mutes them as it should. **Documented one thing float64 does not fix.** Two components summing to a constant should cancel exactly, and in float64 they do. But they arrive already cast to float32, and `C - r` does not survive that cast unchanged: the pair still sums to `C` (the addition rounds back) while the values have drifted, leaving ~1e-4 after standardisation, which step 3 divides by a batch std of the same order to produce advantages of order 1. A group with no signal gets a confident gradient whose direction is rounding noise. GRPO does not have this failure -- it standardises the sum, which is exactly constant. Recorded in examples/gdpo/README.md; a real fix needs a wider reward dtype end to end or a noise floor in step 3, neither of which belongs here. Tests: 1659 passed, same 2 failures + 2 errors as main@98a1274 here, and the two megatron CPU tests pass standalone again.
…ruths
The second review round showed the corrected motivation was still wrong, in
the opposite direction from the original. Both errors turn out to be the same
formula read at two extremes, so this replaces them with the formula.
The combined advantage is `sum_k w_k * z_k` with each `z_k` at unit variance,
so its variance is `sum_k w_k^2 + 2 sum_{i<j} w_i w_j rho_ij` -- `2 + 2*rho`
for two equal weights. GRPO standardises the summed reward instead and lands
on unit variance for every group regardless. What GDPO preserves is therefore
the **correlation structure** between components.
Both previous claims are special cases:
- "GDPO rescues groups whose components sum to a constant" is `rho = -1`.
They cancel. GDPO included. (Found in round one.)
- "Two varying components mean a stronger signal" is `rho = +1`. True there,
false at `rho = -0.8`, where the combination measures 0.63x a single
component. (Found in round two.) The example used to justify it was G=2,
where the correlation can only be +/-1 -- the one place the claim holds.
Measured across rho: +1 -> 2.00x, 0 -> 1.41x, -0.5 -> 1.00x, -1 -> 0.00x.
The scale-disparity half of the motivation was checked again and stands
unchanged: GRPO's direction is dominated by whichever component has the
largest spread, because it standardises the raw sum.
No code change -- `normalize_gdpo_decoupled` already computed this correctly.
What was wrong was every sentence describing it.
A third adversarial review round found two defects that two prior rounds
missed, both outside the GDPO maths those rounds concentrated on.
Two components summing to a constant should standardise to exact opposites
and cancel. Instead the ill-conditioned division left a residue that step 3
divided by the std of that very residue. Measured on rewards summing to
308.95172119140625, a group carrying no signal came out at
`[-0.5770, 1.1539, -0.5770]` — finite, plausible, and pointing wherever the
last bits of the reward happened to fall.
Two changes, and it is worth separating them because the tempting summary
("the floor stops a fake gradient") is not what happens:
- `extract_reward_components` builds float64. This is the half that matters:
the residue drops from 1e-1 (larger than the signal) to 1e-10, and
`GDPO_EPS` in step 3's denominator caps the amplification at ~1e-6.
- `combine_group` applies a noise floor, so cancellation is *exactly* zero.
1e-6 is not zero, and consumers test for zero: the dynamic-sampling filter
would keep a group contributing nothing. The threshold is computed, not
tuned — `component_noise_scale` returns `eps * max|x| / (std + GDPO_EPS)`,
the condition number of the standardisation, which is fifteen orders below
the signal on well-conditioned rewards and matches the observed residue
(bound 5.1e-10, measured 4.1e-10) on the pathological one.
This was previously documented as an unfixable deviation. That was wrong on
the diagnosis as well as the priority: it is not a precision limit, it is a
gap in the collapse check.
`relax/distributed/ray/rollout.py` did not filter `reward is None` while its
twin in `relax/agentic/rollout.py` did, so the shared helper built a float32
tensor from a None and raised `TypeError` — from a logging helper, on the
rollout's way out. `reward is None` is reachable: under `--group-rm` the group
reward is assigned in one shot that is skipped entirely when the rollout
aborts, which is why `sglang_rollout.py`'s "reward is not None" assert exempts
`group_rm` in the first place. All single-reward algorithms were affected,
including the default.
Separately, eval may run a different reward model (`EvalConfig.rm_type`), so
an eval reward legitimately need not carry `--gdpo-reward-keys`. Metrics were
enforcing a training contract that is not in force there, and failing eval for
it.
Both decisions now live in `metrics_group_verdict`, which returns a tri-state
and is importable without `sglang` — the two copies had drifted apart once and
neither was testable, let alone tested.
`_whiten_by_segment` treated `mini_batch_sizes=None` as "use one window". That
is not a coarser version of the same objective: the repository's own
`test_merging_the_batches_would_flip_signs_not_just_rescale` shows half the
advantages changing sign, with every metric finite. It now raises, through the
existing MAX all-reduce so the whole group fails together.
`--gdpo-reward-weights` is checked for non-finite and all-zero-in-float32
values during argument validation rather than at the first rollout.
- `advantage_normalization` had no test pinning its value — dropping
`"token_global"` from a REINFORCE++ spec, or flipping the comparison in
`loss.py`, left the suite green. Both call sites and the exact set are now
transcribed from main.
- `advantage_gae` had only a `co_names` check. It is now compared numerically
against a transcription of main's inline branch. PPO is the algorithm this
refactor changed most and the one with no GPU smoke.
- `test_grouping_uses_group_index` only ever built a contiguous layout, so a
position-based implementation satisfied it. It now interleaves two groups.
- Every guard above was mutation-tested: each one, removed, turns a test red.
The previous round's corrections landed in the main implementation and left
the periphery behind. The smoke script header still carried the motivation
retracted in 9f1e02e; the filter docstring described "every component is flat"
where the code asks whether the combination cancels; a test docstring cited a
`--global-batch-size` the shipped example does not use; the overflow comment
described a mean that no longer overflows; `.detach()` was called
"load-bearing" when both call sites pass lists and never reach it.
`tests/algorithms` 745 passed. Full suite 1686 passed, with the same 2
failures and 2 errors present on the base 98a1274 (verified in a detached
worktree). `pre-commit run --all-files` clean.
Two defects on either side of `combine_group`, both from a decision that was
made in the wrong place rather than made wrongly.
# 🐛 Bug Fix
## The zero-std metrics still died on an unscored sample
`metrics_group_verdict` moved the *verdict* into one testable place and left
the *label* behind in both copies of `_compute_zero_std_metrics`, where they
promptly disagreed: the agentic copy reads it off the first scored sample, the
distributed copy off `group[0]`, scored or not. The crash it was introduced to
remove did not go away, it moved one line down -- from
`torch.tensor([None], dtype=torch.float32)` to `round(None, 1)`, still a
`TypeError` out of a logging helper on the rollout's way out.
Two ways in, and they are not the same bug:
- A group where nothing was scored. The distributed copy counts those
deliberately (`is not False`), then asks for a label that cannot exist.
Reachable on `98a1274` too, where the old predicate also counted them:
pre-existing, not a regression.
- A flat group whose first sample is the unscored one. `[None, 0.5, 0.5]`
verdicts as flat and `group[0]` is the None. The old predicate returned
False here and never reached the label, so this one *is* a regression.
`zero_std_group_label` now answers "which reward does this group get filed
under" in one place, off the first scored sample, returning `None` when there
is no scored sample at all -- not a third policy, just the absence of a label
to file it under. Both callers drop that group. The source assertion that
pinned "both copies delegate the verdict" now pins the label too, since
splitting one decision out and leaving its other half behind is how these
copies drifted the second time.
## The noise floor could zero a group that carries signal
`component_noise_scale` is `eps * max|x| / (std + GDPO_EPS)`, so it grows with
the ratio of a reward's magnitude to its own spread. The claim it shipped with
-- fifteen orders of margin, nothing real is ever suppressed -- holds on
well-conditioned input, which is where the only test measured it. It does not
hold generally:
| base | spread | floor | \|combined\|max | zeroed |
| ---- | ------ | ------ | --------------- | ------ |
| 1e9 | 0.1 | 1.8e-5 | 0.999 | no |
| 1e12 | 1e-3 | 1.65 | 0.907 | yes |
| 1e15 | 1.0 | 1.78 | 0.9999 | yes |
The last two carry an order-1 combined advantage and are zeroed anyway. Such a
reward is finite, well under `_FLOAT32_MAX`, and passes every check in
`extract_reward_components`.
Zeroing it is silent in both directions: groups below the floor are dropped by
the filter with no log at all, and when every group is, the batch warning names
two causes -- rewards that do not vary, weights that cancel -- neither of which
is what happened, so it reads as a reward-function bug that is not there.
`extract_reward_components` already refuses a reward that overflows float32
rather than casting it to `inf`, on the grounds that a silently zeroed
component is indistinguishable from a genuinely collapsed one. A reward with no
significant digits where it varies is the same failure one stage later, and now
gets the same answer.
`_MAX_COMPONENT_NOISE` is 0.01. The values it bounds have unit variance, so it
reads as a fraction: at 1% of the standardised value being rounding, the
gradient's direction is partly noise whether or not the floor zeroes the group.
It needs |reward| to exceed the group's own spread by about 1e14 to trip.
The check lives in `combine_group` rather than its callers for the same reason
the label now does: there are two of them, and a check in one is a check the
other disagrees with. Raising from there also keeps the existing division of
labour -- the filter, which decides whether a group trains, propagates it; the
zero-std metrics turn it into "cannot tell" via `observed_reward_signal` and
keep logging. The observer does not become the enforcer, and eval, which may
run a different reward model entirely, is not taken down by a training-time
contract.
---
# ✅ Tests
## The label, not just the verdict
- A flat group beginning with an unscored sample is counted *and* filed under
the reward its scored samples carry.
- A group with nothing scored has no label.
`relax/distributed/ray/rollout.py` imports `sglang` and cannot be loaded on
CPU, so these run against the shared helper -- which is why it exists.
## Both directions of the threshold
- A reward with no significant digits raises, and the message names the
offending component rather than an index.
- A reward that is merely large (1e9 with a spread of 0.1, eight significant
digits) is left alone.
- The constant-sum group the floor exists for still reaches the floor and
still comes out as exactly zero.
- The metrics report the unreadable group as unknown while the filter refuses
it.
Every guard above was mutation-tested. Restoring `group[0]` reds the label
tests with the production `TypeError`, not an assertion mismatch; removing the
noise check reds three; tightening the threshold to 1e-18 reds ten
*pre-existing* tests. The constant is bounded from both sides, not chosen.
## Verified on GPU
Three Modal H100 smokes on this tree, 8 optimizer steps each: GDPO on 1 and 2
GPUs at `--global-batch-size 16` -- the first runs to reach
`_whiten_by_segment`'s multi-segment path and its MAX all-reduce -- and GRPO on
1 GPU for the registry refactor's default algorithm. Both label paths produced
real counts (`zero_std/count_1.0`, `count_1`, `count_0`) rather than raising,
and no noise check fired on GSM8K's `{0, 1}` rewards, thirteen orders below the
threshold.
# 📝 Documentation ## Point at the guarantee instead of restating the symptom `_whiten_by_segment` lists "segment k holds training batch k on every rank" as relied on and not verified, and said the orders agree because `actor.py` fetches and appends in `batch_index` order. That is true and insufficient: two loops both counting upwards is not a reason for their k-th items to be the same batch. The reason is that the fetch is addressed rather than popped. `_get_data_from_transfer_queue` passes `batch_index` to the TransferQueue sampler, which keys its replay cache on `(partition_id, task_name, dp_rank, batch_index)`, so two ranks asking for the same `batch_index` receive their own shards of the same logical mini-batch. No behaviour change. It matters because the property is unverifiable from inside this function -- it receives a flat tensor and a list of lengths, with no batch identity in either -- so a reader who wants to know whether anything holds it up has to go find that out, and the docstring as written suggested nobody did.
An adversarial consult over 3c616d6 (four independent models, eight reports) found the guard it added does not do what its commit message claims, and that three sentences of that message are wrong. Every measurement below was reproduced locally before being acted on. # ⏪ Revert ## The per-column noise guard could not be made to hold It raised when any column's `component_noise_scale` exceeded 0.01. Six measurements, each reproducible on CPU: - **It does not catch what it was added to catch.** At `base = 4.05e13` two columns measure `noise = [0.0090, 0.0095]` -- both under the threshold, so the guard passes -- and the floor, 0.148, still zeroes a real combined signal of 0.033. The guard sat just above the failure it was named for. - **The floor outgrows any per-column bound.** The floor is `8 * sum(|w_k| * noise_k)`; a per-column test is a max. With 16 components at `noise_k = 0.0064` the floor reaches 0.82 and takes a signal of 0.03..0.07 while every column reads cleaner than 1%. The two quantities do not scale together, so no constant relates them. - **It fires on components that contribute nothing.** `weights = [0.0, 1.0]` with a broken column 0 raises, though `resolve_gdpo_weights` documents a zero weight as the way to mute a component and the floor correctly ignores it. An operator disabling a known-bad component still loses the run. - **float32-provenance rewards walk straight past.** The estimate uses float64's eps. A column varying by exactly one float32 ulp at 1e9 -- its variation *is* rounding -- measures 1.9e-9 and standardises to ±2. - **It weakens as the group grows.** The same offset and step trips at `n = 3` and passes at `n >= 100`, because `std` is a sample estimate. - **The threshold was not bounded by anything.** 0.01 and 0.1 are indistinguishable to the suite; the window is roughly (5e-10, 0.222). A guard that misses the failure it names, fires on configurations the same file documents as supported, and is blind to the most common reward provenance is worth less than the lines it occupies. ## Three claims in 3c616d6's message are wrong - "bounded from both sides, not chosen" -- the mutation argument constrains the threshold to about nine orders of magnitude. Having a bound is not the same as the bound being tight. - "reds ten pre-existing tests" -- it reds 23. The number came from reading a truncated `head -10` of the failure list. - "eval ... is not taken down by a training-time contract" -- `KeyError` is outside the `(TypeError, ValueError)` that `observed_reward_signal` catches, so an eval reward model with a different schema takes the metrics down at the label, past the point that guards them. Fixed below. --- # 🐛 Bug Fix ## The label helper called a sample scored when only its container was `zero_std_group_label` tested `sample.reward is not None`. That is not the same as the *value* being readable, and both gaps reach `round(None, 1)` -- the exact TypeError 3c616d6 set out to remove: - `reward` is a dict and `--reward-key` selects a `None` out of it (a partially failed reward function). The dict is not None. - `reward` is a dict without `--reward-key` at all, raising `KeyError`, which `observed_reward_signal` does not catch. Eval may legitimately run a different reward model (`EvalConfig.rm_type`). The helper now looks for a sample whose reward it can actually read, and returns None when there is none. It is a logging helper; the stage that consumes the reward still refuses the same input. --- # 📝 Documentation ## The floor's real limits, written down where it lives `combine_group`'s docstring now carries the three measurements above -- the floor outgrowing any per-column bound, the two-column case, and the float64 provenance assumption -- plus a fourth: `component_noise_scale` reads as a fraction of the standardised value only while `std >> GDPO_EPS`. Below that the denominator is clamped and a column measuring 0.0022 is 20.7% contaminated. This is the part of the withdrawn guard that was worth keeping. The limitation is real and now stated where the next person will find it, instead of being half-mitigated by a check that raised on the wrong groups. --- # ✅ Tests ## Characterisation, not aspiration The five tests written for the guard are replaced by four that assert the limitation itself -- each is a group the guard would have passed while the floor zeroes real signal. They fail if someone fixes the floor, which is the correct time to revisit them. Two more pin the label holes: a dict holding a None, and a dict missing the key entirely. Mutation-tested both ways: disabling the floor reds five tests including all the new characterisation ones; removing the `value is None` check reds the new label test. 755 passed.
…ut it Three findings from review, all in the same place: the boundary between what GDPO computes and what it decides. **The noise floor is gone.** `combine_group` ended with an all-or-nothing threshold that returned zeros when the combined advantage fell below `8 * sum(|w_k| * noise_k)`. It is not in Eq. 7, and the tests written to document it proved it destroyed real signal: two components at `base = 4.05e13` with every column measuring under 1% noise gave a floor of 0.148 against a true signal of 0.033, and the floor grows with the component count while any per-column measure is a max, so sixteen clean columns reached 0.82. Pinning that as a "known limitation" was the wrong call. The argument for keeping it was that float64 alone leaves 1.4e-10 on a cancelling group rather than a hard zero -- but `GDPO_EPS` caps step 3's amplification, so what reaches the optimizer is 1.2e-6, and the worst pathological case found is 2.6e-3. It traded a 1e-3 error for a 1e-1 one, silently. Both numbers are now tests. What replaces it is a split, not a smaller threshold. The reward stage returns Eq. 7 untouched and *reports* a group whose combined signal is six orders below the terms that produced it. The filter -- `group_carries_reward_signal`, which decides whether a group is worth training on at all -- applies a tolerance, because that is a filtering question and it is the same judgement GRPO's `std > 0` already makes. One test pins the asymmetry so the two cannot be collapsed back together. **Weights stay float64 until the transport cast.** They were cast to float32 one stage before the arithmetic needed it, while the components had been float64 since the previous round. `[16777216, 16777217]` are two distinct configured weights that become the same float32: on a pair of anti-correlated components the configured weights give about +-1 and the quantised ones gave 0. The float32 checks stay -- the combined reward really is carried in float32 -- but as validation of what will survive that cast, not as the dtype of the multiply. **A non-finite advantage raises instead of reading as zero variance.** `whiten_scalar` treated a non-finite std as collapse and returned zeros. The reward stage verifies finiteness in float64; `dict_to_tensordict` and `_as_reward_tensor` then cast to float32, so two same-direction components at weight 3e38 combine to a finite 5.999e38 and arrive as `inf`. The batch was zeroed and nothing was logged. The second half of that guard -- non-finite std from finite input -- is deleted rather than converted, because `distributed_mean_std` accumulates in float64 and it cannot fire. Also: `combine_group` is now checked against an independent transcription of Eq. 4 and Eq. 7 in plain Python, and a second test shows the clamped denominator is the only place the implementation departs from the paper. Tests: tests/algorithms 782 passed; full CPU suite 1774 passed, 323 skipped, with the same 2 failures + 2 errors as main@4899b8f. pre-commit clean.
Every other step-3 test called `advantage_gdpo` or `whiten_scalar` directly, so the wiring between them and `loss.py` was covered only by a regex over the source. That regex pins the text; it says nothing about the numbers. Deleting the `mini_batch_sizes=` argument, or pointing `gdpo` at a different `advantage_fn`, left every numerical test green. These four call `loss.compute_advantages_and_returns` with a real `rollout_data` dict and check what comes back: whitened per training batch, different from whitening the merged rollout, raising rather than defaulting when the segmentation is absent, and not sharing GRPO's branch. Megatron is stubbed rather than skipped. An `importorskip` would mean the file never runs where the rest of the suite runs, which is the same hole in a different shape; only the four `mpu` entry points this path touches are faked, each returning the single-process answer. Both mutations verified: removing the `mini_batch_sizes` forward turns 2 of the 4 red, routing `gdpo` to `grpo_broadcast` turns 3 red. Tests: tests/algorithms 786 passed; full CPU suite 1778 passed, 323 skipped, same 2 failures + 2 errors as main@4899b8f. pre-commit clean.
…asked A four-model consultation on the previous commit found that two of its three fixes were wrong, and that one of them introduced a silent failure. This undoes both and closes three further holes found on the way. **The relative "is this rounding?" ratio is gone.** It replaced the noise floor last round and was worse, because it was wrong in kind rather than in calibration. `magnitude` scales with the *difference* between the weights while `sum_k |w_k| max|z_k|` scales with their *magnitude*, so the ratio measured the weight configuration and not the data -- at G=2 it reduces exactly to `|w1 - w2| / (|w1| + |w2|)`, independent of every reward value in the group. Measured on real inputs it was inverted in both directions at once: it dropped a group whose final advantage was 0.43 and kept one whose 1.08 was pure rounding. The counterexample that settles it uses this branch's own fixture -- `test_weights_closer_than_float32_stay_distinct` asserts the combined value is real signal that must survive, and the filter scored the same input at 2.98e-8 and threw it away. No threshold fixes that. For G >= 3 the centred subspace is at least two-dimensional, so `z_2 = -z_1 + delta*u` with `u` orthogonal to `z_1` is a genuine signal with an arbitrarily small ratio. `group_carries_reward_signal` now asks exactly what the single-reward branch asks, on exactly the values the trainer receives: `min != max` in float32. Zero weights and exact cancellation are still detected -- they produce exact zeros -- and near-cancellation is not, which is now stated as an accepted cost rather than papered over. **`whiten_scalar` was silently zeroing finite batches.** The comment removing its second guard argued float64 accumulation cannot overflow. True and irrelevant: `distributed_mean_std` returns `std.to(values.dtype)`, so `[-FMAX, FMAX]` -- two finite float32 values -- gives a float64 std of `sqrt(2)*FMAX` that becomes `inf` on the way back, and the division returned an all-zero batch. A second path needs no non-finite std at all: `[FMAX]*10 + [-FMAX]` has finite statistics and the float32 `values - mean` still overflows to `-inf`. Whitening now runs in float64 throughout and casts the normalised result back, which closes both. (The arithmetic in that comment was also wrong: ~1.5e231, not 1e153.) **`--custom-config-path` could bypass three RLOO constraints.** Splitting validation into four functions for ordering reasons left only two wired into the override path, so a YAML file could select rloo and then set `--kl-coef`, `--num-steps-per-rollout 4`, or a `global_batch_size` that breaks the one-update guarantee. All four now re-run; ordering does not apply after the merge because every value they read is final. **Two smaller ones, both from the rebase.** `min_group_size` was declared twice in `AlgorithmSpec` (the second silently won and orphaned the first's docstring), and a docstring still pointed at `component_noise_scale`, deleted last round. **The filter's drop label could crash the rollout.** It read `--reward-key` off the first sample, on the drop path only, while the signal test above reads only the component keys -- so a multi-reward run whose rewards carry `--gdpo-reward-keys` but not the scalar raised `KeyError` out of the rollout loop. It now uses `zero_std_group_label`, which the metrics side was already using to refuse the identical input. **What is not fixed, and is now documented as such.** A constant sum leaves a remainder of roughly `ulp(C) / spread`, which grows with the magnitude the components are centred on. At C=1e9 it reaches the optimizer at 2.6e-3; at C=1e13, or with both columns large and straddling a binade, at O(1). Nothing detects it. The previous commit claimed float64 had capped this at 1e-6, and the test asserting so was named "worst measured" while pinning a single point; it now measures three decades and asserts the growth. A companion test records that these are batch-of-one figures -- in a batch that also holds healthy groups the same remainder arrives around 1e-7. Two claims from the consultation were checked and are wrong, recorded so they are not re-investigated: the dynamic filter is not reachable from eval (both `generate_rollout` entry points branch to `eval_rollout` before it), and the removed floor's own failure cases score 1.6e-2 and 1.9e-3 under the ratio criterion, so they were never inherited by it. Tests: tests/algorithms 796 passed; full CPU suite 1788 passed, 323 skipped, same 2 failures + 2 errors as main@4899b8f. The YAML fix is mutation-verified: dropping the three re-added validators turns 3 tests red. pre-commit clean.
…measured A five-model consultation on the previous commit. Three defects it introduced, one it made worse, and four statements in it that measurement contradicts. **The finiteness check was a deadlock.** `whiten_scalar` raised on a local `isfinite` two lines above `is_collapsed`'s all-reduce, so a rank whose shard held the bad value left while every other rank blocked in a reduction it never reached. `is_collapsed`'s own comment warns about exactly this, and `_agree_on_segmentation` was written earlier on this branch to prevent it. The check is now one MAX all-reduce of a flag, so every rank raises together. **A legitimate config was newly rejected.** Re-running `validate_batch_shape` after the YAML merge was half a fix: it reads `global_batch_size`, which the main path derives from `num_steps_per_rollout` *before* the merge. A YAML file switching grpo@4-steps to rloo@1-step should get `rollout * n = 128` and instead was refused against the stale 32. The derivation is now a function both paths call, with the consistency assert skipped on the second run because the value it would compare against is the pre-merge one. **Two contract holes closed.** `whiten_scalar` truncated integer input to all-zero (from the new cast back to the caller's dtype) and still returned zeros for float64 near its own maximum, where the squares overflow inside the reduction. Non-floating input and non-finite statistics now raise. **A missing reward key is no longer filed as a flat group.** Routing the drop label through the tolerant metrics helper fixed a `KeyError` but put "this group scored the same everywhere" and "this reward is missing a key the run requires" in one bucket -- a schema that always omits the scalar would drop and resample forever behind a zero-std count. It gets `unreadable_reward` now. **Four statements the consultation falsified, all mine:** - "in a batch that also holds healthy groups the same remainder arrives around 1e-7" -- six of the ten reports flagged this. It is the `C = 1e9` entry of a table, written as a general rule. Sharing a whitening unit divides by the healthy groups' standard deviation, a constant factor of a few hundred; it does not slow the growth with `C`, and at `C = 1e13` a shared unit still delivers 5e-3. Worse, the unit is the *training batch*, so a degenerate group isolated into its own segment gets no division at all. The docstring now carries the measured table and both caveats; the test is parametrised over three decades and a companion pins the segment case. - "at G = 2 it reduces exactly to |w1 - w2| / (|w1| + |w2|), independent of every reward value in the group" -- only when the two standardised columns come out as exact opposites. Columns that move together give a ratio of 1. The G >= 3 construction is what settles it; the G = 2 identity was decoration and is now marked as the error it was. - "this function exists to have no hole" -- it closes the algorithm-validator holes. The `--ref-load` check, the `kl_coef`/`kl_loss_coef` assert, `_normalize_sync_ppo_kl_args`, the fully-async resource checks and the `rollout_batch_size` derivation all still run before the merge. The comment now lists them instead of claiming coverage it does not have. - `grows_without_bound` -- each standardised column is itself bounded, so the combination saturates: 1e8 to 1e9 is 11x, 1e11 to 1e13 only 6x. Renamed, and the assertions pin monotone growth plus the two ratios that hold rather than a per-decade factor that does not. **The filter's safety is borrowed, and now pinned.** `min != max` is only equivalent to "carries signal" because Eq. 4 centres every column, forcing the combined values to sum to zero within the group so that "all equal" means "all zero". Nothing enforced that. A companion test shows why it matters: step 3 whitens a training batch, not a group, so a constant nonzero group would *not* be whitened away -- the reason the old comment gave for preferring `min != max` was simply wrong. Also: five comments that described the pre-fix behaviour (non-finite std read as collapse; the criterion described as "non-zero"). Tests: tests/algorithms 802 passed; full CPU suite 1794 passed, 323 skipped, same 2 failures + 2 errors as main@4899b8f. pre-commit clean. Two consultation claims were checked and are wrong, recorded so they are not re-investigated: `[-FMAX, FMAX]` is *not* still silently zeroed (the narrowing cast is inside `distributed_mean_std`, whose parameter is now the float64 copy), and weights of (1e4, -1e4) on a constant-sum group do not expose the mixed-batch claim -- opposite weights on anti-correlated columns reinforce, so that group carries full-scale real signal rather than a residue.
…view left The previous commit made the finiteness check collective so all ranks fail together. Nothing exercised it: single-process tests take the `process_group is None` branch, where the reduction is skipped entirely. So the fix for a deadlock was only ever run in the configuration that cannot deadlock. `test_a_non_finite_shard_fails_both_ranks_instead_of_hanging_one` spawns two gloo ranks with the infinity on one of them and asserts both come back refusing. Mutation-verified the only way this one can be: putting the local `isfinite` back makes the test hang -- killed at 75s, against 30s for the suite -- because rank 0 sits in `is_collapsed`'s all-reduce that rank 1 left. **`silent_groups` asked a different question from the filter.** It counted groups whose float64 combination was exactly zero while `group_carries_reward_signal` asks whether the float32 view varies, so the warning an operator reads and the sampler's decision could disagree about the same group. Both now use the float32 predicate, and the message says what it now means. **GDPO declares `forbids_reward_side_kl`.** `advantage_gdpo` hands `kl` to `get_grpo_returns`, which uses it for shape only -- the values are discarded, so `--kl-coef` buys a reference forward pass and changes no advantage. The whole `grpo_broadcast` family has this property and only `rloo` declared it; saying so is free for a new algorithm because no existing configuration is refused, and the four older members are deliberately left alone since rejecting a flag they accept today is an upstream decision. `--use-kl-loss` is unaffected. Tests: tests/algorithms 805 passed, including 9 in the two-rank gloo file.
The review asked for "a CPU/Gloo test that actually goes through loss.compute_advantages_and_returns -> registry -> gdpo". What existed was two halves of that and not their intersection: `test_gdpo_loss_wiring.py` walks the chain with a stubbed `mpu` in one process, and the gloo cases in this file use a real group but call `whiten_scalar` directly. So a `process_group=` dropped from the call in `loss.py` passed both. Two ranks would each whiten their own shard, every rank would come back with plausible numbers, and nothing would say so. This spawns two gloo ranks, points a fake `mpu` at the real group, and calls the production entry point. Rank 1's rewards are twice rank 0's, so shared statistics keep the ratio visible while per-shard whitening flattens both onto the same values. Mutation-verified: setting `process_group=None` in `loss.py` turns this one red and leaves the other nine green. Tests: tests/algorithms 806 passed.
`observed_reward_signal` exists so a reporting stage cannot decide whether training continues: it answers "cannot tell" where the strict version raises. It caught `(TypeError, ValueError)`, which covers `get_reward_components` -- that one raises ValueError for a missing key -- but not the single-reward branch, which goes through `Sample.get_reward_value`. That is a bare subscript (`relax/utils/types.py:171`), so a `--reward-key` absent from the reward dict raised KeyError straight through the handler and took the metrics down. Eval may legitimately use a different reward schema (`EvalConfig.rm_type`), so this is reachable, not hypothetical. Widening the handler rather than narrowing `get_reward_value`: that accessor is shared with the dynamic-sampling filters and the rollout metrics, so changing what it raises is a contract change for callers outside this branch. The comment at the call site records that, and the root cause's location. A test asserts all three levels at once -- `group_carries_reward_signal` still raises KeyError, `observed_reward_signal` returns None, `metrics_group_verdict` returns None -- and reverting the except tuple turns it red. The docstring of the neighbouring label test claimed the gap as a known limitation; it no longer is, so it says what it actually covers. Three stale claims in `rewards.py` docstrings are corrected in place rather than retracted underneath themselves. `combine_group` stated the G = 2 ratio identity as fact and took it back three paragraphs later, so a reader going top to bottom met the false version first; it now states the correct, conditional version once. The remainder table's caption and the filter comment at the `min != max` test both quoted "around 1e-7" as the general mixed-batch case -- it is the C = 1e9 entry of a column that grows to 5.2e-3 at C = 1e13, and a degenerate group isolated in its own training batch gets no division at all.
| bad = torch.tensor( | ||
| float(not bool(torch.isfinite(values).all())), | ||
| dtype=torch.float32, | ||
| device=values.device, | ||
| ) | ||
| if process_group is not None: | ||
| dist.all_reduce(bad, op=dist.ReduceOp.MAX, group=process_group) | ||
| return bool(bad.item()) |
| return torch.zeros_like(values) | ||
| work = values.double() | ||
| mean, std = distributed_mean_std(work, process_group=process_group) | ||
| if not (torch.isfinite(mean) and torch.isfinite(std)): |
|
三条意见和您要的那条接线测试都做了,当前 head 先说一句范围:这条 PR 的 base 还是 [P1] noise floor 删了
我第一版的替代方案(按相对幅度判断「这是不是纯舍入误差」)也是错的,已经删掉。原因:那个比值的分子随两个权重的差变化,分母随权重的大小变化,所以它量的是权重怎么配的,不是数据长什么样。实测两个方向都错——扔掉了一组最终 advantage 是 0.43 的真信号,又放行了一组 1.08 的纯舍入。而且这不是调阈值能救的:组内样本数 ≥ 3 时,可以构造出比值任意小但确实是真信号的输入,任何固定阈值都有反例。 所以现在不做任何「聪明」判断:filter 就在训练实际用的 float32 精度上问一个问题——这一组的值是不是全都一样( [P1] 权重精度配置里的权重和分量合并全程用 float64,只在交给训练的那一步转 float32。 [P2] 交给 float32 时显式失败白化的统计和归一化都在 float64 里算,最后才转回调用方的 dtype。 有一点想请您重点看,因为这段是新写的:判断「有没有非有限值」现在是一次跨 rank 的 all-reduce,不是每个 rank 自己判。 原因是它后面紧跟着 接线测试按您说的补了: 还修了 Copilot 提的一条
我选择加宽这里的 except,而不是去改 验证
还没解决的,如实列出——合入这个 PR 就等于接受这一条两个分量之和恒定时(比如 correctness + format 总是 1),它们标准化之后正好互为相反数,等权重相加应该是 0,但浮点运算留下的舍入残差不是 0,而且这个残差随分量的基数增大。目前没有可靠办法把它和真信号区分开,两种试过的机制都被证伪了,所以我没有再加任何阈值去悄悄把它清零。 补一句量级,因为我之前在这里以偏概全过:如果这一组和健康组落在同一个训练批里,残差会被健康组的标准差除一下,是个几百倍的常数——但它不改变随基数增长的趋势。基数 1e9 时混批是 3.1e-7,基数 1e13 时混批还有 5.2e-3;而且「白化单元」是每个训练批,不是整个 rollout,退化组要是自己独占一个批,这个除法压根不发生。 文档PR 描述里那句关于恒和分量的话已经改了。RFC #218 §2.2 里同样的说法也已更正,记在 #218 (comment) —— 那条评论给出了替换文本和在当前实现上的验证,不劳您代改。 |
Both guards on the whitening path forced the GPU to wait on the CPU once per training batch, for answers the device could have produced itself. `whiten_scalar` spelled its finiteness check `isfinite(mean) and isfinite(std)`. Python's `and` calls `__bool__` on the first tensor and, when that is true, on the second, so the readable spelling costs one or two device-to-host syncs. Stacking the pair reduces once and reads once. `any_rank_has_non_finite` was worse: it called `bool(...)` to decide what to put *into* the flag tensor, so it synced before the collective and then copied the answer back to the device to rebuild it. Building the flag with tensor ops keeps the whole thing on device; the trailing `.item()` is the only sync left and is unavoidable while the function returns a Python `bool`. Neither guard changes behaviour, which is the problem with claiming so: mutation testing found the `whiten_scalar` guard had no test at all. Deleting it left all 807 tests green. It is reachable only from a float64 caller near its own maximum -- float32 cannot get there, 1.2e77 against float64's 1.8e308 -- and unguarded it divides the batch by infinity and returns all zeros, which is indistinguishable from "every sample scored the same". A test now pins that, and reverting the guard turns it red. The flag's cast off `bool` got the same treatment and survived, which first read as "this line carries no meaning". It does not: MAX over 0/1 is dtype-independent, but whether a backend *accepts* the dtype is not, and `relax/utils/device.py` picks the backend from the accelerator -- hccl on NPU, xccl on XPU, neither checkable from here. A two-process NCCL run on the production image (torch 2.11, NCCL 2.28.9) confirms bool is fine on CUDA, so the cast is precaution for the backends we cannot run, not a fix for anything reproduced. The test asserts "not bool" rather than "== float32": float32 is only what the pre-rewrite code sent, and int32 -- what every other flag in this repo uses -- must stay a legal future choice. Pinning the exact dtype would copy the implementation into a test. The docstring says all of this because an earlier draft of it said something false. It justified float32 by rolling-upgrade dtype skew, which cannot happen here: these are data-parallel ranks from one launch of one image.
…f stalling These cases exist because one rank raising while another proceeds strands the second inside a collective, and they record the exception rather than raising so that both ranks coming back is distinguishable from one that never does. `init_process_group` had no timeout, so gloo's 30-minute default decided how long "never" takes. Mutation testing made the cost concrete: deleting the `all_reduce` from `any_rank_has_non_finite` -- a real defect, it loses the cross-rank signal entirely -- did not fail the suite, it hung it past 300s. At the default that regression reaches CI as a stalled job, which reads as infrastructure flake and gets retried rather than investigated. With a 60s bound the same mutation fails in 106s. Every collective here is sub-millisecond and world_size is 2, so the bound is headroom, not a budget. Scope: this file only. Six other bare `init_process_group` sites across tests/distributed, tests/backends/megatron and tests/utils have the same 30-minute default and are left alone -- an earlier version of this claimed the blast radius was a single site, which was wrong.
|
此前 review 指出的 GDPO 数值问题已经处理:非公式 noise floor 已移除,reward weights 和合并使用 不过当前 head b92ce2d 仍建议 Request changes。 1. [验收阻塞] 新增 Megatron 接线测试无法在项目标准环境运行test_gdpo_loss_wiring.py::_load_loss_module() 在当前项目环境中实测: pytest -q tests/algorithms AssertionError: pipeline_model parallel group is not initialized 四个失败均来自 test_gdpo_loss_wiring.py。GitHub CI 全绿是因为其 CPU runner 没有 MCore,只覆盖了 请无论 Megatron 是否已安装,都显式 monkeypatch loss_module.mpu 为单进程测试实现,同时继续调用真 2. RFC 正文与最终实现不一致RFC #218 (#218) 正文仍有多处过期或错误内容:
只在 issue 评论里更正不足以形成稳定的设计记录。请更新 RFC 正文,或者在正文顶部明确标记已被最终 非阻塞验证说明当前没有完整 GDPO 训练曲线,只有单卡/双卡 4~8 step smoke。这不是题面硬性要求,不应单独阻塞验 此外,1e300 这类 Python 中有限、转 float32 后为 inf 的 reward weight 会通过启动校验,直到首次 最后请先完成并合入 #276,再将 #277 rebase 到新的 main,使 #277 的最终 diff 只保留 GDPO 增量。 |
…MCore is installed `_load_loss_module` installed a fake `mpu` only when `import megatron.core` raised. `docker/Dockerfile` installs Megatron and puts it on PYTHONPATH, so in the project image the import succeeds, the real `mpu` answers, and nothing here initialises a pipeline group -- all four cases died on `pipeline_model parallel group is not initialized`. GitHub's runner has no MCore and only ever ran the other branch, so `pytest -q tests/algorithms` reported `4 failed, 805 passed` in the image while CI stayed green. Reported in review; this is that fix. The import still needs the fake package when Megatron is absent, but which `mpu` the code talks to is now decided per test by an autouse fixture, so both environments take the same path. It patches the functions *on* the `mpu` object rather than rebinding `loss_module.mpu`, because rebinding one name is not enough: `loss.py` is not the only module doing `from megatron.core import mpu`, and `cp_utils.maybe_padded_total_lengths` -- reached from `compute_advantages_and_returns` -- holds its own reference. With only the loss module rebound, the suite still failed in the image, at cp_utils.py:31. Every holder shares one module object, so patching its attributes reaches all of them, and `monkeypatch` restores them afterwards rather than leaking a stub into test modules that want the real thing. Verified in both configurations, since passing in one is exactly what hid this: a fake `megatron.core` whose every `mpu` call raises simulates the image on a machine with no MCore. Reverting the fixture reproduces the reported split -- 4 passed without Megatron, 4 failed with it. Full `tests/algorithms` is 809 passed either way.
|
两项阻塞都已处理,head 1. [验收阻塞] 接线测试无法在项目标准环境运行改成 autouse fixture,无论 Megatron 装没装都绑定单进程 一个细节值得单独说:我第一版只重新绑定了 在项目镜像里实测,不是在本机模拟: 2. RFC #218 正文已更新四处都核实过,不是照单接受:
非阻塞两条双卡 smoke 已在当前 head 补跑,正如您指出的,旧 smoke 早于这次「每 batch 分布式 guard」的改动。2×H100、同一个
日志与摘要: 另外顺带做了一次两进程 NCCL 冒烟(同镜像,2×H100),确认非有限值 flag 在 bool/uint8/int32/int64/float32 × 标量/
|
【Task.27】接入 GDPO:多奖励解耦归一化
RFC: #218 · 依赖 #276(注册表),请先合 #276。
Base: #276(
refactor(algorithms): declare each algorithm once in a registry),该 PR 已 rebase 到
main@4899b8f并把 RLOO(#205) 迁进注册表;本 PR 随之 rebase。注意本 PR 现在的 diff 是 8140 行 / 43 文件,其中包含 #276 的全部 3131 行。 GitHub 的 base 只能是
上游仓库里的分支,而 #276 的 head 在我的 fork 上,所以 base 只能设成
main。GDPO 自身的净增量是5113 行 / 37 文件(
git diff pr1/algorithm-registry pr2/gdpo)。合入 #276 之后本 PR 的 diff 会自动收缩到这个数。评审顺序按您给的来:先 #276。
算法
GDPO(arXiv 2601.05242)对每个奖励分量分别做组内标准化再合并,而不是先求和再归一化。
真实的收益有两条,都实测过:
一、组间的相对强度。 组内标准化把每组都拉到单位方差,于是「只有一个分量在变」的组与「两个分量都在变」的组在 GRPO 下完全相同。GDPO 让各分量先各自标准化,后者幅度是两倍;第三步的 batch 白化是跨组的,所以差异保留到最终 advantage:
二、分量间的尺度差异。
correctness ∈ {0,1}与取值上百的分量(论文实验用响应长度)相加时,和的方差几乎全部来自后者,GRPO 的方向由它单独决定。极端情形:correctness=[1,1,0,0]、length=[0,100,200,300](排序相反)时,GRPO 给答错的长响应最高 advantage,GDPO 不会。反过来,若所有分量在组内都恒定,GDPO 与 GRPO 一样返回零,不会无中生有。
三步,落在已有的两个阶段上:
第三步按训练批分段(相对既有方案的主要差异)
论文 Eq. 6 在一个训练批上归一化。但调用方为效率会先用
concat_rollout_batches把num_rollout_minis个训练批合并再进 advantage 阶段。在合并后的整体上白化不是精度差异,是另一个优化目标:两个批被对着共同均值中心化,手算的 8 样本用例里有 4 个符号翻转。
_whiten_by_segment用mini_batch_sizes(ROLLOUT_MINI_LOCAL_SAMPLE_COUNTS_KEY,colocate 与 hybrid 三条路径都写)把它们切回来,每段各自白化并各自跨 DP all-reduce。因此 Eq. 6 在num_rollout_minis > 1时依然成立,不需要rollout_batch_size × n_samples == global_batch_size这条硬约束。示例脚本把4 × 8与--global-batch-size 32设成相等只是让例子最简单,不是必需。分段的死锁面,以及为此加的检查
每段一次集合通信,意味着「各 rank 段数必须一致」从一句注释变成了一个安全条件。原先只写了「
num_rollout_minis来自 minibatch plan,所以一致」——expected 不等于 checked,而猜错的后果是死锁:一个 rank 以为两段、另一个以为三段,双方都卡在第三次 collective 上,没有 traceback、没有退出码。单 rank 元数据损坏更糟:本地 raise 会把同伴留在 collective 序列里。
现在两个判定合并进一次 MAX all-reduce,在任何分段 collective 之前完成,每个 rank 读到同样的数字、在同一处一起失败。测试覆盖跨 rank 不等分片(DP 切分平衡的是 token 不是样本数)、段数不一致、
Nonevs 分段、以及单 rank 元数据损坏——每个用例都断言两个 rank 都返回,所以回归会超时而不是悄悄通过。capability flag
本 PR 只新增 3 个。 另外 3 个(
forbids_normalize_advantages、requires_rewards_normalization、min_group_size)随 RLOO 的注册化一起移到了 #276——评审意见 2的原则是「既有算法的通用注册迁移属于基础 PR」,而 RLOO 恰好也需要这三个,它们因此不再是 GDPO 独有的。
supports_fully_async=Falseuses_reward_components--gdpo-reward-keys/--gdpo-reward-weights校验allows_reward_post_process_hooks=False--custom-reward-post-process-path与--agentic-custom-advantage-path都会在归一化器之前从post_process_rewards返回,静默跳过前两步。上游对reinforce_plus_plus_baseline也是成对拒绝这两个的,所以合成一个 flag 而不是两个forbids_normalize_advantages--normalize-advantages会再叠一层 token 级;RLOO 则是它刻意保留了 reward 的尺度requires_rewards_normalizationmin_group_size=2supports_fully_async与 #276 的requires_on_policy_updates都能拒绝--fully-async,这不是重复:前者说的是「advantage 依赖批级统计量,而那条路径只给一个切片」,后者说的是「目标函数没有比值修正」。
两个不同的理由,spec 里分别写明。
数值决策
GDPO_EPS = 1e-4,不是 GRPO 路径冻结的1e-6。与参考实现(TRLGRPOTrainer的scale_rewardsGDPO 分支)一致,且 GDPO 是新算法,没有既有行为会被破坏。差别只在近乎塌缩的组上显现:二值 reward、组大小 8 时组内标准差约 0.4,两者差 0.02%;但连续 reward(论文数学实验用响应长度)可能让某组标准差落到 1e-3 量级,此时两者对尺度因子的影响相差约 10%。std <= 1e-6 * max|x|会把完全有信息的[10000, 10000.005, 10000.010, 10000.015]判成塌缩。distributed_mean_std用 float64 两遍法。一遍法E[x²] − E[x]²在数值远离零时是两个相近大数相减:[1000.0, 1000.01, 1000.02, 1000.03]返回方差恰好 0(真实 std 1.29e-2),[10000.0, 10000.001, 10000.002, 10000.003]返回 std 4.6 而非 1.3e-3(差 3660 倍)。两者都不响:前者静默把整批 advantage 归零,后者静默缩放。这类量级很常见——论文自己的数学实验就用响应长度做 reward,token 数在千级。extract_reward_components原先只查 float64 的isfinite,1e300 能通过、cast 后变inf,随后被读成非有限 std、整批归零、run 干净退出。现在在边界处拒绝,报错能指向产生它的 reward 函数。1e30 仍然放行——门槛是 float32 的可表示范围,不是对 reward 量级的意见。验证
单卡 H100 smoke,Qwen3-0.6B × GSM8K,correctness + format 双奖励,跑的就是本 PR 的代码。
两种配置各跑一次,因为它们压的是不同的代码路径:
num_rollout_minis--global-batch-size 32(示例默认,4×8 == gbs)_whiten_by_segment的单段分支--global-batch-size 16第二行的 8 步就是分段生效的运行时证据:4 个 rollout × 2 个 mini。
最终一次(PR HEAD,含段数一致性检查与 reward-vector 化的消费者):
全部有限,无 NaN / Inf。
train/loss有负值是正常的——pg_loss带符号,advantage 有正有负。最后一行值得单独说:
_compute_zero_std_metrics跑在 rollout 进程里,单元测试覆盖不到那条路径,所以这次 smoke 的作用之一就是确认改写它没有把 rollout 打挂。关于
rollout/advantages均值为 0:这是构造使然(组内标准化后每组均值为 0,batch 白化后整批均值为 0),不是训练跑空。真正证明有梯度的是旁边两个数——train/pg_loss与train/grad_norm都非零且在变化;若 advantage 真的全零,这两个会恒等于 0。在第三轮修复之后重跑的单卡 smoke,
--global-batch-size 16让num_rollout_minis=2:这一跑不是走过场。第三轮把
mini_batch_sizes is None从「静默退回整段白化」改成报错,所以只要 actor 的任何一条 RL 训练路径其实没写
rollout_mini_local_sample_counts,它就会当场炸。八步全过 = 生产路径确实都写了,这个 invariant 可以安全地强制。
同时确认第三轮的另外三处改动在真实 reward 上不误触发:日志里既没有
combined to exactly zero(噪声地板),也没有
zero-std metrics: skipping(metrics 三态)。第五轮之后重跑(
d11e247),并第一次跑了 2 卡前四次冒烟都是单卡,而单卡下 DP world size 是 1——所有 all-reduce 都是空操作。第五轮我往白化
路径里加了一条新的 collective(
any_rank_has_non_finite,用来让所有 rank 一起失败而不是一个raise、其余卡死),如果只跑单卡,等于「修死锁的补丁只在不可能死锁的配置里被验证过」。
两次都没有触发本轮改动的任何一条告警:
does not vary within the group、non-finite advantage、unreadable_reward、mini_batch_sizes is None。rollout/rewards= −1.6e−08、rollout/advantages= 7.5e−09——正好是第五轮新钉住的那个不变量(Eq. 4 逐组中心化 ⇒ 组内和为零 ⇒ 批均值 ≈ 0)在真实 rollout 上的读数。
一次操作失误,如实记
2 卡第一次跑用的是
modal run而没有--detach。Modal 的 ephemeral app 绑在本地客户端上,客户端一断作业就被拆掉——日志停在模型加载、没有 traceback,看起来像跑挂了,其实是被连带杀的。
白烧 $0.76。重跑加了
--detach就正常了。我当时还据此得出过一个相反的「教训」(说远端活过了客户端),也是错的,一并记在这里。
双卡(dp_world=2)smoke,补上单卡覆盖不到的跨 rank 路径:
那两行来自
numerics.py::_log_group_once,它就是为这件事写的:统计量是全局的还是每分片的,在 loss 曲线里看不出来——一个把多余 GPU 让给张量并行的错配置会让 DP 组退化成 1、all-reduce 变成 identity,而训练照跑不误。两个 rank 各自报告dp_world=2,说明跨 DP 归约这次是真的在做,不是恒等变换。单卡 smoke 无法覆盖这一段(
dp_world=1时所有 all-reduce 都是 identity),这也是本 PR 早期版本的证据缺口。单元测试:
1799 passed, 323 skipped(tests/algorithms单独跑807 passed)。失败集合与main@4899b8f逐条一致——2 failed + 2 errors,验证方式是把 base 检出到一个 detached worktree 里跑同样的用例,而不是 stash(本分支的改动已经提交,stash 什么都不动)。两个 megatron 纯 CPU 测试单独跑也通过:第二轮那个 BLOCKER 正是被全量运行时的 stub 掩盖的。已知边界
std_g + 1e-4、GRPO 除以std_g + 1e-6,各组std_g不同 → 尺度因子逐组不同;step3 还会再做一次 batch 白化。要 GRPO 语义就用--advantage-estimator grpo。--n-samples-per-prompt 2时幅度信息丢失:任意两个不同值标准化后恒为 ±0.7071,分量间的区分度只剩权重。示例用 8。--fully-async被参数校验拒绝,理由见上表。注意--hybrid不受影响:它用 colocate 角色集,advantage 在 Megatron worker 里算,DP 通信域存在。第四轮:review #277 的三条 + 一次会诊推翻的两条
[P1] 移除合并阶段的 noise floor —— 采纳
combine_group末尾有一个 all-or-nothing 阈值:合并结果低于8 · Σ(|wₖ|·noiseₖ)时整组返回零。评审人指出它不属于 Eq. 7,而且我自己写的特征测试已经证明它会吞掉真实信号(两分量
base=4.05e13:地板 0.148 vs 真实信号 0.033;十六个各自「干净」的分量:地板 0.82 vs 信号 0.03)。这条我认。我把「诚实记录一个已知缺陷」当成了「解决它」——docstring 里写着「一个组可以每列都离干净差
百分之一,仍然静默丢掉梯度」,然后把它留在了代码里。
combine_group现在严格是 Eq. 7,另配一份纯 Python 的 Eq.4+Eq.7 oracle 逐位比对,并有一条测试证明「分母加
GDPO_EPS」是实现相对论文的唯一偏差。但我第一版的替代方案也是错的,是一次多模型会诊查出来的
移除 floor 之后我做了一个拆分:训练值保持 Eq. 7 原值,而 filter
(
group_carries_reward_signal)保留一个相对幅度容差——「合并结果比它自己各项的尺度低六个数量级即判为舍入」。理由是「筛选和训练值是两个不同的问题」。
这个判据错得比 floor 更根本:分子正比于权重之差,分母正比于权重的大小,所以它测的是
权重配置而不是数据;
G = 2时精确退化为|w₁−w₂|/(|w₁|+|w₂|),与组内任何奖励值都无关。实测它在两个方向同时判反——扔掉一个最终 advantage 0.43 的真信号,放行一个 1.08 的纯舍入结果。
最干净的一击来自本分支自己的 fixture:
test_weights_closer_than_float32_stay_distinct断言combined = [-0.577, 1.155, -0.577] 是必须保留的真信号,同一份数据在 filter 里的 ratio 是 2.98e-8,
判为残差丢弃。
而且这不是阈值问题:
G ≥ 3时中心化子空间至少二维,可构造z₂ = -z₁ + δu(u ⊥ z₁,δ 任意小),所以真实信号的比值没有正下界,任何固定阈值都存在反例。
判据已删除。
group_carries_reward_signal现在问的和单奖励分支完全相同、并且在训练实际使用的float32 上问:
min != max。零权重静音和精确抵消仍然判得出来(它们是精确的零),近似抵消判不出来——这一点现在是写明的代价,不是被含糊过去的。
[P1] 权重保持 float64 直到 transport cast —— 采纳
[16777216, 16777217]是两个不同的配置值,第二个是 float32 表示不了的第一个整数。两个 float32 检查保留,但含义变成「这套权重能不能活过那次 cast」的校验,不再是乘法的 dtype。
[P2] 非有限 advantage 显式报错 —— 采纳,但我的第一版修法本身是个 bug
我把「有限输入产生非有限 std」那半守卫删掉了,理由是
distributed_mean_std在 float64 累加、需要约 1e153 个样本才可能溢出。这个理由是错的,五家专家独立指出:
distributed_mean_std末尾return mean.to(values.dtype), std.to(values.dtype)把 std 转回输入 dtype。[-FMAX, FMAX]两个有限的 float32 值,float64 std 是
√2·FMAX,cast 回去就是inf,除法返回全零、不报错——正是我声称已经消灭的失败模式。第二条路径连非有限 std 都不需要:
[FMAX]*10 + [-FMAX]的均值和标准差都有限,float32 的
values - mean照样溢出成-inf。(那条注释里的量级也算错了:约 1.5e231,不是1e153。)
现在整个白化在 float64 里完成,最后把已归一化的结果转回调用方 dtype,两条路径一起关掉。
顺带修掉的三条
--custom-config-path能绕过三条 RLOO 约束(【Task.27】refactor(algorithms): declare each algorithm once in a registry #276 引入)。我把校验按推导顺序拆成四个函数,却只把其中两个接回 YAML 覆盖路径,于是 YAML 可以先切到 rloo、再设
--kl-coef、--num-steps-per-rollout 4、或破坏 one-update 等式的global_batch_size。四个现在全部重跑。变异验证:撤掉重新接上的三个,3 条测试变红。
--reward-key,而上面的信号判定只读分量 key——所以一个 reward dict 带
--gdpo-reward-keys但不带标量 key 的多奖励 run 会在这里KeyError。改用zero_std_group_label,metrics 侧本来就用它挡住了同样的输入。min_group_size在AlgorithmSpec里声明了两次(第二次静默覆盖,第一处 docstring 成孤儿);一处 docstring 仍指向上一轮已删除的
component_noise_scale。补的接线测试
tests/algorithms/test_gdpo_loss_wiring.py:stub 掉mpu的四个入口,用真实rollout_data调loss.compute_advantages_and_returns,检查出来的数字。变异验证:删掉mini_batch_sizes=转发→ 2 条红;把
gdpo的advantage_fn改成grpo_broadcast→ 3 条红。仍未解决,如实记录
恒和分量的残差大致是
ulp(C) / 组内展布,随基数增长。C = 1e9时到达 optimizer 是 2.6e-3;C = 1e13、或两列都很大且跨 binade 时是 O(1)——与当年 floor 声称要防的同一量级。没有任何机制识别它。 上一轮我写的「float64 已经把假梯度压到 1e-6」只在
C ≲ 1e9成立,那条叫「最坏情形」的测试其实只钉了一个构造点,现在改成横跨三个数量级并断言其增长。
两个试过的机制都已证明不可行:条件数界无法证明一个小结果不是信号;按白化后幅度筛选需要完整
batch,放在 prompt 级 filter 里是循环依赖。所以这条留作已知限制。
另外必须说明口径:上面所有 O(1) 数字都是整批都是这类组时测的。Eq. 7 逐组中心化,混批时健康组
决定白化尺度,把残差除以它们的标准差——本例是几百倍的常数。这不改变随
C的增长:C=1e9混批是 3.1e-7,
C=1e13混批仍有 5.2e-3。而且「白化单元」是训练批而不是整个 rollout,退化组独占一段时完全没有这个除法。有一条专门的测试钉住这个区别。
会诊里被我核实为错误的两条(记下来免得重复调查)
generate_rollout入口都在filter 之前分支到
eval_rollout,eval 路径根本不调 dynamic filter。两家独立收敛到同一个错误结论。1.9e-3,离 1e-6 差四个数量级。这是我为了让反方论证有力做的过度推断。
第五轮:会诊审这次修复本身,又推翻了我三条
第四轮改完之后又做了一次五模型会诊(codex / grok / GLM / MiniMax / DeepSeek 各正反,kimi 配额未恢复),
这次审的是修复本身。结果是修复的方向都对,但我在修复里引入了三个新缺陷、写了四句被实测推翻的话。
引入的缺陷
1. 多 rank 死锁。 上一轮我给
whiten_scalar加的isfiniteraise 是纯本地判断,而紧随其后的is_collapsed带process_group时要做 all-reduce。一个 rank 的分片含非有限值就抛异常退出,其余rank 卡死。
is_collapsed自己的注释就警告过这件事,而_agree_on_segmentation正是本分支前面为消灭这个模式写的。现在改成一次 MAX all-reduce 传 flag,全 rank 一起失败。
这条有真正的分布式测试:
tests/algorithms/test_distributed_whitening.py起两个 gloo 进程、只让其中一个持有 inf,断言两个都返回且都拒绝。变异验证:换回本地判断,测试挂死(75 秒被杀,对照
整文件 30 秒跑完)。上一轮之所以没发现,是因为单进程测试走
process_group is None分支,那条all-reduce 整个被跳过——修死锁的补丁只在不可能死锁的配置里被验证过。
2. 合法配置被误杀。 我把
validate_batch_shape接回 YAML 覆盖路径,但它读的global_batch_size是合并点之前从
num_steps_per_rollout派生的。YAML 从 grpo@4-steps 切到 rloo@1-step 本应得到rollout × n = 128,却被拿去和残留的 32 比较然后拒绝。派生已抽成函数,两条路径共用。3. 两个契约洞。
whiten_scalar对整数输入返回全零(末尾那次.to(values.dtype)造成的截断),对接近上限的 float64 仍静默全零。两者现在都 raise。
4. 缺
--reward-key从「响亮崩溃」变成「并进 zero_std 桶」。 第四轮我为了消除KeyError改用了宽容的 metrics helper,结果把「这组分数都一样」和「这个 reward 缺一个本 run 要求的 key」放进了同一个
桶——一个系统性缺 scalar 的 reward schema 会永远丢弃并重采,而指标看起来只是 prompt 平坦。现在它有
独立的原因码
unreadable_reward。被实测推翻的四句话(都是我写的)
C=1e9那一格。共享白化单元只是除以健康组的标准差(几百倍常数),不改变随 C 的增长;C=1e13共享单元仍有 5.2e-3。而且「白化单元」是训练批,退化组独占一段就完全没有这个除法grows_without_bound第五轮那次会诊里,五家 CLI 的十份报告中有六份独立指出第一条。 它出现在我专门为「如实记录未解决限制」写的那一段里。docstring 现在直接放
实测表格,让「只有一个数据点」在文本里藏不住。
一个借来的不变量,现在钉住了
min != max之所以等价于「有信号」,完全因为 Eq. 4 逐组中心化把 combined 锁成组内和为零,于是「全相等」蕴含「全为零」。这个承重性质此前没有任何测试保护。配套还有一条测试说明它为什么承重:
step 3 按训练批白化而不是按组,所以一个非零常量组不会被白化掉——我原先给新判据写的理由
(「step 3 会把它白化成零」)本身是错的。
顺带
silent_groups此前用 float64 的.any(),与 filter 的 float32min != max是两个谓词,同一组会得到不同答案;已统一。GDPO 声明了
forbids_reward_side_kl——advantage_gdpo把kl交给get_grpo_returns,那里只用它取形状,值被丢弃,所以--kl-coef只换来一次参考前向。整个grpo_broadcast家族都有这个性质而只有rloo声明过;对新算法说真话不会拒绝任何现存配置,四个老算法故意不动(拒绝一个它们今天接受的 flag 是上游的决定)。
仍未关闭,如实列出
--ref-load存在性、kl_coef/kl_loss_coef互斥assert、
_normalize_sync_ppo_kl_args、fully-async 的 resource 检查、rollout_batch_size派生、over-sampling assert。都不是本 PR 引入的。根治要把 YAML 合并挪到校验之前,比本 PR 大。
被我核实为错误的两条会诊结论
[-FMAX, FMAX]仍全零(distributed_mean_std末尾那次 cast 的形参是它自己的,修复后传进去的已是float64 副本);
(1e4, -1e4)权重推翻混批结论(反号权重配反相关分量是增强,那是满量程真信号)。第三轮会诊修掉的四条
第三轮刻意换了角度:不再审 GDPO 三步数学(前两轮已饱和),改查 PR1 对既有算法的回归面、
metrics/filter 路径、以及测试的可失败性。五位专家中有两条是单独一家发现、经复现成立的,
恰好是本轮最重的两条。
1. 恒和分量在 float32 下产出 O(1) 假梯度(此前作为「已知偏差」写在文档里,定性错了)。
两个分量和为常数时数学上应精确抵消。这里的标准化对这种输入是病态的——要除以接近零的 std,
相对误差被放大
max|x| / std倍。残差量级 1e-1,比信号本身还大,第三步再除以同量级的batch 标准差,输出就是 O(1):实测和为
308.95172119140625的一组奖励,advantage 是[-0.5770, 1.1539, -0.5770],有限、合理、方向由舍入的最后几位决定。两处改动,责任划分要说清楚,因为顺手的那句总结(「地板挡住了假梯度」)不是实际发生的事:
extract_reward_components)。这是起主要作用的一半:残差降到 1e-10,第三步分母里的
GDPO_EPS又钳住放大倍数,最坏输出只剩 ~1e-6。假梯度到这一步就没了。combine_group的噪声地板,让抵消变成精确的零。1e-6 不是零,而下游多处按「等于零」判断——filter 会保留一个不贡献梯度的组。阈值是算出来的不是调出来的:
component_noise_scale返回
eps · max|x| / (std + GDPO_EPS),即标准化的条件数,良态输入下比信号低十五个数量级,病态输入下与实测残差吻合(界 5.1e-10,实测 4.1e-10)。
2. zero-std metrics 能打挂 rollout。
distributed/ray/rollout.py没有过滤reward is None,而它在agentic/rollout.py的孪生函数有——共享 helper 于是拿 None 去建float32 张量,
TypeError,从一个日志函数里抛出来。reward is None是可达的:--group-rm下组奖励一次性赋值,rollout abort 时整段被跳过,这也正是
sglang_rollout.py那句"reward is not None" 的 assert 专门为
group_rm开豁免的原因。影响所有单奖励算法,含默认的 GRPO。另外 eval 可以用与训练不同的奖励模型(
EvalConfig.rm_type),其 reward 合法地不必带--gdpo-reward-keys。metrics 在那里执行了一条并不生效的训练契约并因此打挂 eval。两个判断现在都收进
metrics_group_verdict,返回三态且不依赖sglang即可 import——那两份拷贝已经走岔过一次,而且都测不到。
3.
mini_batch_sizes is None改为报错(原「明确不修」第 1 条)。它不是同一个目标的粗糙版本:仓库自己的
test_merging_the_batches_would_flip_signs_not_just_rescale证明它会翻转一半样本的符号,而所有指标都正常。现在经由已有的 MAX all-reduce 让整组一起失败,不会单 rank 退出集合通信。
4.
--gdpo-reward-weights的非有限值与 float32 全零在启动校验就报(原「明确不修」第 2 条)。补上的测试守卫(每一条都做了变异验证:删掉守卫,对应测试变红)
advantage_normalization此前没有任何测试钉住取值。删掉 REINFORCE++ spec 里的"token_global"、或把loss.py的比较反过来,整套测试全绿——而这个字段是既有算法每一步都读的最后一段数学分派。现在两个调用点和确切集合都从 main 转录。
advantage_gae只有co_names检查,现在与 main 内联分支的转录做数值比对。PPO 是本次重构改动最大的既有算法,也是唯一没有 GPU 冒烟的。
test_grouping_uses_group_index只构造过连续布局,按位置分组的实现照样能通过。现在交错两个组。_compute_zero_std_metrics此前零测试——这正是问题 2 溜过前两轮的原因。文字与代码不符(前一轮的更正只落到了主实现)
冒烟脚本页头仍挂着
9f1e02e已撤回的动机;filter 的 docstring 写「每个分量都平」而代码判的是「加权合并后是否抵消」;一处测试 docstring 引用了发布脚本并不使用的
--global-batch-size;溢出注释描述的均值溢出在改成 float64 后已不会发生;
.detach()被称作 "load-bearing",而两个调用点传的都是 list、根本走不到那条分支。七处已全部更正。
上游消费者的 reward-vector 化(review 建议 6,已在本 PR 完成)
有三处消费者坐在 advantage 阶段之前,各自问「这个 prompt 组还有信号吗」,而三处都用
--reward-key的单个标量回答:engine/filters/dynamic_sampling_filters.py的check_reward_nonzero_std——功能性,它决定组被不被丢弃agentic/rollout.py与distributed/ray/rollout.py的_compute_zero_std_metrics——可观测性,不影响训练但会误报对多奖励算法这是错的问题:
--reward-key标量是否变化,与「GDPO 最终会不会产出非零 advantage」既不充分也不必要。标量恒定的组可能仍有信号(分量尺度悬殊时),标量变化的组也可能没有(分量抵消或被零权重静音)。不修的话,reward 阶段算对了,采样器按另一个标准取舍。group_carries_reward_signal按当前算法该问的方式问:单奖励在 float32 上看--reward-key标量是否变化;多奖励直接算出 GDPO 前两步的组合结果、看它是否非零。三处共用它。顺带两处附带修正:
advantage_estimator == "ppo",又是硬编码算法名。改读needs_critic——那正是这个 gate 的本意(value-based 估计器不做 group-relative)。算法集合相同,行为不变。std > 0、metrics 用精确相等,对同一个问题给出两套判据。统一到 float32 张量上的min != max:它与std > 0在所有 float32 输入上一致(min == max⟺std == 0),同时与is_collapsed用同一种判定。--dynamic-sampling-filter-path的 warning 现在只对自定义 filter 触发;内置的已经正确,不再需要警告。判据本身也在会诊后修正过一次。 最初
group_carries_reward_signal的多奖励分支问的是「有没有任一原始分量在变」——这跟「算法最终会不会产出非零 advantage」不是同一个问题:某个权重为 0 会让变化的分量失声,两个分量标准化后互为相反数会抵消。两种情况下它都会保留一个实际零梯度的组。现在它直接算出前两步的组合结果、按其是否非零判定,与训练拿到的信号严格一致(有一个测试逐例比对这两者)。单奖励分支同样修正过:一度改成对 Python float 做精确不等,并在注释里声称「与之前的
std > 0等价」。并不等价——[0.1+0.2, 0.3, ...]在 float64 里有差异、cast 到 float32 后塌缩,NaN 组更会被读成「有信号」。现在在 float32 张量上判min != max,与旧的std > 0在所有输入上一致。