From f0f7a5389beade07dbec6f94f8256d61373232c4 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Fri, 9 Sep 2016 03:26:06 -0400 Subject: [PATCH] Optimize Subsection computation --- lms/djangoapps/course_blocks/api.py | 7 +++ lms/djangoapps/grades/new/course_grade.py | 3 +- lms/djangoapps/grades/new/subsection_grade.py | 10 +--- lms/djangoapps/grades/signals/handlers.py | 38 ++++++-------- lms/djangoapps/grades/tests/test_signals.py | 49 +++++++++++-------- .../lib/block_structure/block_structure.py | 14 ++++++ openedx/core/lib/block_structure/cache.py | 15 +++--- openedx/core/lib/block_structure/factory.py | 13 ++++- openedx/core/lib/block_structure/manager.py | 10 +++- .../tests/test_block_structure.py | 45 +++++++++++++++++ .../lib/block_structure/tests/test_factory.py | 12 +++++ .../lib/block_structure/tests/test_manager.py | 18 +++++++ 12 files changed, 172 insertions(+), 62 deletions(-) diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py index 57b8db9076..849527915d 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -28,6 +28,7 @@ def get_course_blocks( user, starting_block_usage_key, transformers=None, + collected_block_structure=None, ): """ A higher order function implemented on top of the @@ -45,6 +46,11 @@ def get_course_blocks( transformers whose transform methods are to be called. If None, COURSE_BLOCK_ACCESS_TRANSFORMERS is used. + collected_block_structure (BlockStructureBlockData) - A + block structure retrieved from a prior call to + BlockStructureManager.get_collected. Can be optionally + provided if already available, for optimization. + Returns: BlockStructureBlockData - A transformed block structure, starting at starting_block_usage_key, that has undergone the @@ -61,4 +67,5 @@ def get_course_blocks( return get_block_structure_manager(starting_block_usage_key.course_key).get_transformed( transformers, starting_block_usage_key, + collected_block_structure, ) diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index c648007a3a..152838011e 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/lms/djangoapps/grades/new/course_grade.py @@ -125,7 +125,8 @@ class CourseGrade(object): subsection_grades.append( subsection_grade_factory.create( self.course_structure[subsection_key], - self.course_structure, self.course + self.course_structure, + self.course, ) ) diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index c9d13b68cb..72993640af 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -4,9 +4,6 @@ SubsectionGrade Class from collections import OrderedDict from lazy import lazy -from django.conf import settings - -from course_blocks.api import get_course_blocks from courseware.model_data import ScoresClient from lms.djangoapps.grades.scores import get_score, possibly_scored from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade @@ -173,20 +170,17 @@ class SubsectionGradeFactory(object): self._compute_and_save_grade(subsection, course_structure, course) ) - def update(self, usage_key, course_key): + def update(self, usage_key, course_structure, course): """ Updates the SubsectionGrade object for the student and subsection identified by the given usage key. """ - from courseware.courses import get_course_by_id # avoids circular import with courseware.py - course = get_course_by_id(course_key, depth=0) # save ourselves the extra queries if the course does not use subsection grades if not PersistentGradesEnabledFlag.feature_enabled(course.id): return - course_structure = get_course_blocks(self.student, usage_key) - subsection = course_structure[usage_key] self._prefetch_scores(course_structure, course) + subsection = course_structure[usage_key] return self._compute_and_save_grade(subsection, course_structure, course) def _compute_and_save_grade(self, subsection, course_structure, course): diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index ae9755e358..64073e88ff 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -5,6 +5,8 @@ from logging import getLogger from django.dispatch import receiver +from lms.djangoapps.course_blocks.api import get_course_blocks +from lms.djangoapps.courseware.courses import get_course_by_id from opaque_keys.edx.locator import CourseLocator from opaque_keys.edx.keys import UsageKey from openedx.core.djangoapps.content.block_structure.api import get_course_in_cache @@ -95,33 +97,25 @@ def recalculate_subsection_grade_handler(sender, **kwargs): # pylint: disable=u - course_id: Unicode string representing the course - usage_id: Unicode string indicating the courseware instance """ - try: - course_id = kwargs['course_id'] - usage_id = kwargs['usage_id'] - student = kwargs['user'] - except KeyError: - log.exception( - u"Failed to process SCORE_CHANGED signal, some arguments were missing." - "user: %s, course_id: %s, usage_id: %s.", - kwargs.get('user', None), - kwargs.get('course_id', None), - kwargs.get('usage_id', None), - ) - return - - course_key = CourseLocator.from_string(course_id) + student = kwargs['user'] + course_key = CourseLocator.from_string(kwargs['course_id']) if not PersistentGradesEnabledFlag.feature_enabled(course_key): return - usage_key = UsageKey.from_string(usage_id).replace(course_key=course_key) - block_structure = get_course_in_cache(course_key) + scored_block_usage_key = UsageKey.from_string(kwargs['usage_id']).replace(course_key=course_key) + collected_block_structure = get_course_in_cache(course_key) + course = get_course_by_id(course_key, depth=0) - subsections_to_update = block_structure.get_transformer_block_field( - usage_key, + subsections_to_update = collected_block_structure.get_transformer_block_field( + scored_block_usage_key, GradesTransformer, 'subsections', set() ) - - for subsection in subsections_to_update: - SubsectionGradeFactory(student).update(subsection, course_key) + for subsection_usage_key in subsections_to_update: + transformed_subsection_structure = get_course_blocks( + student, + subsection_usage_key, + collected_block_structure=collected_block_structure, + ) + SubsectionGradeFactory(student).update(subsection_usage_key, transformed_subsection_structure, course) diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index 2ecf0404f0..2125944e47 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -3,10 +3,11 @@ Tests for the score change signals defined in the courseware models module. """ import ddt -from unittest import skip from django.test import TestCase -from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag, CoursePersistentGradesFlag from mock import patch, MagicMock +from unittest import skip + +from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag from student.models import anonymous_id_for_user from student.tests.factories import UserFactory from xmodule.modulestore import ModuleStoreEnum @@ -177,7 +178,7 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): def setUp(self): super(ScoreChangedUpdatesSubsectionGradeTest, self).setUp() self.user = UserFactory() - PersistentGradesEnabledFlag.objects.create(enabled=True) + PersistentGradesEnabledFlag.objects.create(enabled_for_all_courses=True, enabled=True) def set_up_course(self, enable_subsection_grades=True): """ @@ -189,7 +190,8 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): name='course', run='run', ) - CoursePersistentGradesFlag.objects.create(course_id=self.course.id, enabled=enable_subsection_grades) + if not enable_subsection_grades: + PersistentGradesEnabledFlag.objects.create(enabled=False) self.chapter = ItemFactory.create(parent=self.course, category="chapter", display_name="Chapter") self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Open Sequential") @@ -211,23 +213,36 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): def test_subsection_grade_updated_on_signal(self, default_store): with self.store.default_store(default_store): self.set_up_course() - with check_mongo_calls(2) and self.assertNumQueries(19): + self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) + with check_mongo_calls(2) and self.assertNumQueries(15): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) + def test_single_call_to_create_block_structure(self): + self.set_up_course() + self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) + with patch( + 'openedx.core.lib.block_structure.factory.BlockStructureFactory.create_from_cache', + return_value=None, + ) as mock_block_structure_create: + recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) + self.assertEquals(mock_block_structure_create.call_count, 1) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_query_count_does_not_change_with_more_problems(self, default_store): with self.store.default_store(default_store): self.set_up_course() + self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2') ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3') - with check_mongo_calls(2) and self.assertNumQueries(19): + with check_mongo_calls(2) and self.assertNumQueries(15): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_subsection_grades_not_enabled_on_course(self, default_store): with self.store.default_store(default_store): self.set_up_course(enable_subsection_grades=False) - with check_mongo_calls(2) and self.assertNumQueries(2): + self.assertFalse(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) + with check_mongo_calls(2) and self.assertNumQueries(0): recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) @skip("Pending completion of TNL-5089") @@ -242,21 +257,13 @@ class ScoreChangedUpdatesSubsectionGradeTest(ModuleStoreTestCase): PersistentGradesEnabledFlag.objects.create(enabled=feature_flag) with self.store.default_store(default_store): self.set_up_course() - with check_mongo_calls(0) and self.assertNumQueries(19 if feature_flag else 1): + with check_mongo_calls(0) and self.assertNumQueries(15 if feature_flag else 1): SCORE_CHANGED.send(sender=None, **self.score_changed_kwargs) - @ddt.data( - ('points_possible', 2, 19), - ('points_earned', 2, 19), - ('user', 0, 0), - ('course_id', 0, 0), - ('usage_id', 0, 0), - ) - @ddt.unpack - def test_missing_kwargs(self, kwarg, expected_mongo_calls, expected_sql_calls): + @ddt.data('user', 'course_id', 'usage_id') + def test_missing_kwargs(self, kwarg): self.set_up_course() + self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) del self.score_changed_kwargs[kwarg] - with patch('lms.djangoapps.grades.signals.handlers.log') as log_mock: - with check_mongo_calls(expected_mongo_calls) and self.assertNumQueries(expected_sql_calls): - recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) - self.assertEqual(log_mock.exception.called, kwarg not in ['points_possible', 'points_earned']) + with self.assertRaises(KeyError): + recalculate_subsection_grade_handler(None, **self.score_changed_kwargs) diff --git a/openedx/core/lib/block_structure/block_structure.py b/openedx/core/lib/block_structure/block_structure.py index a4550edac3..81cd99a945 100644 --- a/openedx/core/lib/block_structure/block_structure.py +++ b/openedx/core/lib/block_structure/block_structure.py @@ -8,6 +8,7 @@ The following internal data structures are implemented: _BlockRelations - Data structure for a single block's relations. _BlockData - Data structure for a single block's data. """ +from copy import deepcopy from functools import partial from logging import getLogger @@ -413,6 +414,19 @@ class BlockStructureBlockData(BlockStructure): # Map of a transformer's name to its non-block-specific data. self.transformer_data = TransformerDataMap() + def copy(self): + """ + Returns a new instance of BlockStructureBlockData with a + deep-copy of this instance's contents. + """ + from .factory import BlockStructureFactory + return BlockStructureFactory.create_new( + self.root_block_usage_key, + deepcopy(self._block_relations), + deepcopy(self.transformer_data), + deepcopy(self._block_data_map), + ) + def iteritems(self): """ Returns iterator of (UsageKey, BlockData) pairs for all diff --git a/openedx/core/lib/block_structure/cache.py b/openedx/core/lib/block_structure/cache.py index 3a02ed4c2e..c5a021f406 100644 --- a/openedx/core/lib/block_structure/cache.py +++ b/openedx/core/lib/block_structure/cache.py @@ -6,7 +6,8 @@ from logging import getLogger from openedx.core.lib.cache_utils import zpickle, zunpickle -from .block_structure import BlockStructureModulestoreData, BlockStructureBlockData +from .block_structure import BlockStructureBlockData +from .factory import BlockStructureFactory logger = getLogger(__name__) # pylint: disable=C0103 @@ -97,12 +98,12 @@ class BlockStructureCache(object): # Deserialize and construct the block structure. block_relations, transformer_data, block_data_map = zunpickle(zp_data_from_cache) - block_structure = BlockStructureModulestoreData(root_block_usage_key) - block_structure._block_relations = block_relations - block_structure.transformer_data = transformer_data - block_structure._block_data_map = block_data_map - - return block_structure + return BlockStructureFactory.create_new( + root_block_usage_key, + block_relations, + transformer_data, + block_data_map, + ) def delete(self, root_block_usage_key): """ diff --git a/openedx/core/lib/block_structure/factory.py b/openedx/core/lib/block_structure/factory.py index 7a2b253334..4c583b5066 100644 --- a/openedx/core/lib/block_structure/factory.py +++ b/openedx/core/lib/block_structure/factory.py @@ -1,7 +1,7 @@ """ Module for factory class for BlockStructure objects. """ -from .block_structure import BlockStructureModulestoreData +from .block_structure import BlockStructureModulestoreData, BlockStructureBlockData class BlockStructureFactory(object): @@ -82,3 +82,14 @@ class BlockStructureFactory(object): NoneType - If the root_block_usage_key is not found in the cache. """ return block_structure_cache.get(root_block_usage_key) + + @classmethod + def create_new(cls, root_block_usage_key, block_relations, transformer_data, block_data_map): + """ + Returns a new block structure for given the arguments. + """ + block_structure = BlockStructureBlockData(root_block_usage_key) + block_structure._block_relations = block_relations # pylint: disable=protected-access + block_structure.transformer_data = transformer_data + block_structure._block_data_map = block_data_map # pylint: disable=protected-access + return block_structure diff --git a/openedx/core/lib/block_structure/manager.py b/openedx/core/lib/block_structure/manager.py index 616f73b027..ae688a6fdd 100644 --- a/openedx/core/lib/block_structure/manager.py +++ b/openedx/core/lib/block_structure/manager.py @@ -33,7 +33,7 @@ class BlockStructureManager(object): self.modulestore = modulestore self.block_structure_cache = BlockStructureCache(cache) - def get_transformed(self, transformers, starting_block_usage_key=None): + def get_transformed(self, transformers, starting_block_usage_key=None, collected_block_structure=None): """ Returns the transformed Block Structure for the root_block_usage_key, starting at starting_block_usage_key, getting block data from the cache @@ -50,11 +50,17 @@ class BlockStructureManager(object): in the block structure that is to be transformed. If None, root_block_usage_key is used. + collected_block_structure (BlockStructureBlockData) - A + block structure retrieved from a prior call to + get_collected. Can be optionally provided if already available, + for optimization. + Returns: BlockStructureBlockData - A transformed block structure, starting at starting_block_usage_key. """ - block_structure = self.get_collected() + block_structure = collected_block_structure.copy() if collected_block_structure else self.get_collected() + if starting_block_usage_key: # Override the root_block_usage_key so traversals start at the # requested location. The rest of the structure will be pruned diff --git a/openedx/core/lib/block_structure/tests/test_block_structure.py b/openedx/core/lib/block_structure/tests/test_block_structure.py index d56820ad09..77da8fad51 100644 --- a/openedx/core/lib/block_structure/tests/test_block_structure.py +++ b/openedx/core/lib/block_structure/tests/test_block_structure.py @@ -221,3 +221,48 @@ class TestBlockStructureData(TestCase, ChildrenMapTestMixin): block_structure = self.create_block_structure(ChildrenMapTestMixin.LINEAR_CHILDREN_MAP) block_structure.remove_block_traversal(lambda block: block == 2) self.assert_block_structure(block_structure, [[1], [], [], []], missing_blocks=[2]) + + def test_copy(self): + def _set_value(structure, value): + """ + Sets a test transformer block field to the given value in the given structure. + """ + structure.set_transformer_block_field(1, 'transformer', 'test_key', value) + + def _get_value(structure): + """ + Returns the value of the test transformer block field in the given structure. + """ + return structure[1].transformer_data['transformer'].test_key + + # create block structure and verify blocks pre-exist + block_structure = self.create_block_structure(ChildrenMapTestMixin.LINEAR_CHILDREN_MAP) + self.assert_block_structure(block_structure, [[1], [2], [3], []]) + _set_value(block_structure, 'original_value') + + # create a new copy of the structure and verify they are equivalent + new_copy = block_structure.copy() + self.assertEquals(block_structure.root_block_usage_key, new_copy.root_block_usage_key) + for block in block_structure: + self.assertIn(block, new_copy) + self.assertEquals(block_structure.get_parents(block), new_copy.get_parents(block)) + self.assertEquals(block_structure.get_children(block), new_copy.get_children(block)) + self.assertEquals(_get_value(block_structure), _get_value(new_copy)) + + # verify edits to original block structure do not affect the copy + block_structure.remove_block(2, keep_descendants=True) + self.assert_block_structure(block_structure, [[1], [3], [], []], missing_blocks=[2]) + self.assert_block_structure(new_copy, [[1], [2], [3], []]) + + _set_value(block_structure, 'edit1') + self.assertEquals(_get_value(block_structure), 'edit1') + self.assertEquals(_get_value(new_copy), 'original_value') + + # verify edits to copy do not affect the original + new_copy.remove_block(3, keep_descendants=True) + self.assert_block_structure(block_structure, [[1], [3], [], []], missing_blocks=[2]) + self.assert_block_structure(new_copy, [[1], [2], [], []], missing_blocks=[3]) + + _set_value(new_copy, 'edit2') + self.assertEquals(_get_value(block_structure), 'edit1') + self.assertEquals(_get_value(new_copy), 'edit2') diff --git a/openedx/core/lib/block_structure/tests/test_factory.py b/openedx/core/lib/block_structure/tests/test_factory.py index 6298ecd6e8..8ccb5a6207 100644 --- a/openedx/core/lib/block_structure/tests/test_factory.py +++ b/openedx/core/lib/block_structure/tests/test_factory.py @@ -54,3 +54,15 @@ class TestBlockStructureFactory(TestCase, ChildrenMapTestMixin): block_structure_cache=cache, ) ) + + def test_new(self): + block_structure = BlockStructureFactory.create_from_modulestore( + root_block_usage_key=0, modulestore=self.modulestore + ) + new_structure = BlockStructureFactory.create_new( + block_structure.root_block_usage_key, + block_structure._block_relations, # pylint: disable=protected-access + block_structure.transformer_data, + block_structure._block_data_map, # pylint: disable=protected-access + ) + self.assert_block_structure(new_structure, self.children_map) diff --git a/openedx/core/lib/block_structure/tests/test_manager.py b/openedx/core/lib/block_structure/tests/test_manager.py index ccdab59660..4fdf2a280b 100644 --- a/openedx/core/lib/block_structure/tests/test_manager.py +++ b/openedx/core/lib/block_structure/tests/test_manager.py @@ -139,6 +139,24 @@ class TestBlockStructureManager(TestCase, ChildrenMapTestMixin): TestTransformer1.assert_collected(block_structure) TestTransformer1.assert_transformed(block_structure) + def test_get_transformed_with_collected(self): + with mock_registered_transformers(self.registered_transformers): + collected_block_structure = self.bs_manager.get_collected() + + # using the same collected block structure, + # transform at different starting blocks + for (starting_block, expected_structure, expected_missing_blocks) in [ + (0, [[1, 2], [3, 4], [], [], []], []), + (1, [[], [3, 4], [], [], []], [0, 2]), + (2, [[], [], [], [], []], [0, 1, 3, 4]), + ]: + block_structure = self.bs_manager.get_transformed( + self.transformers, + starting_block_usage_key=starting_block, + collected_block_structure=collected_block_structure, + ) + self.assert_block_structure(block_structure, expected_structure, missing_blocks=expected_missing_blocks) + def test_get_transformed_with_nonexistent_starting_block(self): with mock_registered_transformers(self.registered_transformers): with self.assertRaises(UsageKeyNotInBlockStructure):