diff --git a/app/context/numerical.py b/app/context/numerical.py index f179d1b..c563d76 100644 --- a/app/context/numerical.py +++ b/app/context/numerical.py @@ -1,6 +1,35 @@ from sympy import pi, N +def replace_pi(expr, value=float(pi)): + # REMARK: 'pi' should be a reserved symbol but it is sometimes not treated as one, possibly because of input symbols. + # Replacing it with its value fixes the issue but a more robust solution should be found for cases where there + # are other reserved symbols. + pi_symbol = pi + for s in expr.free_symbols: + if str(s) == 'pi': + pi_symbol = s + return expr.subs(pi_symbol, value) + + +def numerically_equal(lhs_expr, rhs_expr): + """ + Whether `lhs_expr` and `rhs_expr` are equal when evaluated to 50 digits: compares numbers + without simplifying them, which can take practically forever (see `app.time_budget`). + + Returns None if either contains symbols other than `pi` or can't be evaluated to a number. + """ + lhs, rhs = replace_pi(lhs_expr, pi), replace_pi(rhs_expr, pi) + if lhs.free_symbols or rhs.free_symbols: + return None + try: + difference = abs(complex(N(lhs-rhs, 50))) + scale = max(1.0, abs(complex(N(lhs, 50))), abs(complex(N(rhs, 50)))) + except (TypeError, ValueError, OverflowError): + return None + return bool(difference <= 1e-30*scale) + + def compare_numerically(lhs_expr, rhs_expr, parameters_dict): """ Compares `lhs_expr` and `rhs_expr` numerically using the `atol` and `rtol` parameters. @@ -13,15 +42,6 @@ def compare_numerically(lhs_expr, rhs_expr, parameters_dict): or float(parameters_dict.get("atol", 0)) > 0): return False - # REMARK: 'pi' should be a reserved symbol but it is sometimes not treated as one, possibly because of input symbols. - # The two lines below this comments fixes the issue but a more robust solution should be found for cases where there - # are other reserved symbols. - def replace_pi(expr): - pi_symbol = pi - for s in expr.free_symbols: - if str(s) == 'pi': - pi_symbol = s - return expr.subs(pi_symbol, float(pi)) # NOTE: This code assumes that the left hand side is the response and the right hand side is the answer # Separates LHS and RHS, parses and evaluates them res = N(replace_pi(lhs_expr)) diff --git a/app/context/symbolic.py b/app/context/symbolic.py index c85e36e..811b371 100644 --- a/app/context/symbolic.py +++ b/app/context/symbolic.py @@ -11,10 +11,11 @@ ) from ..preview.symbolic import preview_function -from .numerical import compare_numerically +from .numerical import compare_numerically, numerically_equal from .set_equivalence import add_set_equivalence_nodes, as_sets from ..feedback.symbolic import feedback_generators as symbolic_feedback_string_generators from ..feedback.syntactical_forms import PATTERNS as syntactical_forms +from ..time_budget import comparison_budget from lambdafeedback.criteria import CriteriaGraph, build_criteria_parser @@ -133,23 +134,28 @@ def check_equality(criterion, parameters_dict, local_substitutions=[]): elif not isinstance(lhs_expr, Equality) and isinstance(rhs_expr, Equality): result = False else: - try: - either_is_infinite = lhs_expr.is_infinite or rhs_expr.is_infinite - except (KeyError, TypeError): - either_is_infinite = False - if either_is_infinite: - result = do_comparison_infinite(criterion.content, lhs_expr, rhs_expr) - else: - result = do_comparison(criterion.content, lhs_expr-rhs_expr) - # There are some types of expression, e.g. those containing hyperbolic trigonometric functions, that can behave - # unpredictably when simplification is applied. For that reason we check several different combinations of - # simplifications here in order to reduce the likelihood of false negatives. - if result is False: - result = do_comparison(criterion.content, lhs_expr-rhs_expr.simplify()) - if result is False: - result = do_comparison(criterion.content, lhs_expr.simplify()-rhs_expr) - if result is False: - result = do_comparison(criterion.content, lhs_expr.simplify()-rhs_expr.simplify()) + result = None + with comparison_budget() as budget: + try: + either_is_infinite = lhs_expr.is_infinite or rhs_expr.is_infinite + except (KeyError, TypeError): + either_is_infinite = False + if either_is_infinite: + result = do_comparison_infinite(criterion.content, lhs_expr, rhs_expr) + else: + result = do_comparison(criterion.content, lhs_expr-rhs_expr) + # There are some types of expression, e.g. those containing hyperbolic trigonometric functions, that can behave + # unpredictably when simplification is applied. For that reason we check several different combinations of + # simplifications here in order to reduce the likelihood of false negatives. + if result is False: + result = do_comparison(criterion.content, lhs_expr-rhs_expr.simplify()) + if result is False: + result = do_comparison(criterion.content, lhs_expr.simplify()-rhs_expr) + if result is False: + result = do_comparison(criterion.content, lhs_expr.simplify()-rhs_expr.simplify()) + if budget.exceeded: + # Simplifying took too long (e.g. 10^8*log(0.1) builds 10^(10^8)), so compare numbers by value instead + result = numerically_equal(lhs_expr, rhs_expr) if criterion.content.strip() == "=" else None if result is False: result = compare_numerically(lhs_expr, rhs_expr, parameters_dict) @@ -159,17 +165,20 @@ def check_equality(criterion, parameters_dict, local_substitutions=[]): def check_order(criterion, parameters_dict, local_substitutions=[]): lhs_expr, rhs_expr = create_expressions_for_comparison(criterion, parameters_dict, local_substitutions) - result = do_comparison(criterion.content, lhs_expr-rhs_expr) + result = None + with comparison_budget(): + result = do_comparison(criterion.content, lhs_expr-rhs_expr) return result def check_proportionality(criterion, parameters_dict, local_substitutions=[]): lhs_expr, rhs_expr = create_expressions_for_comparison(criterion, parameters_dict, local_substitutions) result = None - if lhs_expr.cancel().simplify().simplify() != 0: - result = (rhs_expr/lhs_expr).cancel().simplify() - elif rhs_expr.cancel().simplify().simplify() != 0: - result = (lhs_expr/rhs_expr).cancel().simplify() + with comparison_budget(): + if lhs_expr.cancel().simplify().simplify() != 0: + result = (rhs_expr/lhs_expr).cancel().simplify() + elif rhs_expr.cancel().simplify().simplify() != 0: + result = (lhs_expr/rhs_expr).cancel().simplify() if result == 0 or result is None: result = False else: @@ -275,13 +284,14 @@ def equality_equivalence(unused_input): # TODO: Remove when criteria for checking proportionality is implemented if isinstance(res, Equality) and isinstance(ans, Equality): - if (res.args[0]-res.args[1]).simplify() == 0: - symbols_in_equality_ratio = (ans.args[0]-ans.args[1]).simplify().free_symbols - elif (ans.args[0]-ans.args[1]).simplify() == 0: - symbols_in_equality_ratio = (res.args[0]-res.args[1]).simplify().free_symbols - else: - symbols_in_equality_ratio = ((res.args[0]-res.args[1])/(ans.args[0]-ans.args[1])).simplify().free_symbols - result = {str(s) for s in symbols_in_equality_ratio}.issubset(parameters_dict["parsing_parameters"].constants) + with comparison_budget(): + if (res.args[0]-res.args[1]).simplify() == 0: + symbols_in_equality_ratio = (ans.args[0]-ans.args[1]).simplify().free_symbols + elif (ans.args[0]-ans.args[1]).simplify() == 0: + symbols_in_equality_ratio = (res.args[0]-res.args[1]).simplify().free_symbols + else: + symbols_in_equality_ratio = ((res.args[0]-res.args[1])/(ans.args[0]-ans.args[1])).simplify().free_symbols + result = {str(s) for s in symbols_in_equality_ratio}.issubset(parameters_dict["parsing_parameters"].constants) if result is True: return { label+"_TRUE": None diff --git a/app/evaluation.py b/app/evaluation.py index 8b7f4f4..dac2890 100644 --- a/app/evaluation.py +++ b/app/evaluation.py @@ -13,6 +13,7 @@ from .feedback.symbolic import feedback_generators as symbolic_feedback_string_generators from .feedback.symbolic import internal_feedback from .feedback.tags import FeedbackTag, FeedbackTagName +from .time_budget import comparison_budget, request_budget from collections.abc import Mapping @@ -310,50 +311,59 @@ def evaluation_function(response, answer, params, include_test_data=False) -> di evaluation_result.add_feedback( "NOTATION_WARNING_TRIPLE_FACTORIAL", symbolic_comparison_internal_messages("NOTATION_WARNING_TRIPLE_FACTORIAL")(dict())) - reserved_expressions_success, reserved_expressions = parse_reserved_expressions(reserved_expressions_strings, parameters, evaluation_result) - if reserved_expressions_success is False: - return evaluation_result.serialise(include_test_data) - reserved_expressions_parsed = {**reserved_expressions["learner"], **reserved_expressions["task"]} + # Simplifying can take practically forever (see `time_budget`), so the request + # ends with a grade rather than running into the Lambda timeout. + with request_budget() as budget: + reserved_expressions_success, reserved_expressions = parse_reserved_expressions(reserved_expressions_strings, parameters, evaluation_result) + if reserved_expressions_success is False: + return evaluation_result.serialise(include_test_data) + reserved_expressions_parsed = {**reserved_expressions["learner"], **reserved_expressions["task"]} - try: - preview = context["expression_preview"](response, deepcopy(parameters))["preview"] - except Exception: - evaluation_result.latex = response - evaluation_result.simplified = response - else: - evaluation_result.latex = preview["latex"] - parsed_response = reserved_expressions["learner"]["response"] - if isinstance(parsed_response, list) or isinstance(parsed_response, set): - evaluation_result.simplified = ", ".join([str(ex.simplify()) for ex in parsed_response]) + try: + preview = context["expression_preview"](response, deepcopy(parameters))["preview"] + except Exception: + evaluation_result.latex = response + evaluation_result.simplified = response else: - try: - evaluation_result.simplified = str(parsed_response.simplify()) - except Exception: + evaluation_result.latex = preview["latex"] + parsed_response = reserved_expressions["learner"]["response"] + with comparison_budget() as simplification: + if isinstance(parsed_response, list) or isinstance(parsed_response, set): + evaluation_result.simplified = ", ".join([str(ex.simplify()) for ex in parsed_response]) + else: + try: + evaluation_result.simplified = str(parsed_response.simplify()) + except Exception: + evaluation_result.simplified = response + if simplification.exceeded: evaluation_result.simplified = response - criteria_parser = context["generate_criteria_parser"](reserved_expressions) - criteria = create_criteria_dict(criteria_parser, parameters) + criteria_parser = context["generate_criteria_parser"](reserved_expressions) + criteria = create_criteria_dict(criteria_parser, parameters) - evaluation_parameters = FrozenValuesDictionary( - { - "reserved_expressions_strings": reserved_expressions_strings, - "reserved_expressions": reserved_expressions_parsed, - "criteria": criteria, - "disabled_evaluation_nodes": parameters.get("disabled_evaluation_nodes", set()), - "parsing_parameters": parsing_parameters, - "evaluation_result": evaluation_result, - "syntactical_comparison": parameters.get("syntactical_comparison", False), - "multiple_answers_criteria": parameters.get("multiple_answers_criteria", "all"), - "numerical": parameters.get("numerical", False), - "atol": parameters.get("atol", 0), - "rtol": parameters.get("rtol", 0), - "custom_feedback": parameters.get("custom_feedback", {}), - } - ) + evaluation_parameters = FrozenValuesDictionary( + { + "reserved_expressions_strings": reserved_expressions_strings, + "reserved_expressions": reserved_expressions_parsed, + "criteria": criteria, + "disabled_evaluation_nodes": parameters.get("disabled_evaluation_nodes", set()), + "parsing_parameters": parsing_parameters, + "evaluation_result": evaluation_result, + "syntactical_comparison": parameters.get("syntactical_comparison", False), + "multiple_answers_criteria": parameters.get("multiple_answers_criteria", "all"), + "numerical": parameters.get("numerical", False), + "atol": parameters.get("atol", 0), + "rtol": parameters.get("rtol", 0), + "custom_feedback": parameters.get("custom_feedback", {}), + } + ) - # Performs evaluation of response - feedback_procedures = parameters["context"]["feedback_procedure_generator"](evaluation_parameters) - generate_feedback(criteria, feedback_procedures, evaluation_parameters) + # Performs evaluation of response + feedback_procedures = parameters["context"]["feedback_procedure_generator"](evaluation_parameters) + generate_feedback(criteria, feedback_procedures, evaluation_parameters) + if budget.exceeded: + evaluation_result.is_correct = False + evaluation_result.add_feedback("TIME_LIMIT_EXCEEDED", symbolic_comparison_internal_messages("TIME_LIMIT_EXCEEDED")(dict())) result = evaluation_result.serialise(include_test_data) diff --git a/app/feedback/symbolic.py b/app/feedback/symbolic.py index 69c65f4..180334c 100644 --- a/app/feedback/symbolic.py +++ b/app/feedback/symbolic.py @@ -16,6 +16,7 @@ "ABSOLUTE_VALUE_NOTATION_AMBIGUITY": f"Notation in {inputs.get('name', '')} might be ambiguous, use `Abs(.)` instead of `|.|`", "BRACKET_NOTATION_MISMATCH": f"Brackets in `{inputs.get('x', '')}` do not match. Every `(`, `[` or `{{` must be closed with the corresponding `)`, `]` or `}}` — different bracket types cannot be mixed to close the same group.", "NO_RESPONSE": "No response submitted.", + "TIME_LIMIT_EXCEEDED": "The response could not be evaluated in time. Try writing it in a simpler form.", "MULTIPLE_ANSWER_FAIL_ALL": "At least one answer or response was incorrect.", "MULTIPLE_ANSWER_FAIL_RESPONSE": "At least one response was incorrect.", "MULTIPLE_ANSWER_FAIL_ANSWER": "At least one answer is missing in the response.", diff --git a/app/tests/symbolic_evaluation_test.py b/app/tests/symbolic_evaluation_test.py index 9752e91..9502ba0 100644 --- a/app/tests/symbolic_evaluation_test.py +++ b/app/tests/symbolic_evaluation_test.py @@ -1,5 +1,8 @@ +import time + import pytest +from .. import time_budget from ..evaluation import evaluation_function from lambdafeedback.expression_parsing import substitute from lambdafeedback.expression_parsing.symbol_tables import ELEMENTARY_FUNCTIONS as elementary_functions_names @@ -2258,3 +2261,46 @@ def test_greek_letters_in_expressions(self, unicode_expr, letter_expr): } result = evaluation_function(response, answer, params) assert result["is_correct"] is True + + # Simplifying a number such as 10^8*log(0.1) builds 10^(10^8) exactly, which + # never finishes, so these rely on the time budgets (see `app.time_budget`) + large_power_in_log_params = { + "elementary_functions": True, + "strict_syntax": False, + "physical_quantity": False, + "rtol": 0, + "atol": 0, + "symbols": {"pi": {"latex": r"\pi", "aliases": []}}, + } + + @pytest.fixture + def short_comparison_budget(self, monkeypatch): + monkeypatch.setattr(time_budget, "COMPARISON_BUDGET", 1) + + @pytest.mark.parametrize("response,answer", [ + ("log(0.1)*(10^8/ pi)", "(-1/(100*pi*k))*ln(r)+c"), + ("900000+10^8* pi* log(100)", "1.47*10**8"), + ]) + def test_large_power_times_numeric_log_is_graded_in_time(self, short_comparison_budget, response, answer): + start = time.monotonic() + result = evaluation_function(response, answer, self.large_power_in_log_params) + assert result["is_correct"] is False + assert time.monotonic()-start < 10 + + def test_large_power_times_numeric_log_is_compared_numerically(self, short_comparison_budget): + response = "log(0.1)*(10^8/ pi)" + params = self.large_power_in_log_params + assert evaluation_function(response, "-10^8*log(10)/pi", params)["is_correct"] is True + assert evaluation_function(response, "-10^8*log(10)/pi+10^-20", params)["is_correct"] is False + + def test_large_power_times_symbolic_log_is_simplified(self): + start = time.monotonic() + result = evaluation_function("-10^8/( pi) log(r)", "(-1/(100*pi*k))*ln(r)+c", self.large_power_in_log_params) + assert result["is_correct"] is False + assert time.monotonic()-start < 5 + + def test_evaluation_that_runs_out_of_time(self, monkeypatch): + monkeypatch.setattr(time_budget, "REQUEST_BUDGET", 1) + result = evaluation_function("log(0.1)*(10^8/ pi)", "(-1/(100*pi*k))*ln(r)+c", self.large_power_in_log_params) + assert result["is_correct"] is False + assert "TIME_LIMIT_EXCEEDED" in result["tags"] diff --git a/app/tests/time_budget_test.py b/app/tests/time_budget_test.py new file mode 100644 index 0000000..903b8a3 --- /dev/null +++ b/app/tests/time_budget_test.py @@ -0,0 +1,88 @@ +import signal +import threading +import time + +import pytest + +from ..time_budget import time_budget + + +def busy(seconds): + """Python-level work (SIGALRM is handled between bytecodes, like during SymPy's loops).""" + end = time.monotonic()+seconds + count = 0 + while time.monotonic() < end: + count += 1 + return count + + +def test_work_within_budget_finishes(): + with time_budget(1) as budget: + result = busy(0.01) + assert result > 0 + assert budget.exceeded is False + + +def test_expired_budget_stops_work(): + result = None + start = time.monotonic() + with time_budget(0.1) as budget: + result = busy(5) + assert result is None + assert budget.exceeded is True + assert time.monotonic()-start < 1 + + +def test_expired_budget_is_not_swallowed_by_except_exception(): + with time_budget(0.1) as budget: + try: + busy(5) + except Exception: + pytest.fail("TimeBudgetExceeded was caught as an Exception") + assert budget.exceeded is True + + +def test_inner_budget_expires_inside_outer(): + with time_budget(5) as outer: + with time_budget(0.1) as inner: + busy(5) + busy(0.01) + assert inner.exceeded is True + assert outer.exceeded is False + + +def test_outer_budget_expiry_passes_through_inner(): + with time_budget(0.1) as outer: + with time_budget(5) as inner: + busy(5) + pytest.fail("work after the inner budget ran after the outer budget expired") + assert outer.exceeded is True + assert inner.exceeded is False + + +def test_timer_and_handler_are_restored(): + def handler(signum, frame): + pass + previous = signal.signal(signal.SIGALRM, handler) + try: + with time_budget(0.1): + busy(5) + assert signal.getsignal(signal.SIGALRM) is handler + assert signal.getitimer(signal.ITIMER_REAL) == (0.0, 0.0) + finally: + signal.signal(signal.SIGALRM, previous) + + +def test_budget_does_nothing_off_the_main_thread(): + outcome = {} + + def work(): + with time_budget(0.01) as budget: + outcome["result"] = busy(0.1) + outcome["exceeded"] = budget.exceeded + + thread = threading.Thread(target=work) + thread.start() + thread.join() + assert outcome == {"result": outcome["result"], "exceeded": False} + assert signal.getitimer(signal.ITIMER_REAL) == (0.0, 0.0) diff --git a/app/time_budget.py b/app/time_budget.py new file mode 100644 index 0000000..bb10f31 --- /dev/null +++ b/app/time_budget.py @@ -0,0 +1,101 @@ +""" + Time budgets for SymPy work that can run practically forever. + + Simplifying can build huge exact numbers: `simplify` calls `logcombine`, + which writes the coefficient of `-10**8*log(10)/pi` into the logarithm as + `log(10**(-10**8))`, a number of a hundred million digits. A budget + interrupts such work with SIGALRM, so the request still gets a grade + instead of running into the Lambda timeout. + + Budgets nest: the timer is set for the earliest deadline, and each budget + only stops the work for its own expiry, so an outer budget's expiry + passes through inner ones. Off the main thread, where signals can't be + handled, a budget does nothing. +""" +import signal +import threading +import time +from contextlib import contextmanager + +REQUEST_BUDGET = 60 +"""Seconds for evaluating one request, well within the 120 s Lambda timeout.""" + +COMPARISON_BUDGET = 5 +"""Seconds for one symbolic comparison (or simplification) before falling back.""" + + +class TimeBudgetExceeded(BaseException): + """ + Raised when `budget` expires. A `BaseException`, so the `except Exception` + around many SymPy calls doesn't swallow it. + """ + def __init__(self, budget): + super().__init__(f"Time budget of {budget.seconds} s exceeded.") + self.budget = budget + + +class Budget: + def __init__(self, seconds): + self.seconds = seconds + self.deadline = time.monotonic() + seconds + self.exceeded = False + + +_active = [] # budgets in force, outermost first + + +def _arm_timer(): + if len(_active) > 0: + earliest = min(budget.deadline for budget in _active) + signal.setitimer(signal.ITIMER_REAL, max(earliest - time.monotonic(), 1e-3)) + else: + signal.setitimer(signal.ITIMER_REAL, 0) + + +def _expire(signum, frame): + if len(_active) == 0: + return + earliest = min(_active, key=lambda budget: budget.deadline) + if earliest.deadline > time.monotonic() + 0.01: + # The budget the timer was set for has ended since; wait for the next one + _arm_timer() + return + raise TimeBudgetExceeded(earliest) + + +@contextmanager +def time_budget(seconds): + """ + Stop the work in the `with` block after `seconds`. Yields a `Budget` + whose `exceeded` is set if it was stopped; the block's own assignments + after that point are skipped. + """ + budget = Budget(seconds) + if threading.current_thread() is not threading.main_thread(): + yield budget + return + previous_handler = None + if len(_active) == 0: + previous_handler = signal.getsignal(signal.SIGALRM) + signal.signal(signal.SIGALRM, _expire) + _active.append(budget) + _arm_timer() + try: + yield budget + except TimeBudgetExceeded as e: + if e.budget is not budget: + raise + budget.exceeded = True + finally: + _active.remove(budget) + _arm_timer() + if len(_active) == 0: + signal.signal(signal.SIGALRM, signal.SIG_DFL if previous_handler is None else previous_handler) + + +def request_budget(): + return time_budget(REQUEST_BUDGET) + + +def comparison_budget(): + return time_budget(COMPARISON_BUDGET)