Remove MaxScoresCache (#12878)

Performance improvement, this cache is no longer needed thanks
to block_structure caching data.

TNL-4874

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -36,7 +36,6 @@ from openedx.core.djangoapps.content.block_structure.api import get_course_in_ca
     'django.conf.settings.FEATURES',
     {
         'ENABLE_XBLOCK_VIEW_ENDPOINT': True,
-        'ENABLE_MAX_SCORE_CACHE': False,
     }
 )
 @ddt.ddt

lms/djangoapps/courseware/grades.py

@@ -36,100 +36,6 @@ from .transformers.grades import GradesTransformer
 log = logging.getLogger("edx.courseware")
 
 
-class MaxScoresCache(object):
-    """
-    A cache for unweighted max scores for problems.
-
-    The key assumption here is that any problem that has not yet recorded a
-    score for a user is worth the same number of points. An XBlock is free to
-    score one student at 2/5 and another at 1/3. But a problem that has never
-    issued a score -- say a problem two students have only seen mentioned in
-    their progress pages and never interacted with -- should be worth the same
-    number of points for everyone.
-    """
-    def __init__(self, cache_prefix):
-        self.cache_prefix = cache_prefix
-        self._max_scores_cache = {}
-        self._max_scores_updates = {}
-
-    @classmethod
-    def create_for_course(cls, course):
-        """
-        Given a CourseDescriptor, return a correctly configured `MaxScoresCache`
-
-        This method will base the `MaxScoresCache` cache prefix value on the
-        last time something was published to the live version of the course.
-        This is so that we don't have to worry about stale cached values for
-        max scores -- any time a content change occurs, we change our cache
-        keys.
-        """
-        if course.subtree_edited_on is None:
-            # check for subtree_edited_on because old XML courses doesn't have this attribute
-            cache_key = u"{}".format(course.id)
-        else:
-            cache_key = u"{}.{}".format(course.id, course.subtree_edited_on.isoformat())
-        return cls(cache_key)
-
-    def fetch_from_remote(self, locations):
-        """
-        Populate the local cache with values from django's cache
-        """
-        remote_dict = cache.get_many([self._remote_cache_key(loc) for loc in locations])
-        self._max_scores_cache = {
-            self._local_cache_key(remote_key): value
-            for remote_key, value in remote_dict.items()
-            if value is not None
-        }
-
-    def push_to_remote(self):
-        """
-        Update the remote cache
-        """
-        if self._max_scores_updates:
-            cache.set_many(
-                {
-                    self._remote_cache_key(key): value
-                    for key, value in self._max_scores_updates.items()
-                },
-                60 * 60 * 24  # 1 day
-            )
-
-    def _remote_cache_key(self, location):
-        """Convert a location to a remote cache key (add our prefixing)."""
-        return u"grades.MaxScores.{}___{}".format(self.cache_prefix, unicode(location))
-
-    def _local_cache_key(self, remote_key):
-        """Convert a remote cache key to a local cache key (i.e. location str)."""
-        return remote_key.split(u"___", 1)[1]
-
-    def num_cached_from_remote(self):
-        """How many items did we pull down from the remote cache?"""
-        return len(self._max_scores_cache)
-
-    def num_cached_updates(self):
-        """How many local updates are we waiting to push to the remote cache?"""
-        return len(self._max_scores_updates)
-
-    def set(self, location, max_score):
-        """
-        Adds a max score to the max_score_cache
-        """
-        loc_str = unicode(location)
-        if self._max_scores_cache.get(loc_str) != max_score:
-            self._max_scores_updates[loc_str] = max_score
-
-    def get(self, location):
-        """
-        Retrieve a max score from the cache
-        """
-        loc_str = unicode(location)
-        max_score = self._max_scores_updates.get(loc_str)
-        if max_score is None:
-            max_score = self._max_scores_cache.get(loc_str)
-
-        return max_score
-
-
 class ProgressSummary(object):
     """
     Wrapper class for the computation of a user's scores across a course.
@@ -413,15 +319,9 @@ def _grade(student, course, keep_raw_scores, course_structure=None):
             course.id.to_deprecated_string(),
             anonymous_id_for_user(student, course.id)
         )
-        max_scores_cache = MaxScoresCache.create_for_course(course)
-
-        # For the moment, scores_client is ignorant of scorable_locations
-        # in the submissions API. As a further refactoring step, submissions should
-        # be hidden behind the ScoresClient.
-        max_scores_cache.fetch_from_remote(scorable_locations)
 
     totaled_scores, raw_scores = _calculate_totaled_scores(
-        student, grading_context_result, max_scores_cache, submissions_scores, scores_client, keep_raw_scores
+        student, grading_context_result, submissions_scores, scores_client, keep_raw_scores
     )
 
     with outer_atomic():
@@ -441,15 +341,12 @@ def _grade(student, course, keep_raw_scores, course_structure=None):
             # so grader can be double-checked
             grade_summary['raw_scores'] = raw_scores
 
-        max_scores_cache.push_to_remote()
-
     return grade_summary
 
 
 def _calculate_totaled_scores(
         student,
         grading_context_result,
-        max_scores_cache,
         submissions_scores,
         scores_client,
         keep_raw_scores,
@@ -492,7 +389,6 @@ def _calculate_totaled_scores(
                             descendant,
                             scores_client,
                             submissions_scores,
-                            max_scores_cache,
                         )
                         if correct is None and total is None:
                             continue
@@ -617,12 +513,6 @@ def _progress_summary(student, course, course_structure=None):
             unicode(course.id), anonymous_id_for_user(student, course.id)
         )
 
-        max_scores_cache = MaxScoresCache.create_for_course(course)
-        # For the moment, scores_client is ignorant of scorable_locations
-        # in the submissions API. As a further refactoring step, submissions should
-        # be hidden behind the ScoresClient.
-        max_scores_cache.fetch_from_remote(scorable_locations)
-
     # Check for gated content
     gated_content = gating_api.get_gated_content(course, student)
 
@@ -652,7 +542,6 @@ def _progress_summary(student, course, course_structure=None):
                     descendant,
                     scores_client,
                     submissions_scores,
-                    max_scores_cache,
                 )
                 if correct is None and total is None:
                     continue
@@ -688,8 +577,6 @@ def _progress_summary(student, course, course_structure=None):
             'sections': sections
         })
 
-    max_scores_cache.push_to_remote()
-
     return ProgressSummary(chapters, locations_to_weighted_scores, course_structure.get_children)
 
 
@@ -701,7 +588,7 @@ def weighted_score(raw_correct, raw_total, weight):
     return (float(raw_correct) * weight / raw_total, float(weight))
 
 
-def get_score(user, block, scores_client, submissions_scores_cache, max_scores_cache):
+def get_score(user, block, scores_client, submissions_scores_cache):
     """
     Return the score for a user on a problem, as a tuple (correct, total).
     e.g. (5,7) if you got 5 out of 7 points.
@@ -714,7 +601,6 @@ def get_score(user, block, scores_client, submissions_scores_cache, max_scores_c
     scores_client: an initialized ScoresClient
     submissions_scores_cache: A dict of location names to (earned, possible) point tuples.
            If an entry is found in this cache, it takes precedence.
-    max_scores_cache: a MaxScoresCache
     """
     submissions_scores_cache = submissions_scores_cache or {}
 
@@ -735,16 +621,10 @@ def get_score(user, block, scores_client, submissions_scores_cache, max_scores_c
     # older version of the problem -- they're still graded on what was possible
     # when they tried the problem, not what it's worth now.
     score = scores_client.get(block.location)
-    cached_max_score = max_scores_cache.get(block.location)
     if score and score.total is not None:
         # We have a valid score, just use it.
         correct = score.correct if score.correct is not None else 0.0
         total = score.total
-    elif cached_max_score is not None and settings.FEATURES.get("ENABLE_MAX_SCORE_CACHE"):
-        # We don't have a valid score entry but we know from our cache what the
-        # max possible score is, so they've earned 0.0 / cached_max_score
-        correct = 0.0
-        total = cached_max_score
     else:
         # This means we don't have a valid score entry and we don't have a
         # cached_max_score on hand. We know they've earned 0.0 points on this.
@@ -755,8 +635,6 @@ def get_score(user, block, scores_client, submissions_scores_cache, max_scores_c
         # In which case total might be None
         if total is None:
             return (None, None)
-        else:
-            max_scores_cache.set(block.location, total)
 
     return weighted_score(correct, total, block.weight)
 

lms/djangoapps/courseware/tests/test_grades.py

@@ -13,7 +13,6 @@ from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator
 from courseware.grades import (
     grade,
     iterate_grades_for,
-    MaxScoresCache,
     ProgressSummary,
     get_module_score
 )
@@ -145,54 +144,6 @@ class TestGradeIteration(SharedModuleStoreTestCase):
         return students_to_gradesets, students_to_errors
 
 
-class TestMaxScoresCache(SharedModuleStoreTestCase):
-    """
-    Tests for the MaxScoresCache
-    """
-
-    ENABLED_CACHES = ['default', 'mongo_metadata_inheritance', 'loc_cache']
-
-    @classmethod
-    def setUpClass(cls):
-        super(TestMaxScoresCache, cls).setUpClass()
-        cls.course = CourseFactory.create()
-        cls.problems = []
-        for _ in xrange(3):
-            cls.problems.append(
-                ItemFactory.create(category='problem', parent=cls.course)
-            )
-
-    def setUp(self):
-        super(TestMaxScoresCache, self).setUp()
-        self.student = UserFactory.create()
-
-        CourseEnrollment.enroll(self.student, self.course.id)
-        self.request = RequestFactory().get('/')
-        self.locations = [problem.location for problem in self.problems]
-
-    def test_max_scores_cache(self):
-        """
-        Tests the behavior fo the MaxScoresCache
-        """
-        max_scores_cache = MaxScoresCache("test_max_scores_cache")
-        self.assertEqual(max_scores_cache.num_cached_from_remote(), 0)
-        self.assertEqual(max_scores_cache.num_cached_updates(), 0)
-
-        # add score to cache
-        max_scores_cache.set(self.locations[0], 1)
-        self.assertEqual(max_scores_cache.num_cached_updates(), 1)
-
-        # push to remote cache
-        max_scores_cache.push_to_remote()
-
-        # create a new cache with the same params, fetch from remote cache
-        max_scores_cache = MaxScoresCache("test_max_scores_cache")
-        max_scores_cache.fetch_from_remote(self.locations)
-
-        # see cache is populated
-        self.assertEqual(max_scores_cache.num_cached_from_remote(), 1)
-
-
 class TestFieldDataCacheScorableLocations(SharedModuleStoreTestCase):
     """
     Make sure we can filter the locations we pull back student state for via

lms/djangoapps/courseware/tests/test_submitting_problems.py

@@ -459,10 +459,9 @@ class TestCourseGrader(TestSubmittingProblems):
         csmh = BaseStudentModuleHistory.get_history(student_module)
         self.assertEqual(len(csmh), 3)
 
-    def test_grade_with_max_score_cache(self):
+    def test_grade_with_collected_max_score(self):
         """
-        Tests that the max score cache is populated after a grading run
-        and that the results of grading runs before and after the cache
+        Tests that the results of grading runs before and after the cache
         warms are the same.
         """
         self.basic_setup()
@@ -473,17 +472,11 @@ class TestCourseGrader(TestSubmittingProblems):
                 module_state_key=self.problem_location('p2')
             ).exists()
         )
-        location_to_cache = unicode(self.problem_location('p2'))
-        max_scores_cache = grades.MaxScoresCache.create_for_course(self.course)
 
-        # problem isn't in the cache
-        max_scores_cache.fetch_from_remote([location_to_cache])
-        self.assertIsNone(max_scores_cache.get(location_to_cache))
+        # problem isn't in the cache, but will be when graded
         self.check_grade_percent(0.33)
 
-        # problem is in the cache
-        max_scores_cache.fetch_from_remote([location_to_cache])
-        self.assertIsNotNone(max_scores_cache.get(location_to_cache))
+        # problem is in the cache, should be the same result
         self.check_grade_percent(0.33)
 
     def test_none_grade(self):
@@ -503,13 +496,6 @@ class TestCourseGrader(TestSubmittingProblems):
         self.check_grade_percent(0.33)
         self.assertEqual(self.get_grade_summary()['grade'], 'B')
 
-    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_MAX_SCORE_CACHE": False})
-    def test_grade_no_max_score_cache(self):
-        """
-        Tests grading when the max score cache is disabled
-        """
-        self.test_b_grade_exact()
-
     def test_b_grade_above(self):
         """
         Check grade between cutoffs.

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -869,60 +869,6 @@ class TestProblemReportCohortedContent(TestReportMixin, ContentGroupTestCase, In
         expected_grades = [self._format_user_grade(header_row, **user_grade) for user_grade in user_grades]
         self.verify_rows_in_csv(expected_grades)
 
-    @patch('courseware.grades.MaxScoresCache.get', Mock(return_value=1))
-    def test_cohort_content_with_maxcache(self):
-        """
-        Tests the cohoted course grading to test the scenario in which `max_scores_cache` is set for the course
-        problems.
-        """
-        # Course is cohorted
-        self.assertTrue(cohorts.is_course_cohorted(self.course.id))
-
-        # Verify user groups
-        self.assertEquals(
-            cohorts.get_cohort(self.alpha_user, self.course.id).id,
-            self.course.user_partitions[0].groups[0].id,
-            "alpha_user should be assigned to the correct cohort"
-        )
-        self.assertEquals(
-            cohorts.get_cohort(self.beta_user, self.course.id).id,
-            self.course.user_partitions[0].groups[1].id,
-            "beta_user should be assigned to the correct cohort"
-        )
-
-        # Verify user enrollment
-        for user in [self.alpha_user, self.beta_user, self.non_cohorted_user]:
-            self.assertTrue(CourseEnrollment.is_enrolled(user, self.course.id))
-
-        self.submit_student_answer(self.alpha_user.username, u'Pröblem0', ['Option 1', 'Option 1'])
-        resp = self.submit_student_answer(self.alpha_user.username, u'Pröblem1', ['Option 1', 'Option 1'])
-        self.assertEqual(resp.status_code, 404)
-
-        resp = self.submit_student_answer(self.beta_user.username, u'Pröblem0', ['Option 1', 'Option 2'])
-        self.assertEqual(resp.status_code, 404)
-        self.submit_student_answer(self.beta_user.username, u'Pröblem1', ['Option 1', 'Option 2'])
-
-        with patch('instructor_task.tasks_helper._get_current_task'):
-            result = upload_problem_grade_report(None, None, self.course.id, None, 'graded')
-            self.assertDictContainsSubset(
-                {'action_name': 'graded', 'attempted': 4, 'succeeded': 4, 'failed': 0}, result
-            )
-        problem_names = [u'Homework 1: Problem - Pröblem0', u'Homework 1: Problem - Pröblem1']
-        header_row = [u'Student ID', u'Email', u'Username', u'Final Grade']
-        for problem in problem_names:
-            header_row += [problem + ' (Earned)', problem + ' (Possible)']
-
-        user_grades = [
-            {'user': self.staff_user, 'grade': [u'0.0', u'N/A', u'N/A', u'N/A', u'N/A']},
-            {'user': self.alpha_user, 'grade': [u'1.0', u'2.0', u'2.0', u'N/A', u'N/A']},
-            {'user': self.beta_user, 'grade': [u'0.5', u'N/A', u'N/A', u'1.0', u'2.0']},
-            {'user': self.non_cohorted_user, 'grade': [u'0.0', u'N/A', u'N/A', u'N/A', u'N/A']},
-        ]
-
-        # Verify generated grades and expected grades match
-        expected_grades = [self._format_user_grade(header_row, **grade) for grade in user_grades]
-        self.verify_rows_in_csv(expected_grades)
-
 
 @ddt.ddt
 class TestExecutiveSummaryReport(TestReportMixin, InstructorTaskCourseTestCase):

lms/envs/common.py

@@ -343,9 +343,6 @@ FEATURES = {
     # The block types to disable need to be specified in "x block disable config" in django admin.
     'ENABLE_DISABLING_XBLOCK_TYPES': True,
 
-    # Enable the max score cache to speed up grading
-    'ENABLE_MAX_SCORE_CACHE': True,
-
     # Enable LTI Provider feature.
     'ENABLE_LTI_PROVIDER': False,