From 12de61bf3cd43e9a7a2d862505e8bd40d62b3503 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 08:39:25 -0400 Subject: [PATCH 1/8] Changing lookup of experiment in decision service to use experiment ID instead of key to resolve bug --- optimizely/decision_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 98060e8e..6bc92333 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -342,7 +342,7 @@ def get_variation_for_rollout(self, project_config, rollout, user_id, attributes if rollout and len(rollout.experiments) > 0: for idx in range(len(rollout.experiments) - 1): logging_key = str(idx + 1) - rollout_rule = project_config.get_experiment_from_key(rollout.experiments[idx].get('key')) + rollout_rule = project_config.get_experiment_from_id(rollout.experiments[idx].get('id')) # Check if user meets audience conditions for targeting rule audience_conditions = rollout_rule.get_audience_conditions_or_ids() @@ -387,7 +387,7 @@ def get_variation_for_rollout(self, project_config, rollout, user_id, attributes break # Evaluate last rule i.e. "Everyone Else" rule - everyone_else_rule = project_config.get_experiment_from_key(rollout.experiments[-1].get('key')) + everyone_else_rule = project_config.get_experiment_from_id(rollout.experiments[-1].get('id')) audience_conditions = everyone_else_rule.get_audience_conditions_or_ids() audience_eval, audience_reasons = audience_helper.does_user_meet_audience_conditions( project_config, From bf7e0991091a4dcbee8a00a083a965d5f22fef6e Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 14:25:46 -0400 Subject: [PATCH 2/8] [BUGFIX] - Update to bucketer and added proper ID map to project_config. --- optimizely/bucketer.py | 2 +- optimizely/project_config.py | 41 ++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/optimizely/bucketer.py b/optimizely/bucketer.py index ca5e0f28..24852100 100644 --- a/optimizely/bucketer.py +++ b/optimizely/bucketer.py @@ -142,7 +142,7 @@ def bucket(self, project_config, experiment, user_id, bucketing_id): variation_id = self.find_bucket(project_config, bucketing_id, experiment.id, experiment.trafficAllocation) if variation_id: - variation = project_config.get_variation_from_id(experiment.key, variation_id) + variation = project_config.get_variation_from_id_by_experiment_id(experiment.id, variation_id) return variation, decide_reasons else: diff --git a/optimizely/project_config.py b/optimizely/project_config.py index ac97fac6..98677ab2 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -97,14 +97,23 @@ def __init__(self, datafile, logger, error_handler): self.variation_key_map = {} self.variation_id_map = {} self.variation_variable_usage_map = {} + self.variation_id_map_by_experiment_id = {} + self.variation_key_map_by_experiment_id = {} + for experiment in self.experiment_key_map.values(): self.experiment_id_map[experiment.id] = experiment self.variation_key_map[experiment.key] = self._generate_key_map( experiment.variations, 'key', entities.Variation ) + self.variation_id_map[experiment.key] = {} + self.variation_id_map_by_experiment_id[experiment.id] = {} + self.variation_key_map_by_experiment_id[experiment.id] = {} + for variation in self.variation_key_map.get(experiment.key).values(): self.variation_id_map[experiment.key][variation.id] = variation + self.variation_id_map_by_experiment_id[experiment.id][variation.id] = variation + self.variation_key_map_by_experiment_id[experiment.id][variation.key] = variation self.variation_variable_usage_map[variation.id] = self._generate_key_map( variation.variables, 'id', entities.Variation.VariableUsage ) @@ -557,3 +566,35 @@ def is_feature_experiment(self, experiment_id): """ return experiment_id in self.experiment_feature_map + + def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id): + """ Gets experiment id and variation id + + Returns: + The variation for the experiment id and variation id + or empty dict if not found + """ + if (experiment_id in self.variation_id_map_by_experiment_id and + variation_id in self.variation_id_map_by_experiment_id[experiment_id]): + return self.variation_id_map_by_experiment_id[experiment_id][variation_id] + + self.logger.error('Variation with id "%s" not defined in the datafile for experiment "%s".', + variation_id, experiment_id) + + return {} + + def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key): + """ Gets experiment id and variation key + + Returns: + The variation for the experiment id and variation key + or empty dict if not found + """ + if (experiment_id in self.variation_key_map_by_experiment_id and + variation_key in self.variation_key_map_by_experiment_id[experiment_id]): + return self.variation_key_map_by_experiment_id[experiment_id][variation_key] + + self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".', + variation_key, experiment_id) + + return {} From ff9625b9b9af53a8983412ee955821c4aff2a2a8 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 15:04:15 -0400 Subject: [PATCH 3/8] Switch to generate experiments_key_map from experiment_id_map rather than the other way around. --- optimizely/project_config.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 98677ab2..d6c9d46f 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -68,7 +68,7 @@ def __init__(self, datafile, logger, error_handler): # Utility maps for quick lookup self.group_id_map = self._generate_key_map(self.groups, 'id', entities.Group) - self.experiment_key_map = self._generate_key_map(self.experiments, 'key', entities.Experiment) + self.experiment_id_map = self._generate_key_map(self.experiments, 'id', entities.Experiment) self.event_key_map = self._generate_key_map(self.events, 'key', entities.Event) self.attribute_key_map = self._generate_key_map(self.attributes, 'key', entities.Attribute) @@ -84,24 +84,24 @@ def __init__(self, datafile, logger, error_handler): self.rollout_id_map = self._generate_key_map(self.rollouts, 'id', entities.Layer) for layer in self.rollout_id_map.values(): for experiment in layer.experiments: - self.experiment_key_map[experiment['key']] = entities.Experiment(**experiment) + self.experiment_id_map[experiment['id']] = entities.Experiment(**experiment) self.audience_id_map = self._deserialize_audience(self.audience_id_map) for group in self.group_id_map.values(): - experiments_in_group_key_map = self._generate_key_map(group.experiments, 'key', entities.Experiment) - for experiment in experiments_in_group_key_map.values(): + experiments_in_group_id_map = self._generate_key_map(group.experiments, 'id', entities.Experiment) + for experiment in experiments_in_group_id_map.values(): experiment.__dict__.update({'groupId': group.id, 'groupPolicy': group.policy}) - self.experiment_key_map.update(experiments_in_group_key_map) + self.experiment_id_map.update(experiments_in_group_id_map) - self.experiment_id_map = {} + self.experiment_key_map = {} self.variation_key_map = {} self.variation_id_map = {} self.variation_variable_usage_map = {} self.variation_id_map_by_experiment_id = {} self.variation_key_map_by_experiment_id = {} - for experiment in self.experiment_key_map.values(): - self.experiment_id_map[experiment.id] = experiment + for experiment in self.experiment_id_map.values(): + self.experiment_key_map[experiment.key] = experiment self.variation_key_map[experiment.key] = self._generate_key_map( experiment.variations, 'key', entities.Variation ) From b6324c90a6da43db59590e591f5f5d3d50932b39 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 15:46:07 -0400 Subject: [PATCH 4/8] Updated with unit tests. --- tests/test_config.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/test_config.py b/tests/test_config.py index c9051054..fe0f8f38 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -41,6 +41,7 @@ def test_init(self): self.config_dict['groups'][0]['trafficAllocation'], ) } + expected_experiment_key_map = { 'test_experiment': entities.Experiment( '111127', @@ -1213,3 +1214,27 @@ def test_is_feature_experiment(self): self.assertStrictFalse(project_config.is_feature_experiment(experiment.id)) self.assertStrictTrue(project_config.is_feature_experiment(feature_experiment.id)) + + def test_get_variation_from_id_by_experiment_id(self): + + opt_obj = optimizely.Optimizely(json.dumps(self.config_dict)) + project_config = opt_obj.config_manager.get_config() + + experiment_id = '111127' + variation_id = '111128' + + variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) + + self.assertIsInstance(variation, entities.Variation) + + def test_get_variation_from_key_by_experiment_id(self): + + opt_obj = optimizely.Optimizely(json.dumps(self.config_dict)) + project_config = opt_obj.config_manager.get_config() + + experiment_id = '111127' + variation_key = 'control' + + variation = project_config.get_variation_from_key_by_experiment_id(experiment_id, variation_key) + + self.assertIsInstance(variation, entities.Variation) From ba99b6fa074d6f1fa7f968d5d58f017bcd8c8663 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 15:52:28 -0400 Subject: [PATCH 5/8] Change line break before operator to avoid W504 Flake8 issue. --- optimizely/project_config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index d6c9d46f..55b497e2 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -590,8 +590,8 @@ def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key): The variation for the experiment id and variation key or empty dict if not found """ - if (experiment_id in self.variation_key_map_by_experiment_id and - variation_key in self.variation_key_map_by_experiment_id[experiment_id]): + if (experiment_id in self.variation_key_map_by_experiment_id + and variation_key in self.variation_key_map_by_experiment_id[experiment_id]): return self.variation_key_map_by_experiment_id[experiment_id][variation_key] self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".', From 4e25c212844cb3dd7b93c68cd349a2d5ac6b7127 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 15:59:19 -0400 Subject: [PATCH 6/8] Flake8 changes to follow best practice and ignore the W504 --- .flake8 | 2 +- optimizely/project_config.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.flake8 b/.flake8 index f31217bf..42b7878d 100644 --- a/.flake8 +++ b/.flake8 @@ -1,5 +1,5 @@ [flake8] # E722 - do not use bare 'except' -ignore = E722 +ignore = E722, W504 exclude = optimizely/lib/pymmh3.py,*virtualenv* max-line-length = 120 diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 55b497e2..8220ced8 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -590,8 +590,8 @@ def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key): The variation for the experiment id and variation key or empty dict if not found """ - if (experiment_id in self.variation_key_map_by_experiment_id - and variation_key in self.variation_key_map_by_experiment_id[experiment_id]): + if (experiment_id in self.variation_key_map_by_experiment_id and + variation_key in self.variation_key_map_by_experiment_id[experiment_id]): return self.variation_key_map_by_experiment_id[experiment_id][variation_key] self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".', From f0c17332bb68907cae1bc09746902929781424d3 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 16:02:21 -0400 Subject: [PATCH 7/8] Remove trailing whitespace. --- optimizely/project_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 8220ced8..d6c9d46f 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -590,7 +590,7 @@ def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key): The variation for the experiment id and variation key or empty dict if not found """ - if (experiment_id in self.variation_key_map_by_experiment_id and + if (experiment_id in self.variation_key_map_by_experiment_id and variation_key in self.variation_key_map_by_experiment_id[experiment_id]): return self.variation_key_map_by_experiment_id[experiment_id][variation_key] From 617cc4d37ce7223fed6f3ada27b7a369af72e623 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Fri, 9 Jul 2021 19:21:40 -0400 Subject: [PATCH 8/8] Changes to address phrasing and comment to explain flake8 ignore W504 --- .flake8 | 3 +++ optimizely/project_config.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.flake8 b/.flake8 index 42b7878d..f5990a83 100644 --- a/.flake8 +++ b/.flake8 @@ -1,5 +1,8 @@ [flake8] # E722 - do not use bare 'except' +# W504 - Either W503 (Line break after Operand) or W503 ( +# Line break before operand needs to be ignored for line lengths +# greater than max-line-length. Best practice shows W504 ignore = E722, W504 exclude = optimizely/lib/pymmh3.py,*virtualenv* max-line-length = 120 diff --git a/optimizely/project_config.py b/optimizely/project_config.py index d6c9d46f..8a696599 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -568,7 +568,7 @@ def is_feature_experiment(self, experiment_id): return experiment_id in self.experiment_feature_map def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id): - """ Gets experiment id and variation id + """ Gets variation from variation id and specific experiment id Returns: The variation for the experiment id and variation id @@ -584,7 +584,7 @@ def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id): return {} def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key): - """ Gets experiment id and variation key + """ Gets variation from variation key and specific experiment id Returns: The variation for the experiment id and variation key