Repository navigation
【No.5】fix(metrics): deduplicate spec decoding metrics - #406
AllureCurtain wants to merge 1 commit into
Conversation
# 🐛 Bug Fix ## Aggregate committed generations once - Preserve request-scoped generation identities and sparse speculative counters through Agentic export. - Deduplicate shared generations while keeping identical text from independent requests and separate Sessions distinct. - Compute ratios from summed counter pairs and report full-counter and pair-level coverage. - Preserve explicit zero, missing, invalid, resumed-request, and legacy serialized states. --- # ✅ Tests ## Cover metric and export regressions - Add CPU coverage for weighted aggregation, shared-node deduplication, missing fields, zero denominators, JSON round trips, and resumed requests. - Exercise the metadata-to-SessionForest-to-TrainingFieldArtifact-to-rollout path. --- # 📝 Documentation ## Document metric semantics and compatibility - Add bilingual guidance for new metrics, coverage, generation identity, resume accounting, and legacy payloads. - Add human-checkable weighted and shared-node examples.
|
Thanks for contributing to Relax, @AllureCurtain! 感谢你为 Relax 做出贡献！ Contribution guide / 贡献指南Describe the problem, your changes, and how you validated them. Keep each PR focused and run 请说明问题、改动和验证方式，保持 PR 聚焦，并在提交前运行 CI commands / CI 指令
Put one command on the first line of a new PR comment. Rerun/cancel require PR authorship or repository write access. 在新 PR 评论的首行写一条指令。PR 作者或有仓库写权限的贡献者可以重跑、取消 CI。 |
|
@rai-studio-bot review |
|
/review |
Nyanpasu 审查看板审查状态： 💬 已完成 · 有补充意见 审查版本： 71afe8f 目标分支： main 常规与深度审查完成，无阻塞缺陷。深度阶段通过独立设计参考与测试价值审计子任务完成比对：独立参考确认序列化边界携带生成记录为不可约核心，但其更小替代方案（状态哈希身份、仅键存在性）经逐项比对被否决——不满足'同 session 相同文本独立请求分别计数'的验收标准，且会失去 legacy 遥测；本 PR 的请求级身份与三字段可用性追踪为可辩护取舍。测试套件经 11 个定向变异验证全部具备检出力。最终提出 2 条非阻塞 P3 建议（死代码删除、单测预言加强），处理与否不影响合并。
审查发现待处理
已解决或已取代
精简审查与验证依据
生产代码的必要性与替代方案
测试的必要性与替代方案
Powered by Nyanpasu with glm-5.3[1m] xhigh, please check the suggestions carefully.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
常规审查结论：未发现阻塞性问题（深度审查进行中）
常规审查已完成（head 71afe8f0）：对照 Task 5 验收标准核对了实现路径——计数加权聚合（1/2 与 9/10 汇总为 10/12）、按提交生成节点去重、相同文本独立请求与跨 Session 隔离、零值与缺失计数的区分、覆盖率指标、旧序列化载荷兼容，以及双语文档与代码行为的一致性（含 req_<session_id>_<sequence> 标识格式、rollout/ 与 eval/<dataset>/ 日志前缀）。本地复跑 tests/utils/test_spec_decoding_metrics.py、tests/test_agentic_rollout.py 及 PR 声明的相关回归共 115 passed / 2 skipped（与 PR 描述一致），ruff 与 pre-commit 钩子均通过。
目前仅 1 条非阻塞简化建议（见行内评论：新增的 spec_token_counts_reported 辅助函数无调用点）。独立设计比较与测试价值审计仍在进行，完成后我会更新最终结论。
审查看板：#406 (comment)
|
|
||
| def spec_token_counts_reported(meta_info: dict[str, Any]) -> bool: | ||
| """Whether the backend reported speculative decoding counters for this | ||
| request. | ||
|
|
||
| ``get_spec_token_counts`` returns ``(0, 0)`` both for a missing report and | ||
| for a genuine zero, so coverage accounting needs this separate signal. | ||
| """ | ||
| return any(key != "completion_token_num" for key in get_spec_counter_values(meta_info)) | ||
|
|
There was a problem hiding this comment.
spec_token_counts_reported 辅助函数（非阻塞简化建议）
全仓库检索显示 spec_token_counts_reported 没有任何调用点（仅 relax/utils/types.py:35 处的定义）。它所表达的"该请求是否上报过投机解码计数"判断在本 PR 的其余位置均为内联实现：
relax/utils/types.py:197（SpecInfo.add，基于known_totals）relax/agentic/session/service.py:1649（_accumulate_request_meta，基于request.pending_spec_counters）relax/agentic/session/state.py:826（build_sample，基于各 node record）
三处判断的对象都不是 meta_info，因此该辅助函数按当前签名无法被它们复用，属于新增的死代码，建议直接删除以避免后续维护两套"上报判定"语义。
| def spec_token_counts_reported(meta_info: dict[str, Any]) -> bool: | |
| """Whether the backend reported speculative decoding counters for this | |
| request. | |
| ``get_spec_token_counts`` returns ``(0, 0)`` both for a missing report and | |
| for a genuine zero, so coverage accounting needs this separate signal. | |
| """ | |
| return any(key != "completion_token_num" for key in get_spec_counter_values(meta_info)) |
另注（无需在本 PR 处理）：本 PR 移除 service.py 中的旧调用后，get_spec_token_counts（relax/utils/types.py:18）也不再有任何调用方；因其属于公开 API，按仓库约定删除前需另行确认，可作为后续跟进项。
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
最终审查结论：无阻塞问题，2 条非阻塞建议
深度审查已完成（head 71afe8f0）：独立设计参考与测试价值审计均已收敛。独立参考设计确认"序列化边界必须携带生成记录"是该需求的不可约核心，其提出的更小替代方案（按状态哈希去重、仅保留键存在性）经逐项比对被否决——按状态哈希去重无法满足"session 内不误去重、相同文本独立请求分别计数"的验收标准（本 PR 的 test_agentic_spec_metrics_keep_identical_requests_and_sessions_apart 恰好覆盖该边界）；三字段可用性追踪支撑了零值/缺失区分与 spec_legacy_samples 遥测，属可辩护的设计取舍。测试套件经 11 个定向变异验证全部具备检出力（每项变异均被至少一个用例捕获；唯一例外是依赖 megatron.core 的接线用例，仅在 Docker CI 环境运行，已验证其在有 megatron 的环境下可检出）。生产与测试必要性审计已记录在看板。
结论：常规与深度审查均未发现阻塞性缺陷；2 条 P3 非阻塞建议见行内评论（relax/utils/types.py 删除无调用点的 spec_token_counts_reported；tests/utils/test_spec_decoding_metrics.py 序列化往返用例改用直接字段断言）。两者处理与否不影响合并判断。
审查看板：#406 (comment)
|
|
||
| assert info.nodes == {"req_sess-a_0": _counters(1, 2), "req_sess-a_1": {}} | ||
| assert info.counts_reported is True | ||
| assert Sample.SpecInfo.from_dict(info.to_dict()).to_dict() == info.to_dict() |
There was a problem hiding this comment.
第 251 行的断言是自比较预言：from_dict(to_dict()).to_dict() == to_dict()。若 to_dict 写入侧丢失字段（例如漏写 nodes），两侧同时丢失，等式依然成立，该用例无法发现。定向变异验证（在 to_dict 中删除 nodes 键）：本用例仍通过，仅由 tests/test_agentic_rollout.py 的端到端序列化路径（TrainingFieldArtifact）以 6 处失败检出。建议直接断言往返后对象的字段，使写入侧回归在本用例内即可发现：
| assert Sample.SpecInfo.from_dict(info.to_dict()).to_dict() == info.to_dict() | |
| round_tripped = Sample.SpecInfo.from_dict(info.to_dict()) | |
| assert round_tripped.nodes == {"req_sess-a_0": _counters(1, 2), "req_sess-a_1": {}} | |
| assert round_tripped.counts_reported is True |
现有前两行断言（249-250 行）保留即可；它们只覆盖读取侧，此修改补上写入侧。
This branch has not been deployed
4 participants
What
Implement Task 5: deduplicate and counter-weight speculative decoding metrics for Agentic Rollout.
accepted,proposed,verify, andcompletioncounters through session export and JSON serialization.Why
The previous Agentic metric path averaged per-sample ratios and could count a shared generation node once for every exported trajectory. It also treated missing backend counters as zero, which could produce fabricated zero-valued ratios or divide by zero for an empty batch. Task 5 requires generation-level deduplication, weighted aggregation, explicit missing-counter handling, and compatibility documentation.
How
Generation records are keyed by the committed Agentic request ID (
req_<session_id>_<sequence>), not by text or state hash. Session nodes retain sparse counter records and export them throughTrainingFieldArtifactandSampleserialization. The rollout adapter validates records, deduplicates each generation once per metric batch, sums available counters, and computes ratios only from complete numerator/denominator pairs. Resumed requests require every backend attempt to report a field before that field is treated as complete; duplicate exports can fill a missing field without double-counting an existing value.The compatibility metrics keep their existing names. New metrics report counted nodes, missing counters, full-counter coverage, acceptance-pair coverage, length-pair coverage, and legacy samples that lack generation identities.
Task 5 Acceptance
1/2and9/10produce10/12(approximately83.33%) instead of the arithmetic mean0.7.A -> BandA -> C,Ais counted once andB/Care counted; identical text from independent requests and different sessions remain distinct.Testing
pre-commit run --all-files --show-diff-on-failure115 passed, 2 skippedin the focused and related regression suite:The two skipped tests require the optional
megatron.coredependency, which is unavailable in this Windows environment.Manual report covering weighted aggregation, shared-node deduplication, independent identical text, missing counters, explicit zeros, legacy payloads, and empty batches.
git diff --check, Ruff checks, and formatting checks pass.No GPU or multi-node integration run is claimed because this validation environment has no such hardware; the task's CPU integration requirements are covered by the regression suite above.
Type of Change
Reference