Conversation
80a263a to
10fc164
Compare
|
@SigureMo 任务已完成，请求 review。 |
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
审查版本：10fc164e（3 个文件、6 个 hunk；CI 全绿，新增测试在 H20 上实际执行并通过）。
停止原因分布与轮数分位数的实现本身没问题：比例分母取当前 samples 总数（各类 ratio 之和为 1）、rollout_turns 缺失时保持默认 1、min/mean/max 行为不变，且只接在 compute_metrics_from_samples 这一既有聚合入口上，没有改动 rollout 主循环与调度。PR 描述的 stop_reason 兼容回退已实现，并有 test_rollout_stop_reason_fallback_and_precedence 覆盖优先级与回退。
两项非阻塞意见见行内评论：
- 回退读取的 agent 侧原因可能是自由文本（如
env_error:<detail>），会原样进入指标名并持续新增序列（P2）。 compute_metrics_from_samples的空输入早退分支在生产调用点不可达，仅影响直接调用者的行为（P3）。
| for sample in samples: | ||
| reason = sample.metadata.get("rollout_stop_reason") | ||
| if not isinstance(reason, str) or not reason.strip(): | ||
| reason = sample.metadata.get("stop_reason") |
There was a problem hiding this comment.
回退到 agent 侧写入的 stop_reason 后，该值会原样进入指标名（relax/utils/metrics/metric_utils.py:118 的 prefix = f"stop_reason/{reason}/"），但它并不保证是有限枚举：examples/deepeyes_v2_agentic/app/agent.py:230 写的是 f"env_error:{obs.error}"，错误串里可能带具体细节。relax/utils/tracking_utils.py:34 会把这个 dict 直接交给 wandb.log（TensorBoard :38、ClearML :42 同理），于是每个不同的错误串都会持续新增 2 条序列（/count、/ratio），指标基数随错误多样性增长。
建议在拼 key 前归一化到有限桶，例如：
if not isinstance(reason, str) or not reason.strip():
reason = "unknown"
else:
# 自由文本（如 "env_error:<detail>"）只保留类别，避免指标基数膨胀
reason = reason.split(":", 1)[0].strip()若确实需要细分错误类型，建议由 agent 侧额外写入有限枚举字段，而不是把自由文本拼进指标名。
There was a problem hiding this comment.
这条检查了一下是的，修了
现在只对 fallback 的 stop_reason 做类别归一化，比如 env_error:connection refused 会统一统计成 env_error；主字段 rollout_stop_reason 不变
同时补了对应的单测，验证 env_error:<detail> 不会直接进入 metric key
测试结果：
python -m pytest -q tests/utils/test_metric_utils_agentic.py
12 passed, 17 warnings in 25.58s
There was a problem hiding this comment.
确认这一版修得没问题：
reason.strip().split(":", 1)[0]只作用于 fallback 分支：env_error:connection refused归为env_error；退化值（如":detail"或只剩空白）会在下一层落到unknown，非字符串也仍走unknown，没有引入新的行为回退。- 主字段
rollout_stop_reason保持原值同样合理：当前唯一写入方是examples/deepeyes/rollout.py:434（_record_rollout_stats），取值来自有限集合（finish_length/finish_abort/budget_exhausted/env_done等），确实不需要归一化。若将来有 agent 往该字段写自由文本，同一问题会复现，届时再处理即可。 - 新增的
env_error:connection refused参数化用例在 H20 上实际执行通过，CI 全绿，12 项用例覆盖完整。 - 本条按已解决记录；同一 PR 上另一条 P3（
compute_metrics_from_samples的空输入早退分支）本版未改动，仍为未解决，非阻塞。
感谢补充测试。
| if not samples: | ||
| return {} |
There was a problem hiding this comment.
这个早退分支在当前生产调用点上不可达，且 compute_rollout_stop_reason_and_turn_metrics 本身已经对空输入返回 {}（任务要求「空输入返回空指标，不触发统计异常」，helper 层面已经满足）：
_log_rollout_data用if samples:保护（relax/distributed/ray/rollout.py:4817），空列表时根本不会调用本函数_log_eval_rollout_data在rewards为空时已经continue（relax/distributed/ray/rollout.py:4768），而samples与rewards由同一批样本构造（agentic 见relax/agentic/rollout.py:929-930，sglang 见relax/engine/rollout/sglang_rollout.py:1292-1294），两者长度一致，空samples必然对应空rewards
它的唯一实际效果，是把空输入时原有的硬失败（compute_statistics 里 np.max([]) 抛 ValueError）变成静默返回 {}；目前只有新增测试 tests/utils/test_metric_utils_agentic.py:108 直接以空列表调用它。如果目的是覆盖任务里的「空输入」约定，放在 helper 上已经足够；若保留这一层，建议补一行注释说明面向的直接调用场景，否则可以删掉以缩小对既有入口函数的改动（任务同时要求不新增控制分支）。
There was a problem hiding this comment.
F3改好了，不过F2有一个小的分歧是：
bot 的理解是：生产调用链已经保证 samples 非空，所以 compute_metrics_from_samples 里的这个 early return 是重复保护，No.9 的空输入要求只让新 helper 满足就可以
我这边的理解是：No.9 写了“空输入返回空指标，不触发统计异常”，而且新指标最终是接到 compute_metrics_from_samples 里的，所以我原本理解总入口直接传空列表时也应该安全
感觉都能说得通，所以暂时不改，麻烦 @xiaoliang0601 帮忙确认一下
2026/9/18/20:51 二编...
刚刚闲来无事瞅了一眼，刚看到 #336 里有类似F2的review建议， @rai-studio-bot 你咋左右脑互搏了，那边没有 compute_metrics_from_samples 的空输入 early return 然后建议加上 if not samples: return {}
所以感觉这里应该可以保留🤔
There was a problem hiding this comment.
你指出的前后不一致是对的，这条 F2 撤回，保留现有 early return 即可，不需要为此额外加注释。
No.9 正式任务要求空输入返回空指标；681ae833 中 compute_metrics_from_samples 在读取参数、计算统计前返回 {}，也有直接调用的回归测试。这是合理的入口边界保护，不改变 rollout 调度或停止决策。生产调用者已有保护，并不足以构成删除它的理由。我之前将「不新增控制分支」套用到这里不恰当，抱歉造成困扰。
上一条汇总审查已撤回 F2，这里同步到原线程；F3 的 fallback 归一化修复仍按已解决记录。
Nyanpasu 审查看板审查状态： ✅ 已通过 审查版本： 752ab2d 已通过 752ab2d 的复核（maintainer 将 main 合入分支触发的新版本）。PR 自身实现与已通过的 681ae83 逐字节一致；本轮增量核对了合并结果：main 对共享文件 relax/utils/metrics/metric_utils.py 的修改（has_repetition 改为 detect_repetition 委托）与新指标无语义冲突、无循环导入，PR 的入口集成测试在 CI 中实际流经合并后的调用路径。新版本 8 项 CI 检查全部通过（含 Python 3.10/3.11/3.12 全量单测与 H20 GPU 单测）。F1、F3 维持已解决，F2、F4 维持已撤回，本轮无新增发现。深度必要性审计（production/tests）均按全 PR 范围完成，结论为保留现有实现；未运行本地定向变异实验（本机无 torch/megatron 环境，记录为缺口，由 CI 实际执行与 F3 回归测试历史佐证）。
审查发现待处理
已解决或已取代
提交范围 · 接收 3 · 建议移出 0 · 待确认 0接收 3 个文件 · 建议移出 0 个文件 · 待确认 0 个文件。移出与待确认部分暂停深审，不代表审查通过。
精简审查与验证依据
生产代码的必要性与替代方案
测试的必要性与替代方案
Powered by Nyanpasu with glm-5.3[1m] xhigh, please check the suggestions carefully.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
辛苦 merge 下最新 main，新增了两条流水线 |
@SigureMo 已 merge 最新流水线，无冲突，CI 全部通过 |
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
复核 681ae833（含 main 合并）：
- F3（agent 侧自由文本停止原因进入指标名）已在
51744804修复，归一化与单测都已验证。 - F2（
compute_metrics_from_samples空输入早退分支）撤回：按 issue #326 原文核对，第 4 节写明「总入口也需要在任何统计和参数读取前处理空输入」，验收标准也要求「辅助函数和总入口对空输入返回{}，无统计异常」，且实现范围明确「指标入口的空输入保护及局部字段归一化不属于调度控制改动」——我之前引用的「不新增控制分支」不适用于这条入口保护，保留现有实现是正确的。
本轮新增 1 项非阻塞意见（P2）：metadata=None 时新 helper 与 staleness 读取仍会抛 AttributeError，与验收矩阵的「缺失 metadata」「partial 模式缺 metadata」两条必测项不符，详见行内评论。
| reason = sample.metadata.get("rollout_stop_reason") | ||
| if not isinstance(reason, str) or not reason.strip(): | ||
| reason = sample.metadata.get("stop_reason") | ||
| if isinstance(reason, str) and reason.strip(): | ||
| reason = reason.strip().split(":", 1)[0] | ||
| if not isinstance(reason, str) or not reason.strip(): | ||
| reason = "unknown" | ||
| else: | ||
| reason = reason.strip() | ||
|
|
||
| stop_reasons.append(reason) | ||
| turns.append(sample.metadata.get("rollout_turns", 1)) |
There was a problem hiding this comment.
这段读取假定 sample.metadata 一定是 dict，但本任务的验收矩阵把 metadata=None 列为必测项，当前实现会抛 AttributeError：
- 本行以及
metric_utils.py:106、:115都直接sample.metadata.get(...)。Sample.metadata是普通 dict 字段（relax/utils/types.py:84），构造期没有校验，--custom-generate-function-path这类外部入口可以传None。 - 同一路径上的 staleness 读取也仍是
sample.metadata.get("start_rollout_id", rollout_id)（relax/distributed/ray/rollout.py:4873），同样是None会失败。issue 【No.9】Agentic Rollout 停止原因分布与轮数分位数指标 #326 第 4 节写明「已有 partial rollout staleness 统计读取 metadata 时，兼容metadata=None，缺失start_rollout_id继续按当前 rollout id 处理，即 gap 为 0」。
对应的矩阵条目：「缺失 metadata：{}、None、字段缺失有确定结果；全部缺失时 unknown/ratio = 1，轮数为 1」与「partial 模式缺 metadata：传入 rollout id 且进入 staleness 分支时，metadata=None 不报错」。现有测试只覆盖了 metadata 为 dict 的情形（tests/utils/test_metric_utils_agentic.py 中没有 metadata=None 用例）。
同任务的另一份实现（#336）在两处都用了 sample.metadata if isinstance(sample.metadata, dict) else {}，并补了 Sample(metadata=None) 的单测，可作参考。这里建议在循环开头取一次安全 metadata：
| reason = sample.metadata.get("rollout_stop_reason") | |
| if not isinstance(reason, str) or not reason.strip(): | |
| reason = sample.metadata.get("stop_reason") | |
| if isinstance(reason, str) and reason.strip(): | |
| reason = reason.strip().split(":", 1)[0] | |
| if not isinstance(reason, str) or not reason.strip(): | |
| reason = "unknown" | |
| else: | |
| reason = reason.strip() | |
| stop_reasons.append(reason) | |
| turns.append(sample.metadata.get("rollout_turns", 1)) | |
| metadata = sample.metadata if isinstance(sample.metadata, dict) else {} | |
| reason = metadata.get("rollout_stop_reason") | |
| if not isinstance(reason, str) or not reason.strip(): | |
| reason = metadata.get("stop_reason") | |
| if isinstance(reason, str) and reason.strip(): | |
| reason = reason.strip().split(":", 1)[0] | |
| if not isinstance(reason, str) or not reason.strip(): | |
| reason = "unknown" | |
| else: | |
| reason = reason.strip() | |
| stop_reasons.append(reason) | |
| turns.append(metadata.get("rollout_turns", 1)) |
staleness 一行可改为 (sample.metadata or {}).get(...)，两处各补一条 metadata=None 的单测即可覆盖该矩阵行。
There was a problem hiding this comment.
补充更正这条意见的依据和优先级：#326 是另一位贡献者整理的任务细化，不能直接当作本 PR 新增的强制验收要求。正式 No.9要求覆盖缺失 metadata，但没有明确要求支持 metadata=None；Sample.metadata 的声明类型是 dict，默认也是 {}，当前测试已覆盖该默认情况。
None 会在 helper 的 .get() 处报错这一代码事实成立，但此前的轮数统计也有同样前提，尚无证据表明本 PR 引入了生产路径回归。因此将本条降为 P3 可选健壮性建议，不作为必须修改项。若选择扩展对 None 的支持，再同时处理 helper、staleness 并补相应测试；前述建议代码的替换范围已核对为 104–115 行。
There was a problem hiding this comment.
metadata=None 这个 edge case 我认你了... 实在不行我给你 helper 里补个防御😭
不过你小汁偷偷拿开发者自己写issue来糊弄我是吧， No.9 里面只写了“缺失 metadata”，没明确要求 metadata=None，也没要求顺手改 partial rollout 的 staleness；
我瞅了一眼， Sample 当前定义本身也是：metadata: dict = field(default_factory=dict) ，你不信你瞅瞅https://github.com/redai-studio/Relax/blob/main/relax/utils/types.py#L84
staleness 那行也是 main 里原本就有的逻辑，不是本 PR 引入！！！！你瞅瞅这个 https://github.com/redai-studio/Relax/blob/main/relax/distributed/ray/rollout.py#L4871-L4876
严格反对（除非研发觉得可以补）你的Review ！！！
There was a problem hiding this comment.
这条反对成立。已核对基线：Sample.metadata 的契约是 dict、默认 {}；staleness 读取 也确实是已有逻辑。
我错误地把贡献者自己的 issue 细化当成正式验收要求，扩大了本 PR 的修改范围，抱歉。上一条虽降为可选建议，却仍将它挂为未解决项，也不妥：F4 现撤回，不再作为本 PR 的待办。你无需为满足这条审查补 helper 防御，也无需修改 staleness。 后续若维护者希望扩展 None 的支持，可另行明确契约和范围。
There was a problem hiding this comment.
@rai-studio-bot 不要过度设计，不要过度防御，请参考 https://github.com/ShigureLab/nyanpasu/blob/main/AGENTS.md 重新 review
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
复核 681ae833 后通过。F1、F3 已解决，F2、F4 已撤回；关于任务范围与 metadata 契约的更正见原线程。目前没有剩余审查问题，7 项 CI 检查通过。本轮未重复运行本地或多节点 GPU 测试。
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
复核通过（752ab2df）
本轮由 maintainer 将 main 合入分支触发：PR 自身实现与已通过的 681ae83 逐字节一致。增量核对确认合并结果无问题——main 对共享文件 relax/utils/metrics/metric_utils.py 的修改（has_repetition 改为 detect_repetition 委托）与新指标所在区域无重叠、无循环导入，PR 的入口集成测试在 CI 中实际流经合并后的调用路径；新版本 8 项 CI 检查全部通过（含 Python 3.10/3.11/3.12 全量单测与 H20 GPU 单测）。
无新增发现，F1–F4 状态不变；全 PR 范围的生产/测试必要性审计已完成，结论为保留现有实现（详见看板）。
This branch has not been deployed
4 participants
What
为 Agentic Rollout 增加停止原因分布和轮数分位数指标。
停止原因会按类别分别统计 count 和 ratio，例如：
stop_reason/max_turns/countstop_reason/max_turns/ratiostop_reason/unknown/countstop_reason/unknown/ratio轮数新增：
num_turn/p50num_turn/p90num_turn/p95num_turn/p99同时保留已有的
num_turn/min、num_turn/mean和num_turn/max。停止原因优先读取
rollout_stop_reason；当该字段缺失、为空或无效时，兼容读取现有 Agentic metadata 中的stop_reason。两者均无有效值时统一统计到unknown。Why
Relates to #321.
【No.9】Agentic Rollout 停止原因分布与轮数分位数指标
Task No.9 需要补充 Agentic Rollout 的停止原因计数、占比和轮数分位数，方便观察异常停止情况和长尾轨迹。
项目现有数据路径中同时存在
rollout_stop_reason和stop_reason。聚合时保持任务建议的rollout_stop_reason为主字段，并在主字段无有效值时兼容stop_reason，避免已有 Agentic 停止原因被误统计为unknown。rollout_turns缺失时继续沿用原来的默认值1，停止原因占比以当前 samples 总数为分母。How
compute_rollout_stop_reason_and_turn_metrics聚合停止原因和轮数指标。rollout_stop_reason，主字段无有效值时回退到stop_reason。unknown。rollout_stop_reason优先。Counter统计每种停止原因的 count 和 ratio。np.percentile计算 P50、P90、P95、P99。compute_metrics_from_samples，空输入返回{}。rollout_turns缺失时默认值为1的原有统计语义。unknown、停止原因 fallback 与字段优先级、占比之和、轮数分位数和输入不变性。Testing
Result:
upstream/mainpre-commit run --all-filespassesType of Change
新增 Rollout 指标和对应测试。
Screenshots / Logs
No screenshots. Test output is included above.