Skip to content

E
edx-platform
Commit 0853e485 by Nimisha Asthagiri Committed by GitHub
Options

Merge pull request #13429 from edx/tnl/progress-sql-queries 

Optimize Subsection Grades update
parents 2d9e38ca f0f7a538
Showing with 172 additions and 62 deletions
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):
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment