Skip to content

refactor(datesets): 重构swebp镜像操作逻辑,优化管理粒度#384

Closed
ivanbao9783 wants to merge 1 commit into
AISBench:masterfrom
ivanbao9783:fix/380-swebp-image-clean-enhance
Closed

refactor(datesets): 重构swebp镜像操作逻辑,优化管理粒度#384
ivanbao9783 wants to merge 1 commit into
AISBench:masterfrom
ivanbao9783:fix/380-swebp-image-clean-enhance

Conversation

@ivanbao9783

Copy link
Copy Markdown
Collaborator

Thanks for your contribution; we appreciate it a lot. The following instructions will make your pull request healthier and help you get feedback more easily. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.
感谢您的贡献,我们非常重视。以下说明将使您的拉取请求更健康,更易于获得反馈。如果您不理解某些项目,请不要担心,只需提交拉取请求并从维护人员那里寻求帮助即可。

PR Type / PR类型

  • Feature(功能新增)
  • Bugfix(Bug 修复)
  • Docs(文档更新)
  • CI/CD(持续集成/持续部署)
  • Refactor(代码重构)
  • Perf(性能优化)
  • Dependency(依赖项更新)
  • Test-Cases(测试用例更新)
  • Other(其他)

Related Issue | 关联 Issue
Fixes #380

🔍 Motivation / 变更动机

  1. 归一swe和swebp两个数据集的镜像操作,消除冗余代码。
  2. 优化swebp临时容器清理逻辑,以Session粒度管理生命周期

📝 Modification / 修改内容

  1. 归一swe和swebp两个数据集的镜像操作,消除冗余代码。
  2. 优化swebp临时容器清理逻辑,以Session粒度管理生命周期

📐 Associated Test Results / 关联测试结果

Please provide links to the related test results, such as CI pipelines, test reports, etc.
请提供相关测试结果的链接,例如 CI 管道、测试报告等。

⚠️ BC-breaking (Optional) / 向后不兼容变更(可选)

Does the modification introduce changes that break the backward compatibility of the downstream repositories? If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.
是否引入了会破坏下游存储库向后兼容性的更改?如果是,请描述它如何破坏兼容性,以及下游项目应该如何修改其代码以保持与此 PR 的兼容性。

⚠️ Performance degradation (Optional) / 性能下降(可选)

If the modification introduces performance degradation, please describe the impact of the performance degradation and the expected performance improvement.
如果引入了性能下降,请描述性能下降的影响和预期的性能改进。

🌟 Use cases (Optional) / 使用案例(可选)

If this PR introduces a new feature, it is better to list some use cases here and update the documentation.
如果此拉取请求引入了新功能,最好在此处列出一些用例并更新文档。

✅ Checklist / 检查列表

Before PR:

  • Pre-commit or other linting tools are used to fix the potential lint issues. / 使用预提交或其他 linting 工具来修复潜在的 lint 问题。
  • Bug fixes are fully covered by unit tests, the case that causes the bug should be added in the unit tests. / 修复的 Bug 已完全由单元测试覆盖,导致 Bug 的情况应在单元测试中添加。
  • The modification is covered by complete unit tests. If not, please add more unit tests to ensure the correctness. / 此拉取请求中的修改已完全由单元测试覆盖。如果不是,请添加更多单元测试以确保正确性。
  • All relevant documentation (API docs, docstrings, example tutorials) has been updated to reflect these changes. / 所有相关文档(API 文档、文档字符串、示例教程)已更新以反映这些更改。

After PR:

  • If the modification has potential influence on downstream or other related projects, this PR should be tested with those projects. / 如果此拉取请求对下游或其他相关项目有潜在影响,应在那些项目中测试此 PR。
  • CLA has been signed and all committers have signed the CLA in this PR. / CLA 已签署,且本 PR 中的所有提交者均已签署 CLA。

👥 Collaboration Info / 协作信息

  • Suggested Reviewers / 建议审核人: @xxx
  • Relevant Module Owners / 相关模块负责人: @xxx
  • Other Collaboration Notes / 其他协作说明:

🌟 Useful CI Command / 实用的CI命令

Command / 命令 Introduction / 介绍
/gemini review Performs a code review for the current pull request in its current state by Gemini. / 对当前拉取请求在当前状态下由 Gemini 执行代码审核。
/gemini summary Provides a summary of the current pull request in its current state by Gemini. / 对当前拉取请求在当前状态下由 Gemini 提供摘要。
/gemini help Displays a list of available commands of Gemini. / 显示 Gemini 可用命令的列表。
/readthedocs build Triggers a build of the documentation for the current pull request in its current state by Read the Docs. / 触发当前拉取请求在当前状态下由 Read the Docs 构建文档。

1. 归一swe和swebp两个数据集的镜像操作,消除冗余代码。
2. 优化swebp临时容器清理逻辑,以Session粒度管理生命周期

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces session-scoped container cleanup for SWE-bench Pro tasks to prevent concurrent tasks from interfering with each other's containers. It parameterizes the existing SWE-bench session labeling utilities to support custom label keys and integrates them into SWE-bench Pro evaluation and inference. Feedback on the newly added tests highlights a potential issue where unconditionally stubbing Unix-only modules (fcntl and resource) can break subsequent tests, and points out that several cleanup tests mock the wrong function (list_swebench_pro_container_ids instead of list_swebench_container_ids), rendering the mocks and assertions ineffective.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +46 to +50
def _setup_stubs():
# Unix-only modules referenced transitively by the ais_bench package.
for name in ("fcntl", "resource"):
if name not in sys.modules:
sys.modules[name] = types.ModuleType(name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

在 Unix 系统上,如果该测试先于其他测试运行,fcntlresource 可能尚未加载到 sys.modules 中。此时,_setup_stubs 会将它们替换为不具备任何实际功能的虚拟模块(dummy modules)。当后续的其他测试或业务代码尝试导入并使用这些模块的真实方法(例如 fcntl.flock)时,将会抛出 AttributeError,从而导致测试套件在 Unix/Linux 环境下运行失败。建议仅在导入失败时才进行 stub。

Suggested change
def _setup_stubs():
# Unix-only modules referenced transitively by the ais_bench package.
for name in ("fcntl", "resource"):
if name not in sys.modules:
sys.modules[name] = types.ModuleType(name)
def _setup_stubs():
# Unix-only modules referenced transitively by the ais_bench package.
for name in ("fcntl", "resource"):
if name not in sys.modules:
try:
__import__(name)
except ImportError:
sys.modules[name] = types.ModuleType(name)

Comment on lines +621 to +625
def test_cleanup_removes_only_session_tagged_ids(self):
with patch.object(
utils_module, "list_swebench_pro_container_ids", return_value={"session-a", "session-b"}
), patch.object(utils_module.subprocess, "run") as mock_run:
self.utils.cleanup_swebench_pro_containers(session_id="session-1")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

test_cleanup_removes_only_session_tagged_ids 测试中,您尝试通过 patch.object(utils_module, "list_swebench_pro_container_ids", ...) 来模拟容器 ID 的返回。然而,cleanup_swebench_pro_containers 实际上调用的是 _cleanup_session_containers(即 swebench/utils.py 中的 cleanup_swebench_containers),而该函数内部直接调用的是 list_swebench_container_ids,而不是 list_swebench_pro_container_ids。因此,您对 list_swebench_pro_container_ids 的 Mock 并不会生效。这会导致 cleanup_swebench_containers 内部调用未被 Mock 的 list_swebench_container_ids,进而调用被 Mock 的 subprocess.run。由于 subprocess.run 默认返回的 Mock 对象的 returncode 不为 0,list_swebench_container_ids 将返回空集,导致 cleanup_swebench_containers 提前返回,最终使得 mock_run.assert_called_once_with 断言失败。建议直接 Mock ais_bench.benchmark.tasks.swebench.utils.list_swebench_container_ids

Suggested change
def test_cleanup_removes_only_session_tagged_ids(self):
with patch.object(
utils_module, "list_swebench_pro_container_ids", return_value={"session-a", "session-b"}
), patch.object(utils_module.subprocess, "run") as mock_run:
self.utils.cleanup_swebench_pro_containers(session_id="session-1")
def test_cleanup_removes_only_session_tagged_ids(self):
with patch(
"ais_bench.benchmark.tasks.swebench.utils.list_swebench_container_ids", return_value={"session-a", "session-b"}
), patch.object(utils_module.subprocess, "run") as mock_run:
self.utils.cleanup_swebench_pro_containers(session_id="session-1")

Comment on lines +639 to +646
def test_cleanup_handles_docker_not_found(self):
with patch.object(
utils_module, "list_swebench_pro_container_ids", return_value={"c1"}
), patch.object(
utils_module.subprocess, "run", side_effect=FileNotFoundError("docker not found")
):
# Should not raise even when docker CLI is missing.
self.utils.cleanup_swebench_pro_containers(session_id="session-1")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

test_cleanup_removes_only_session_tagged_ids 类似,由于 Mock 的是 list_swebench_pro_container_ids 而不是实际调用的 list_swebench_container_ids,该测试实际上并没有测试到 cleanup_swebench_pro_containersdocker rm 抛出 FileNotFoundError 时的异常处理逻辑(因为 targets 为空,函数直接提前返回了)。建议直接 Mock ais_bench.benchmark.tasks.swebench.utils.list_swebench_container_ids

Suggested change
def test_cleanup_handles_docker_not_found(self):
with patch.object(
utils_module, "list_swebench_pro_container_ids", return_value={"c1"}
), patch.object(
utils_module.subprocess, "run", side_effect=FileNotFoundError("docker not found")
):
# Should not raise even when docker CLI is missing.
self.utils.cleanup_swebench_pro_containers(session_id="session-1")
def test_cleanup_handles_docker_not_found(self):
with patch(
"ais_bench.benchmark.tasks.swebench.utils.list_swebench_container_ids", return_value={"c1"}
), patch.object(
utils_module.subprocess, "run", side_effect=FileNotFoundError("docker not found")
):
# Should not raise even when docker CLI is missing.
self.utils.cleanup_swebench_pro_containers(session_id="session-1")

Comment on lines +648 to +656
def test_cleanup_handles_timeout(self):
with patch.object(
utils_module, "list_swebench_pro_container_ids", return_value={"c1"}
), patch.object(
utils_module.subprocess, "run",
side_effect=subprocess.TimeoutExpired(cmd="docker", timeout=30),
):
# Should not raise even when docker rm times out.
self.utils.cleanup_swebench_pro_containers(session_id="session-1")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

与前两个测试类似,由于 Mock 的是 list_swebench_pro_container_ids 而不是实际调用的 list_swebench_container_ids,该测试实际上并没有测试到 cleanup_swebench_pro_containersdocker rm 超时时的异常处理逻辑(因为 targets 为空,函数直接提前返回了)。建议直接 Mock ais_bench.benchmark.tasks.swebench.utils.list_swebench_container_ids

Suggested change
def test_cleanup_handles_timeout(self):
with patch.object(
utils_module, "list_swebench_pro_container_ids", return_value={"c1"}
), patch.object(
utils_module.subprocess, "run",
side_effect=subprocess.TimeoutExpired(cmd="docker", timeout=30),
):
# Should not raise even when docker rm times out.
self.utils.cleanup_swebench_pro_containers(session_id="session-1")
def test_cleanup_handles_timeout(self):
with patch(
"ais_bench.benchmark.tasks.swebench.utils.list_swebench_container_ids", return_value={"c1"}
), patch.object(
utils_module.subprocess, "run",
side_effect=subprocess.TimeoutExpired(cmd="docker", timeout=30),
):
# Should not raise even when docker rm times out.
self.utils.cleanup_swebench_pro_containers(session_id="session-1")

@ivanbao9783 ivanbao9783 deleted the fix/380-swebp-image-clean-enhance branch June 30, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[需求] 优化SWE-Bench_Pro数据集资源清理逻辑

1 participant