From 64e1eb9903933b234220ada66889b920a2682c22 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 15 Mar 2013 11:35:27 -0700 Subject: [PATCH 1/2] Fix randomness bug in cohort placement capa re-seeds the global RNG all the time, resulting in non-random cohort placement. Switch to using a local random module. (Thanks Cale for helping me figure this out!) --- common/djangoapps/course_groups/cohorts.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index c362ed4e89..ded20e1613 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/common/djangoapps/course_groups/cohorts.py @@ -15,6 +15,12 @@ from .models import CourseUserGroup log = logging.getLogger(__name__) +# tl;dr: global state is bad. capa reseeds random every time a problem is loaded. Even +# if and when that's fixed, it's a good idea to have a local generator to avoid any other +# code that messes with the global random module. +local_random = random.Random() + + def is_course_cohorted(course_id): """ Given a course id, return a boolean for whether or not the course is @@ -129,13 +135,7 @@ def get_cohort(user, course_id): return None # Put user in a random group, creating it if needed - choice = random.randrange(0, n) - group_name = choices[choice] - - # Victor: we are seeing very strange behavior on prod, where almost all users - # end up in the same group. Log at INFO to try to figure out what's going on. - log.info("DEBUG: adding user {0} to cohort {1}. choice={2}".format( - user, group_name,choice)) + group_name = local_random.choice(choices) group, created = CourseUserGroup.objects.get_or_create( course_id=course_id, From d91008b73a14e5b1599a2bc965e39d75d1785138 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 15 Mar 2013 19:36:14 -0700 Subject: [PATCH 2/2] Prevent random.Random() from running at import time. --- common/djangoapps/course_groups/cohorts.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index ded20e1613..7924012bfe 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/common/djangoapps/course_groups/cohorts.py @@ -18,8 +18,20 @@ log = logging.getLogger(__name__) # tl;dr: global state is bad. capa reseeds random every time a problem is loaded. Even # if and when that's fixed, it's a good idea to have a local generator to avoid any other # code that messes with the global random module. -local_random = random.Random() +_local_random = None +def local_random(): + """ + Get the local random number generator. In a function so that we don't run + random.Random() at import time. + """ + # ironic, isn't it? + global _local_random + + if _local_random is None: + _local_random = random.Random() + + return _local_random def is_course_cohorted(course_id): """ @@ -135,7 +147,7 @@ def get_cohort(user, course_id): return None # Put user in a random group, creating it if needed - group_name = local_random.choice(choices) + group_name = local_random().choice(choices) group, created = CourseUserGroup.objects.get_or_create( course_id=course_id,