Repository navigation
Conversation
student-jhz
left a comment
student-jhz
left a comment
There was a problem hiding this comment.
整体结论:comment(无 blocker/major,3 条 minor 建议处理后合入)。
理由:本 PR 为 SGLang UCM Direct Linker 增加请求级缓存匹配与加载日志,实现防御完备(sglang wrapper 缺失/字段漂移均安全降级且不影响原行为),核心的 matched is result 同一性判断已对照上游 sglang UnifiedCacheLinkerWrapper.match 源码逐分支核实(无外部命中时原样返回输入、外部命中经 _replace 返回新对象),测试 23 例与描述一致且断言质量高,CI(py-linter/cpp-linter/gtest/e2e/Docs)全绿。
[minor] PR 标题与 commit 前缀不符合仓库惯例:近期 PR(#1635/#1636/#1630 等)均用 [Feat]/[Fix] 括号前缀(见仓库规范),本 PR 用 feat(sglang):。建议改为 [Feat] Add request-level cache match and load logs for SGLang,commit 同步调整。
[minor] PR 描述与模板不一致:模板为 Purpose/Modifications/Test/Documentation 四段,本 PR 用 Purpose/Changes/Validation,缺 Documentation 段。本次为纯日志增强(无配置项/接口/错误码变更),无需强制刷新文档,但请在 Documentation 段明示"无需文档更新",章节名建议对齐模板。
[minor] 自验证未完成:描述明示 "The latest first-match-only adjustment awaits runtime verification"。首匹配-only 是本 PR 核心行为之一(避免外部 KV 装入 HBM 后重复上报 INFO),建议合入前完成运行时验证并把结论更新到描述。
[nit] 可选文档增强:docs/docs-site/docs/en/user-guide/observability/verify-cache.md 的 "Investigate unexpected results" 一节可补充提及 [CacheMatch]/[CacheLoad] 日志字段,便于用户核对每请求命中与加载结果(注意 zh 版本成对更新)。
已核实亮点:
matched is result同一性判断与上游 sglang match 行为吻合,既有 host-cache 命中不会误计为 UCM 外部命中(有专门测试);- 幂等标记先设后赋值无双重包装窗口;首匹配状态存于请求对象而非无界 rid 缓存,rid 复用的新请求不被抑制;
- 日志等级设计合理(首匹配 INFO、重复 DEBUG、lookup/入队 DEBUG、load 完成 INFO),全部 %s 参数化,禁用级别下不读字段不读时钟(均有测试);
ucm_direct_linker.pylogger 切到init_logger,统一受 UCM_LOG_LEVEL 控制,与仓内规范及 patch 模块既有写法对齐;[LookUp]的result[-1]语义与 adapter.lookup 升序返回及既有_lookup_progress用法一致;- 范围聚焦、单一主题,commit 与 PR 内容一致。
| from ucm.logger import init_logger | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger = init_logger(__name__) |
There was a problem hiding this comment.
[minor] 本 PR 将 linker 日志统一到 init_logger(受 UCM_LOG_LEVEL 控制、走 UCM 格式),但同一集成的 ucm/integration/sglang/ucm_direct_store_adapter.py:21 仍是 logger = logging.getLogger(__name__):adapter 的 "UCM lookup failed / offload submit failed" 等错误日志不跟随 UCM 日志配置,排查缓存问题时两类日志通道不一致(例如 UCM_LOG_LEVEL=debug 能看到 linker 的 [LookUp] 详情,却看不到 adapter 的对应错误上下文)。建议将 adapter 一并切换为 from ucm.logger import init_logger(logger 名同获 ucm. 前缀、挂同一 root handler,无额外风险),或在 PR 描述说明保留原因。
| logger = init_logger(__name__) | ||
|
|
||
|
|
||
| def install_cache_match_logging(linker_type): |
There was a problem hiding this comment.
[nit] 公共函数 install_cache_match_logging(linker_type) 缺类型注解,同目录 apply_patch.py / boot_patch.py 的公共函数均带注解(如 patch_package_for(version: str) -> str | None、install_hook() -> None)。建议改为 def install_cache_match_logging(linker_type: type) -> None:。
| else: | ||
| caplog.set_level(logging.WARNING, logger=module.logger.name) | ||
| # No request fields may be read when logging is disabled for this call. | ||
| assert wrapper.match(None, object(), object()) is original.return_value |
There was a problem hiding this comment.
[nit] 上一行注释声称 "No request fields may be read when logging is disabled for this call",但当前用 object() 作为 req:即使包装器违规读取字段,AttributeError 也会被实现的 except (AttributeError, TypeError) 吞掉(warning_once 已被 Mock,assert not caplog.records 仍会通过),该属性实际未被守护。建议改用 __getattr__ 抛非 AttributeError 异常的哨兵对象:
class _NoFields:
def __getattr__(self, name):
raise AssertionError("request fields must not be read")
assert wrapper.match(None, _NoFields(), object()) is original.return_value这样任何字段读取都会直接失败测试,注释声称的属性才真正被断言。
Purpose
Expose request-level cache hits and load results for the SGLang UCM Direct Linker.
Changes
CacheRematchat DEBUG to avoid repeated INFO reports after loading KV into HBM.Cache-match summaries are emitted by the designated rank of each DP replica. Load results remain per rank.
Validation