Conversation
31b9d2d to
635e1e7
Compare
dengwx2026
left a comment
There was a problem hiding this comment.
Code Review: add swe-bench-verified demo (PR #508)
总体评价
本 PR 新增了 SWE-Bench Verified 评测 demo,整体功能结构清晰、代码可读性较好。以下是详细的审查意见,按优先级分类。
[Critical] 必须修复的问题
1. _install_uv 中 uv 版本硬编码且无校验
文件: examples/evaluation/swe_bench/common.py:22-28
async def _install_uv(sandbox: Sandbox, session: str):
uv_install_script_commands = [
"wget https://github.com/astral-sh/uv/releases/download/0.10.5/uv-x86_64-unknown-linux-gnu.tar.gz",
"tar -xzf uv-x86_64-unknown-linux-gnu.tar.gz --strip-components=1 -C /usr/local/bin",
]
for uv_install_script in uv_install_script_commands:
await sandbox.arun(uv_install_script, session=session, mode=RunMode.NOHUP)问题:
- uv 版本
0.10.5硬编码在 URL 中,应该提取为常量或参数,方便维护和升级。 - 架构硬编码为
x86_64,如果 sandbox 是 ARM 架构会失败。 - 最关键的问题:
wget和tar两条命令的执行结果完全没有检查。如果wget下载失败(网络问题、URL 过期等),后续tar会静默失败,uv不会被安装,但不会有任何错误传播。这会导致后续测试环境 setup 在一个不确定的状态下继续运行。
建议:
UV_VERSION = "0.10.5"
UV_ARCH = "x86_64-unknown-linux-gnu"
async def _install_uv(sandbox: Sandbox, session: str):
uv_install_script_commands = [
f"wget https://github.com/astral-sh/uv/releases/download/{UV_VERSION}/uv-{UV_ARCH}.tar.gz",
f"tar -xzf uv-{UV_ARCH}.tar.gz --strip-components=1 -C /usr/local/bin",
"uv --version", # 验证安装成功
]
for cmd in uv_install_script_commands:
result = await sandbox.arun(cmd, session=session, mode=RunMode.NOHUP)
if result.exit_code != 0:
raise RuntimeError(f"Failed to install uv: {cmd}, output: {result.output}")2. setup_test_env 中 upload_dir 和 upload_by_path 返回类型不一致,错误处理不对称
文件: examples/evaluation/swe_bench/common.py:31-44
async def setup_test_env(...) -> bool:
await _install_uv(sandbox, session) # 无错误处理
res = await sandbox.fs.upload_dir(test_folder, test_dir)
if res.exit_code != 0: # upload_dir 返回 Observation,用 exit_code 判断
return False
res = await sandbox.upload_by_path(run_tests_scripts, f"{test_dir}/{run_tests_scripts.name}")
if not res.success: # upload_by_path 返回 UploadResponse,用 success 判断
return False问题:
_install_uv的调用没有任何错误处理(如上所述)。- 虽然两个 upload 方法返回不同类型是 SDK 本身的设计,但这里只返回
False而没有记录任何错误日志,调用方很难排查是哪一步失败的。
建议: 至少添加 logger 输出失败原因:
if res.exit_code != 0:
logger.error(f"Failed to upload test folder: {res.failure_reason}")
return False[Important] 应该修复的问题
3. iflow_config.yaml 文件末尾缺少换行符
文件: examples/evaluation/swe_bench/iflow_config.yaml:15
IFLOW_MODEL_NAME: "<MODEL_NAME>"文件末尾没有换行符 (no newline at end of file)。这是 POSIX 标准要求的,也是大多数 linter 会报警的。请在文件末尾添加一个空行。
4. load_task_config 不需要是 async 函数
文件: examples/evaluation/swe_bench/common.py:9-18
async def load_task_config(task_dir: Path) -> dict:
"""Load task configuration from task.yaml."""
task_yaml_path = task_dir / "task.yaml"
if not task_yaml_path.exists():
raise FileNotFoundError(f"task.yaml not found in {task_dir}")
with open(task_yaml_path, encoding="utf-8") as f:
config = yaml.safe_load(f)
return config这个函数内部没有任何 await 操作,纯粹是同步的文件 I/O 和 YAML 解析。将其标记为 async 会误导调用者,并增加不必要的协程开销。应改为普通函数:
def load_task_config(task_dir: Path) -> dict:5. iflow_config.yaml 与已有的 rock_agent_config.yaml 高度重复
文件: examples/evaluation/swe_bench/iflow_config.yaml vs examples/agents/iflow_cli/rock_agent_config.yaml
新增的配置文件与现有的 examples/agents/iflow_cli/rock_agent_config.yaml 内容高度重复,只是多了 project_path、agent_run_timeout 和 IFLOW_SEARCH_KEY 字段。建议:
- 在 README 或注释中说明这两个配置的关系和差异。
- 考虑是否可以复用或引用现有配置,避免维护两份几乎相同的文件。
6. swe_bench_verified_demo.py 的 main 部分缺少使用说明和 admin server 提示
文件: examples/evaluation/swe_bench/swe_bench_verified_demo.py:90-106
if __name__ == "__main__":
cur_dir = Path(__file__).resolve().parent
agent_config_path = f"{cur_dir}/iflow_config.yaml"
tasks_folder = "path_to_swebench_verified_tasks"
task_name = "task_name"问题:
- 其他 demo(如
claude_code_demo.py、sandbox_demo.py)都包含 admin server 运行提示:print("IMPORTANT: Make sure the admin server is running...")。本 demo 缺少此提示,不符合项目一致性约定。 tasks_folder和task_name是占位符字符串,但没有通过argparse或环境变量来传入,用户必须修改源码才能运行。建议至少通过sys.argv接收参数或添加argparse。
7. start_sandbox 函数中缺少镜像存在性校验和超时配置
文件: examples/evaluation/swe_bench/common.py:60-65
async def start_sandbox(swe_task_name: str) -> Sandbox:
image = f"slimshetty/swebench-verified:sweb.eval.x86_64.{swe_task_name}"
config = SandboxConfig(image=image)
sandbox = Sandbox(config)
await sandbox.start()
return sandbox- SWE-Bench 镜像较大,
sandbox.start()可能需要较长时间拉取镜像。建议添加超时配置或至少在文档中说明。 - 如果
swe_task_name格式不正确,镜像名会无效,但没有任何预校验。
[Suggestion] 建议改进
8. swe_bench_verified_demo.py 缺少模块级别的 docstring
文件: examples/evaluation/swe_bench/swe_bench_verified_demo.py
作为一个示例/demo 文件,应该在文件顶部添加模块级 docstring,说明:
- 这个 demo 的用途
- 前置条件(admin server、数据集下载地址等)
- 如何运行
9. common.py 中 parse_swebench_result 的标记字符串建议提取为常量
文件: examples/evaluation/swe_bench/common.py:47-57
r"SWEBench results starts here\s*(.*?)\s*SWEBench results ends here"这些标记字符串 "SWEBench results starts here" 和 "SWEBench results ends here" 如果在 run-tests.sh 中也有定义,建议统一管理为常量,避免两边不一致导致匹配失败。
10. uv.lock 中 fakeredis 依赖变更缺少说明
文件: uv.lock
fakeredis[json] 从 test 依赖组移到了 admin optional dependencies 中。这个变更与 SWE-Bench demo 的主题无关,应该在 PR 描述中说明原因,或者拆分为单独的 PR/commit。混合不相关的变更会增加 review 难度和 revert 风险。
11. 缺少 __init__.py 文件
目录: examples/evaluation/swe_bench/
新增的 examples/evaluation/ 和 examples/evaluation/swe_bench/ 目录没有 __init__.py。虽然 swe_bench_verified_demo.py 中使用了:
from examples.evaluation.swe_bench.common import ...这种导入方式在没有 __init__.py 的情况下,依赖于项目根目录在 sys.path 中,并且仅在 Python 3.3+ 的 namespace packages 特性下生效。但考虑到其他 examples/agents/ 目录也没有 __init__.py,这可能是项目约定,仅作为提醒。
12. f-string 中日志格式不一致
文件: examples/evaluation/swe_bench/swe_bench_verified_demo.py
logger.info(f"Task name: {task_name}, sandbox id : {sandbox.sandbox_id}, Agent run result: {result}")注意 sandbox id : 中冒号前有一个多余的空格,与 Task name: 的格式不一致。建议统一为 sandbox_id: 或 sandbox id:。
关于 uv.lock 变更的说明
版本从 1.2.1 升到 1.3.0,以及 fakeredis 依赖位置的调整,看起来是与本 PR 主题无关的变更。建议:
- 在 PR 描述中说明为什么需要这些变更。
- 如果是无关变更,最好拆分到单独的 PR 中。
总结
| 类别 | 数量 |
|---|---|
| Critical (必须修复) | 2 |
| Important (应该修复) | 5 |
| Suggestion (建议改进) | 5 |
主要关注点:
- 错误处理不足 --
_install_uv和setup_test_env中多处缺少结果校验和错误日志,可能导致调试困难。 - 与现有 demo 风格不一致 -- 缺少 admin server 提示、参数硬编码等。
- 不相关变更混入 --
uv.lock和fakeredis的变更应有说明或拆分。
CI 检查已全部通过 (build, deploy, precommit, test, license/cla),代码功能层面基本完整。修复上述 Critical 和 Important 问题后可以合入。
dengwx2026
left a comment
There was a problem hiding this comment.
Code Review: add swe-bench-verified demo (PR #508)
总体评价
本 PR 新增了 SWE-Bench Verified 评测 demo,整体功能结构清晰、代码可读性较好。以下是详细的审查意见,按优先级分类。
[Critical] 必须修复的问题
1. _install_uv 中 uv 版本硬编码且命令执行无校验
文件: examples/evaluation/swe_bench/common.py:22-28
async def _install_uv(sandbox: Sandbox, session: str):
uv_install_script_commands = [
"wget https://github.com/astral-sh/uv/releases/download/0.10.5/uv-x86_64-unknown-linux-gnu.tar.gz",
"tar -xzf uv-x86_64-unknown-linux-gnu.tar.gz --strip-components=1 -C /usr/local/bin",
]
for uv_install_script in uv_install_script_commands:
await sandbox.arun(uv_install_script, session=session, mode=RunMode.NOHUP)问题:
- uv 版本
0.10.5硬编码在 URL 中,应该提取为常量或参数,方便维护和升级。 - 架构硬编码为
x86_64,如果 sandbox 是 ARM 架构会失败。 - 最关键的问题:
wget和tar两条命令的执行结果完全没有检查。如果wget下载失败(网络问题、URL 过期等),后续tar会静默失败,uv不会被安装,但不会有任何错误传播。这会导致后续测试环境 setup 在一个不确定的状态下继续运行。
建议添加返回值校验并将版本/架构提取为常量。
2. setup_test_env 中 _install_uv 调用无错误处理
文件: examples/evaluation/swe_bench/common.py:31-44
async def setup_test_env(...) -> bool:
await _install_uv(sandbox, session) # 无错误处理
res = await sandbox.fs.upload_dir(test_folder, test_dir)
if res.exit_code != 0: # upload_dir 返回 Observation,用 exit_code 判断
return False
res = await sandbox.upload_by_path(run_tests_scripts, ...)
if not res.success: # upload_by_path 返回 UploadResponse,用 success 判断
return False问题:
_install_uv的调用没有任何错误处理。- 两个 upload 失败时只返回
False而没有记录任何错误日志,调用方很难排查是哪一步失败的。
建议至少添加 logger 输出失败原因。
[Important] 应该修复的问题
3. iflow_config.yaml 文件末尾缺少换行符
文件: examples/evaluation/swe_bench/iflow_config.yaml:15
文件末尾没有换行符 (no newline at end of file)。这是 POSIX 标准要求的,也是大多数 linter 会报警的。请在文件末尾添加一个空行。
4. load_task_config 不需要是 async 函数
文件: examples/evaluation/swe_bench/common.py:9-18
async def load_task_config(task_dir: Path) -> dict:这个函数内部没有任何 await 操作,纯粹是同步的文件 I/O 和 YAML 解析。将其标记为 async 会误导调用者。应改为普通函数。
5. iflow_config.yaml 与已有配置高度重复
文件: examples/evaluation/swe_bench/iflow_config.yaml vs examples/agents/iflow_cli/rock_agent_config.yaml
新增的配置文件与现有的 rock_agent_config.yaml 内容高度重复,只是多了 project_path、agent_run_timeout 和 IFLOW_SEARCH_KEY 字段。建议在注释中说明这两个配置的关系和差异。
6. swe_bench_verified_demo.py 缺少 admin server 提示和参数化
文件: examples/evaluation/swe_bench/swe_bench_verified_demo.py:90-106
if __name__ == "__main__":
tasks_folder = "path_to_swebench_verified_tasks" # 硬编码占位符
task_name = "task_name" # 硬编码占位符问题:
- 其他 demo(如
claude_code_demo.py、sandbox_demo.py)都包含 admin server 运行提示。本 demo 缺少此提示,不符合项目一致性约定。 tasks_folder和task_name是占位符字符串,用户必须修改源码才能运行。建议通过argparse或sys.argv传入参数。
7. start_sandbox 缺少超时配置说明
文件: examples/evaluation/swe_bench/common.py:60-65
SWE-Bench 镜像较大,sandbox.start() 可能需要较长时间拉取镜像。建议添加超时配置或在注释中说明。
[Suggestion] 建议改进
8. 缺少模块级 docstring
文件: examples/evaluation/swe_bench/swe_bench_verified_demo.py
作为一个示例 demo 文件,应该在文件顶部添加模块级 docstring,说明用途、前置条件和运行方式。
9. parse_swebench_result 中标记字符串建议提取为常量
文件: examples/evaluation/swe_bench/common.py:47-57
"SWEBench results starts here" / "SWEBench results ends here" 如果在 run-tests.sh 中也有定义,建议统一管理为常量。
10. uv.lock 中 fakeredis 依赖变更缺少说明
文件: uv.lock
fakeredis[json] 从 test 依赖组移到了 admin optional dependencies 中。这个变更与 SWE-Bench demo 的主题无关,应该在 PR 描述中说明原因,或者拆分为单独的 PR/commit。
11. f-string 中日志格式不一致
文件: examples/evaluation/swe_bench/swe_bench_verified_demo.py
logger.info(f"Task name: {task_name}, sandbox id : {sandbox.sandbox_id}, ...")sandbox id : 中冒号前有多余空格,与 Task name: 格式不一致。建议统一。
总结
| 类别 | 数量 |
|---|---|
| Critical (必须修复) | 2 |
| Important (应该修复) | 5 |
| Suggestion (建议改进) | 4 |
主要关注点:
- 错误处理不足 --
_install_uv和setup_test_env中多处缺少结果校验和错误日志 - 与现有 demo 风格不一致 -- 缺少 admin server 提示、参数硬编码等
- 不相关变更混入 --
uv.lock和fakeredis的变更应有说明或拆分
CI 检查已全部通过 (build, deploy, precommit, test, license/cla),代码功能层面基本完整。修复上述 Critical 和 Important 问题后可以合入。
635e1e7 to
f22635c
Compare
close #499