feat(smolagents-daytona): smolagents DaytonaExecutor - #58
Conversation
Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…nager Exercises the exact usage the README documents: CodeAgent.__exit__ calls cleanup(), which releases the sandbox. Also simpler than the previous try/finally form. Addresses review feedback on PR #58. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
…the live interpreter Daytona sandboxes run Python with a non-writable system site-packages, so pip install silently falls back to a user-site install while exiting 0. The long-running interpreter context started before that directory existed, so it is missing from sys.path: the install reports success but the package is not importable. Override install_packages to run a plain pip install and then repair user-site visibility in the live context (sys.path append + importlib.invalidate_caches). pip already behaves correctly across virtualenv, writable, and non-writable environments; only the visibility gap needs fixing. The live test fixture now also installs a package that is never preinstalled in sandbox base images (emoji), so the suite genuinely validates installation instead of passing on preinstalled numpy. Uncovered by review feedback on PR #58. The same repair is proposed upstream as the RemotePythonExecutor default (huggingface/smolagents#2724); this override is removable once that lands. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Appending the user-site directory to sys.path left preinstalled copies of a package earlier on the path, so an agent-requested version installed into user site could be silently shadowed by an older copy baked into the image. Insert user site before the first system site-packages entry instead, mirroring interpreter-startup ordering (site.py does the same), so agent-requested versions win. Adds a live test asserting the resulting sys.path ordering in a real sandbox. Addresses review feedback on PR #58. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
…ided upstream huggingface/smolagents#2724 (5d49484) adopted the user-site visibility repair with startup-like ordering as the RemotePythonExecutor default, matching the implementation this package carried. The override is now redundant: DaytonaExecutor inherits the fixed default. The executor is down to its final shape: the FINAL_ANSWER_EXCEPTION_BASE knob plus __init__, run_code_raise_errors, and cleanup, with no overrides of base behavior. The live tests that exposed the original issue (install of a package absent from the base image, sys.path ordering assertion) stay in place, now validating the inherited default against real sandboxes. Also bumps the pinned smolagents SHA in CI to the fixed head. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/smolagents-daytona/tests/unit_tests/test_executor.py">
<violation number="1" location="packages/smolagents-daytona/tests/unit_tests/test_executor.py:171">
P3: This delta drops the local `install_packages` override and makes the test assert on the exact private implementation of the inherited smolagents base-class method, hard-coupling the unit test to upstream internals (`"site.getusersitepackages()"`, `"sys.path.insert"`, `"getsitepackages"`, `"importlib.invalidate_caches()"`, and `not "sys.path.append"`). The upstream base-class default is still being actively refined (the correct snippet is being sent to huggingface/smolagents#2724 per the PR discussion), so a benign upstream refactor to a different but equally valid mechanism (e.g. `site.addusersitepackages()` or a named helper) will break CI even though the runtime behavior stays correct. Prefer asserting the observable contract (installed packages importable, order of `user_site` vs system site on `sys.path`) over matching the generated source text.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| sent_code = executor.run_code_raise_errors.call_args.args[0] | ||
| assert "class FinalAnswerException(Exception):" in sent_code | ||
|
|
||
| def test_inherited_install_packages_repairs_user_site_visibility(self): |
There was a problem hiding this comment.
P3: This delta drops the local install_packages override and makes the test assert on the exact private implementation of the inherited smolagents base-class method, hard-coupling the unit test to upstream internals ("site.getusersitepackages()", "sys.path.insert", "getsitepackages", "importlib.invalidate_caches()", and not "sys.path.append"). The upstream base-class default is still being actively refined (the correct snippet is being sent to huggingface/smolagents#2724 per the PR discussion), so a benign upstream refactor to a different but equally valid mechanism (e.g. site.addusersitepackages() or a named helper) will break CI even though the runtime behavior stays correct. Prefer asserting the observable contract (installed packages importable, order of user_site vs system site on sys.path) over matching the generated source text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/smolagents-daytona/tests/unit_tests/test_executor.py, line 171:
<comment>This delta drops the local `install_packages` override and makes the test assert on the exact private implementation of the inherited smolagents base-class method, hard-coupling the unit test to upstream internals (`"site.getusersitepackages()"`, `"sys.path.insert"`, `"getsitepackages"`, `"importlib.invalidate_caches()"`, and `not "sys.path.append"`). The upstream base-class default is still being actively refined (the correct snippet is being sent to huggingface/smolagents#2724 per the PR discussion), so a benign upstream refactor to a different but equally valid mechanism (e.g. `site.addusersitepackages()` or a named helper) will break CI even though the runtime behavior stays correct. Prefer asserting the observable contract (installed packages importable, order of `user_site` vs system site on `sys.path`) over matching the generated source text.</comment>
<file context>
@@ -168,13 +168,15 @@ def test_final_answer_exception_base_renders_exception_subclass(self):
- def test_install_packages_repairs_user_site_visibility(self):
- """Packages must be importable by the live interpreter, not just pip-installed.
+ def test_inherited_install_packages_repairs_user_site_visibility(self):
+ """The inherited smolagents default must keep user-site packages importable.
</file context>
Summary by cubic
Adds the
smolagents-daytonapackage, a Daytona sandbox executor for smolagentsCodeAgents. Installing it registers thedaytonaexecutor type via entry point, soCodeAgent(executor_type="daytona")runs generated code in an isolated, stateful sandbox.Bug fixes
install_packagesfix that puts user-site before system site-packages, so the earlier local override was dropped.CI and release
CodeAgentcontext-manager usage and assertssys.pathordering in a real sandbox.smolagents>=1.27.0ships, CI installs smolagents from a specific commit and uses--no-deps.Written for commit f460970. Summary will update on new commits.