From 130f7d4705983c202f4bbda24b19ca2a799e4fd1 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Fri, 31 Jul 2026 15:06:08 -0400 Subject: [PATCH] perf: bulk-prefetch grades in ProblemGradeReport and bound its per-learner caches ProblemGradeReport read persisted grades one learner at a time, unlike CourseGradeReport, whose _rows_for_users opens with a _CourseGradeBulkContext. With nothing prefetched, PersistentSubsectionGrade.bulk_read_grades missed the per-course RequestCache and fell through to a query per learner -- and this report walks course_grade.problem_scores, so it touches more subsection data per learner than the course report does. Measured with CaptureQueriesContext over the report, varying cohort size: learners 5 10 25 50 before 20 30 60 110 (fits 2N + 10) after 13 13 13 13 Adds a regression test asserting the flat count at two cohort sizes; a single-size assertion would pass while a per-learner read crept back in. _ProblemGradeBulkContext is deliberately narrower than _CourseGradeBulkContext: the problem report emits no cohort, team, certificate or course-tag columns, so prefetching those would be wasted work. It loads persisted course and subsection grades plus enrollment states, which is what the rows actually read. Also in the same report path: * Pass the already-loaded block structure to grading_context() instead of calling grading_context_for_course(), which re-enters get_course_in_cache. BlockStructureManager.get_collected() deserializes fresh from the cache backend on every call, so a large course paid that cost, and its peak allocation, twice. CourseGradeReport already does this correctly. * Clear the visible-blocks and subsection-override caches between batches. RequestCache is only flushed when the task ends, and unlike the course-keyed grade prefetches -- replaced wholesale each batch -- these are keyed per (user, course) and only ever grow, though nothing reads an entry again once that learner's row is written. The _clear_caches hook already existed and was called per batch; the base implementation was empty. Adds the missing clear_prefetched_data() counterparts to VisibleBlocks and PersistentSubsectionGradeOverride, and a clear_prefetched_grade_overrides_and_visible_blocks() api function pairing with the existing prefetch_grade_overrides_and_visible_blocks(). * Wire USER_BATCH_SIZE through to grouper(). It was defined on CourseGradeReport but never referenced, so the effective batch size was grouper's hardcoded default and tuning the constant did nothing. Moved to GradeReportBase so both reports honour it. CSV contents and format are unchanged by all of the above. Refs: https://github.com/openedx/openedx-platform/issues/38943 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ki1NYMjSjVB4uz1gdgsjBh --- lms/djangoapps/grades/models.py | 23 ++++++++ lms/djangoapps/grades/models_api.py | 14 +++++ .../instructor_task/tasks_helper/grades.py | 53 ++++++++++++++++--- .../tests/test_tasks_helper.py | 23 ++++++++ 4 files changed, 106 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index 50cfa2819ca4..881ff7836e76 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -287,6 +287,18 @@ def _update_cache(cls, user_id, course_key, visible_blocks): {visible_block.hashed: visible_block for visible_block in visible_blocks} ) + @classmethod + def clear_prefetched_data(cls): + """ + Clears all prefetched visible blocks from the RequestCache. + + Unlike the course-keyed grade caches, this one is keyed per + (user, course) and is only ever added to, so it grows without bound + across a task that reads grades for many learners in turn. Callers + iterating over a large population should drop it periodically. + """ + get_cache(cls._CACHE_NAMESPACE).clear() + @classmethod def _cache_key(cls, user_id, course_key): return f"visible_blocks_cache.{course_key}.{user_id}" @@ -862,3 +874,14 @@ def _prepare_override_params(subsection_grade_model, override_data): @classmethod def clear_prefetched_overrides_for_learner(cls, user_id, course_key): get_cache(cls._CACHE_NAMESPACE).pop((user_id, str(course_key)), None) + + @classmethod + def clear_prefetched_data(cls): + """ + Clears all prefetched overrides from the RequestCache. + + Like the VisibleBlocks cache, this is keyed per (user, course) and only + ever grows, so a task reading grades for many learners needs to drop it + periodically rather than relying on the per-learner variant above. + """ + get_cache(cls._CACHE_NAMESPACE).clear() diff --git a/lms/djangoapps/grades/models_api.py b/lms/djangoapps/grades/models_api.py index 9c38296ead6d..7137cac6304a 100644 --- a/lms/djangoapps/grades/models_api.py +++ b/lms/djangoapps/grades/models_api.py @@ -17,6 +17,20 @@ def prefetch_grade_overrides_and_visible_blocks(user, course_key): _VisibleBlocks.bulk_read(user.id, course_key) +def clear_prefetched_grade_overrides_and_visible_blocks(): + """ + Clears the caches populated by prefetch_grade_overrides_and_visible_blocks. + + Both are keyed per (user, course) rather than per course, so -- unlike the + course-keyed prefetches below, which are replaced wholesale on each call -- + they accumulate an entry per learner and are never evicted for the life of + the request or task. Long-running work that walks a large learner + population should call this between batches. + """ + _PersistentSubsectionGradeOverride.clear_prefetched_data() + _VisibleBlocks.clear_prefetched_data() + + def prefetch_course_grades(course_key, users): _PersistentCourseGrade.prefetch(course_key, users) diff --git a/lms/djangoapps/instructor_task/tasks_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index 9b6a764137db..92b16a22d3c3 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -26,7 +26,11 @@ from lms.djangoapps.course_blocks.api import get_course_block_access_transformers, get_course_blocks from lms.djangoapps.course_blocks.transformers import library_content from lms.djangoapps.courseware.user_state_client import DjangoXBlockUserStateClient -from lms.djangoapps.grades.api import CourseGradeFactory, prefetch_course_and_subsection_grades +from lms.djangoapps.grades.api import ( + CourseGradeFactory, + clear_prefetched_grade_overrides_and_visible_blocks, + prefetch_course_and_subsection_grades, +) from lms.djangoapps.grades.api import context as grades_context from lms.djangoapps.instructor_analytics.basic import list_problem_responses from lms.djangoapps.instructor_analytics.csvs import format_dictlist @@ -206,7 +210,12 @@ def graded_scorable_blocks_header(self): headers in the final report. """ scorable_blocks_map = OrderedDict() - grading_context = grades_context.grading_context_for_course(self.course) + # Pass the already-loaded structure rather than using + # grading_context_for_course, which calls get_course_in_cache again. + # BlockStructureManager.get_collected() deserializes fresh from the cache + # backend on every call, so the convenience helper would make a large + # course pay that cost -- and its peak allocation -- a second time. + grading_context = grades_context.grading_context(self.course, self.course_structure) for assignment_type_name, subsection_infos in grading_context['all_graded_subsections_by_type'].items(): for subsection_index, subsection_info in enumerate(subsection_infos, start=1): for scorable_block in subsection_info['scored_descendants']: @@ -266,6 +275,23 @@ def __init__(self, context, users): self.verified_users = set(IDVerificationService.get_verified_user_ids(users)) +class _ProblemGradeBulkContext: + """ + Bulk-loads the per-learner data the problem grade report reads, so that a + batch costs a fixed number of queries instead of scaling with batch size. + + Deliberately narrower than _CourseGradeBulkContext: the problem report emits + no cohort, team, certificate or course-tag columns, so prefetching those + would be wasted work. It does read persisted course and subsection grades + (via course_grade.problem_scores, which walks every graded subsection) and + each learner's enrollment status. + """ + + def __init__(self, context, users): + prefetch_course_and_subsection_grades(context.course_id, users) + CourseEnrollment.bulk_fetch_enrollment_states(users, context.course_id) + + class _CourseGradeBulkContext: # pylint: disable=missing-class-docstring def __init__(self, context, users): self.certs = _CertificateBulkContext(context, users) @@ -418,6 +444,8 @@ class GradeReportBase: """ Base class for grade reports (ProblemGradeReport and CourseGradeReport). """ + # Batch size for chunking the list of enrollees in the course. + USER_BATCH_SIZE = 100 def __init__(self, context): self.context = context @@ -451,7 +479,7 @@ def _batch_users(self): """ Returns a generator of batches of users. """ - def grouper(iterable, chunk_size=100, fillvalue=None): + def grouper(iterable, chunk_size=self.USER_BATCH_SIZE, fillvalue=None): args = [iter(iterable)] * chunk_size return zip_longest(*args, fillvalue=fillvalue) @@ -496,9 +524,19 @@ def log_additional_info_for_testing(self, message): def _clear_caches(self): """ - Override if a report type wants to clear caches after a batch of learners has - been processed + Clear per-learner caches after a batch of learners has been processed. + + RequestCache is only flushed when the task ends, so anything keyed per + learner accumulates for the whole run. The grade prefetches are keyed + per course and replaced on each batch, but the visible-blocks and + subsection-override caches are keyed per (user, course) and are only + ever added to -- and nothing reads an entry again once that learner's + row has been written. + + Subclasses that need to drop additional caches should override this and + call super(). """ + clear_prefetched_grade_overrides_and_visible_blocks() def _batched_rows(self): """ @@ -513,8 +551,6 @@ class CourseGradeReport(GradeReportBase): """ Class to encapsulate functionality related to generating user/row had header data for Corse Grade Reports. """ - # Batch size for chunking the list of enrollees in the course. - USER_BATCH_SIZE = 100 @classmethod def generate(cls, _xblock_instance_args, _entry_id, course_id, _task_input, action_name): @@ -753,6 +789,8 @@ def _rows_for_users(self, users): """ Returns a list of rows for the given users for this report. """ + _ProblemGradeBulkContext(self.context, users) + success_rows, error_rows = [], [] for student, course_grade, error in CourseGradeFactory().iter( users, @@ -793,6 +831,7 @@ def _rows_for_users(self, users): return success_rows, error_rows def _clear_caches(self): + super()._clear_caches() get_cache('get_enrollment').clear() get_cache(CourseEnrollment.MODE_CACHE_NAMESPACE).clear() diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 6a20da1ee434..66e08d48b2e4 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -897,6 +897,29 @@ def setUp(self): self.student_2 = self.create_student('üser_2') self.csv_header_row = ['Student ID', 'Email', 'Username', 'Enrollment Status', 'Grade'] + def test_query_counts_do_not_scale_with_learner_count(self): + """ + The report's query count should be independent of how many learners are + enrolled, because _ProblemGradeBulkContext prefetches once per batch + rather than reading per learner. + + Asserting the same count at two cohort sizes is the point of the test. + Before the bulk prefetch the count fit 2N + 10 -- 20 queries at 5 + learners, 110 at 50 -- so a single-size assertion would pass while the + per-learner read crept back in. Measured after: a flat 13 at 5, 10, 25 + and 50 learners. + """ + for extra_learners in (3, 20): + for i in range(extra_learners): + self.create_student(f'query_count_üser_{extra_learners}_{i}') + + bs_api.update_course_in_cache(self.course.id) + RequestCache.clear_all_namespaces() + + with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): + with self.assertNumQueries(13, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): + ProblemGradeReport.generate(None, None, self.course.id, {}, 'graded') + @patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task') @ddt.data(True, False) def test_no_problems(self, use_tempfile, _): # noqa: PT019