Repository navigation
Conversation
Direct print() calls are intercepted by name in evaluate_call, but passing print as a value (e.g. list(map(print, values))) invoked the no-op custom_print without any interception, silently losing output. Bind the print tool to each run's PrintContainer in evaluate_python_code so every invocation path is captured. Closes huggingface#2941
VANDRANKI
left a comment
There was a problem hiding this comment.
This is a community review. It does not clear the merge gate.
I read the diff and ran it. The fix works for the reported case. There is one behavior change I want to point out.
What I ran (Windows, Python 3.13, PR head against main 96f33fa), calling evaluate_python_code and LocalPythonExecutor directly:
list(map(print, ['a']))withBASE_PYTHON_TOOLS: main returns[None]with empty logs, the PR returns[None]with logs'a\n'.- A class with
__str__printed throughmap(print, [A()])is captured as'AA\n'on the PR (empty on main). LocalPythonExecutoraftersend_tools({}):list(map(print,[1,2]))gives logs'1\n2\n'on the PR (empty on main), and a following directprint(3)gives'3\n'.- A user-defined function that calls
print(*a)still logs'1 2\n', same as main. tests/test_local_python_executor.py: 396 passed, 1 failed, 3 skipped on the PR; 395 passed, 1 failed, 3 skipped on main. The one failure is the same on both:TestLocalPythonExecutorSecurity::test_vulnerability_for_all_dangerous_functions[os.system]. I did not investigate why it fails.
Behavior change to be aware of: evaluate_python_code now always puts print into its copy of static_tools. On main, evaluate_python_code("print('hi')", {}) raises InterpreterError: Forbidden function evaluation: 'print' is not among the explicitly allowed tools, because evaluate_call looks the name up in static_tools before the func_name == "print" shortcut. On the PR the same call succeeds and logs 'hi\n'. In practice LocalPythonExecutor.send_tools always includes BASE_PYTHON_TOOLS, so the agent path is not affected, and no existing test failed because of it. It only matters for callers that pass a custom static_tools without print. If that allowlist behavior is meant to be kept, the assignment could be guarded with if "print" in static_tools. I am not sure which behavior the maintainers want.
Smaller notes:
- The direct-call path in
evaluate_calland the newcapturing_printboth ignoresep,endandfile.print('x', 'y', sep='-')logs'x y\n'on main and on the PR.capturing_printaccepts**kwargsand drops them, so a callback with keyword arguments no longer raisesTypeErrorthe waycustom_print(*args)did, but it does not honor them either. I did not test this throughfunctools.partialbecausefunctoolsis not in the default authorized imports. static_toolsis copied at the top ofevaluate_python_code, so the caller's dict is not mutated. I confirmed that in the source.- I did not test the timeout path or print from a worker thread.
Unconditionally injecting capturing_print into static_tools widened the
allowlist: evaluate_python_code("print('hi')", {}) used to raise
'Forbidden function evaluation' on main, but started succeeding. Guard the
swap with 'print' in static_tools so custom allowlists keep main's behavior;
the callback-capture fix still applies on every path that allows print.
Adds a test pinning the allowlist behavior.
Direct
print()calls show up in the executor logs, but passingprintas a callback loses all output:evaluate_callintercepts direct calls by the nameprint, but whenprintis passed as a value (e.g. tomap), the resolved callable was the no-opcustom_print, invoked by nativemapwithout ever going throughevaluate_call— so nothing reached the logs.The fix binds the
printtool to the current run'sPrintContainerinsideevaluate_python_code, so every invocation path (direct calls,map(print, ...), aliasedf = print) is captured. Direct-call behavior is unchanged. Verified the reproducer from the issue, plus aliasing and per-run isolation (a fresh container is bound on each call). Addedtest_print_as_callback_is_captured; fulltest_local_python_executor.pysuite passes (the only 2 failures are pre-existing scipy/sklearn import errors from missing optional deps in my env).Closes #2941