Repository navigation
Bind function arguments with CPython semantics in LocalPythonExecutor - #2755
ShayanMuhammad-CS wants to merge 1 commit into
Conversation
`create_function` bound call arguments by hand, which diverged from CPython in several ways that silently produced wrong values rather than errors: - `*args` captured every positional argument instead of only the extras, so `def f(a, b, *rest)` called as `f(1, 2, 3, 4)` gave `rest == (1, 2, 3, 4)`. For methods this also leaked `self` into `*args`. - `**kwargs` captured every keyword argument, including ones already bound to named or keyword-only parameters. - Positional-only parameters were never bound, raising "The variable `a` is not defined". - Keyword-only defaults (`kw_defaults`) were ignored. - Defaults were re-evaluated on every call instead of once at definition time, so mutable defaults never accumulated (memoisation caches stayed empty). - Argument-count and duplicate/unexpected-keyword mismatches were not detected, so calls that should raise `TypeError` silently ran with missing bindings. Build an `inspect.Signature` from the `ast.arguments` node once at definition time and bind through `Signature.bind()`, which reproduces CPython's rules and its `TypeError` messages. `evaluate_lambda` shared the same defects and now uses the same helpers, so lambdas support defaults, keyword arguments, `*args` and `**kwargs`. Two existing tests asserted the old behaviour and are corrected: the expected value in `test_variable_args` (CPython returns 14, not 15, because `self` consumes the first positional argument), and the stray `self` parameter in `test_exceptions`, whose function was called with no arguments and so would raise `TypeError` in CPython.
e5c65aa to
e27da9d
Compare
Notes for the reviewerA few things I'd rather surface myself than have you find. Suggested reading order. The intentional behaviour change to weigh. Calls that CPython rejects now raise The two test edits are the part I'd scrutinise if I were reviewing. Both are described in the PR body. Short version: Deliberately out of scope. I found these by differential-testing the executor against CPython, which turned up other divergences I did not touch here, to keep this reviewable:
Happy to file the decorator one separately if it's wanted; I didn't want to bundle unrelated semantics into an argument-binding PR. Naming. On the environment caveats in the PR body. I ran the suite on Windows/Python 3.11, so the |
VANDRANKI
left a comment
There was a problem hiding this comment.
Community review, not a merge-gate approval.
This replaces the sandboxed interpreter's hand-rolled positional/keyword argument binding in create_function/evaluate_lambda (src/smolagents/local_python_executor.py) with inspect.Signature.bind(), and I think this is the right approach: rather than re-deriving CPython's argument-binding rules by hand (which is exactly what the old zip(arg_names, args) + separate vararg/kwarg/defaults handling was doing, incompletely), it builds a real inspect.Signature from the ast.arguments node once at function-definition time and delegates the actual binding to the standard library on every call.
I checked build_signature maps each AST argument kind correctly: posonlyargs before the positional-or-keyword args list get POSITIONAL_ONLY, vararg gets VAR_POSITIONAL, each kwonlyarg gets KEYWORD_ONLY with its default pulled from the parallel kw_defaults list (correctly handling that kw_defaults can contain None at a given index when that particular kwonly arg has no default, which is how the AST represents it), and kwarg gets VAR_KEYWORD. Defaults are evaluated once, at signature-build time, matching real CPython (and covered directly by test_default_values_are_evaluated_once_at_definition, which checks the classic mutable-default-argument sharing behavior across two calls).
I traced test_full_signature_binding (def func(p, /, a, b=1, *args, d, e=5, **kwargs) called as func(0, 1, 2, 3, 4, d=9, z=8)) by hand against real Python semantics and it matches the asserted (0, 1, 2, (3, 4), 9, 5, {"z": 8}), and the four test_argument_mismatch_raises_type_error cases (too many positional args, a missing required arg, a duplicate value for a named param, an unexpected keyword) all rely on Signature.bind() raising TypeError itself rather than the sandbox silently doing something wrong, which is a meaningfully different failure mode than a naive zip()-based binder (which mostly just silently drops or misassigns extra/missing arguments instead of raising).
The var_args_method test result changing from 15 to 14 is a real behavior fix, not a regression: the old code set func_state[vararg_name] = args using the entire positional-args tuple passed to new_func, which for a bound method call includes self as the first element, so *args inside the method incorrectly contained self too. The new code excludes self from *args because self is just the first ordinary positional_args entry that the signature already binds by name, and only the genuinely leftover positional arguments land in *args. test_method_star_args_excludes_self and test_star_args_only_receives_the_extra_positional_arguments both target this directly.
One thing worth a second look from a maintainer, not a bug I found: create_function no longer explicitly sets func_state["self"], relying entirely on the signature binding to place it (since self is just the first positional_args name), while still keeping a separate explicit func_state["__class__"] = args[0].__class__ line for super() support. That split is correct as far as I traced it (the self binding happens through the normal path now, __class__ genuinely needs special-casing since it isn't a real parameter), but it's a subtle enough split that I'd flag it for a second pair of eyes rather than assert it's airtight without running the full existing method/super() test suite myself.
Overall this reads as a correct, well-motivated rewrite with tests that actually probe CPython-specific edge cases (positional-only params, keyword-only defaults, **kwargs exclusion of named/keyword-only params, mismatch errors) rather than just re-testing the happy path.
|
@VANDRANKI thanks for the careful trace. On the @albertvillanova would you be able to take a look when you have time, and approve the CI workflow run? Local tests pass on Windows, but Linux CI is the better signal for the (For reference, the decorator divergence I mentioned as out of scope is now tracked separately in #2771.) |
Closes #2754.
What
create_functioninlocal_python_executor.pybinds call arguments to parameters by hand, and the binding diverges from CPython in six ways. Most of them produce a silently wrong value rather than an error, so an agent's code action keeps running on bad data.evaluate_lambdahad the same defects independently.maindef f(a, b, *rest)→f(1, 2, 3, 4)rest == (3, 4)rest == (1, 2, 3, 4)A().m(1, 2, 3)fordef m(self, a, *rest)rest == (2, 3)rest == (<A object>, 1, 2, 3)def f(a, *, d=2, **kw)→f(1, d=4, z=5)kw == {'z': 5}kw == {'d': 4, 'z': 5}def f(a, /, b)→f(1, 2)(1, 2)InterpreterError: The variableais not defined.def f(a, *, b=7)→f(1)(1, 7)InterpreterError: The variablebis not defined.f = lambda x, y=10: (x, y)→f(1)(1, 10)InterpreterError: The variableyis not defined.Two further consequences of the same code:
Defaults were re-evaluated on every call instead of once at definition time, so a mutable default never accumulated. The practical effect is that the standard memoisation idiom silently degraded from linear to exponential — the cache was empty on every call:
main: 2.0s. This branch: 0.02s (andfib(30)returns instantly rather than taking minutes).Argument mismatches went unreported. Against
def f(a): ..., CPython raisesTypeErrorforf(1, 2),f(1, a=2),f(1, z=2)andf(); onmainall four ran, leaving parameters unbound and typically failing later and elsewhere with a confusing "The variablebis not defined".How
Build an
inspect.Signaturefrom theast.argumentsnode once, at definition time (so defaults are evaluated once, as CPython does), then bind each call throughSignature.bind()+apply_defaults(). That reproduces CPython's rules for positional-only, keyword-only,*args,**kwargsand defaults, and surfaces CPython's ownTypeErrormessages on a mismatch — rather than re-deriving those rules by hand.The two new helpers (
build_signature,bind_arguments) are shared withevaluate_lambda, so lambdas now support defaults, keyword arguments,*argsand**kwargstoo.super()still works:__class__is injected as before, andselfis now bound by name through the signature.Two existing tests asserted the old behaviour
Both are corrected in this PR, and I want to flag them explicitly for review rather than bury them:
test_variable_argsasserted15forvar_args_method(1, 2, 3, x=4, y=5)againstdef var_args_method(self, *args, **kwargs). CPython returns 14 —selftakes1, soargs == (2, 3)andkwargs == {"x": 4, "y": 5}, giving5 + 9. The old15was exactly the bug in case 1 above. Only the expected value changed.test_exceptionsdefineddef method_that_raises(self)and called it with no arguments. In CPython that is aTypeError, not theValueErrorthe test means to catch. The strayselfparameter is removed, preserving what the test is actually about (an exception raised inside a function being catchable).Tests
14 regression tests added, covering every case in the table plus
self-exclusion, full-signature binding (def f(p, /, a, b=1, *args, d, e=5, **kwargs)), lambda defaults/keywords, definition-time default evaluation, and the fourTypeErrormismatches.tests/test_local_python_executor.py: 409 passed, 3 skipped.test_vulnerability_for_all_dangerous_functions[os.system], also fails on cleanmainin this environment (Windows) and is unrelated to this change.ruff checkandruff format --checkclean oversrcandtests.test_agents.py,test_tools.py,test_default_tools.py,test_types.py,test_memory.py,test_utils.py,test_monitoring.py,test_final_answer.py,test_serialization.pyandtest_tool_validation.pyby running them againstmainand against this branch and comparing: this branch fails no test thatmainpasses.I found these by differential-testing the executor against CPython over a corpus of snippets; 16 of 20 argument-binding cases diverged before this change and 0 diverge after.
Disclosure, per CONTRIBUTING: I used an AI assistant (Claude Code) to help find and write this change. I have read the diff, run the tests, and will stand behind it in review.