fix(session): use system Python for external containers#144
fix(session): use system Python for external containers#144MohamedMostafa259 wants to merge 1 commit into
Conversation
dosubot
Bot
added
the
size:M
📝 WalkthroughWalkthroughRevised the condition that supplies sandbox venv paths to runtime_context: venv paths are now used only when environment setup is not skipped AND an existing container is not used. Tests were parameterized to validate both skip-environment and external-container scenarios. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
llm_sandbox/core/session_base.py (1)
485-498: Align library installation with the new system‑Python path logic.Line 485-498 correctly avoid venv paths for external containers, but
run()still callsinstall(libraries)andinstall()always injects venv python/pip paths for Python. For external containers withlibraries, this likely still resolves to a non‑existent.../.sandbox-venv/bin/pipand fails. Consider reusing the sameuse_venv_pathsdecision ininstall()or explicitly blocking installs for existing containers with a clear error.Possible fix (apply in install())
use_venv_paths = self.language_handler.name == "python" and ( not self.config.skip_environment_setup and not self.using_existing_container ) runtime_context = RuntimeContext( workdir=self.config.workdir, python_executable_path=self.python_executable_path if use_venv_paths else None, pip_executable_path=self.pip_executable_path if use_venv_paths else None, pip_cache_dir=self.pip_cache_dir_path if use_venv_paths else None, )
MohamedMostafa259
force-pushed
the
fix/use-system-python-for-external-containers
branch
from
f5466a8 to
05374f7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
=====================================
Coverage 91.8% 91.8%
=====================================
Files 42 42
Lines 2662 2662
Branches 330 330
=====================================
Hits 2445 2445
Misses 120 120
Partials 97 97 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When connecting to containers via container_id, use system Python instead of assuming the container has llm-sandbox's venv structure. External containers created by users likely don't have the /sandbox/.sandbox-venv/bin/python path that llm-sandbox creates. The previous logic incorrectly assumed all existing containers had this venv structure. Fixes vndee#127
MohamedMostafa259
force-pushed
the
fix/use-system-python-for-external-containers
branch
from
05374f7 to
2dab798
Compare
|
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix an issue where external containers (connected via container_id) were incorrectly assumed to have llm-sandbox's virtualenv structure at /sandbox/.sandbox-venv/bin/python. The fix changes the logic to use system Python for external containers instead of the venv path.
Changes:
- Modified the
use_venv_pathslogic insession_base.pyto use system Python when using existing containers - Updated comments to clarify when venv paths should and shouldn't be used
- Added parameterized tests to verify system Python is used for both
skip_environment_setup=Trueand external container scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| llm_sandbox/core/session_base.py | Changed venv path logic from OR to AND condition to exclude external containers from using venv paths |
| tests/test_session_base.py | Added parameterized test covering skip_environment_setup and container_id scenarios, with improved error messages |
| # Note: For pooled containers, they use a different code path via PooledSandboxSession | ||
| # which explicitly sets up the venv and knows it exists. |
There was a problem hiding this comment.
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.
| """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 | ||
| """ |
There was a problem hiding this comment.
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.
| # 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. | ||
| 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 | ||
| ) |
There was a problem hiding this comment.
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.
When connecting to containers via container_id, use system Python instead of assuming the container has llm-sandbox's venv structure.
External containers created by users likely don't have the /sandbox/.sandbox-venv/bin/python path that llm-sandbox creates. The previous logic incorrectly assumed all existing containers had this venv structure.
Fixes #127
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.