From cf524906e0a3082f4b54424b3bc5c1fb92e787b6 Mon Sep 17 00:00:00 2001
From: Adam Palay <adam@edx.org>
Date: Tue, 12 Nov 2013 13:18:27 -0500
Subject: [PATCH] more granular transactions in grading [LMS-1480]
remove field_data_cache from grades.grade and grades.progress_summary
cleans grading code by adding wrappers
---
lms/djangoapps/courseware/grades.py | 183 ++++++++++--------
.../tests/test_submitting_problems.py | 26 +--
lms/djangoapps/courseware/views.py | 67 ++++---
3 files changed, 154 insertions(+), 122 deletions(-)
diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py
index 438466c0eaa..4cc0f909431 100644
--- a/lms/djangoapps/courseware/grades.py
+++ b/lms/djangoapps/courseware/grades.py
@@ -4,6 +4,7 @@ from collections import defaultdict
import random
import logging
+from contextlib import contextmanager
from collections import defaultdict
from django.conf import settings
from django.contrib.auth.models import User
@@ -126,8 +127,20 @@ def answer_distributions(request, course):
return counts
-def grade(student, request, course, field_data_cache=None, keep_raw_scores=False):
+@transaction.commit_manually
+def grade(student, request, course, keep_raw_scores=False):
"""
+ Wraps "_grade" with the manual_transaction context manager just in case
+ there are unanticipated errors.
+ """
+ with manual_transaction():
+ return _grade(student, request, course, keep_raw_scores)
+
+
+def _grade(student, request, course, keep_raw_scores):
+ """
+ Unwrapped version of "grade"
+
This grades a student as quickly as possible. It returns the
output from the course grader, augmented with the final letter
grade. The keys in the output are:
@@ -137,20 +150,17 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False
- grade : A final letter grade.
- percent : The final percent for the class (rounded up).
- section_breakdown : A breakdown of each section that makes
- up the grade. (For display)
+ up the grade. (For display)
- grade_breakdown : A breakdown of the major components that
- make up the final grade. (For display)
- - keep_raw_scores : if True, then value for key 'raw_scores' contains scores for every graded module
+ make up the final grade. (For display)
+ - keep_raw_scores : if True, then value for key 'raw_scores' contains scores
+ for every graded module
More information on the format is in the docstring for CourseGrader.
"""
-
grading_context = course.grading_context
raw_scores = []
- if field_data_cache is None:
- field_data_cache = FieldDataCache(grading_context['all_descriptors'], course.id, student)
-
totaled_scores = {}
# This next complicated loop is just to collect the totaled_scores, which is
# passed to the grader
@@ -160,26 +170,22 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False
section_descriptor = section['section_descriptor']
section_name = section_descriptor.display_name_with_default
- should_grade_section = False
- # If we haven't seen a single problem in the section, we don't have to grade it at all! We can assume 0%
- for moduledescriptor in section['xmoduledescriptors']:
- # some problems have state that is updated independently of interaction
- # with the LMS, so they need to always be scored. (E.g. foldit.)
- if moduledescriptor.always_recalculate_grades:
- should_grade_section = True
- break
+ # some problems have state that is updated independently of interaction
+ # with the LMS, so they need to always be scored. (E.g. foldit.,
+ # combinedopenended)
+ should_grade_section = any(
+ descriptor.always_recalculate_grades for descriptor in section['xmoduledescriptors']
+ )
- # Create a fake key to pull out a StudentModule object from the FieldDataCache
-
- key = DjangoKeyValueStore.Key(
- Scope.user_state,
- student.id,
- moduledescriptor.location,
- None
- )
- if field_data_cache.find(key):
- should_grade_section = True
- break
+ # If we haven't seen a single problem in the section, we don't have to grade it at all! We can assume 0%
+ if not should_grade_section:
+ with manual_transaction():
+ should_grade_section = StudentModule.objects.filter(
+ student=student,
+ module_state_key__in=[
+ descriptor.location for descriptor in section['xmoduledescriptors']
+ ]
+ ).exists()
if should_grade_section:
scores = []
@@ -188,11 +194,13 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False
'''creates an XModule instance given a descriptor'''
# TODO: We need the request to pass into here. If we could forego that, our arguments
# would be simpler
+ with manual_transaction():
+ field_data_cache = FieldDataCache([descriptor], course.id, student)
return get_module_for_descriptor(student, request, descriptor, field_data_cache, course.id)
for module_descriptor in yield_dynamic_descriptor_descendents(section_descriptor, create_module):
- (correct, total) = get_score(course.id, student, module_descriptor, create_module, field_data_cache)
+ (correct, total) = get_score(course.id, student, module_descriptor, create_module)
if correct is None and total is None:
continue
@@ -261,11 +269,23 @@ def grade_for_percentage(grade_cutoffs, percentage):
return letter_grade
+@transaction.commit_manually
+def progress_summary(student, request, course):
+ """
+ Wraps "_progress_summary" with the manual_transaction context manager just
+ in case there are unanticipated errors.
+ """
+ with manual_transaction():
+ return _progress_summary(student, request, course)
+
+
# TODO: This method is not very good. It was written in the old course style and
# then converted over and performance is not good. Once the progress page is redesigned
# to not have the progress summary this method should be deleted (so it won't be copied).
-def progress_summary(student, request, course, field_data_cache):
+def _progress_summary(student, request, course):
"""
+ Unwrapped version of "progress_summary".
+
This pulls a summary of all problems in the course.
Returns
@@ -278,20 +298,21 @@ def progress_summary(student, request, course, field_data_cache):
Arguments:
student: A User object for the student to grade
course: A Descriptor containing the course to grade
- field_data_cache: A FieldDataCache initialized with all
- instance_modules for the student
If the student does not have access to load the course module, this function
will return None.
"""
-
- # TODO: We need the request to pass into here. If we could forego that, our arguments
- # would be simpler
- course_module = get_module_for_descriptor(student, request, course, field_data_cache, course.id)
- if not course_module:
- # This student must not have access to the course.
- return None
+ with manual_transaction():
+ field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
+ course.id, student, course, depth=None
+ )
+ # TODO: We need the request to pass into here. If we could
+ # forego that, our arguments would be simpler
+ course_module = get_module_for_descriptor(student, request, course, field_data_cache, course.id)
+ if not course_module:
+ # This student must not have access to the course.
+ return None
chapters = []
# Don't include chapters that aren't displayable (e.g. due to error)
@@ -301,50 +322,52 @@ def progress_summary(student, request, course, field_data_cache):
continue
sections = []
+
for section_module in chapter_module.get_display_items():
# Skip if the section is hidden
- if section_module.hide_from_toc:
- continue
-
- # Same for sections
- graded = section_module.graded
- scores = []
-
- module_creator = section_module.xmodule_runtime.get_module
+ with manual_transaction():
+ if section_module.hide_from_toc:
+ continue
- for module_descriptor in yield_dynamic_descriptor_descendents(section_module, module_creator):
+ graded = section_module.graded
+ scores = []
- course_id = course.id
- (correct, total) = get_score(course_id, student, module_descriptor, module_creator, field_data_cache)
- if correct is None and total is None:
- continue
+ module_creator = section_module.xmodule_runtime.get_module
- scores.append(Score(correct, total, graded, module_descriptor.display_name_with_default))
+ for module_descriptor in yield_dynamic_descriptor_descendents(section_module, module_creator):
- scores.reverse()
- section_total, _ = graders.aggregate_scores(
- scores, section_module.display_name_with_default)
+ course_id = course.id
+ (correct, total) = get_score(course_id, student, module_descriptor, module_creator)
+ if correct is None and total is None:
+ continue
- module_format = section_module.format if section_module.format is not None else ''
- sections.append({
- 'display_name': section_module.display_name_with_default,
- 'url_name': section_module.url_name,
- 'scores': scores,
- 'section_total': section_total,
- 'format': module_format,
- 'due': section_module.due,
- 'graded': graded,
- })
+ scores.append(Score(correct, total, graded, module_descriptor.display_name_with_default))
- chapters.append({'course': course.display_name_with_default,
- 'display_name': chapter_module.display_name_with_default,
- 'url_name': chapter_module.url_name,
- 'sections': sections})
+ scores.reverse()
+ section_total, _ = graders.aggregate_scores(
+ scores, section_module.display_name_with_default)
+
+ module_format = section_module.format if section_module.format is not None else ''
+ sections.append({
+ 'display_name': section_module.display_name_with_default,
+ 'url_name': section_module.url_name,
+ 'scores': scores,
+ 'section_total': section_total,
+ 'format': module_format,
+ 'due': section_module.due,
+ 'graded': graded,
+ })
+
+ chapters.append({
+ 'course': course.display_name_with_default,
+ 'display_name': chapter_module.display_name_with_default,
+ 'url_name': chapter_module.url_name,
+ 'sections': sections
+ })
return chapters
-
-def get_score(course_id, user, problem_descriptor, module_creator, field_data_cache):
+def get_score(course_id, user, problem_descriptor, module_creator):
"""
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.
@@ -377,15 +400,14 @@ def get_score(course_id, user, problem_descriptor, module_creator, field_data_ca
# These are not problems, and do not have a score
return (None, None)
- # Create a fake KeyValueStore key to pull out the StudentModule
- key = DjangoKeyValueStore.Key(
- Scope.user_state,
- user.id,
- problem_descriptor.location,
- None
- )
-
- student_module = field_data_cache.find(key)
+ try:
+ student_module = StudentModule.objects.get(
+ student=user,
+ course_id=course_id,
+ module_state_key=problem_descriptor.location
+ )
+ except StudentModule.DoesNotExist:
+ student_module = None
if student_module is not None and student_module.max_grade is not None:
correct = student_module.grade if student_module.grade is not None else 0
@@ -478,4 +500,3 @@ def iterate_grades_for(course_id, students):
exc.message
)
yield student, {}, exc.message
-
diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py
index 0736e08baf3..854750f8d1e 100644
--- a/lms/djangoapps/courseware/tests/test_submitting_problems.py
+++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py
@@ -236,14 +236,11 @@ class TestCourseGrader(TestSubmittingProblems):
make up the final grade. (For display)
"""
- field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
- self.course.id, self.student_user, self.course)
-
- fake_request = self.factory.get(reverse('progress',
- kwargs={'course_id': self.course.id}))
+ fake_request = self.factory.get(
+ reverse('progress', kwargs={'course_id': self.course.id})
+ )
- return grades.grade(self.student_user, fake_request,
- self.course, field_data_cache)
+ return grades.grade(self.student_user, fake_request, self.course)
def get_progress_summary(self):
"""
@@ -257,16 +254,13 @@ class TestCourseGrader(TestSubmittingProblems):
etc.
"""
- field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
- self.course.id, self.student_user, self.course)
-
- fake_request = self.factory.get(reverse('progress',
- kwargs={'course_id': self.course.id}))
+ fake_request = self.factory.get(
+ reverse('progress', kwargs={'course_id': self.course.id})
+ )
- progress_summary = grades.progress_summary(self.student_user,
- fake_request,
- self.course,
- field_data_cache)
+ progress_summary = grades.progress_summary(
+ self.student_user, fake_request, self.course
+ )
return progress_summary
def check_grade_percent(self, percent):
diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py
index 819f65fca69..6646ea1e637 100644
--- a/lms/djangoapps/courseware/views.py
+++ b/lms/djangoapps/courseware/views.py
@@ -14,6 +14,7 @@ from django.shortcuts import redirect
from mitxmako.shortcuts import render_to_response, render_to_string
from django_future.csrf import ensure_csrf_cookie
from django.views.decorators.cache import cache_control
+from django.db import transaction
from markupsafe import escape
from courseware import grades
@@ -643,8 +644,9 @@ def mktg_course_about(request, course_id):
except (ValueError, Http404) as e:
# if a course does not exist yet, display a coming
# soon button
- return render_to_response('courseware/mktg_coming_soon.html',
- {'course_id': course_id})
+ return render_to_response(
+ 'courseware/mktg_coming_soon.html', {'course_id': course_id}
+ )
registered = registered_for_course(course, request.user)
@@ -659,21 +661,36 @@ def mktg_course_about(request, course_id):
settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'))
course_modes = CourseMode.modes_for_course(course.id)
- return render_to_response('courseware/mktg_course_about.html',
- {
- 'course': course,
- 'registered': registered,
- 'allow_registration': allow_registration,
- 'course_target': course_target,
- 'show_courseware_link': show_courseware_link,
- 'course_modes': course_modes,
- })
+ return render_to_response(
+ 'courseware/mktg_course_about.html',
+ {
+ 'course': course,
+ 'registered': registered,
+ 'allow_registration': allow_registration,
+ 'course_target': course_target,
+ 'show_courseware_link': show_courseware_link,
+ 'course_modes': course_modes,
+ }
+ )
@login_required
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
+@transaction.commit_manually
def progress(request, course_id, student_id=None):
- """ User progress. We show the grade bar and every problem score.
+ """
+ Wraps "_progress" with the manual_transaction context manager just in case
+ there are unanticipated errors.
+ """
+ with grades.manual_transaction():
+ return _progress(request, course_id, student_id)
+
+
+def _progress(request, course_id, student_id):
+ """
+ Unwrapped version of "progress".
+
+ User progress. We show the grade bar and every problem score.
Course staff are allowed to see the progress of students in their class.
"""
@@ -696,26 +713,26 @@ def progress(request, course_id, student_id=None):
# additional DB lookup (this kills the Progress page in particular).
student = User.objects.prefetch_related("groups").get(id=student.id)
- field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
- course_id, student, course, depth=None)
+ courseware_summary = grades.progress_summary(student, request, course)
- courseware_summary = grades.progress_summary(student, request, course,
- field_data_cache)
- grade_summary = grades.grade(student, request, course, field_data_cache)
+ grade_summary = grades.grade(student, request, course)
if courseware_summary is None:
#This means the student didn't have access to the course (which the instructor requested)
raise Http404
- context = {'course': course,
- 'courseware_summary': courseware_summary,
- 'grade_summary': grade_summary,
- 'staff_access': staff_access,
- 'student': student,
- }
- context.update()
+ context = {
+ 'course': course,
+ 'courseware_summary': courseware_summary,
+ 'grade_summary': grade_summary,
+ 'staff_access': staff_access,
+ 'student': student,
+ }
+
+ with grades.manual_transaction():
+ response = render_to_response('courseware/progress.html', context)
- return render_to_response('courseware/progress.html', context)
+ return response
@login_required
--
GitLab