Conversation
`--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.
…not the name
`needs_critic` reached `ALGOS`, and stopped there. Everything downstream of the
role table still asked whether the estimator was literally `"ppo"`, so
registering a second value-based algorithm would have been accepted by argparse
and by `ALGOS` and then quietly not switched on any of the value plumbing: no
critic role walked, no critic placement group, `values` left on CPU, the critic
consumer handed the wrong rollout fields, the critic never waiting for data.
That is the class of bug the registry exists to remove, one layer further in
than it had reached.
Eight call sites, not the five the review listed. Three more compare with `!=`
and do not turn up in a search for `== "ppo"`:
core/registry.py:128 role topology
core/controller.py:107 critic co-hosted on the actor's PG
backends/megatron/actor.py:790 critic's `values` moved to GPU
backends/megatron/actor.py:823 who computes GAE under fully_async
backends/megatron/actor.py:2250 `_put_critic_values_to_transfer_queue`
components/critic.py:123 the critic's own wait loop
utils/training/data_fields.py the critic consumer's field set
utils/training/ppo_utils.py:21 the `--resource` critic entry check
`algorithm_needs_critic(config)` reads the spec rather than `args.use_critic`,
which already carries the same answer: `use_critic` is only set once
`validate_algorithm_args` has run, and `process_role` and the controller's
placement logic read a config that may not have been through it. Reading the
registry makes the answer independent of call order. Unknown or absent
estimators answer False, because SFT and the debug-only role paths reach these
sites with no estimator at all.
Two `== "ppo"` checks are deliberately left: `_compute_zero_std_metrics` in
`distributed/ray/rollout.py` and `agentic/rollout.py` asks whether one prompt
has several responses, which is not the critic capability and is not
`min_group_size` either -- grpo has `min_group_size=1` and is group-based. That
needs a field of its own and is not this change.
The test registers a second `needs_critic=True` spec and asserts the role
topology, the rollout fields and the resource check all follow it. It compares
topology *identity*, not member names: every role set carries a `critic` member
and `ALGOS` is what filters it, so the first draft passed while reading the
non-critic topology. Reverting any of the three converted sites turns it red.
…r it `apply_custom_config_overrides` merges the YAML and then re-derives `global_batch_size` from `num_steps_per_rollout`. With `num_steps_per_rollout` set, a YAML file naming `global_batch_size` had its value written by the merge loop and replaced one statement later, so the run used neither the configured number nor an error -- the single outcome the "a YAML key overrides the argument" contract rules out. It also moves the training batch and the token budget without saying so. `enforce_consistency=False` was right for the case it was added for and is kept: a YAML that switches `num_steps_per_rollout` from 4 to 1 must get `rollout * n`, and the pre-merge value is stale by construction, so comparing against it rejects a legitimate config. What the call could not distinguish is where the current value came from. `data` already knows: a key the YAML names is an intent, a key it does not name is a leftover. Derive first so the error can quote both numbers, then refuse the conflict rather than picking a winner. Three tests: the conflict is refused, a YAML that names the value the derivation would reach anyway still passes, and the re-derivation the `enforce_consistency` flag exists for still happens. Only the first is a mutation target -- the second guards against over-triggering and the third against breaking the original fix.
… its bytecode
`advantage_gae` had only a `co_names` check, which asserts the kernel's name
appears in the adapter's bytecode. That check survives every way this adapter
can actually be wrong, and it is the adapter with the most to get wrong: it
shapes the reward in place before delegating, and it carries
`padded_total_lengths`, the one argument main's two call sites disagreed on.
Four mutations, all of which the old check passes and these fail:
padded_total_lengths dropped -> reads the wrong token positions
kl_coef sign flipped -> KL pushes the wrong way
terminal reward dropped -> trains on KL alone
gamma and lambd swapped -> wrong discounting
main's PPO branch is transcribed from components/advantages.py:181-193 and the
megatron duplicate at loss.py:585-602 rather than regenerated, per this file's
existing rule -- regenerating turns the comparison into the implementation
checking itself. Each side gets its own tensors: `advantage_gae` mutates `kl`
in place (`k *= -args.kl_coef`), so sharing them would make the second call
read already-shaped rewards.
The `cp_disabled` fixture that made the reinforce++ adapter testable does the
same here. That fixes the reachability problem and creates a coverage boundary
worth stating: `padded_total_lengths` is only *consumed* when `cp_size > 1`, and
these run at 1. So the numeric tests pin the values and the spy pins that the
argument survives the adapter in the right keyword and position -- the padded
slicing itself needs a real context-parallel group and is not covered here. The
test says so rather than leaving the gap to be inferred.
…ation `kl_level` and `advantage_normalization` are enum-like strings consumed by equality checks -- `advantage_normalization == "token_global"` at loss.py:659 and 819, `kl_level == "sequence"` at loss.py:919. Every other string takes the else branch, so `"token-global"` or `"Sequence"` in a registry entry does not fail: the run starts, trains, and uses a different formula than the one the spec meant to select. The other spec fields do not have this problem, which is why these two were missed. `advantage_fn` and `policy_loss_fn` are dictionary keys, so a typo raises a KeyError -- and `_assert_spec_implementations_resolve` already pulls that failure forward to startup so it names the culprit instead of surfacing inside a worker. These two needed the same treatment and had none. `__post_init__` rather than a startup validator, because `ALGORITHM_SPECS` is a module-level literal: the check runs at import, so a bad entry cannot reach a worker, let alone a training step. The allow-lists sit next to the class with the call sites they mirror named in the comment, and a test asserts the shipped specs stay inside them, so the lists cannot drift away from the registry they guard.
`docformatter` runs in pre-commit but not in the ruff pass I was checking locally, so four summary lines went in too long and the hook rewrapped them -- splitting `k *= ...` and `(`k *= ...`)` across a line break in the process. Shortening the summaries is the fix that keeps both the enforced format and a readable first line, rather than committing the wrap.
…algorithm-registry # Conflicts: # relax/backends/megatron/loss.py # relax/components/advantages.py # relax/core/controller.py # relax/core/registry.py # relax/utils/arguments.py # relax/utils/utils.py
…algorithm-registry
li126com
requested review from
Aurelius84,
NINGBENZHE,
Yangruipis and
yxyOo
as code owners
September 14, 2026 13:05
Nyanpasu 审查看板审查状态： 💬 已完成 · 有补充意见 审查版本： 已完成审查，未发现阻塞性回归；有一项轻量测试依赖问题需跟进。GitHub 四项 CI 全部通过；本地 CPU 验证未全量通过（Ray 缺失及旧版 PyTorch FP16 支持限制），未执行多节点 GPU、真实 CP/PP 或 NPU 集成测试。
Powered by Nyanpasu with gpt-6-astra medium, please check the suggestions carefully.
|
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
已完成对 7852b07403bf65a436e7de091b4dce668ef3954a 的审查，未发现阻塞性回归；轻量测试入口的依赖问题见行内意见。
GitHub 四项 CI 全部通过。本地 CPU 验证受 Ray 缺失及旧版 PyTorch 的 FP16 支持限制，未全量通过；未执行多节点 GPU、真实 CP/PP 或 NPU 集成测试。
Powered by Nyanpasu with gpt-6-astra medium, please check the suggestions carefully.
|
|
||
| torch = pytest.importorskip("torch") | ||
|
|
||
| import relax.utils.utils as utils_mod # noqa: E402 |
There was a problem hiding this comment.
请让这个测试入口满足新增接入指南承诺的轻量依赖。这里在模块收集阶段直接导入 relax.utils.utils，该模块顶层又导入 ray 和 tensordict；因此在已安装 pytest/torch、未安装 Ray 的 CPU 环境执行文档中的 pytest tests/algorithms/ -v，会直接报 ModuleNotFoundError: No module named 'ray' 并中止整套测试收集，importorskip("torch") 无法保护它。
建议像参数测试一样隔离这些无关依赖，保留真实 reward dispatcher 的执行覆盖；若确实要依赖完整运行栈，则请同步修正中英文指南的依赖与运行命令，将轻量单测入口单独列出。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
本 PR 基于社区的 PR #276 继续完善算法注册表重构，结合原 PR 的评审意见及后续 main 分支变更，补齐现有算法迁移、能力驱动和兼容性验证。
目前，新增算法需要同步修改 reward 处理、advantage 计算、policy loss、角色配置和参数校验中的多处分支，容易出现漏接或配置漂移。本次通过 AlgorithmSpec 和统一 dispatcher 集中描述算法行为，让复用
已有实现的新算法主要通过注册完成接入。
主要改动：
兼容性与范围：
验证：