Optimize Subsection computation

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,
     )

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,
                     )
                 )
 

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):

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)

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)

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

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):
         """

openedx/core/lib/block_structure/factory.py

 """
 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

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

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')

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)

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):