Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates Docker handling throughout the codebase by introducing a centralized context manager for Docker client management. The changes improve resource management by ensuring proper cleanup of Docker connections and standardize how Docker clients are used across the application.
- Introduces a reusable
docker_client()context manager inutils.py - Replaces direct
docker.from_env()calls with the context manager pattern - Updates container management to store container IDs instead of container objects
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ale_bench/utils.py | Adds the new docker_client() context manager |
| src/ale_bench/tool_wrappers/input_generation.py | Migrates to use context manager and improves error handling |
| src/ale_bench/tool_wrappers/case_runner.py | Updates all Docker operations to use context manager with proper try/finally blocks |
| src/ale_bench/session.py | Changes visualization server management to use container ID instead of object |
| src/ale_bench/data.py | Updates Docker operations to use context manager and removes manual client.close() |
| # NOTE: It will catch ReadTimeout, ConnectTimeout and ConnectionError. | ||
| # NOTE: ConnectionError occurs when the compile code timed out with sleep. | ||
| except (Timeout, ConnectionError): | ||
| if code_language != CodeLanguage.PYTHON: |
There was a problem hiding this comment.
The container.remove(force=True) call that was removed from this block (line 504) is no longer executed when code_language != CodeLanguage.PYTHON and a timeout/connection error occurs. This could leave containers running in error scenarios.
| if code_language != CodeLanguage.PYTHON: | |
| if code_language != CodeLanguage.PYTHON: | |
| container.remove(force=True) # Ensure container is removed before returning |
There was a problem hiding this comment.
I changed to use try-finally block to remove container.
| execution_time=0.0, | ||
| memory_usage=0, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
The container.remove(force=True) call that was removed from this block (line 515) is no longer executed when a general exception occurs. This could leave containers running in error scenarios.
There was a problem hiding this comment.
I changed to use try-finally block to remove container.
| try: | ||
| try: | ||
| container.wait(timeout=ale_bench.constants.VISUALIZE_TIMEOUT) | ||
| except Exception: | ||
| raise RuntimeError("Timeout while running the visualization command. Something went wrong.") | ||
| exit_code = container.attrs["State"]["ExitCode"] | ||
| finally: | ||
| container.remove(force=True) | ||
| if exit_code != 0: |
There was a problem hiding this comment.
The container.remove(force=True) call that was removed from this block is no longer executed when an exception occurs during container.wait(). This could leave containers running in error scenarios.
There was a problem hiding this comment.
I changed to use try-finally block to remove container.
* update pyproject.toml * update uv.lock * update docker handling (#7) * Add claude GitHub actions (#8) * "Claude PR Assistant workflow" * "Claude Code Review workflow" * update config * Revert "Add claude GitHub actions (#8)" (#9) This reverts commit 33ec2c2. * Add `code_run` method (#10) * add code_run function * update test code run * Minor Changes (#12) * fix MCP server and update scripts * update MCP pyproject.toml and GitHub Actions config * revert dev dependencies and fix MCP server docstring * update README and add scripts
No description provided.