From 55c8f5d88178349b512e2c70d53d5ff12fe5bb20 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 12:52:19 +0100 Subject: [PATCH 01/13] Test evaluate_condition with pandas --- tests/test_local_python_executor.py | 51 +++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index 2abef6994..5097a2d1d 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -19,6 +19,7 @@ from textwrap import dedent import numpy as np +import pandas as pd import pytest from smolagents.default_tools import BASE_PYTHON_TOOLS @@ -1201,6 +1202,56 @@ def test_evaluate_condition(condition, state, expected_result): assert result == expected_result +@pytest.mark.parametrize( + "condition, state, expected_result", + [ + ("a == b", {"a": pd.Series([1, 2, 3]), "b": pd.Series([2, 2, 2])}, pd.Series([False, True, False])), + ("a != b", {"a": pd.Series([1, 2, 3]), "b": pd.Series([2, 2, 2])}, pd.Series([True, False, True])), + ("a < b", {"a": pd.Series([1, 2, 3]), "b": pd.Series([2, 2, 2])}, pd.Series([True, False, False])), + ("a <= b", {"a": pd.Series([1, 2, 3]), "b": pd.Series([2, 2, 2])}, pd.Series([True, True, False])), + ("a > b", {"a": pd.Series([1, 2, 3]), "b": pd.Series([2, 2, 2])}, pd.Series([False, False, True])), + ("a >= b", {"a": pd.Series([1, 2, 3]), "b": pd.Series([2, 2, 2])}, pd.Series([False, True, True])), + ( + "a == b", + {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [1, 2], "y": [3, 5]})}, + pd.DataFrame({"x": [True, True], "y": [True, False]}), + ), + ( + "a != b", + {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [1, 2], "y": [3, 5]})}, + pd.DataFrame({"x": [False, False], "y": [False, True]}), + ), + ( + "a < b", + {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [2, 2], "y": [2, 2]})}, + pd.DataFrame({"x": [True, False], "y": [False, False]}), + ), + ( + "a <= b", + {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [2, 2], "y": [2, 2]})}, + pd.DataFrame({"x": [True, True], "y": [False, False]}), + ), + ( + "a > b", + {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [2, 2], "y": [2, 2]})}, + pd.DataFrame({"x": [False, False], "y": [True, True]}), + ), + ( + "a >= b", + {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [2, 2], "y": [2, 2]})}, + pd.DataFrame({"x": [False, True], "y": [True, True]}), + ), + ], +) +def test_evaluate_condition_with_pandas(condition, state, expected_result): + condition_ast = ast.parse(condition, mode="eval").body + result = evaluate_condition(condition_ast, state, {}, {}, []) + if isinstance(result, pd.Series): + pd.testing.assert_series_equal(result, expected_result) + else: + pd.testing.assert_frame_equal(result, expected_result) + + def test_get_safe_module_handle_lazy_imports(): class FakeModule(types.ModuleType): def __init__(self, name): From 6a0edb60d900eb83f5128979760ee4b7c63ee7e4 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 12:58:21 +0100 Subject: [PATCH 02/13] Remove call to .all method --- src/smolagents/local_python_executor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/smolagents/local_python_executor.py b/src/smolagents/local_python_executor.py index a5b12f3f6..6da0c1e5b 100644 --- a/src/smolagents/local_python_executor.py +++ b/src/smolagents/local_python_executor.py @@ -756,7 +756,7 @@ def evaluate_condition( if isinstance(result, bool) and not result: break - return result if isinstance(result, (bool, pd.Series)) else result.all() + return result def evaluate_if( From 5f7bbb3e862ad0f516e2cb5817fbd9b6b1d75b85 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 13:22:25 +0100 Subject: [PATCH 03/13] Test composite condition with non-bool results --- tests/test_local_python_executor.py | 44 +++++++++++++++-------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index 5097a2d1d..e9cf98e7f 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -1272,29 +1272,31 @@ def __dir__(self): assert getattr(safe_module, "non_lazy_attribute") == "ok" -def test_non_standard_comparisons(): - code = """ -class NonStdEqualsResult: - def __init__(self, left:object, right:object): - self._left = left - self._right = right - def __str__(self) -> str: - return f'{self._left}=={self._right}' - -class NonStdComparisonClass: - def __init__(self, value: str ): - self._value = value - def __str__(self): - return self._value - def __eq__(self, other): - return NonStdEqualsResult(self, other) -a = NonStdComparisonClass("a") -b = NonStdComparisonClass("b") -result = a == b - """ +@pytest.mark.parametrize("expected_result", ["a == b", "a == b == c"]) +def test_non_standard_comparisons(expected_result): + code = dedent(f"""\ + class NonStdEqualsResult: + def __init__(self, left:object, right:object): + self._left = left + self._right = right + def __str__(self) -> str: + return f'{{self._left}} == {{self._right}}' + + class NonStdComparisonClass: + def __init__(self, value: str ): + self._value = value + def __str__(self): + return self._value + def __eq__(self, other): + return NonStdEqualsResult(self, other) + a = NonStdComparisonClass("a") + b = NonStdComparisonClass("b") + c = NonStdComparisonClass("c") + result = {expected_result} + """) result, _ = evaluate_python_code(code, state={}) assert not isinstance(result, bool) - assert str(result) == "a==b" + assert str(result) == expected_result class TestPrintContainer: From b901b6ce0e0de9fd6387c0f2274d8904f746d106 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 14:26:31 +0100 Subject: [PATCH 04/13] Test evaluate_condition with pandas chained conditions --- tests/test_local_python_executor.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index e9cf98e7f..7a8a3f77f 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -1189,7 +1189,7 @@ def test_evaluate_delete(code, state, expectation): ("a in b", {"a": 4, "b": [1, 2, 3]}, False), ("a not in b", {"a": 1, "b": [1, 2, 3]}, False), ("a not in b", {"a": 4, "b": [1, 2, 3]}, True), - # Composite conditions: + # Chained conditions: ("a == b == c", {"a": 1, "b": 1, "c": 1}, True), ("a == b == c", {"a": 1, "b": 2, "c": 1}, False), ("a == b < c", {"a": 1, "b": 1, "c": 1}, False), @@ -1241,6 +1241,16 @@ def test_evaluate_condition(condition, state, expected_result): {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [2, 2], "y": [2, 2]})}, pd.DataFrame({"x": [False, True], "y": [True, True]}), ), + # Chained conditions: + ( + "a == b == c", + { + "a": pd.Series([1, 2, 3]), + "b": pd.Series([2, 2, 2]), + "c": pd.Series([3, 3, 3]), + }, + pd.Series([False, False, False]), + ), ], ) def test_evaluate_condition_with_pandas(condition, state, expected_result): From e99690e1bf83a17dd68801cf16405b202d40e4e9 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 14:28:29 +0100 Subject: [PATCH 05/13] Fix evaluate_condition --- src/smolagents/local_python_executor.py | 41 +++++++++---------------- 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/src/smolagents/local_python_executor.py b/src/smolagents/local_python_executor.py index 6da0c1e5b..3505d0961 100644 --- a/src/smolagents/local_python_executor.py +++ b/src/smolagents/local_python_executor.py @@ -715,47 +715,34 @@ def evaluate_condition( authorized_imports: List[str], ) -> bool | object: left = evaluate_ast(condition.left, state, static_tools, custom_tools, authorized_imports) - comparators = [ - evaluate_ast(c, state, static_tools, custom_tools, authorized_imports) for c in condition.comparators - ] - ops = [type(op) for op in condition.ops] - - result = True - current_left = left - - for op, comparator in zip(ops, comparators): + for op, comparator in zip(condition.ops, condition.comparators): + op = type(op) + right = evaluate_ast(comparator, state, static_tools, custom_tools, authorized_imports) if op == ast.Eq: - current_result = current_left == comparator + result = left == right elif op == ast.NotEq: - current_result = current_left != comparator + result = left != right elif op == ast.Lt: - current_result = current_left < comparator + result = left < right elif op == ast.LtE: - current_result = current_left <= comparator + result = left <= right elif op == ast.Gt: - current_result = current_left > comparator + result = left > right elif op == ast.GtE: - current_result = current_left >= comparator + result = left >= right elif op == ast.Is: - current_result = current_left is comparator + result = left is right elif op == ast.IsNot: - current_result = current_left is not comparator + result = left is not right elif op == ast.In: - current_result = current_left in comparator + result = left in right elif op == ast.NotIn: - current_result = current_left not in comparator + result = left not in right else: raise InterpreterError(f"Operator not supported: {op}") - - if not isinstance(current_result, bool): - return current_result - - result = result & current_result - current_left = comparator - if isinstance(result, bool) and not result: break - + left = result return result From fe1756f9bb3ed066667adc8135eb1b390022fbf0 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 14:28:57 +0100 Subject: [PATCH 06/13] Fix test_non_standard_comparisons for chained condition --- tests/test_local_python_executor.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index 7a8a3f77f..07a50d4b3 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -1291,6 +1291,8 @@ def __init__(self, left:object, right:object): self._right = right def __str__(self) -> str: return f'{{self._left}} == {{self._right}}' + def __eq__(self, other): + return NonStdEqualsResult(self, other) class NonStdComparisonClass: def __init__(self, value: str ): From f44919df5ac33708d7fabb048bc6a549bc1807a3 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 14:30:41 +0100 Subject: [PATCH 07/13] Make code explicit --- src/smolagents/local_python_executor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/smolagents/local_python_executor.py b/src/smolagents/local_python_executor.py index 3505d0961..a45e74708 100644 --- a/src/smolagents/local_python_executor.py +++ b/src/smolagents/local_python_executor.py @@ -740,7 +740,7 @@ def evaluate_condition( result = left not in right else: raise InterpreterError(f"Operator not supported: {op}") - if isinstance(result, bool) and not result: + if isinstance(result, bool) and result is False: break left = result return result From 32e98487190cd8293c03aa767cd445745269799a Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 14:31:23 +0100 Subject: [PATCH 08/13] Make code simpler --- src/smolagents/local_python_executor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/smolagents/local_python_executor.py b/src/smolagents/local_python_executor.py index a45e74708..86d4af9e8 100644 --- a/src/smolagents/local_python_executor.py +++ b/src/smolagents/local_python_executor.py @@ -740,7 +740,7 @@ def evaluate_condition( result = left not in right else: raise InterpreterError(f"Operator not supported: {op}") - if isinstance(result, bool) and result is False: + if result is False: break left = result return result From e5cff32a3b14a3b158095e2b884163a8a73caf69 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Thu, 13 Feb 2025 15:07:40 +0100 Subject: [PATCH 09/13] Refactor test_evaluate_condition for chained conditions --- tests/test_local_python_executor.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index 07a50d4b3..6cd2c9318 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -1192,8 +1192,8 @@ def test_evaluate_delete(code, state, expectation): # Chained conditions: ("a == b == c", {"a": 1, "b": 1, "c": 1}, True), ("a == b == c", {"a": 1, "b": 2, "c": 1}, False), - ("a == b < c", {"a": 1, "b": 1, "c": 1}, False), - ("a == b < c", {"a": 1, "b": 1, "c": 2}, True), + ("a == b < c", {"a": 2, "b": 2, "c": 2}, False), + ("a == b < c", {"a": 0, "b": 0, "c": 1}, True), ], ) def test_evaluate_condition(condition, state, expected_result): From 08970442e0b4f60d9bd74072fc9622b1980a3fcd Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Fri, 14 Feb 2025 09:03:46 +0100 Subject: [PATCH 10/13] Revert Test composite condition with non-bool results --- tests/test_local_python_executor.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index 6cd2c9318..8b90a4f8c 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -1282,17 +1282,14 @@ def __dir__(self): assert getattr(safe_module, "non_lazy_attribute") == "ok" -@pytest.mark.parametrize("expected_result", ["a == b", "a == b == c"]) -def test_non_standard_comparisons(expected_result): - code = dedent(f"""\ +def test_non_standard_comparisons(): + code = dedent("""\ class NonStdEqualsResult: def __init__(self, left:object, right:object): self._left = left self._right = right def __str__(self) -> str: - return f'{{self._left}} == {{self._right}}' - def __eq__(self, other): - return NonStdEqualsResult(self, other) + return f'{self._left} == {self._right}' class NonStdComparisonClass: def __init__(self, value: str ): @@ -1303,12 +1300,11 @@ def __eq__(self, other): return NonStdEqualsResult(self, other) a = NonStdComparisonClass("a") b = NonStdComparisonClass("b") - c = NonStdComparisonClass("c") - result = {expected_result} + result = a == b """) result, _ = evaluate_python_code(code, state={}) assert not isinstance(result, bool) - assert str(result) == expected_result + assert str(result) == "a == b" class TestPrintContainer: From 191e1fc82292376b50261f34fc86d25fee8570e1 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Fri, 14 Feb 2025 09:05:48 +0100 Subject: [PATCH 11/13] Test evaluate_condition with pandas raises with chained conditions --- tests/test_local_python_executor.py | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index 8b90a4f8c..6b86c3b71 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -1241,6 +1241,20 @@ def test_evaluate_condition(condition, state, expected_result): {"a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), "b": pd.DataFrame({"x": [2, 2], "y": [2, 2]})}, pd.DataFrame({"x": [False, True], "y": [True, True]}), ), + ], +) +def test_evaluate_condition_with_pandas(condition, state, expected_result): + condition_ast = ast.parse(condition, mode="eval").body + result = evaluate_condition(condition_ast, state, {}, {}, []) + if isinstance(result, pd.Series): + pd.testing.assert_series_equal(result, expected_result) + else: + pd.testing.assert_frame_equal(result, expected_result) + + +@pytest.mark.parametrize( + "condition, state, expected_exception", + [ # Chained conditions: ( "a == b == c", @@ -1249,17 +1263,17 @@ def test_evaluate_condition(condition, state, expected_result): "b": pd.Series([2, 2, 2]), "c": pd.Series([3, 3, 3]), }, - pd.Series([False, False, False]), + ValueError( + "The truth value of a Series is ambiguous. Use a.empty, a.bool(), a.item(), a.any() or a.all()." + ), ), ], ) -def test_evaluate_condition_with_pandas(condition, state, expected_result): +def test_evaluate_condition_with_pandas_exceptions(condition, state, expected_exception): condition_ast = ast.parse(condition, mode="eval").body - result = evaluate_condition(condition_ast, state, {}, {}, []) - if isinstance(result, pd.Series): - pd.testing.assert_series_equal(result, expected_result) - else: - pd.testing.assert_frame_equal(result, expected_result) + with pytest.raises(type(expected_exception)) as exception_info: + _ = evaluate_condition(condition_ast, state, {}, {}, []) + assert str(expected_exception) in str(exception_info.value) def test_get_safe_module_handle_lazy_imports(): From f6fe673ec0fdd21b4823692afae1593cca7f073b Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Fri, 14 Feb 2025 09:06:48 +0100 Subject: [PATCH 12/13] Fix evaluate_condition --- src/smolagents/local_python_executor.py | 33 ++++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/smolagents/local_python_executor.py b/src/smolagents/local_python_executor.py index 86d4af9e8..d8fe84e69 100644 --- a/src/smolagents/local_python_executor.py +++ b/src/smolagents/local_python_executor.py @@ -714,35 +714,38 @@ def evaluate_condition( custom_tools: Dict[str, Callable], authorized_imports: List[str], ) -> bool | object: + result = True left = evaluate_ast(condition.left, state, static_tools, custom_tools, authorized_imports) - for op, comparator in zip(condition.ops, condition.comparators): + for i, (op, comparator) in enumerate(zip(condition.ops, condition.comparators)): op = type(op) right = evaluate_ast(comparator, state, static_tools, custom_tools, authorized_imports) if op == ast.Eq: - result = left == right + current_result = left == right elif op == ast.NotEq: - result = left != right + current_result = left != right elif op == ast.Lt: - result = left < right + current_result = left < right elif op == ast.LtE: - result = left <= right + current_result = left <= right elif op == ast.Gt: - result = left > right + current_result = left > right elif op == ast.GtE: - result = left >= right + current_result = left >= right elif op == ast.Is: - result = left is right + current_result = left is right elif op == ast.IsNot: - result = left is not right + current_result = left is not right elif op == ast.In: - result = left in right + current_result = left in right elif op == ast.NotIn: - result = left not in right + current_result = left not in right else: - raise InterpreterError(f"Operator not supported: {op}") - if result is False: - break - left = result + raise InterpreterError(f"Unsupported comparison operator: {op}") + + if current_result is False: + return False + result = current_result if i == 0 else (result and current_result) + left = right return result From 11cee85446a21126bcd9eb702d00008235af62d8 Mon Sep 17 00:00:00 2001 From: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com> Date: Fri, 14 Feb 2025 09:18:25 +0100 Subject: [PATCH 13/13] Test also for DataFrame --- tests/test_local_python_executor.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/test_local_python_executor.py b/tests/test_local_python_executor.py index 6b86c3b71..f7ecc91ee 100644 --- a/tests/test_local_python_executor.py +++ b/tests/test_local_python_executor.py @@ -1267,13 +1267,24 @@ def test_evaluate_condition_with_pandas(condition, state, expected_result): "The truth value of a Series is ambiguous. Use a.empty, a.bool(), a.item(), a.any() or a.all()." ), ), + ( + "a == b == c", + { + "a": pd.DataFrame({"x": [1, 2], "y": [3, 4]}), + "b": pd.DataFrame({"x": [2, 2], "y": [2, 2]}), + "c": pd.DataFrame({"x": [3, 3], "y": [3, 3]}), + }, + ValueError( + "The truth value of a DataFrame is ambiguous. Use a.empty, a.bool(), a.item(), a.any() or a.all()." + ), + ), ], ) def test_evaluate_condition_with_pandas_exceptions(condition, state, expected_exception): condition_ast = ast.parse(condition, mode="eval").body with pytest.raises(type(expected_exception)) as exception_info: _ = evaluate_condition(condition_ast, state, {}, {}, []) - assert str(expected_exception) in str(exception_info.value) + assert str(expected_exception) in str(exception_info.value) def test_get_safe_module_handle_lazy_imports():