Feat/custom cell timeout (#321)

Custom cell timeout feature allows users to set fixed cell timeout per assignment. 

* WIP: add cell_timeout to assignment settings

* WIP: create function to validate user custom cell timeout against config cell timeout

* WIP: initial API endpoint for config

* WIP: set default cell timeout in config file

* test: write API test for api/config endpoint

* fix: rewrite tests to adhere to new implementation of timeout_func

* test: test setting default_cell_timeout to an invalid value

* fix: change timeout_func trait type to Int

* fix: validate config values of default, min and max cell timeout

* config: set default_cell_timeoutin grader_service_config.py for docker

* fix: pass the result of _determine_cell_timeout correctly

Author: nadjajovancevic

api-tests/api/get-config.bru

@@ -0,0 +1,16 @@
+meta {
+  name: get-config
+  type: http
+  seq: 2
+}
+
+get {
+  url: {{grader-base-url}}/api/config
+  body: none
+  auth: inherit
+}
+
+settings {
+  encodeUrl: true
+  timeout: 0
+}

docs/source/_static/openapi/schemas.yml

@@ -92,6 +92,9 @@ components:
         group:
           type: "string"
           example: "Chapter 1: Data Types"
+        cell_timeout:
+          type: "integer"
+          example: "300"
       example:  
         late_submission:
           - period: "P1W1D"

examples/dev_environment/grader_service_config.py

@@ -23,6 +23,7 @@
 c.GraderService.log_level = "DEBUG"
 
 c.RequestHandlerConfig.autograde_executor_class = LocalAutogradeExecutor
+c.LocalAutogradeExecutor.default_cell_timeout = 200
 
 c.CeleryApp.conf = dict(
     broker_url="amqp://localhost",

examples/docker_compose/grader_service_config.py

@@ -33,6 +33,7 @@
 c.GraderService.db_url = db_url
 
 c.RequestHandlerConfig.autograde_executor_class = LocalAutogradeExecutor
+c.LocalAutogradeExecutor.default_cell_timeout = 200
 
 # get rabbitmq username and password
 rabbit_mq_username = os.getenv("RABBITMQ_GRADER_SERVICE_USERNAME")

grader_service/api/models/assignment_settings.py

@@ -14,7 +14,7 @@ class AssignmentSettings(Model):
     Do not edit the class manually.
     """
 
-    def __init__(self, deadline=None, max_submissions=None, allowed_files=[], late_submission=None, autograde_type='auto', group=None):  # noqa: E501
+    def __init__(self, deadline=None, max_submissions=None, allowed_files=[], late_submission=None, autograde_type='auto', group=None, cell_timeout=None):  # noqa: E501
         """AssignmentSettings - a model defined in OpenAPI
 
         :param deadline: The deadline of this AssignmentSettings.  # noqa: E501
@@ -29,14 +29,17 @@ def __init__(self, deadline=None, max_submissions=None, allowed_files=[], late_s
         :type autograde_type: str
         :param group: The group of this AssignmentSettings.  # noqa: E501
         :type group: str
+        :param cell_timeout: The cell_timeout of this AssignmentSettings.  # noqa: E501
+        :type cell_timeout: int
         """
         self.openapi_types = {
             'deadline': datetime,
             'max_submissions': int,
             'allowed_files': List[str],
             'late_submission': List[SubmissionPeriod],
             'autograde_type': str,
-            'group': str
+            'group': str,
+            'cell_timeout': int
         }
 
         self.attribute_map = {
@@ -45,7 +48,8 @@ def __init__(self, deadline=None, max_submissions=None, allowed_files=[], late_s
             'allowed_files': 'allowed_files',
             'late_submission': 'late_submission',
             'autograde_type': 'autograde_type',
-            'group': 'group'
+            'group': 'group',
+            'cell_timeout': 'cell_timeout'
         }
 
         self._deadline = deadline
@@ -54,6 +58,7 @@ def __init__(self, deadline=None, max_submissions=None, allowed_files=[], late_s
         self._late_submission = late_submission
         self._autograde_type = autograde_type
         self._group = group
+        self._cell_timeout = cell_timeout
 
     @classmethod
     def from_dict(cls, dikt) -> 'AssignmentSettings':
@@ -197,3 +202,24 @@ def group(self, group: str):
         """
 
         self._group = group
+
+    @property
+    def cell_timeout(self) -> int:
+        """Gets the cell_timeout of this AssignmentSettings.
+
+
+        :return: The cell_timeout of this AssignmentSettings.
+        :rtype: int
+        """
+        return self._cell_timeout
+
+    @cell_timeout.setter
+    def cell_timeout(self, cell_timeout: int):
+        """Sets the cell_timeout of this AssignmentSettings.
+
+
+        :param cell_timeout: The cell_timeout of this AssignmentSettings.
+        :type cell_timeout: int
+        """
+
+        self._cell_timeout = cell_timeout

grader_service/autograding/kube/kube_grader.py

@@ -350,7 +350,7 @@ def _start_pod(self) -> GraderPod:
             "-p",
             "*.ipynb",
             "--log-level=INFO",
-            f"--ExecutePreprocessor.timeout={self.timeout_func(self.assignment.lecture)}",
+            f"--ExecutePreprocessor.timeout={self.cell_timeout}",
         ]
         volumes = [self.volume] + self.extra_volumes
         volume_mounts = [

grader_service/autograding/local_grader.py

@@ -17,23 +17,18 @@
 from sqlalchemy.orm import Session
 from traitlets.config import Config
 from traitlets.config.configurable import LoggingConfigurable
-from traitlets.traitlets import Callable, TraitError, Type, Unicode, validate
+from traitlets.traitlets import Int, TraitError, Type, Unicode, validate
 
 from grader_service.autograding.git_manager import GitSubmissionManager
 from grader_service.autograding.utils import collect_logs, executable_validator, rmtree
 from grader_service.convert.converters.autograde import Autograde
 from grader_service.convert.gradebook.models import GradeBookModel
 from grader_service.orm.assignment import Assignment
-from grader_service.orm.lecture import Lecture
 from grader_service.orm.submission import AutoStatus, ManualStatus, Submission
 from grader_service.orm.submission_logs import SubmissionLogs
 from grader_service.orm.submission_properties import SubmissionProperties
 
 
-def default_timeout_func(lecture: Lecture) -> int:
-    return 360
-
-
 class LocalAutogradeExecutor(LoggingConfigurable):
     """
     Runs an autograde job on the local machine
@@ -46,10 +41,19 @@ class LocalAutogradeExecutor(LoggingConfigurable):
     relative_output_path = Unicode("convert_out", allow_none=True).tag(config=True)
     git_manager_class = Type(GitSubmissionManager, allow_none=False).tag(config=True)
 
-    timeout_func = Callable(
-        default_timeout_func,
+    cell_timeout = Int(
         allow_none=False,
-        help="Function that takes a lecture as an argument and returns the cell timeout in seconds.",
+        help="Returns the cell timeout in seconds, either user-defined, from configuration or default values.",
+    ).tag(config=True)
+
+    default_cell_timeout = Int(300, help="Default cell timeout in seconds, defaults to 300").tag(
+        config=True
+    )
+
+    min_cell_timeout = Int(10, help="Min cell timeout in seconds, defaults to 10.").tag(config=True)
+
+    max_cell_timeout = Int(
+        86400, help="Max cell timeout in seconds, defaults to 86400 (24 hours)"
     ).tag(config=True)
 
     def __init__(
@@ -86,6 +90,8 @@ def __init__(
         # Git manager performs the git operations when creating a new repo for the grading results
         self.git_manager = self.git_manager_class(grader_service_dir, self.submission)
 
+        self.cell_timeout = self._determine_cell_timeout()
+
     def start(self):
         """
         Starts the autograding job.
@@ -219,7 +225,7 @@ def _put_grades_in_assignment_properties(self) -> str:
     def _get_autograde_config(self) -> Config:
         """Returns the autograde config, with the timeout set for ExecutePreprocessor."""
         c = Config()
-        c.ExecutePreprocessor.timeout = self.timeout_func(self.assignment.lecture)
+        c.ExecutePreprocessor.timeout = self.cell_timeout
         return c
 
     def _get_whitelist_patterns(self) -> set[str]:
@@ -320,6 +326,41 @@ def _cleanup(self) -> None:
         if self.close_session:
             self.session.close()
 
+    def _determine_cell_timeout(self):
+        cell_timeout = self.default_cell_timeout
+        # check if the cell timeout was set by user
+        if self.assignment.settings.cell_timeout is not None:
+            custom_cell_timeout = self.assignment.settings.cell_timeout
+            self.log.info(
+                f"Found custom cell timeout in assignment settings: {custom_cell_timeout} seconds."
+            )
+            cell_timeout = min(
+                self.max_cell_timeout, max(custom_cell_timeout, self.min_cell_timeout)
+            )
+            self.log.info(f"Setting custom cell timeout to {cell_timeout}.")
+
+        return cell_timeout
+
+    @validate("min_cell_timeout", "default_cell_timeout", "max_cell_timeout")
+    def _validate_cell_timeouts(self, proposal):
+        trait_name = proposal["trait"].name
+        value = proposal["value"]
+
+        # Get current or proposed values
+        min_t = value if trait_name == "min_cell_timeout" else self.min_cell_timeout
+        default_t = value if trait_name == "default_cell_timeout" else self.default_cell_timeout
+        max_t = value if trait_name == "max_cell_timeout" else self.max_cell_timeout
+
+        # Validate the constraint
+        if not (0 < min_t < default_t < max_t):
+            raise TraitError(
+                f"Invalid {trait_name} value ({value}). "
+                f"Timeout values must satisfy: 0 < min_cell_timeout < default_cell_timeout < max_cell_timeout. "
+                f"Got min={min_t}, default={default_t}, max={max_t}."
+            )
+
+        return value
+
     @validate("relative_input_path", "relative_output_path")
     def _validate_service_dir(self, proposal):
         path: str = proposal["value"]
@@ -358,7 +399,7 @@ def _run(self):
             self.output_path,
             "-p",
             "*.ipynb",
-            f"--ExecutePreprocessor.timeout={self.timeout_func(self.assignment.lecture)}",
+            f"--ExecutePreprocessor.timeout={self.cell_timeout}",
         ]
         self.log.info(f"Running {command}")
         process = subprocess.run(

grader_service/handlers/config.py

@@ -1,18 +1,48 @@
+import traitlets.config
+
+from grader_service.autograding.local_grader import LocalAutogradeExecutor
 from grader_service.handlers.base_handler import GraderBaseHandler, authorize
 from grader_service.orm.takepart import Scope
 from grader_service.registry import VersionSpecifier, register_handler
 
 
+def _get_effective_executor_value(
+    app_cfg: traitlets.config.Config, executor_class: type[LocalAutogradeExecutor], trait_name: str
+):
+    """
+    Return the configured value for trait_name if present in app_cfg,
+    otherwise return the class default pulled from the trait metadata.
+    """
+
+    # 1) retrieve the provided class field from the config
+    executor_class_field = app_cfg.get(executor_class.__name__, None)
+
+    # executor_class_field may be None, or a Config object / dict-like. Use mapping access.
+    if executor_class_field is not None and trait_name in executor_class_field:
+        return executor_class_field.get(trait_name)
+
+    # 2) fallback to the trait's default from the class metadata
+    return executor_class.class_traits()[trait_name].default()
+
+
 @register_handler(path=r"\/api\/config\/?", version_specifier=VersionSpecifier.ALL)
 class ConfigHandler(GraderBaseHandler):
     """
     Handler class for requests to /config
     """
 
-    @authorize([Scope.student, Scope.tutor, Scope.instructor])
+    @authorize([Scope.tutor, Scope.instructor])
     async def get(self):
-        """
-        Gathers useful config for the grader labextension and returns it.
-        :return: config in dict
-        """
-        self.write({})
+        app_cfg = self.application.config
+        executor_class = app_cfg.RequestHandlerConfig.autograde_executor_class
+
+        def resolve(name):
+            return _get_effective_executor_value(app_cfg, executor_class, name)
+
+        self.write_json(
+            {
+                "default_cell_timeout": resolve("default_cell_timeout"),
+                "min_cell_timeout": resolve("min_cell_timeout"),
+                "max_cell_timeout": resolve("max_cell_timeout"),
+            }
+        )

grader_service/tests/autograding/test_local_grader.py

@@ -6,12 +6,13 @@
 from unittest.mock import MagicMock, Mock, patch
 
 import pytest
+import traitlets.traitlets
 
 from grader_service.autograding.local_grader import (
     LocalAutogradeExecutor,
     LocalAutogradeProcessExecutor,
 )
-from grader_service.orm import Assignment, Lecture
+from grader_service.orm import Assignment
 from grader_service.orm.submission import AutoStatus
 
 
@@ -223,8 +224,8 @@ def side_effect():
 def test_timeout_function_default(local_autograde_executor):
     """Test default timeout function"""
 
-    timeout = local_autograde_executor.timeout_func(Lecture())
-    assert timeout == 360  # Default timeout
+    timeout = local_autograde_executor.cell_timeout
+    assert timeout == 300  # Default timeout
 
 
 @patch("grader_service.autograding.local_grader.Session")
@@ -235,20 +236,34 @@ def test_timeout_function_default(local_autograde_executor):
 def test_timeout_function_custom(mock_git, mock_session_class, tmp_path, submission_123):
     """Test custom timeout function"""
 
-    def custom_timeout(lecture):
-        return 720
+    custom_timeout = 720
 
     executor = LocalAutogradeExecutor(
         grader_service_dir=str(tmp_path),
         submission=submission_123,
         close_session=False,
-        timeout_func=custom_timeout,
+        default_cell_timeout=custom_timeout,
     )
 
-    timeout = executor.timeout_func(Lecture())
+    timeout = executor.cell_timeout
     assert timeout == 720
 
 
+def test_invalid_custom_default_timeout(tmp_path, submission_123):
+    invalid_timeout = -1
+
+    executor = LocalAutogradeExecutor(
+        grader_service_dir=str(tmp_path), submission=submission_123, close_session=False
+    )
+    with pytest.raises(traitlets.traitlets.TraitError) as exc_info:
+        executor.default_cell_timeout = invalid_timeout
+    assert exc_info.value.args[0] == (
+        f"Invalid default_cell_timeout value ({invalid_timeout}). "
+        "Timeout values must satisfy: 0 < min_cell_timeout < default_cell_timeout < max_cell_timeout. "
+        f"Got min={executor.min_cell_timeout}, default={invalid_timeout}, max={executor.max_cell_timeout}."
+    )
+
+
 def test_gradebook_writing(local_autograde_executor):
     """Test that gradebook is written correctly"""
 
@@ -302,7 +317,7 @@ def test_process_executor_run_failure(mock_run, process_executor):
         process_executor.output_path,
         "-p",
         "*.ipynb",
-        "--ExecutePreprocessor.timeout=360",
+        "--ExecutePreprocessor.timeout=300",
     ]
 
     mock_run.assert_called_once_with(

grader_service/tests/handlers/test_base_handler.py

@@ -64,6 +64,7 @@ def test_assignment_serialization():
             "late_submission": None,
             "deadline": datetime.now(tz=timezone.utc).isoformat(),
             "group": None,
+            "cell_timeout": None,
             "max_submissions": 1,
             "autograde_type": "unassisted",
             "allowed_files": None,