Merge pull request #14984 from edx/neem/refactor-instructor-grades

Refactor Grade Report in prep for parallelization

lms/djangoapps/ccx/views.py

@@ -564,7 +564,7 @@ def ccx_grades_csv(request, course, ccx=None):
             courseenrollment__course_id=ccx_key,
             courseenrollment__is_active=1
         ).order_by('username').select_related("profile")
-        grades = CourseGradeFactory().iter(course, enrolled_students)
+        grades = CourseGradeFactory().iter(enrolled_students, course)
 
         header = None
         rows = []

lms/djangoapps/grades/new/course_data.py

 from lms.djangoapps.course_blocks.api import get_course_blocks
+from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager
 from xmodule.modulestore.django import modulestore
 from ..transformer import GradesTransformer
 
@@ -57,6 +58,12 @@ class CourseData(object):
         return self._structure
 
     @property
+    def collected_structure(self):
+        if not self._collected_block_structure:
+            self._collected_block_structure = get_block_structure_manager(self.course_key).get_collected()
+        return self._collected_block_structure
+
+    @property
     def course(self):
         if not self._course:
             self._course = modulestore().get_course(self.course_key)

lms/djangoapps/grades/new/course_grade_factory.py

@@ -2,7 +2,6 @@ from collections import namedtuple
 import dogstats_wrapper as dog_stats_api
 from logging import getLogger
 
-from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager
 from openedx.core.djangoapps.signals.signals import COURSE_GRADE_CHANGED
 
 from ..config import assume_zero_if_absent, should_persist_grades
@@ -77,7 +76,15 @@ class CourseGradeFactory(object):
         course_data = CourseData(user, course, collected_block_structure, course_structure, course_key)
         return self._update(user, course_data, read_only=False)
 
-    def iter(self, course, students, force_update=False):
+    def iter(
+            self,
+            users,
+            course=None,
+            collected_block_structure=None,
+            course_structure=None,
+            course_key=None,
+            force_update=False,
+    ):
         """
         Given a course and an iterable of students (User), yield a GradeResult
         for every student enrolled in the course.  GradeResult is a named tuple of:
@@ -92,25 +99,27 @@ class CourseGradeFactory(object):
         #    compute the grade for all students.
         # 2. Optimization: the collected course_structure is not
         #    retrieved from the data store multiple times.
-
-        collected_block_structure = get_block_structure_manager(course.id).get_collected()
-        for student in students:
-            with dog_stats_api.timer('lms.grades.CourseGradeFactory.iter', tags=[u'action:{}'.format(course.id)]):
+        course_data = CourseData(None, course, collected_block_structure, course_structure, course_key)
+        for user in users:
+            with dog_stats_api.timer(
+                    'lms.grades.CourseGradeFactory.iter',
+                    tags=[u'action:{}'.format(course_data.course_key)]
+            ):
                 try:
-                    operation = CourseGradeFactory().update if force_update else CourseGradeFactory().create
-                    course_grade = operation(student, course, collected_block_structure)
-                    yield self.GradeResult(student, course_grade, "")
+                    method = CourseGradeFactory().update if force_update else CourseGradeFactory().create
+                    course_grade = method(user, course, course_data.collected_structure, course_structure, course_key)
+                    yield self.GradeResult(user, course_grade, "")
 
                 except Exception as exc:  # pylint: disable=broad-except
                     # Keep marching on even if this student couldn't be graded for
                     # some reason, but log it for future reference.
                     log.exception(
                         'Cannot grade student %s in course %s because of exception: %s',
-                        student.id,
-                        course.id,
+                        user.id,
+                        course_data.course_key,
                         exc.message
                     )
-                    yield self.GradeResult(student, None, exc.message)
+                    yield self.GradeResult(user, None, exc.message)
 
     @staticmethod
     def _create_zero(user, course_data):

lms/djangoapps/grades/tasks.py

@@ -96,7 +96,7 @@ def compute_grades_for_course(course_key, offset, batch_size, **kwargs):  # pyli
     course = courses.get_course_by_id(CourseKey.from_string(course_key))
     enrollments = CourseEnrollment.objects.filter(course_id=course.id).order_by('created')
     student_iter = (enrollment.user for enrollment in enrollments[offset:offset + batch_size])
-    list(CourseGradeFactory().iter(course, students=student_iter, force_update=True))
+    list(CourseGradeFactory().iter(users=student_iter, course=course, force_update=True))
 
 
 @task(bind=True, base=_BaseTask, default_retry_delay=30, routing_key=settings.RECALCULATE_GRADES_ROUTING_KEY)

lms/djangoapps/grades/tests/test_grades.py

@@ -60,7 +60,7 @@ class TestGradeIteration(SharedModuleStoreTestCase):
         If we don't pass in any students, it should return a zero-length
         iterator, but it shouldn't error.
         """
-        grade_results = list(CourseGradeFactory().iter(self.course, []))
+        grade_results = list(CourseGradeFactory().iter([], self.course))
         self.assertEqual(grade_results, [])
 
     def test_all_empty_grades(self):
@@ -130,7 +130,7 @@ class TestGradeIteration(SharedModuleStoreTestCase):
         students_to_course_grades = {}
         students_to_errors = {}
 
-        for student, course_grade, err_msg in CourseGradeFactory().iter(course, students):
+        for student, course_grade, err_msg in CourseGradeFactory().iter(students, course):
             students_to_course_grades[student] = course_grade
             if err_msg:
                 students_to_errors[student] = err_msg

lms/djangoapps/instructor_task/tasks.py

@@ -39,9 +39,9 @@ from lms.djangoapps.instructor_task.tasks_helper.enrollments import (
     upload_students_csv,
 )
 from lms.djangoapps.instructor_task.tasks_helper.grades import (
-    generate_course_grade_report,
-    generate_problem_grade_report,
-    upload_problem_responses_csv,
+    CourseGradeReport,
+    ProblemGradeReport,
+    ProblemResponses,
 )
 from lms.djangoapps.instructor_task.tasks_helper.misc import (
     cohort_students_and_upload,
@@ -160,7 +160,7 @@ def calculate_problem_responses_csv(entry_id, xmodule_instance_args):
     """
     # Translators: This is a past-tense verb that is inserted into task progress messages as {action}.
     action_name = ugettext_noop('generated')
-    task_fn = partial(upload_problem_responses_csv, xmodule_instance_args)
+    task_fn = partial(ProblemResponses.generate, xmodule_instance_args)
     return run_main_task(entry_id, task_fn, action_name)
 
 
@@ -176,7 +176,7 @@ def calculate_grades_csv(entry_id, xmodule_instance_args):
         xmodule_instance_args.get('task_id'), entry_id, action_name
     )
 
-    task_fn = partial(generate_course_grade_report, xmodule_instance_args)
+    task_fn = partial(CourseGradeReport.generate, xmodule_instance_args)
     return run_main_task(entry_id, task_fn, action_name)
 
 
@@ -193,7 +193,7 @@ def calculate_problem_grade_report(entry_id, xmodule_instance_args):
         xmodule_instance_args.get('task_id'), entry_id, action_name
     )
 
-    task_fn = partial(generate_problem_grade_report, xmodule_instance_args)
+    task_fn = partial(ProblemGradeReport.generate, xmodule_instance_args)
     return run_main_task(entry_id, task_fn, action_name)
 
 

lms/djangoapps/instructor_task/tests/test_integration.py

@@ -32,7 +32,7 @@ from lms.djangoapps.instructor_task.api import (
     submit_delete_problem_state_for_all_students
 )
 from lms.djangoapps.instructor_task.models import InstructorTask
-from lms.djangoapps.instructor_task.tasks_helper.grades import generate_course_grade_report
+from lms.djangoapps.instructor_task.tasks_helper.grades import CourseGradeReport
 from lms.djangoapps.instructor_task.tests.test_base import (
     InstructorTaskModuleTestCase,
     TestReportMixin,
@@ -572,10 +572,10 @@ class TestGradeReportConditionalContent(TestReportMixin, TestConditionalContent,
     def verify_csv_task_success(self, task_result):
         """
         Verify that all students were successfully graded by
-        `generate_course_grade_report`.
+        `CourseGradeReport`.
 
         Arguments:
-            task_result (dict): Return value of `generate_course_grade_report`.
+            task_result (dict): Return value of `CourseGradeReport.generate`.
         """
         self.assertDictContainsSubset({'attempted': 2, 'succeeded': 2, 'failed': 0}, task_result)
 
@@ -636,7 +636,7 @@ class TestGradeReportConditionalContent(TestReportMixin, TestConditionalContent,
         self.submit_student_answer(self.student_b.username, problem_b_url, [OPTION_1, OPTION_2])
 
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            result = generate_course_grade_report(None, None, self.course.id, None, 'graded')
+            result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded')
             self.verify_csv_task_success(result)
             self.verify_grades_in_csv(
                 [
@@ -669,7 +669,7 @@ class TestGradeReportConditionalContent(TestReportMixin, TestConditionalContent,
         self.submit_student_answer(self.student_a.username, problem_a_url, [OPTION_1, OPTION_1])
 
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            result = generate_course_grade_report(None, None, self.course.id, None, 'graded')
+            result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded')
             self.verify_csv_task_success(result)
             self.verify_grades_in_csv(
                 [

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -59,14 +59,13 @@ from lms.djangoapps.instructor_task.tasks_helper.enrollments import (
     upload_students_csv,
 )
 from lms.djangoapps.instructor_task.tasks_helper.grades import (
-    generate_course_grade_report,
-    generate_problem_grade_report,
-    upload_problem_responses_csv,
+    CourseGradeReport,
+    ProblemGradeReport,
+    ProblemResponses,
 )
 from lms.djangoapps.instructor_task.tasks_helper.misc import (
     cohort_students_and_upload,
     upload_course_survey_report,
-    upload_proctored_exam_results_report,
     upload_ora2_data,
 )
 from ..tasks_helper.utils import (
@@ -89,7 +88,7 @@ class InstructorGradeReportTestCase(TestReportMixin, InstructorTaskCourseTestCas
         Verify cell data in the grades CSV for a particular user.
         """
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            result = generate_course_grade_report(None, None, course_id, None, 'graded')
+            result = CourseGradeReport.generate(None, None, course_id, None, 'graded')
             self.assertDictContainsSubset({'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
             report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD')
             report_csv_filename = report_store.links_for(course_id)[0][0]
@@ -121,7 +120,7 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase):
         self.current_task.update_state = Mock()
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task') as mock_current_task:
             mock_current_task.return_value = self.current_task
-            result = generate_course_grade_report(None, None, self.course.id, None, 'graded')
+            result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded')
         num_students = len(emails)
         self.assertDictContainsSubset({'attempted': num_students, 'succeeded': num_students, 'failed': 0}, result)
 
@@ -135,7 +134,7 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase):
         mock_grades_iter.return_value = [
             (self.create_student('username', 'student@example.com'), None, 'Cannot grade student')
         ]
-        result = generate_course_grade_report(None, None, self.course.id, None, 'graded')
+        result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded')
         self.assertDictContainsSubset({'attempted': 1, 'succeeded': 0, 'failed': 1}, result)
 
         report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD')
@@ -319,7 +318,7 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase):
                 '',
             )
         ]
-        result = generate_course_grade_report(None, None, self.course.id, None, 'graded')
+        result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded')
         self.assertDictContainsSubset({'attempted': 1, 'succeeded': 1, 'failed': 0}, result)
 
 
@@ -378,7 +377,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskCourseTestCase):
                     {'username': 'user1', 'state': u'state1'},
                     {'username': 'user2', 'state': u'state2'},
                 ]
-                result = upload_problem_responses_csv(None, None, self.course.id, task_input, 'calculated')
+                result = ProblemResponses.generate(None, None, self.course.id, task_input, 'calculated')
         report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD')
         links = report_store.links_for(self.course.id)
 
@@ -609,7 +608,7 @@ class TestProblemGradeReport(TestReportMixin, InstructorTaskModuleTestCase):
         Verify that we see no grade information for a course with no graded
         problems.
         """
-        result = generate_problem_grade_report(None, None, self.course.id, None, 'graded')
+        result = ProblemGradeReport.generate(None, None, self.course.id, None, 'graded')
         self.assertDictContainsSubset({'action_name': 'graded', 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
         self.verify_rows_in_csv([
             dict(zip(
@@ -633,7 +632,7 @@ class TestProblemGradeReport(TestReportMixin, InstructorTaskModuleTestCase):
         self.define_option_problem(u'Problem1', parent=vertical)
 
         self.submit_student_answer(self.student_1.username, u'Problem1', ['Option 1'])
-        result = generate_problem_grade_report(None, None, self.course.id, None, 'graded')
+        result = ProblemGradeReport.generate(None, None, self.course.id, None, 'graded')
         self.assertDictContainsSubset({'action_name': 'graded', 'attempted': 2, 'succeeded': 2, 'failed': 0}, result)
         problem_name = u'Homework 1: Subsection - Problem1'
         header_row = self.csv_header_row + [problem_name + ' (Earned)', problem_name + ' (Possible)']
@@ -670,7 +669,7 @@ class TestProblemGradeReport(TestReportMixin, InstructorTaskModuleTestCase):
         mock_grades_iter.return_value = [
             (student, None, error_message)
         ]
-        result = generate_problem_grade_report(None, None, self.course.id, None, 'graded')
+        result = ProblemGradeReport.generate(None, None, self.course.id, None, 'graded')
         self.assertDictContainsSubset({'attempted': 1, 'succeeded': 0, 'failed': 1}, result)
 
         report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD')
@@ -720,7 +719,7 @@ class TestProblemReportSplitTestContent(TestReportMixin, TestConditionalContent,
         self.submit_student_answer(self.student_b.username, self.problem_b_url, [self.OPTION_1, self.OPTION_2])
 
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            result = generate_problem_grade_report(None, None, self.course.id, None, 'graded')
+            result = ProblemGradeReport.generate(None, None, self.course.id, None, 'graded')
             self.assertDictContainsSubset(
                 {'action_name': 'graded', 'attempted': 2, 'succeeded': 2, 'failed': 0}, result
             )
@@ -812,7 +811,7 @@ class TestProblemReportSplitTestContent(TestReportMixin, TestConditionalContent,
             header_row += [problem + ' (Earned)', problem + ' (Possible)']
 
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            generate_problem_grade_report(None, None, self.course.id, None, 'graded')
+            ProblemGradeReport.generate(None, None, self.course.id, None, 'graded')
         self.assertEquals(self.get_csv_row_with_headers(), header_row)
 
 
@@ -868,7 +867,7 @@ class TestProblemReportCohortedContent(TestReportMixin, ContentGroupTestCase, In
         self.submit_student_answer(self.beta_user.username, u'Problem1', ['Option 1', 'Option 2'])
 
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            result = generate_problem_grade_report(None, None, self.course.id, None, 'graded')
+            result = ProblemGradeReport.generate(None, None, self.course.id, None, 'graded')
             self.assertDictContainsSubset(
                 {'action_name': 'graded', 'attempted': 4, 'succeeded': 4, 'failed': 0}, result
             )
@@ -1579,7 +1578,7 @@ class TestGradeReport(TestReportMixin, InstructorTaskModuleTestCase):
         self.submit_student_answer(self.student.username, u'Problem1', ['Option 1'])
 
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            result = generate_course_grade_report(None, None, self.course.id, None, 'graded')
+            result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded')
             self.assertDictContainsSubset(
                 {'action_name': 'graded', 'attempted': 1, 'succeeded': 1, 'failed': 0},
                 result,
@@ -1654,7 +1653,7 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas
         Verify grade report data.
         """
         with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):
-            generate_course_grade_report(None, None, self.course.id, None, 'graded')
+            CourseGradeReport.generate(None, None, self.course.id, None, 'graded')
             report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD')
             report_csv_filename = report_store.links_for(self.course.id)[0][0]
             report_path = report_store.path_to(self.course.id, report_csv_filename)