fix: Feedback generation when some notebooks are missing in a submission

Author: natkam

grader_service/convert/converters/generate_feedback.py

@@ -56,12 +56,20 @@ def __init__(
         self.force = True  # always overwrite generated assignments
 
     def init_notebooks(self) -> None:
-        # We only generate feedback for the notebooks for which there is a gradebook entry,
-        # and ignore e.g. additional notebooks created by the student, because feedback
-        # generation would fail for them anyway.
+        super().init_notebooks()
         json_path = os.path.join(self._output_directory, "gradebook.json")
         with Gradebook(json_path) as gb:
-            self.notebooks = [f"{nb_id}.ipynb" for nb_id in gb.model.notebook_id_set]
+            # `self.notebooks` contains the notebooks actually submitted by the student.
+            # `notebook_id_set` contains the original notebooks from the assignment.
+            # We generate feedback for the notebooks belonging to these both sets.
+            student_nbs = {
+                self.init_single_notebook_resources(nb)["unique_key"]: nb for nb in self.notebooks
+            }
+            assign_nb_ids = gb.model.notebook_id_set
+            self.notebooks = [path for id, path in student_nbs.items() if id in assign_nb_ids]
+
+        if len(self.notebooks) == 0:
+            self.log.warning("No notebooks to generate feedback")
 
     def get_include_patterns(self, gb: Gradebook) -> list[str]:
         """Get glob patterns specifying for which submission files to generate feedback.

grader_service/tests/convert/converters/test_autograde.py

@@ -1,3 +1,4 @@
+import os
 import shutil
 from unittest.mock import patch
 
@@ -116,18 +117,13 @@ def test_autograde_with_student_notebooks_copied_over(tmp_path):
     _generate_test_submission(input_dir, output_dir)
 
     student_nb = output_dir / "student.ipynb"
-    student_nb.touch()
+    shutil.copyfile(input_dir / "simple.ipynb", student_nb)
 
     output_dir2 = tmp_path / "output_dir2"
     output_dir2.mkdir()
     shutil.copyfile(output_dir / "gradebook.json", output_dir2 / "gradebook.json")
 
-    with (
-        patch.object(NotebookClient, "kernel_name", "python3"),
-        patch(
-            "grader_service.convert.converters.autograde.Autograde.convert_single_notebook"
-        ) as convert_mock,
-    ):
+    with patch.object(NotebookClient, "kernel_name", "python3"):
         Autograde(
             input_dir=str(output_dir),
             output_dir=str(output_dir2),
@@ -140,4 +136,31 @@ def test_autograde_with_student_notebooks_copied_over(tmp_path):
     assert (output_dir2 / "gradebook.json").exists()
     assert (output_dir2 / "student.ipynb").exists()
 
-    assert convert_mock.call_count == 2  # `convert_single_notebook` was called for both notebooks
+
+def test_autograde_with_missing_submission_file(tmp_path):
+    """Test that Autograde handles files missing in the student submission gracefully"""
+    input_dir, output_dir = _create_input_output_dirs(tmp_path, ["simple.ipynb"])
+    _generate_test_submission(input_dir, output_dir)
+
+    # Student didn't submit the required notebook, but instead submitted a different one
+    os.remove(output_dir / "simple.ipynb")
+    student_nb = output_dir / "student.ipynb"
+    student_nb.touch()
+
+    output_dir2 = tmp_path / "output_dir2"
+    output_dir2.mkdir()
+    shutil.copyfile(output_dir / "gradebook.json", output_dir2 / "gradebook.json")
+
+    with patch.object(NotebookClient, "kernel_name", "python3"):
+        autograder = Autograde(
+            input_dir=str(output_dir),
+            output_dir=str(output_dir2),
+            file_pattern="*.ipynb",
+            assignment_settings=AssignmentSettings(allowed_files=["*.ipynb"]),
+            config=None,
+        )
+        autograder.start()
+
+    assert autograder.notebooks == [str(student_nb)]
+    assert (output_dir2 / "gradebook.json").exists()
+    assert (output_dir2 / "student.ipynb").exists()

grader_service/tests/convert/converters/test_generate_feedback.py

@@ -1,3 +1,4 @@
+import os
 import shutil
 
 from grader_service.api.models.assignment_settings import AssignmentSettings
@@ -84,7 +85,7 @@ def test_generate_feedback_with_student_notebooks(tmp_path):
     input_dir, output_dir = _create_input_output_dirs(tmp_path, ["simple.ipynb"])
 
     _generate_test_submission(
-        input_dir, output_dir, assignment_settings_kwargs={"allowed_files": ["*"]}
+        input_dir, output_dir, assignment_settings_kwargs={"allowed_files": ["*.ipynb"]}
     )
 
     student_nb = output_dir / "student.ipynb"
@@ -95,7 +96,7 @@ def test_generate_feedback_with_student_notebooks(tmp_path):
     shutil.copyfile(output_dir / "gradebook.json", output_dir2 / "gradebook.json")
 
     _autograde_test_submission(
-        str(output_dir), str(output_dir2), assignment_settings_kwargs={"allowed_files": ["*"]}
+        str(output_dir), str(output_dir2), assignment_settings_kwargs={"allowed_files": ["*.ipynb"]}
     )
 
     output_dir3 = tmp_path / "output_dir3"
@@ -107,7 +108,7 @@ def test_generate_feedback_with_student_notebooks(tmp_path):
         output_dir=str(output_dir3),
         file_pattern="*.ipynb",
         config=None,
-        assignment_settings=AssignmentSettings(allowed_files=["*"]),
+        assignment_settings=AssignmentSettings(allowed_files=["*.ipynb"]),
     ).start()
 
     # Only the original notebook should be copied to the feedback
@@ -116,7 +117,7 @@ def test_generate_feedback_with_student_notebooks(tmp_path):
     assert not (output_dir3 / "student.html").exists()
 
 
-def test_generate_assignment_copy_with_dirs(tmp_path):
+def test_generate_feedback_with_dirs(tmp_path):
     input_dir, output_dir = _create_input_output_dirs(tmp_path, ["simple.ipynb"])
 
     dir_1 = input_dir / "dir_1"
@@ -161,3 +162,40 @@ def test_generate_assignment_copy_with_dirs(tmp_path):
     assert not (output_dir3 / "dir_1/dir_2").exists()
     assert not (output_dir3 / "dir_1/dir_2/dir_3").exists()
     assert not (output_dir3 / "dir_1/dir_2/dir_3/test.txt").exists()
+
+
+def test_generate_feedback_with_missing_submission_file(tmp_path):
+    """Test that GenerateFeedback handles files missing in the student submission gracefully"""
+    input_dir, output_dir = _create_input_output_dirs(tmp_path, ["simple.ipynb"])
+
+    _generate_test_submission(
+        input_dir, output_dir, assignment_settings_kwargs={"allowed_files": ["*.ipynb"]}
+    )
+
+    # Student didn't submit the required notebook, but instead submitted a different one
+    os.remove(output_dir / "simple.ipynb")
+    student_nb = output_dir / "student.ipynb"
+    student_nb.touch()
+
+    output_dir2 = tmp_path / "output_dir2"
+    output_dir2.mkdir()
+    shutil.copyfile(output_dir / "gradebook.json", output_dir2 / "gradebook.json")
+
+    _autograde_test_submission(
+        str(output_dir), str(output_dir2), assignment_settings_kwargs={"allowed_files": ["*.ipynb"]}
+    )
+
+    output_dir3 = tmp_path / "output_dir3"
+    output_dir3.mkdir()
+    shutil.copyfile(output_dir2 / "gradebook.json", output_dir3 / "gradebook.json")
+
+    gf = GenerateFeedback(
+        input_dir=str(output_dir2),
+        output_dir=str(output_dir3),
+        file_pattern="*.ipynb",
+        config=None,
+        assignment_settings=AssignmentSettings(allowed_files=["*.ipynb"]),
+    )
+    gf.start()
+
+    assert gf.notebooks == []