Merge pull request #364 from MITx/MITx/feature/bridger/fast_course_grading

Even Faster Course Grading

common/lib/xmodule/xmodule/abtest_module.py

@@ -31,8 +31,8 @@ class ABTestModule(XModule):
     Implements an A/B test with an aribtrary number of competing groups
     """
 
-    def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs)
+    def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs):
+        XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs)
 
         if shared_state is None:
 

common/lib/xmodule/xmodule/capa_module.py

@@ -11,13 +11,13 @@ from datetime import timedelta
 from lxml import etree
 from pkg_resources import resource_string
 
-from xmodule.x_module import XModule
-from xmodule.raw_module import RawDescriptor
-from xmodule.exceptions import NotFoundError
-from progress import Progress
 from capa.capa_problem import LoncapaProblem
 from capa.responsetypes import StudentInputError
 from capa.util import convert_files_to_filenames
+from progress import Progress
+from xmodule.x_module import XModule
+from xmodule.raw_module import RawDescriptor
+from xmodule.exceptions import NotFoundError
 
 log = logging.getLogger("mitx.courseware")
 
@@ -80,9 +80,9 @@ class CapaModule(XModule):
     js_module_name = "Problem"
     css = {'scss': [resource_string(__name__, 'css/capa/display.scss')]}
 
-    def __init__(self, system, location, definition, instance_state=None,
+    def __init__(self, system, location, definition, descriptor, instance_state=None,
                  shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition, instance_state,
+        XModule.__init__(self, system, location, definition, descriptor, instance_state,
                          shared_state, **kwargs)
 
         self.attempts = 0
@@ -134,12 +134,14 @@ class CapaModule(XModule):
 
         if self.rerandomize == 'never':
             seed = 1
-        elif self.rerandomize == "per_student" and hasattr(system, 'id'):
+        elif self.rerandomize == "per_student" and hasattr(self.system, 'id'):
             seed = system.id
         else:
             seed = None
 
         try:
+            # TODO (vshnayder): move as much as possible of this work and error
+            # checking to descriptor load time
             self.lcp = LoncapaProblem(self.definition['data'], self.location.html_id(),
                                       instance_state, seed=seed, system=self.system)
         except Exception as err:
@@ -426,7 +428,7 @@ class CapaModule(XModule):
         event_info = dict()
         event_info['state'] = self.lcp.get_state()
         event_info['problem_id'] = self.location.url()
-         
+
         answers = self.make_dict_of_responses(get)
         event_info['answers'] = convert_files_to_filenames(answers)
 
@@ -563,6 +565,9 @@ class CapaDescriptor(RawDescriptor):
 
     module_class = CapaModule
 
+    stores_state = True
+    has_score = True
+
     # Capa modules have some additional metadata:
     # TODO (vshnayder): do problems have any other metadata?  Do they
     # actually use type and points?

common/lib/xmodule/xmodule/course_module.py

@@ -99,10 +99,10 @@ class CourseDescriptor(SequenceDescriptor):
             sections = []
             for s in c.get_children():
                 if s.metadata.get('graded', False):
-                    # TODO: Only include modules that have a score here
-                    xmoduledescriptors = [child for child in yield_descriptor_descendents(s)]
-
-                    section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : xmoduledescriptors}
+                    xmoduledescriptors = list(yield_descriptor_descendents(s))
+                    
+                    # The xmoduledescriptors included here are only the ones that have scores.
+                    section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : filter(lambda child: child.has_score, xmoduledescriptors) }
 
                     section_format = s.metadata.get('format', "")
                     graded_sections[ section_format ] = graded_sections.get( section_format, [] ) + [section_description]

common/lib/xmodule/xmodule/html_module.py

@@ -18,9 +18,9 @@ class HtmlModule(XModule):
     def get_html(self):
         return self.html
 
-    def __init__(self, system, location, definition,
+    def __init__(self, system, location, definition, descriptor,
                  instance_state=None, shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition,
+        XModule.__init__(self, system, location, definition, descriptor,
                          instance_state, shared_state, **kwargs)
         self.html = self.definition['data']
 

common/lib/xmodule/xmodule/seq_module.py

@@ -25,9 +25,9 @@ class SequenceModule(XModule):
     css = {'scss': [resource_string(__name__, 'css/sequence/display.scss')]}
     js_module_name = "Sequence"
 
-    def __init__(self, system, location, definition, instance_state=None,
+    def __init__(self, system, location, definition, descriptor, instance_state=None,
                  shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition,
+        XModule.__init__(self, system, location, definition, descriptor,
                          instance_state, shared_state, **kwargs)
         self.position = 1
 
@@ -107,6 +107,8 @@ class SequenceModule(XModule):
 class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor):
     mako_template = 'widgets/sequence-edit.html'
     module_class = SequenceModule
+    
+    stores_state = True # For remembering where in the sequence the student is
 
     @classmethod
     def definition_from_xml(cls, xml_object, system):

common/lib/xmodule/xmodule/template_module.py

@@ -26,12 +26,26 @@ class CustomTagModule(XModule):
         More information given in <a href="/book/234">the text</a>
     """
 
-    def __init__(self, system, location, definition,
+    def __init__(self, system, location, definition, descriptor,
                  instance_state=None, shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition,
+        XModule.__init__(self, system, location, definition, descriptor,
                          instance_state, shared_state, **kwargs)
+        self.html = definition['html']
 
-        xmltree = etree.fromstring(self.definition['data'])
+    def get_html(self):
+        return self.html
+
+
+class CustomTagDescriptor(RawDescriptor):
+    """ Descriptor for custom tags.  Loads the template when created."""
+    module_class = CustomTagModule
+
+    @classmethod
+    def definition_from_xml(cls, xml_object, system):
+        definition = RawDescriptor.definition_from_xml(xml_object, system)
+
+        # Render the template and save it.
+        xmltree = etree.fromstring(definition['data'])
         if 'impl' in xmltree.attrib:
             template_name = xmltree.attrib['impl']
         else:
@@ -45,13 +59,8 @@ class CustomTagModule(XModule):
                                 .format(location))
 
         params = dict(xmltree.items())
-        with self.system.filestore.open(
-                'custom_tags/{name}'.format(name=template_name)) as template:
-            self.html = Template(template.read()).render(**params)
-
-    def get_html(self):
-        return self.html
+        with system.resources_fs.open('custom_tags/{name}'
+                                   .format(name=template_name)) as template:
+            definition['html'] = Template(template.read()).render(**params)
 
-
-class CustomTagDescriptor(RawDescriptor):
-    module_class = CustomTagModule
+        return definition

common/lib/xmodule/xmodule/tests/__init__.py

@@ -708,6 +708,6 @@ class ModuleProgressTest(unittest.TestCase):
     '''
     def test_xmodule_default(self):
         '''Make sure default get_progress exists, returns None'''
-        xm = x_module.XModule(i4xs, 'a://b/c/d/e', {})
+        xm = x_module.XModule(i4xs, 'a://b/c/d/e', None, {})
         p = xm.get_progress()
         self.assertEqual(p, None)

common/lib/xmodule/xmodule/vertical_module.py

@@ -10,8 +10,8 @@ class_priority = ['video', 'problem']
 class VerticalModule(XModule):
     ''' Layout module for laying out submodules vertically.'''
 
-    def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs)
+    def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs):
+        XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs)
         self.contents = None
 
     def get_html(self):

common/lib/xmodule/xmodule/video_module.py

@@ -23,9 +23,9 @@ class VideoModule(XModule):
     css = {'scss': [resource_string(__name__, 'css/video/display.scss')]}
     js_module_name = "Video"
 
-    def __init__(self, system, location, definition,
+    def __init__(self, system, location, definition, descriptor,
                  instance_state=None, shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition,
+        XModule.__init__(self, system, location, definition, descriptor,
                          instance_state, shared_state, **kwargs)
         xmltree = etree.fromstring(self.definition['data'])
         self.youtube = xmltree.get('youtube')
@@ -80,3 +80,5 @@ class VideoModule(XModule):
 
 class VideoDescriptor(RawDescriptor):
     module_class = VideoModule
+    
+    stores_state = True

common/lib/xmodule/xmodule/x_module.py

@@ -143,7 +143,7 @@ class XModule(HTMLSnippet):
     # in the module
     icon_class = 'other'
 
-    def __init__(self, system, location, definition,
+    def __init__(self, system, location, definition, descriptor,
                  instance_state=None, shared_state=None, **kwargs):
         '''
         Construct a new xmodule
@@ -189,6 +189,7 @@ class XModule(HTMLSnippet):
         self.system = system
         self.location = Location(location)
         self.definition = definition
+        self.descriptor = descriptor
         self.instance_state = instance_state
         self.shared_state = shared_state
         self.id = self.location.url()
@@ -303,6 +304,13 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
     """
     entry_point = "xmodule.v1"
     module_class = XModule
+    
+    # Attributes for inpsection of the descriptor
+    stores_state = False # Indicates whether the xmodule state should be 
+    # stored in a database (independent of shared state)
+    has_score = False # This indicates whether the xmodule is a problem-type.
+    # It should respond to max_score() and grade(). It can be graded or ungraded
+    # (like a practice problem).
 
     # A list of metadata that this module can inherit from its parent module
     inheritable_metadata = (
@@ -427,6 +435,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
             system,
             self.location,
             self.definition,
+            self,
             metadata=self.metadata
         )
 

lms/djangoapps/courseware/grades.py

@@ -12,10 +12,12 @@ from models import StudentModule
 log = logging.getLogger("mitx.courseware")
 
 def yield_module_descendents(module):
-    for child in module.get_display_items():
-        yield child
-        for module in yield_module_descendents(child):
-            yield module
+    stack = module.get_display_items()
+    
+    while len(stack) > 0:
+        next_module = stack.pop()
+        stack.extend( next_module.get_display_items() )
+        yield next_module
    
 def grade(student, request, course, student_module_cache=None):
     """
@@ -89,7 +91,7 @@ def grade(student, request, course, student_module_cache=None):
             if graded_total.possible > 0:
                 format_scores.append(graded_total)
             else:
-                log.exception("Unable to grade a section with a total possible score of zero. " + str(section_descriptor.id))
+                log.exception("Unable to grade a section with a total possible score of zero. " + str(section_descriptor.location))
         
         totaled_scores[section_format] = format_scores
     
@@ -183,6 +185,10 @@ def get_score(user, problem, student_module_cache):
     problem: an XModule
     cache: A StudentModuleCache
     """
+    if not (problem.descriptor.stores_state and problem.descriptor.has_score):
+        # These are not problems, and do not have a score
+        return (None, None)
+    
     correct = 0.0
 
     # If the ID is not in the cache, add the item

lms/djangoapps/courseware/models.py

@@ -69,10 +69,10 @@ class StudentModuleCache(object):
     """
     def __init__(self, user, descriptors):
         '''
-        Find any StudentModule objects that are needed by any child modules of the
-        supplied descriptor, or caches only the StudentModule objects specifically
-        for every descriptor in descriptors. Avoids making multiple queries to the 
-        database.
+        Find any StudentModule objects that are needed by any descriptor
+        in descriptors. Avoids making multiple queries to the database.
+        Note: Only modules that have store_state = True or have shared
+        state will have a StudentModule.
         
         Arguments
         user: The user for which to fetch maching StudentModules
@@ -134,7 +134,8 @@ class StudentModuleCache(object):
         '''
         keys = []
         for descriptor in descriptors:
-            keys.append(descriptor.location.url())
+            if descriptor.stores_state:
+                keys.append(descriptor.location.url())
             
             shared_state_key = getattr(descriptor, 'shared_state_key', None)
             if shared_state_key is not None:

lms/djangoapps/courseware/module_render.py

@@ -127,19 +127,18 @@ def get_module(user, request, location, student_module_cache, position=None):
     descriptor = modulestore().get_item(location)
     
     #TODO Only check the cache if this module can possibly have state
+    instance_module = None
+    shared_module = None
     if user.is_authenticated():
-        instance_module = student_module_cache.lookup(descriptor.category,
-                                              descriptor.location.url())
+        if descriptor.stores_state:
+            instance_module = student_module_cache.lookup(descriptor.category,
+                                                  descriptor.location.url())
         
         shared_state_key = getattr(descriptor, 'shared_state_key', None)
         if shared_state_key is not None:
             shared_module = student_module_cache.lookup(descriptor.category,
                                                         shared_state_key)
-        else:
-            shared_module = None
-    else:
-        instance_module = None
-        shared_module = None
+        
         
 
     instance_state = instance_module.state if instance_module is not None else None
@@ -206,6 +205,11 @@ def get_instance_module(user, module, student_module_cache):
         or None if this is an anonymous user
     """
     if user.is_authenticated():
+        if not module.descriptor.stores_state:
+            log.exception("Attempted to get the instance_module for a module " 
+                          + str(module.id) + " which does not store state.")
+            return None
+        
         instance_module = student_module_cache.lookup(module.category,
                                               module.location.url())
         

lms/templates/gradebook.html

@@ -11,11 +11,11 @@
   <%static:css group='course'/>
   
   <style type="text/css">
-  .grade_a {color:green;}
-  .grade_b {color:Chocolate;}
-  .grade_c {color:DarkSlateGray;}
-  .grade_f {color:DimGray;}
-  .grade_none {color:LightGray;}
+  .grade_A {color:green;}
+  .grade_B {color:Chocolate;}
+  .grade_C {color:DarkSlateGray;}
+  .grade_F {color:DimGray;}
+  .grade_None {color:LightGray;}
   </style>
   
 </%block>
@@ -54,7 +54,7 @@
           
           data_class = "grade_" + letter_grade
         %>
-        <td class="${data_class}">${ "{0:.0%}".format( percentage ) }</td>
+        <td class="${data_class}" data-percent="${percentage}">${ "{0:.0%}".format( percentage ) }</td>
       </%def>
       
       %for student in students: