From f22d6739809b17451bb335fb6bccded00130202a Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Tue, 28 Jun 2016 14:04:00 -0400 Subject: [PATCH] Remove MaxScoresCache (#12878) Performance improvement, this cache is no longer needed thanks to block_structure caching data. TNL-4874 --- .../tests/test_field_override_performance.py | 1 - lms/djangoapps/courseware/grades.py | 126 +----------------- .../courseware/tests/test_grades.py | 49 ------- .../tests/test_submitting_problems.py | 22 +-- .../tests/test_tasks_helper.py | 54 -------- lms/envs/common.py | 3 - 6 files changed, 6 insertions(+), 249 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index d9123dedc8..46cc461614 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/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 diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 69ac5b171f..4acf3e8fb1 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/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) diff --git a/lms/djangoapps/courseware/tests/test_grades.py b/lms/djangoapps/courseware/tests/test_grades.py index 2a9279aaf0..557e159e3f 100644 --- a/lms/djangoapps/courseware/tests/test_grades.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index ad1745344f..f5bb832a7b 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/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. diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 3f1e038974..6d8487f72d 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/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): diff --git a/lms/envs/common.py b/lms/envs/common.py index f21e027f5f..4913dcb83a 100644 --- a/lms/envs/common.py +++ b/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,