From 61d247d4bb12cec4282e8e84ad30d1341d9f34b2 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Wed, 24 May 2017 15:49:45 -0400 Subject: [PATCH 1/2] Prefer iters and sets to lists --- .../contentstore/tests/test_course_listing.py | 52 +++++----- cms/djangoapps/contentstore/views/course.py | 99 ++++++++++--------- 2 files changed, 78 insertions(+), 73 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 0f3fa3c8ed..b821c49492 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -17,11 +17,11 @@ from common.test.utils import XssTestMixin from xmodule.course_module import CourseSummary from contentstore.views.course import ( - _accessible_courses_list, + _accessible_courses_iter, _accessible_courses_list_from_groups, AccessListFallback, get_courses_accessible_to_user, - _accessible_courses_summary_list, + _accessible_courses_summary_iter, ) from contentstore.utils import delete_course_and_groups from contentstore.tests.utils import AjaxEnabledTestClient @@ -132,11 +132,12 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): self._create_course_with_access_groups(course_location, self.user) # get courses through iterating all courses - courses_list, __ = _accessible_courses_list(self.request) + courses_iter, __ = _accessible_courses_iter(self.request) + courses_list = list(courses_iter) self.assertEqual(len(courses_list), 1) - courses_summary_list, __ = _accessible_courses_summary_list(self.request) - self.assertEqual(len(courses_summary_list), 1) + courses_summary_list, __ = _accessible_courses_summary_iter(self.request) + self.assertEqual(len(list(courses_summary_list)), 1) # get courses by reversing group name formats courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request) @@ -179,8 +180,8 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): # Verify that CCX courses are filtered out while iterating over all courses mocked_ccx_course = Mock(id=ccx_course_key) with patch('xmodule.modulestore.mixed.MixedModuleStore.get_courses', return_value=[mocked_ccx_course]): - courses_list, __ = _accessible_courses_list(self.request) - self.assertEqual(len(courses_list), 0) + courses_iter, __ = _accessible_courses_iter(self.request) + self.assertEqual(len(list(courses_iter)), 0) @ddt.data( (ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'), @@ -201,8 +202,8 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor) # get courses through iterating all courses - courses_list, __ = _accessible_courses_list(self.request) - self.assertEqual(courses_list, []) + courses_iter, __ = _accessible_courses_iter(self.request) + self.assertEqual(list(courses_iter), []) # get courses by reversing group name formats courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request) @@ -231,14 +232,14 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): # Fetch accessible courses list & verify their count courses_list_by_staff, __ = get_courses_accessible_to_user(self.request) - self.assertEqual(len(courses_list_by_staff), TOTAL_COURSES_COUNT) + self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT) # Verify fetched accessible courses list is a list of CourseSummery instances self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_list_by_staff)) # Now count the db queries for staff with check_mongo_calls(mongo_calls): - _accessible_courses_summary_list(self.request) + list(_accessible_courses_summary_iter(self.request)) @ddt.data( (ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'), @@ -261,7 +262,8 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor) # get courses through iterating all courses - courses_list, __ = _accessible_courses_list(self.request) + courses_iter, __ = _accessible_courses_iter(self.request) + courses_list = list(courses_iter) self.assertEqual(courses_list, []) # get courses by reversing group name formats @@ -279,10 +281,12 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): self._create_course_with_access_groups(course_key, self.user, store) # get courses through iterating all courses - courses_list, __ = _accessible_courses_list(self.request) + courses_iter, __ = _accessible_courses_iter(self.request) + courses_list = list(courses_iter) self.assertEqual(len(courses_list), 1) - courses_summary_list, __ = _accessible_courses_summary_list(self.request) + courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) + courses_summary_list = list(courses_summary_iter) # Verify fetched accessible courses list is a list of CourseSummery instances and only one course # is returned @@ -302,17 +306,17 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): CourseInstructorRole(course_key).add_users(self.user) # Get courses through iterating all courses - courses_list, __ = _accessible_courses_list(self.request) + courses_iter, __ = _accessible_courses_iter(self.request) # Get course summaries by iterating all courses - courses_summary_list, __ = _accessible_courses_summary_list(self.request) + courses_summary_iter, __ = _accessible_courses_summary_iter(self.request) # Get courses by reversing group name formats courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request) # Test that course list returns no course self.assertEqual( - [len(courses_list), len(courses_list_by_groups), len(courses_summary_list)], + [len(list(courses_iter)), len(courses_list_by_groups), len(list(courses_summary_iter))], [0, 0, 0] ) @@ -344,13 +348,13 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): # time the get courses by iterating through all courses with Timer() as iteration_over_courses_time_1: - courses_list, __ = _accessible_courses_list(self.request) - self.assertEqual(len(courses_list), USER_COURSES_COUNT) + courses_iter, __ = _accessible_courses_iter(self.request) + self.assertEqual(len(list(courses_iter)), USER_COURSES_COUNT) # time again the get courses by iterating through all courses with Timer() as iteration_over_courses_time_2: - courses_list, __ = _accessible_courses_list(self.request) - self.assertEqual(len(courses_list), USER_COURSES_COUNT) + courses_iter, __ = _accessible_courses_iter(self.request) + self.assertEqual(len(list(courses_iter)), USER_COURSES_COUNT) # time the get courses by reversing django groups with Timer() as iteration_over_groups_time_1: @@ -372,7 +376,7 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): _accessible_courses_list_from_groups(self.request) with check_mongo_calls(courses_list_calls): - _accessible_courses_list(self.request) + list(_accessible_courses_iter(self.request)) # Calls: # 1) query old mongo # 2) get_more on old mongo @@ -424,7 +428,7 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): # Verify fetched accessible courses list is a list of CourseSummery instances and test expacted # course count is returned - self.assertEqual(len(courses_list), 2) + self.assertEqual(len(list(courses_list)), 2) self.assertTrue(all(isinstance(course, CourseSummary) for course in courses_list)) def test_course_listing_with_actions_in_progress(self): @@ -447,7 +451,7 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): ) # verify return values - for method in (_accessible_courses_list_from_groups, _accessible_courses_list): + for method in (_accessible_courses_list_from_groups, _accessible_courses_iter): def set_of_course_keys(course_list, key_attribute_name='id'): """Returns a python set of course keys by accessing the key with the given attribute name.""" return set(getattr(c, key_attribute_name) for c in course_list) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index e3a5e45cea..35d8ef367c 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -17,6 +17,7 @@ import django.utils from django.utils.translation import ugettext as _ from django.views.decorators.http import require_http_methods, require_GET from django.views.decorators.csrf import ensure_csrf_cookie +import six from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey @@ -357,13 +358,14 @@ def get_in_process_course_actions(request): return [ course for course in CourseRerunState.objects.find_all( - exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED}, should_display=True + exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED}, + should_display=True, ) if has_studio_read_access(request.user, course.course_key) ] -def _accessible_courses_summary_list(request): +def _accessible_courses_summary_iter(request): """ List all courses available to the logged in user by iterating through all the courses """ @@ -378,12 +380,12 @@ def _accessible_courses_summary_list(request): return has_studio_read_access(request.user, course_summary.id) - courses_summary = filter(course_filter, modulestore().get_course_summaries()) + courses_summary = six.moves.filter(course_filter, modulestore().get_course_summaries()) in_process_course_actions = get_in_process_course_actions(request) return courses_summary, in_process_course_actions -def _accessible_courses_list(request): +def _accessible_courses_iter(request): """ List all courses available to the logged in user by iterating through all the courses """ @@ -406,7 +408,7 @@ def _accessible_courses_list(request): return has_studio_read_access(request.user, course.id) - courses = filter(course_filter, modulestore().get_courses()) + courses = six.moves.filter(course_filter, modulestore().get_courses()) in_process_course_actions = get_in_process_course_actions(request) return courses, in_process_course_actions @@ -455,12 +457,12 @@ def _accessible_courses_list_from_groups(request): return courses_list.values(), in_process_course_actions -def _accessible_libraries_list(user): +def _accessible_libraries_iter(user): """ List all libraries available to the logged in user by iterating through all libraries """ # No need to worry about ErrorDescriptors - split's get_libraries() never returns them. - return [lib for lib in modulestore().get_libraries() if has_studio_read_access(user, lib.location.library_key)] + return (lib for lib in modulestore().get_libraries() if has_studio_read_access(user, lib.location.library_key)) @login_required @@ -469,29 +471,29 @@ def course_listing(request): """ List all courses available to the logged in user """ - courses, in_process_course_actions = get_courses_accessible_to_user(request) + courses_iter, in_process_course_actions = get_courses_accessible_to_user(request) user = request.user - libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED else [] + libraries = _accessible_libraries_iter(request.user) if LIBRARIES_ENABLED else [] def format_in_process_course_view(uca): """ Return a dict of the data which the view requires for each unsucceeded course """ return { - 'display_name': uca.display_name, - 'course_key': unicode(uca.course_key), - 'org': uca.course_key.org, - 'number': uca.course_key.course, - 'run': uca.course_key.run, - 'is_failed': True if uca.state == CourseRerunUIStateManager.State.FAILED else False, - 'is_in_progress': True if uca.state == CourseRerunUIStateManager.State.IN_PROGRESS else False, - 'dismiss_link': reverse_course_url( - 'course_notifications_handler', + u'display_name': uca.display_name, + u'course_key': unicode(uca.course_key), + u'org': uca.course_key.org, + u'number': uca.course_key.course, + u'run': uca.course_key.run, + u'is_failed': True if uca.state == CourseRerunUIStateManager.State.FAILED else False, + u'is_in_progress': True if uca.state == CourseRerunUIStateManager.State.IN_PROGRESS else False, + u'dismiss_link': reverse_course_url( + u'course_notifications_handler', uca.course_key, kwargs={ - 'action_state_id': uca.id, + u'action_state_id': uca.id, }, - ) if uca.state == CourseRerunUIStateManager.State.FAILED else '' + ) if uca.state == CourseRerunUIStateManager.State.FAILED else u'' } def format_library_for_view(library): @@ -499,29 +501,29 @@ def course_listing(request): Return a dict of the data which the view requires for each library """ return { - 'display_name': library.display_name, - 'library_key': unicode(library.location.library_key), - 'url': reverse_library_url('library_handler', unicode(library.location.library_key)), - 'org': library.display_org_with_default, - 'number': library.display_number_with_default, - 'can_edit': has_studio_write_access(request.user, library.location.library_key), + u'display_name': library.display_name, + u'library_key': unicode(library.location.library_key), + u'url': reverse_library_url(u'library_handler', unicode(library.location.library_key)), + u'org': library.display_org_with_default, + u'number': library.display_number_with_default, + u'can_edit': has_studio_write_access(request.user, library.location.library_key), } - courses = _remove_in_process_courses(courses, in_process_course_actions) + courses_iter = _remove_in_process_courses(courses_iter, in_process_course_actions) in_process_course_actions = [format_in_process_course_view(uca) for uca in in_process_course_actions] - return render_to_response('index.html', { - 'courses': courses, - 'in_process_course_actions': in_process_course_actions, - 'libraries_enabled': LIBRARIES_ENABLED, - 'libraries': [format_library_for_view(lib) for lib in libraries], - 'show_new_library_button': get_library_creator_status(user), - 'user': user, - 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), - 'course_creator_status': _get_course_creator_status(user), - 'rerun_creator_status': GlobalStaff().has_user(user), - 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), - 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), + return render_to_response(u'index.html', { + u'courses': list(courses_iter), + u'in_process_course_actions': in_process_course_actions, + u'libraries_enabled': LIBRARIES_ENABLED, + u'libraries': [format_library_for_view(lib) for lib in libraries], + u'show_new_library_button': get_library_creator_status(user), + u'user': user, + u'request_course_creator_url': reverse(u'contentstore.views.request_course_creator'), + u'course_creator_status': _get_course_creator_status(user), + u'rerun_creator_status': GlobalStaff().has_user(user), + u'allow_unicode_course_id': settings.FEATURES.get(u'ALLOW_UNICODE_COURSE_ID', False), + u'allow_course_reruns': settings.FEATURES.get(u'ALLOW_COURSE_RERUNS', True), }) @@ -623,18 +625,18 @@ def get_courses_accessible_to_user(request): """ if GlobalStaff().has_user(request.user): # user has global access so no need to get courses from django groups - courses, in_process_course_actions = _accessible_courses_summary_list(request) + courses, in_process_course_actions = _accessible_courses_summary_iter(request) else: try: courses, in_process_course_actions = _accessible_courses_list_from_groups(request) except AccessListFallback: # user have some old groups or there was some error getting courses from django groups # so fallback to iterating through all courses - courses, in_process_course_actions = _accessible_courses_summary_list(request) + courses, in_process_course_actions = _accessible_courses_summary_iter(request) return courses, in_process_course_actions -def _remove_in_process_courses(courses, in_process_course_actions): +def _remove_in_process_courses(courses_iter, in_process_course_actions): """ removes any in-process courses in courses list. in-process actually refers to courses that are in the process of being generated for re-run @@ -654,13 +656,12 @@ def _remove_in_process_courses(courses, in_process_course_actions): 'run': course.location.run } - in_process_action_course_keys = [uca.course_key for uca in in_process_course_actions] - courses = [ + in_process_action_course_keys = {uca.course_key for uca in in_process_course_actions} + return ( format_course_for_view(course) - for course in courses + for course in courses_iter if not isinstance(course, ErrorDescriptor) and (course.id not in in_process_action_course_keys) - ] - return courses + ) def course_outline_initial_state(locator_to_show, course_structure): @@ -1014,10 +1015,10 @@ def settings_handler(request, course_key_string): if is_prerequisite_courses_enabled(): courses, in_process_course_actions = get_courses_accessible_to_user(request) # exclude current course from the list of available courses - courses = [course for course in courses if course.id != course_key] + courses = (course for course in courses if course.id != course_key) if courses: courses = _remove_in_process_courses(courses, in_process_course_actions) - settings_context.update({'possible_pre_requisite_courses': courses}) + settings_context.update({'possible_pre_requisite_courses': list(courses)}) if credit_eligibility_enabled: if is_credit_course(course_key): From 6c6f271e0ef369c75164fa9ab96d96031ed81ccb Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Thu, 25 May 2017 10:18:43 -0400 Subject: [PATCH 2/2] Comment out broken speed test. --- .../contentstore/tests/test_course_listing.py | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index b821c49492..cf34eb9013 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -366,10 +366,18 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): courses_list, __ = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list), USER_COURSES_COUNT) - # test that the time taken by getting courses through reversing django groups is lower then the time - # taken by traversing through all courses (if accessible courses are relatively small) - self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed) - self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed) + # TODO (cdyer) : iteration over courses was optimized, and is now + # sometimes faster than iteration over groups. One of the following + # should be done to resolve this: + # * Iteration over groups should be sped up. + # * Iteration over groups should be removed, as it no longer saves time. + # * Or this part of the test should be removed. + + # Test that the time taken by getting courses through reversing django + # groups is lower then the time taken by traversing through all courses + # (if accessible courses are relatively small). + #self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed) + #self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed) # Now count the db queries with check_mongo_calls(courses_list_from_group_calls): @@ -436,11 +444,17 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin): num_courses_to_create = 3 courses = [ - self._create_course_with_access_groups(CourseLocator('Org', 'CreatedCourse' + str(num), 'Run'), self.user) + self._create_course_with_access_groups( + CourseLocator('Org', 'CreatedCourse' + str(num), 'Run'), + self.user, + ) for num in range(num_courses_to_create) ] courses_in_progress = [ - self._create_course_with_access_groups(CourseLocator('Org', 'InProgressCourse' + str(num), 'Run'), self.user) + self._create_course_with_access_groups( + CourseLocator('Org', 'InProgressCourse' + str(num), 'Run'), + self.user, + ) for num in range(num_courses_to_create) ]