From 0a7c117387b750352e528450324c69b512add22d Mon Sep 17 00:00:00 2001
From: Violet Shreve <shreve@umich.edu>
Date: Tue, 7 Mar 2023 12:01:08 -0500
Subject: [PATCH] Update Execute preprocessor traits

Update the traits on the Execute preprocessor based on the discussion in
https://github.com/jupyter/nbgrader/pull/1690, including adding a
version pin of ipython >= 8.10.0

Also, reorder traits to match the upstream parent, NotebookClient.
---
 nbgrader/preprocessors/execute.py             | 57 ++++---------------
 .../tests/apps/test_nbgrader_autograde.py     |  7 ++-
 pyproject.toml                                |  1 +
 3 files changed, 19 insertions(+), 46 deletions(-)

diff --git a/nbgrader/preprocessors/execute.py b/nbgrader/preprocessors/execute.py
index 7af28ce14..6f84ee8b1 100644
--- a/nbgrader/preprocessors/execute.py
+++ b/nbgrader/preprocessors/execute.py
@@ -14,57 +14,31 @@ class UnresponsiveKernelError(Exception):
 
 class Execute(NbGraderPreprocessor, ExecutePreprocessor):
 
+    timeout = Integer(
+        30,
+        help=ExecutePreprocessor.timeout.help,
+        allow_none=True,
+    ).tag(config=True)
+
     interrupt_on_timeout = Bool(
         True,
-        help=dedent(
-            """
-            If execution of a cell times out, interrupt the kernel and
-            continue executing other cells rather than throwing an error and
-            stopping.
-            """
-        )
+        help=ExecutePreprocessor.interrupt_on_timeout.help
     ).tag(config=True)
 
     allow_errors = Bool(
         True,
         help=dedent(
             """
-            If ``False``, when a cell raises an error the
-            execution is stopped and a ``CellExecutionError``
-            is raised, except if the error name is in
-            ``allow_error_names``.
-            If ``True`` (default), execution errors are ignored and the execution
-            is continued until the end of the notebook. Output from
-            exceptions is included in the cell output in both cases.
+            When a cell execution results in an error, continue executing the rest of
+            the notebook. If False, the thrown nbclient exception would break aspects of
+            output rendering.
             """
         ),
-    ).tag(config=True)
+    )
 
     raise_on_iopub_timeout = Bool(
         True,
-        help=dedent(
-            """
-            If ``False``, then the kernel will continue waiting for
-            iopub messages until it receives a kernel idle message, or until a
-            timeout occurs, at which point the currently executing cell will be
-            skipped. If ``True`` (default), then an error will be raised after the first
-            timeout. This option generally does not need to be used, but may be
-            useful in contexts where there is the possibility of executing
-            notebooks with memory-consuming infinite loops.
-            """
-        ),
-    ).tag(config=True)
-
-    timeout = Integer(
-        30,
-        help=dedent(
-            """
-            The time to wait (in seconds) for output from executions.
-            If a cell execution takes longer, a TimeoutError is raised.
-            ``None`` or ``-1`` will disable the timeout. If ``timeout_func`` is set,
-            it overrides ``timeout``.
-            """
-        )
+        help=ExecutePreprocessor.raise_on_iopub_timeout.help
     ).tag(config=True)
 
     error_on_timeout = {
@@ -89,13 +63,6 @@ class Execute(NbGraderPreprocessor, ExecutePreprocessor):
         """)
     ).tag(config=True)
 
-    @validate('timeout')
-    def _validate_timeout(self, proposal):
-        value = int(proposal['value'])
-        if value <= 0:
-            raise TraitError("the timeout should be a positive value")
-        return value
-
     def on_cell_executed(self, **kwargs):
         cell = kwargs['cell']
         reply = kwargs['execute_reply']
diff --git a/nbgrader/tests/apps/test_nbgrader_autograde.py b/nbgrader/tests/apps/test_nbgrader_autograde.py
index 6fdbd3853..8fa82bb60 100644
--- a/nbgrader/tests/apps/test_nbgrader_autograde.py
+++ b/nbgrader/tests/apps/test_nbgrader_autograde.py
@@ -105,6 +105,11 @@ def test_showtraceback_exploit(self, db, course_dir):
         self._copy_file(join("files", "submitted-unchanged.ipynb"), join(course_dir, "source", "ps1", "p1.ipynb"))
         run_nbgrader(["generate_assignment", "ps1", "--db", db])
 
+        # This exploit previously caused cell executions that would indefinitely hang.
+        # See: https://github.com/ipython/ipython/commit/fd34cf5
+        with open("nbgrader_config.py", "a") as fh:
+            fh.write("""c.Execute.timeout = None""")
+
         self._copy_file(join("files", "submitted-unchanged.ipynb"), join(course_dir, "submitted", "foo", "ps1", "p1.ipynb"))
         self._copy_file(join("files", "submitted-changed.ipynb"), join(course_dir, "submitted", "bar", "ps1", "p1.ipynb"))
         self._copy_file(join("files", "submitted-cheat-attempt.ipynb"), join(course_dir, "submitted", "spam", "ps1", "p1.ipynb"))
@@ -995,7 +1000,7 @@ def test_infinite_loop(self, db, course_dir):
         run_nbgrader(["db", "student", "add", "foo", "--db", db])
         run_nbgrader(["db", "student", "add", "bar", "--db", db])
         with open("nbgrader_config.py", "a") as fh:
-            fh.write("""c.ExecutePreprocessor.timeout = 1""")
+            fh.write("""c.Execute.timeout = 1""")
 
         self._copy_file(join("files", "infinite-loop.ipynb"), join(course_dir, "source", "ps1", "p1.ipynb"))
         run_nbgrader(["generate_assignment", "ps1", "--db", db])
diff --git a/pyproject.toml b/pyproject.toml
index e09f0a315..f8f5cfc6c 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -32,6 +32,7 @@ classifiers = [
 ]
 dependencies = [
     "alembic>=1.7",
+    "ipython>=8.10.0",
     "ipywidgets>=7.6",
     "Jinja2>=3",
     "jsonschema>=3",