Merge pull request #83 from reddit/use_RustDecider_choose_all

Use `RustDecider#choose_all()` in `get_all_variants_without_expose()` & `get_all_variants_for_identifier_without_expose()`

Author: mrlevitas

docs/index.rst

@@ -68,7 +68,7 @@ Upgrade or integrate reddit-experiments package:
 .. code-block:: python
 
     # import latest reddit-experiments package in service requirements.txt
-    reddit-experiments>=1.3.9
+    reddit-experiments>=1.3.11
 
 Initialize :code:`decider` instance on Baseplate context
 --------------------------------------------------------
@@ -126,7 +126,7 @@ Make sure :code:`EdgeContext` is accessible on :code:`request` object like so:
     #   - locale
     #   - origin_service
     #   - is_employee
-    #   - loid_created_ms (>=1.3.9)
+    #   - loid_created_ms (>=1.3.11)
 
     # Customized fields can be defined below to be extracted from a baseplate request
     # and will override above edge_context fields.

docs/requirements.txt

@@ -4,7 +4,7 @@ docutils==0.16
 Jinja2==2.11.2
 MarkupSafe==1.1.1
 pydocstyle==5.1.1
-reddit-decider==1.2.15
+reddit-decider==1.2.29
 reddit-edgecontext==1.0.0a3
 Sphinx==3.4.0
 sphinx-autodoc-typehints==1.11.1

reddit_decider/__init__.py

@@ -24,6 +24,7 @@
 from reddit_edgecontext import ValidatedAuthenticationToken
 from rust_decider import Decider as RustDecider
 from rust_decider import DeciderException
+from rust_decider import Decision
 from rust_decider import FeatureNotFoundException
 from rust_decider import make_ctx
 from typing_extensions import Literal
@@ -610,47 +611,40 @@ def get_all_variants_without_expose(self) -> List[Dict[str, Union[str, int]]]:
 
         :return: list of experiment dicts with non-:code:`None` variants.
         """
-        decider = self._get_decider()
-        if decider is None:
-            return []
-
-        ctx = self._get_ctx()
-        ctx_err = ctx.err()
-        if ctx_err is not None:
-            logger.info(f"Encountered error in rust_decider.make_ctx(): {ctx_err}")
+        if self._internal is None:
+            logger.error("rs_decider is None--did not initialize.")
             return []
 
-        all_decisions_result = decider.choose_all(ctx)
+        ctx = self._decider_context.to_dict()
 
-        error = all_decisions_result.err()
-        if error:
-            logger.info(f"Encountered error in decider.choose_all(): {error}")
+        try:
+            all_decisions = self._internal.choose_all(ctx)
+        except DeciderException as exc:
+            logger.info(str(exc))
             return []
 
-        all_decisions = all_decisions_result.decisions()
         parsed_choices = []
 
         event_context_fields = self._decider_context.to_event_dict()
 
-        for exp_name, decision in all_decisions.items():
-            decision_error = decision.err()
-            if decision_error:
-                logger.info(
-                    f"Encountered error for experiment: {exp_name} in decider.choose_all(): {decision_error}"
-                )
-                continue
-
-            decision_dict = decision.decision_dict()
-
-            if decision_dict:
-                parsed_choices.append(self._format_decision(decision_dict))
+        for decision in all_decisions.values():
+            if decision.variant:
+                parsed_choices.append(self._decision_to_dict(decision))
 
             # expose Holdout if the experiment is part of one
-            for event in decision.events():
+            for event in decision.events:
                 self._send_expose_if_holdout(event=event, exposure_fields=event_context_fields)
 
         return parsed_choices
 
+    def _decision_to_dict(self, decision: Decision) -> Dict[str, Any]:
+        return {
+            "name": decision.variant,
+            "id": decision.feature_id,
+            "version": str(decision.feature_version),
+            "experimentName": decision.feature_name,
+        }
+
     def get_all_variants_for_identifier_without_expose(
         self, identifier: str, identifier_type: Literal["user_id", "device_id", "canonical_url"]
     ) -> List[Dict[str, Union[str, int]]]:
@@ -691,48 +685,32 @@ def get_all_variants_for_identifier_without_expose(
             )
             return []
 
-        decider = self._get_decider()
-        if decider is None:
-            return []
-
-        ctx = self._get_ctx_with_set_identifier(
-            identifier=identifier, identifier_type=identifier_type
-        )
-        ctx_err = ctx.err()
-        if ctx_err is not None:
-            logger.info(f"Encountered error in rust_decider.make_ctx(): {ctx_err}")
+        if self._internal is None:
+            logger.error("rs_decider is None--did not initialize.")
             return []
 
-        all_decisions_result = decider.choose_all(ctx=ctx, identifier_type=identifier_type)
+        ctx = self._decider_context.to_dict()
+        ctx[identifier_type] = identifier
 
-        error = all_decisions_result.err()
-        if error:
-            logger.info(f"Encountered error in decider.choose_all(): {error}")
+        try:
+            all_decisions = self._internal.choose_all(
+                context=ctx, bucketing_field_filter=identifier_type
+            )
+        except DeciderException as exc:
+            logger.info(exc)
             return []
 
-        all_decisions = all_decisions_result.decisions()
         parsed_choices = []
 
         event_context_fields = self._decider_context.to_event_dict()
 
-        for exp_name, decision in all_decisions.items():
-            decision_error = decision.err()
-            if decision_error:
-                logger.info(
-                    f"Encountered error for experiment: {exp_name} in decider.choose_all(): {decision_error}"
-                )
-                continue
-
-            decision_dict = decision.decision_dict()
-
-            if decision_dict:
-                parsed_choices.append(self._format_decision(decision_dict))
+        for decision in all_decisions.values():
+            if decision.variant:
+                parsed_choices.append(self._decision_to_dict(decision))
 
             # expose Holdout if the experiment is part of one
-            for event in decision.events():
-                self._send_expose_if_holdout(
-                    event=event, exposure_fields=event_context_fields, overwrite_identifier=True
-                )
+            for event in decision.events:
+                self._send_expose_if_holdout(event=event, exposure_fields=event_context_fields)
 
         return parsed_choices
 

requirements.txt

@@ -1,7 +1,7 @@
 -r requirements-transitive.txt
 baseplate==2.0.0a1
 black==21.4b2
-reddit-decider==1.2.28
+reddit-decider==1.2.29
 flake8==3.9.1
 mypy==0.790
 pyramid==2.0   # required for `from baseplate.frameworks.pyramid import BaseplateRequest` which calls `import pyramid.events`

setup.py

@@ -19,7 +19,7 @@
     install_requires=[
         "baseplate>=2.0.0a1,<3.0",
         "reddit-edgecontext>=1.0.0a3,<2.0",
-        "reddit-decider~=1.2.28",
+        "reddit-decider~=1.2.29",
         "typing_extensions>=3.10.0.0,<5.0",
     ],
     package_data={"reddit_experiments": ["py.typed"]},

tests/decider_tests.py

@@ -972,6 +972,36 @@ def test_get_all_variants_without_expose_with_hg(self):
                 experiment_name="hg", variant="holdout", event_fields=event_fields
             )
 
+    def test_get_all_variants_without_expose_ctx_missing_device_id(self):
+        # no device_id in ctx
+        dc = DeciderContext(user_id=USER_ID)
+
+        self.exp_base_config["exp_1"]["experiment"].update({"bucket_val": "device_id"})
+        self.exp_base_config.update(self.additional_two_exp)
+
+        with create_temp_config_file(self.exp_base_config) as f:
+            decider = setup_decider(f, dc, self.mock_span, self.event_logger)
+
+            self.assertEqual(self.event_logger.log.call_count, 0)
+
+            decision_arr = decider.get_all_variants_without_expose()
+
+            # device_id experiment not bucketed since
+            # device_id is missing in ctx
+            self.assertEqual(len(decision_arr), 2)
+
+            self.assertEqual(
+                first_occurrence_of_key_in(decision_arr, "experimentName", "e1"),
+                {"id": 6, "name": "e1treat", "version": "4", "experimentName": "e1"},
+            )
+            self.assertEqual(
+                first_occurrence_of_key_in(decision_arr, "experimentName", "e2"),
+                {"id": 7, "name": "e2treat", "version": "5", "experimentName": "e2"},
+            )
+
+            # no exposures should be triggered
+            self.assertEqual(self.event_logger.log.call_count, 0)
+
     def test_get_all_variants_for_identifier_without_expose_user_id(self):
         identifier = USER_ID
         bucket_val = "user_id"
@@ -1182,7 +1212,9 @@ def test_get_all_variants_for_identifier_without_expose_canonical_url(self):
         # add 2 more experiments
         self.exp_base_config.update(self.additional_two_exp)
 
-        for exp_name in self.exp_base_config.keys():
+        # update 2 of 3 experiments to have bucket_val: 'canonical_url'
+        # so that the 3rd one is filtered out
+        for exp_name in list(self.exp_base_config.keys())[0:2]:
             self.exp_base_config[exp_name]["experiment"].update({"bucket_val": bucket_val})
 
         with create_temp_config_file(self.exp_base_config) as f:
@@ -1193,7 +1225,8 @@ def test_get_all_variants_for_identifier_without_expose_canonical_url(self):
                 identifier=identifier, identifier_type=bucket_val
             )
 
-            self.assertEqual(len(variant_arr), len(self.exp_base_config))
+            # non-canonical_url experiment is not included in result
+            self.assertEqual(len(variant_arr), 2)
             self.assertEqual(
                 first_occurrence_of_key_in(variant_arr, "experimentName", "exp_1"),
                 {"id": 1, "name": "variant_3", "version": "2", "experimentName": "exp_1"},
@@ -1202,10 +1235,6 @@ def test_get_all_variants_for_identifier_without_expose_canonical_url(self):
                 first_occurrence_of_key_in(variant_arr, "experimentName", "e1"),
                 {"id": 6, "name": "e1treat", "version": "4", "experimentName": "e1"},
             )
-            self.assertEqual(
-                first_occurrence_of_key_in(variant_arr, "experimentName", "e2"),
-                {"id": 7, "name": "e2treat", "version": "5", "experimentName": "e2"},
-            )
 
             # no exposures should be triggered
             self.assertEqual(self.event_logger.log.call_count, 0)