Refact: Refactored Forced decision #365

msohailhussain · 2021-11-29T20:07:55Z

Summary

Removed get_variation_from_experiment_rule and get_variation_from_delivery_rule from decision service.
flag_variations_map logic improved using existing loops and removed get_all_variations_for_each_rule.
Unit test fixed.

Test plan

All FSC and unit tests should pass

msohailhussain · 2021-11-29T21:00:41Z

optimizely/project_config.py

@@ -100,6 +100,7 @@ def __init__(self, datafile, logger, error_handler):
        self.variation_variable_usage_map = {}
        self.variation_id_map_by_experiment_id = {}
        self.variation_key_map_by_experiment_id = {}
+        self.flag_variations_map = {}


holding this PR for now...

msohailhussain · 2021-11-29T21:01:16Z

optimizely/project_config.py

@@ -215,6 +195,28 @@ def get_rollout_experiments(self, rollout):

        return rollout_experiments

+    def get_rules_for_flag(self, feature_flag):


doesn't make sense to add this method.

jaeopt

One change suggested

jaeopt · 2021-11-29T23:44:33Z

optimizely/project_config.py

+        if rollout:
+            rollout_experiments = rollout.experiments
+            for exp in rollout_experiments:
+                rules.append(self.experiment_id_map[exp['id']])


I understand experiment_id_map is for ab-tests only, not including rollout rules. Did it pass our tests?

All unit tests and FSC is passing.

jaeopt · 2021-11-29T23:51:38Z

optimizely/project_config.py

@@ -120,46 +121,25 @@ def __init__(self, datafile, logger, error_handler):
                )

        self.feature_key_map = self._generate_key_map(self.feature_flags, 'key', entities.FeatureFlag)
-
+        self.flag_variations_map = self.generate_feature_variation_map(self.feature_flags)


Remove line 103

coveralls · 2021-12-01T13:05:27Z

Coverage decreased (-0.2%) to 95.879% when pulling 39ab0b0 on uzair/project-config-refactor into ab40d9e on mpirnovar/forced_decisions.

…le removed

The-inside-man · 2021-12-01T14:18:07Z

optimizely/decision_service.py


+                if experiment:
+                    # variation, variation_reasons = self.get_variation_from_experiment_rule(


Should we remove commented out code?

mnoman09

Added some comments overall looks good.

mnoman09 · 2021-12-01T14:12:37Z

optimizely/decision_service.py


+                if experiment:
+                    # variation, variation_reasons = self.get_variation_from_experiment_rule(


Remove this commented code

I was testing this code with travis. Will remove it on finalization.

mnoman09 · 2021-12-01T14:16:52Z

optimizely/project_config.py

-
-                if rollout and rollout.experiments:
-                    experiments.extend(rollout_experiments)
+        # Dict containing map of experiment ID to feature ID.


Suggested change

# Dict containing map of experiment ID to feature ID.

# Dictionary containing dictionary of experiment ID to feature ID.

mnoman09 · 2021-12-01T14:21:11Z

optimizely/project_config.py

+        # Dict containing map of experiment ID to feature ID.
+        # for checking that experiment is a feature experiment or not.
+        self.experiment_feature_map = {}
+        for feature in self.feature_key_map.values():


Why are we running this loop again see line 127 can we reuse it?

jaeopt

LGTM

The-inside-man

All lgtm, seems a few tests are failing in CI for pypy3, even after rerunning several times. I dont see this as a concern right now and can be addressed later as this seems to be a common issue with these versions of Python.

msohailhussain · 2021-12-02T19:11:48Z

All lgtm, seems a few tests are failing in CI for pypy3, even after rerunning several times. I dont see this as a concern right now and can be addressed later as this seems to be a common issue with these versions of Python.

true.

* add maps to project config * initial code * feat: add remaining implementation * WIP: addressed implementation PR comments and fixed failing unit tests * Fixed lint errors * fix failing tests in py 3.5 * fixed failing logger import for Py2 * add OptimizelyDecisionContext and OptmizelyForcedDecisions * testcases added * Update optimizely/optimizely_user_context.py Co-authored-by: ozayr-zaviar <54209343+ozayr-zaviar@users.noreply.github.com> * Update optimizely/optimizely_user_context.py Co-authored-by: ozayr-zaviar <54209343+ozayr-zaviar@users.noreply.github.com> * Update optimizely/optimizely_user_context.py Co-authored-by: ozayr-zaviar <54209343+ozayr-zaviar@users.noreply.github.com> * make rule key optional in OptimizelyDecisionContext * Mutex lock and testcases added * Update optimizely/optimizely_user_context.py Co-authored-by: ozayr-zaviar <54209343+ozayr-zaviar@users.noreply.github.com> * use get() vs [] in remove_forced_decision * add missing colon * fix displaying reasons * Update optimizely/optimizely.py Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com> * address PR comments * more PR review fixes * fixed few more PR comments * added bucket reasons * FSC fixes * addressed more PR comments, fixed FSC test failuer about impressin events * address more PR comments * use is_valid check on opti client * addressed more PR comments * reasons and key name fixed * create get_default method for empty experiment object * fixed further PR comments * fix logger so we use the top logger in optimizely client * Refact: Refactored Forced decision (#365) * project config refactor * use existing loop to generate flag_variation_map * get_variation_from_experiment_rule and get_variation_from_delivery_rule removed * fsc test fix * comment addressed * commented code removed * comments from main forced decision PR resolved Co-authored-by: ozayr-zaviar <uzairzaviar@gmail.com> * coupl of corrections * remove check on config * remove redundant import * remove redundant test about invalid datafile * add reasons to return Co-authored-by: ozayr-zaviar <uzairzaviar@gmail.com> Co-authored-by: ozayr-zaviar <54209343+ozayr-zaviar@users.noreply.github.com> Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com> Co-authored-by: msohailhussain <mirza.sohailhussain@gmail.com>

project config refactor

a9f6b12

msohailhussain requested a review from a team as a code owner November 29, 2021 20:07

optimizely-codereview-bot assigned msohailhussain Nov 29, 2021

msohailhussain commented Nov 29, 2021

View reviewed changes

jaeopt requested changes Nov 29, 2021

View reviewed changes

use existing loop to generate flag_variation_map

3955db0

get_variation_from_experiment_rule and get_variation_from_delivery_ru…

6768457

…le removed

The-inside-man reviewed Dec 1, 2021

View reviewed changes

mnoman09 suggested changes Dec 1, 2021

View reviewed changes

ozayr-zaviar added 3 commits December 1, 2021 19:53

fsc test fix

981ab70

comment addressed

dcf0a48

commented code removed

ed543fa

ozayr-zaviar changed the title ~~project config refactor~~ Refact: Refactored Forced decision Dec 2, 2021

ozayr-zaviar mentioned this pull request Dec 2, 2021

Add forced-decisions APIs to OptimizelyUserContext #361

Merged

comments from main forced decision PR resolved

39ab0b0

jaeopt approved these changes Dec 2, 2021

View reviewed changes

The-inside-man approved these changes Dec 2, 2021

View reviewed changes

msohailhussain merged commit dbbc051 into mpirnovar/forced_decisions Dec 2, 2021

msohailhussain deleted the uzair/project-config-refactor branch December 2, 2021 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refact: Refactored Forced decision #365

Refact: Refactored Forced decision #365

msohailhussain commented Nov 29, 2021 •

edited by ozayr-zaviar

Loading

msohailhussain Nov 29, 2021

msohailhussain Nov 29, 2021

jaeopt left a comment

jaeopt Nov 29, 2021

ozayr-zaviar Dec 2, 2021

jaeopt Nov 29, 2021

coveralls commented Dec 1, 2021 •

edited

Loading

The-inside-man Dec 1, 2021

mnoman09 left a comment •

edited

Loading

mnoman09 Dec 1, 2021

ozayr-zaviar Dec 1, 2021

mnoman09 Dec 1, 2021

mnoman09 Dec 1, 2021

jaeopt left a comment

The-inside-man left a comment

msohailhussain commented Dec 2, 2021

		@@ -215,6 +195,28 @@ def get_rollout_experiments(self, rollout):

		return rollout_experiments

		def get_rules_for_flag(self, feature_flag):


		if experiment:
		# variation, variation_reasons = self.get_variation_from_experiment_rule(

	# Dict containing map of experiment ID to feature ID.
	# Dictionary containing dictionary of experiment ID to feature ID.

Refact: Refactored Forced decision #365

Refact: Refactored Forced decision #365

Conversation

msohailhussain commented Nov 29, 2021 • edited by ozayr-zaviar Loading

Summary

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

mnoman09 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

The-inside-man left a comment

Choose a reason for hiding this comment

msohailhussain commented Dec 2, 2021

msohailhussain commented Nov 29, 2021 •

edited by ozayr-zaviar

Loading

coveralls commented Dec 1, 2021 •

edited

Loading

mnoman09 left a comment •

edited

Loading