Skip to content

fix(session): use system Python for external containers #144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MohamedMostafa259 wants to merge 1 commit into from
Closed

fix(session): use system Python for external containers #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions llm_sandbox/core/session_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,15 +482,13 @@ def _run_code() -> ConsoleOutput:
self.copy_to_runtime(temp_file_path, code_dest_path_posix)

# Create runtime context for execution
# Use venv paths for Python when:
# 1. Not skipping environment setup (normal case)
# 2. OR when skip_environment_setup=True but using existing container
# (pooled containers have venv)
# Note: For pooled containers, skip_environment_setup=True means
# "don't set up again", but the venv already exists from pool
# initialization, so we should use it.
# Use venv paths for Python ONLY when:
# 1. Not skipping environment setup (normal case - we created the venv)
# 2. AND not using an existing container (external containers may not have venv)
# Note: For pooled containers, they use a different code path via PooledSandboxSession
# which explicitly sets up the venv and knows it exists.
Comment on lines +488 to +489

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

This comment is misleading. PooledSandboxSession does call BaseSession.run(), which executes this code path. The comment suggests pooled containers use a different code path, but they actually delegate to the base session's run() method (see llm_sandbox/pool/session.py line 312). Additionally, pooled containers DO have venv (created during pool initialization), but the new logic will make them use system Python, which is incorrect.

Copilot uses AI. Check for mistakes.
use_venv_paths = self.language_handler.name == "python" and (
not self.config.skip_environment_setup or self.using_existing_container
not self.config.skip_environment_setup and not self.using_existing_container
)
Comment on lines +485 to 492

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The new logic breaks pooled containers. Pooled containers are created with venv during initialization (without skip_environment_setup or container_id flags), but when PooledSandboxSession connects to them, it passes both container_id and skip_environment_setup=True. With the new logic, this evaluates to False, causing pooled containers to use system Python instead of the venv that exists.

The old logic correctly handled this: not skip_environment_setup or using_existing_container evaluated to True for pooled containers (True or True), making them use venv.

The fix for issue #127 (external containers without venv) should not break pooled containers (which have venv). Consider adding a flag to distinguish between external user-provided containers and internal pooled containers, or checking if the venv path actually exists before deciding which Python to use.

Copilot uses AI. Check for mistakes.

@vndee vndee Jan 22, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@MohamedMostafa259 could you help me check this

runtime_context = RuntimeContext(
workdir=self.config.workdir,
Expand Down
41 changes: 35 additions & 6 deletions tests/test_session_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,19 +662,44 @@ def test_skip_environment_setup_config_explicit_false(self) -> None:

assert config.skip_environment_setup is False

@pytest.mark.parametrize(
("config_kwargs", "test_id"),
[
({"skip_environment_setup": True}, "skip_environment_setup"),
({"container_id": "external-container-id"}, "existing_container"),
],
ids=["skip_environment_setup=True", "container_id (external)"],
)
@patch("tempfile.NamedTemporaryFile")
@patch.object(MockBaseSession, "install")
@patch.object(MockBaseSession, "copy_to_runtime")
@patch.object(MockBaseSession, "execute_commands")
def test_run_uses_system_python_when_skip_environment_setup(
self, mock_execute_commands: Mock, mock_copy_to_runtime: Mock, mock_install: Mock, mock_tempfile: MagicMock
def test_run_uses_system_python_not_venv(
self,
mock_execute_commands: Mock,
mock_copy_to_runtime: Mock,
mock_install: Mock,
mock_tempfile: MagicMock,
config_kwargs: dict,
test_id: str,
) -> None:
"""Test that run() uses system Python when skip_environment_setup=True."""
"""Test that run() uses system Python instead of venv in specific scenarios.

Scenarios:
- skip_environment_setup=True: venv is not created, use system Python
- container_id (external container): venv may not exist, use system Python

Related to: https://github.com/vndee/llm-sandbox/issues/127
"""
Comment on lines +686 to +693

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The test covers the external container scenario (container_id set by user), but doesn't verify that pooled containers still use venv correctly. Pooled containers also set both skip_environment_setup=True and container_id, but they should use venv since it exists. Consider adding a test case for pooled containers or verifying that the venv path actually exists before deciding which Python to use.

Copilot uses AI. Check for mistakes.
with patch.object(LanguageHandlerFactory, "create_handler") as mock_create_handler:
mock_handler = MockLanguageHandler(name=SupportedLanguage.PYTHON)
mock_create_handler.return_value = mock_handler

config = SessionConfig(lang=SupportedLanguage.PYTHON, workdir="/sandbox", skip_environment_setup=True)
config = SessionConfig(
lang=SupportedLanguage.PYTHON,
workdir="/sandbox",
**config_kwargs,
)
session = MockBaseSession(config)
session.container = Mock()
session.is_open = True
Expand All @@ -697,8 +722,12 @@ def test_run_uses_system_python_when_skip_environment_setup(
# Verify that the command uses system python, not venv python
call_args = mock_execute_commands.call_args
commands = call_args[0][0]
assert any("python " in cmd if isinstance(cmd, str) else "python " in cmd[0] for cmd in commands)
assert not any(".sandbox-venv" in (cmd if isinstance(cmd, str) else cmd[0]) for cmd in commands)
assert any(
"python " in cmd if isinstance(cmd, str) else "python " in cmd[0] for cmd in commands
), f"Expected system python for {test_id}"
assert not any(
".sandbox-venv" in (cmd if isinstance(cmd, str) else cmd[0]) for cmd in commands
), f"Unexpected venv path for {test_id}"


class TestBaseSessionCodeExecution:
Expand Down
Loading