-
Notifications
You must be signed in to change notification settings - Fork 2
Cast provided categorical attributes to strings. Still log and return variation if no actions #59
Changes from all commits
b1313d1
4441e50
e13c9b3
f0b9fd3
13e466a
19e5a28
4b0ef38
80470e8
95356c9
a97e8aa
726d8c5
83ab617
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -256,18 +256,20 @@ def get_bandit_action( | |
| Args: | ||
| flag_key (str): The feature flag key that contains the bandit as one of the variations. | ||
| subject_key (str): The key identifying the subject. | ||
| subject_context (ActionContexts | ActionAttributes): The subject context. | ||
| subject_context (Union[ContextAttributes, Attributes]): The subject context. | ||
| If supplying an ActionAttributes, it gets converted to an ActionContexts instance | ||
| actions (ActionContexts | ActionAttributes): The dictionary that maps action keys | ||
| actions (Union[ActionContexts, ActionAttributes]): The dictionary that maps action keys | ||
| to their context of actions with their contexts. | ||
| If supplying an ActionAttributes, it gets converted to an ActionContexts instance. | ||
| default (str): The default variation to use if the subject is not part of the bandit. | ||
| default (str): The default variation to use if an error is encountered retrieving the | ||
| assigned variation. | ||
|
|
||
| Returns: | ||
| BanditResult: The result containing either the bandit action if the subject is part of the bandit, | ||
| or the assignment if they are not. The BanditResult includes: | ||
| - variation (str): The assignment key indicating the subject's variation. | ||
| - action (str): The key of the selected action if the subject is part of the bandit. | ||
| - action (Optional[str]): The key of the selected action if the subject was assigned one | ||
| by the bandit. | ||
|
|
||
| Example: | ||
| result = client.get_bandit_action( | ||
|
|
@@ -286,102 +288,116 @@ def get_bandit_action( | |
| }, | ||
| "default" | ||
| ) | ||
| if result.action is None: | ||
| do_variation(result.variation) | ||
| if result.action: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is more representative of how we expect users to use our bandits for now (with the two variation template)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||
| do_action(result.variation) | ||
| else: | ||
| do_action(result.action) | ||
| do_status_quo() | ||
| """ | ||
| variation = default | ||
| action = None | ||
| try: | ||
| return self.get_bandit_action_detail( | ||
| subject_attributes = convert_context_attributes_to_attributes( | ||
| subject_context | ||
| ) | ||
|
|
||
| # first, get experiment assignment | ||
| variation = self.get_string_assignment( | ||
| flag_key, | ||
| subject_key, | ||
| subject_context, | ||
| actions, | ||
| subject_attributes, | ||
| default, | ||
| ) | ||
|
|
||
| if variation in self.get_bandit_keys(): | ||
| # next, if assigned a bandit, get the selected action | ||
| action = self.evaluate_bandit_action( | ||
| flag_key, | ||
| variation, # for now, we assume the variation value is always equal to the bandit key | ||
| subject_key, | ||
| subject_context, | ||
| actions, | ||
| ) | ||
| except Exception as e: | ||
| if self.__is_graceful_mode: | ||
| logger.error("[Eppo SDK] Error getting bandit action: " + str(e)) | ||
| return BanditResult(default, None) | ||
| raise e | ||
| else: | ||
| raise e | ||
|
|
||
| return BanditResult(variation, action) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By having this be the only return statement, if we got a variation but hit an error later on evaluating the bandit, we'll still return the variation (to match our logging of it) |
||
|
|
||
| def get_bandit_action_detail( | ||
| def evaluate_bandit_action( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved assignment getting upstream, so now this method's only responsibility is to get and log the selected action |
||
| self, | ||
| flag_key: str, | ||
| bandit_key: str, | ||
| subject_key: str, | ||
| subject_context: Union[ContextAttributes, Attributes], | ||
| actions: Union[ActionContexts, ActionAttributes], | ||
| default: str, | ||
| ) -> BanditResult: | ||
| subject_attributes = convert_subject_context_to_attributes(subject_context) | ||
| action_contexts = convert_actions_to_action_contexts(actions) | ||
| ) -> Union[str, None]: | ||
| # if no actions are given--a valid use case--return the variation with no action | ||
| if len(actions) == 0: | ||
| return None | ||
|
|
||
| # get experiment assignment | ||
| # ignoring type because Dict[str, str] satisfies Dict[str, str | ...] but mypy does not understand | ||
| variation = self.get_string_assignment( | ||
| flag_key, | ||
| subject_key, | ||
| subject_attributes.categorical_attributes | ||
| | subject_attributes.numeric_attributes, # type: ignore | ||
| default, | ||
| ) | ||
|
|
||
| # if the variation is not the bandit key, then the subject is not allocated in the bandit | ||
| if variation not in self.get_bandit_keys(): | ||
| return BanditResult(variation, None) | ||
|
|
||
| # for now, assume that the variation is equal to the bandit key | ||
| bandit_data = self.__config_requestor.get_bandit_model(variation) | ||
| bandit_data = self.__config_requestor.get_bandit_model(bandit_key) | ||
|
|
||
| if not bandit_data: | ||
| logger.warning( | ||
| f"[Eppo SDK] No assigned action. Bandit not found for flag: {flag_key}" | ||
| ) | ||
| return BanditResult(variation, None) | ||
| return None | ||
|
|
||
| subject_context_attributes = convert_attributes_to_context_attributes( | ||
| subject_context | ||
| ) | ||
| action_contexts = convert_actions_to_action_contexts(actions) | ||
|
|
||
| evaluation = self.__bandit_evaluator.evaluate_bandit( | ||
| flag_key, | ||
| subject_key, | ||
| subject_attributes, | ||
| subject_context_attributes, | ||
| action_contexts, | ||
| bandit_data.bandit_model_data, | ||
| ) | ||
|
|
||
| # log bandit action | ||
| bandit_event = { | ||
| "flagKey": flag_key, | ||
| "banditKey": bandit_data.bandit_key, | ||
| "subject": subject_key, | ||
| "action": evaluation.action_key if evaluation else None, | ||
| "actionProbability": evaluation.action_weight if evaluation else None, | ||
| "optimalityGap": evaluation.optimality_gap if evaluation else None, | ||
| "modelVersion": bandit_data.bandit_model_version if evaluation else None, | ||
| "timestamp": datetime.datetime.utcnow().isoformat(), | ||
| "subjectNumericAttributes": ( | ||
| subject_attributes.numeric_attributes | ||
| if evaluation.subject_attributes | ||
| else {} | ||
| ), | ||
| "subjectCategoricalAttributes": ( | ||
| subject_attributes.categorical_attributes | ||
| if evaluation.subject_attributes | ||
| else {} | ||
| ), | ||
| "actionNumericAttributes": ( | ||
| evaluation.action_attributes.numeric_attributes | ||
| if evaluation.action_attributes | ||
| else {} | ||
| ), | ||
| "actionCategoricalAttributes": ( | ||
| evaluation.action_attributes.categorical_attributes | ||
| if evaluation.action_attributes | ||
| else {} | ||
| ), | ||
| "metaData": {"sdkLanguage": "python", "sdkVersion": __version__}, | ||
| } | ||
| self.__assignment_logger.log_bandit_action(bandit_event) | ||
| try: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We didn't have a try-catch surrounding bandit logging, so putting this in place. This way, if the (customer-provided) bandit log method fails, the assignment is still recorded. |
||
| bandit_event = { | ||
| "flagKey": flag_key, | ||
| "banditKey": bandit_data.bandit_key, | ||
| "subject": subject_key, | ||
| "action": evaluation.action_key if evaluation else None, | ||
| "actionProbability": evaluation.action_weight if evaluation else None, | ||
| "optimalityGap": evaluation.optimality_gap if evaluation else None, | ||
| "modelVersion": ( | ||
| bandit_data.bandit_model_version if evaluation else None | ||
| ), | ||
| "timestamp": datetime.datetime.utcnow().isoformat(), | ||
| "subjectNumericAttributes": ( | ||
| subject_context_attributes.numeric_attributes | ||
| if evaluation.subject_attributes | ||
| else {} | ||
| ), | ||
| "subjectCategoricalAttributes": ( | ||
| subject_context_attributes.categorical_attributes | ||
| if evaluation.subject_attributes | ||
| else {} | ||
| ), | ||
| "actionNumericAttributes": ( | ||
| evaluation.action_attributes.numeric_attributes | ||
| if evaluation.action_attributes | ||
| else {} | ||
| ), | ||
| "actionCategoricalAttributes": ( | ||
| evaluation.action_attributes.categorical_attributes | ||
| if evaluation.action_attributes | ||
| else {} | ||
| ), | ||
| "metaData": {"sdkLanguage": "python", "sdkVersion": __version__}, | ||
| } | ||
| self.__assignment_logger.log_bandit_action(bandit_event) | ||
| except Exception as e: | ||
| logger.warn("[Eppo SDK] Error logging bandit event: " + str(e)) | ||
|
|
||
| return BanditResult(variation, evaluation.action_key if evaluation else None) | ||
| return evaluation.action_key | ||
|
|
||
| def get_flag_keys(self): | ||
| """ | ||
|
|
@@ -406,6 +422,9 @@ def get_bandit_keys(self): | |
| """ | ||
| return self.__config_requestor.get_bandit_keys() | ||
|
|
||
| def set_is_graceful_mode(self, is_graceful_mode: bool): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. useful to be able to toggle this outside of client instantiation |
||
| self.__is_graceful_mode = is_graceful_mode | ||
|
|
||
| def is_initialized(self): | ||
| """ | ||
| Returns True if the client has successfully initialized | ||
|
|
@@ -443,18 +462,33 @@ def check_value_type_match( | |
| return False | ||
|
|
||
|
|
||
| def convert_subject_context_to_attributes( | ||
| def convert_context_attributes_to_attributes( | ||
| subject_context: Union[ContextAttributes, Attributes] | ||
| ) -> Attributes: | ||
| if isinstance(subject_context, dict): | ||
| return subject_context | ||
|
|
||
| # ignoring type because Dict[str, str] satisfies Dict[str, str | ...] but mypy does not understand | ||
| return subject_context.numeric_attributes | subject_context.categorical_attributes # type: ignore | ||
|
|
||
|
|
||
| def convert_attributes_to_context_attributes( | ||
| subject_context: Union[ContextAttributes, Attributes] | ||
| ) -> ContextAttributes: | ||
| if isinstance(subject_context, dict): | ||
| return ContextAttributes.from_dict(subject_context) | ||
| return subject_context | ||
|
|
||
| stringified_categorical_attributes = { | ||
| key: str(value) for key, value in subject_context.categorical_attributes.items() | ||
| } | ||
|
Comment on lines
+481
to
+483
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lisaah's original fix, which is to stringify the values of all explicitly passed in categorical attributes so they can be evaluated correctly against the model There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also ensure conversion to string when subject_context is a dictionary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess in that case we don't know what values should be converted, so bugs can still arise but we have no way to fix.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| return ContextAttributes( | ||
| numeric_attributes=subject_context.numeric_attributes, | ||
| categorical_attributes=stringified_categorical_attributes, | ||
| ) | ||
|
|
||
|
|
||
| def convert_actions_to_action_contexts( | ||
| actions: Union[ActionContexts, ActionAttributes] | ||
| ) -> ActionContexts: | ||
| return { | ||
| k: ContextAttributes.from_dict(v) if isinstance(v, dict) else v | ||
| for k, v in actions.items() | ||
| } | ||
| return {k: convert_attributes_to_context_attributes(v) for k, v in actions.items()} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| # Note to developers: When ready to bump to 4.0, please change | ||
| # the `POLL_INTERVAL_SECONDS` constant in `eppo_client/constants.py` | ||
| # to 30 seconds to match the behavior of the other server SDKs. | ||
| __version__ = "3.5.0" | ||
| __version__ = "3.5.1" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,8 @@ | |
| import os | ||
| from time import sleep | ||
| from typing import Dict, List | ||
| from eppo_client.bandit import BanditResult, ContextAttributes | ||
| from unittest.mock import patch | ||
| from eppo_client.bandit import BanditEvaluator, BanditResult, ContextAttributes | ||
|
|
||
| import httpretty # type: ignore | ||
| import pytest | ||
|
|
@@ -81,15 +82,24 @@ def init_fixture(): | |
| httpretty.reset() | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def clear_event_arrays(): | ||
| # Reset graceful mode to off | ||
| get_instance().set_is_graceful_mode(False) | ||
| # Clear captured logger events | ||
| mock_assignment_logger.assignment_events.clear() | ||
| mock_assignment_logger.bandit_events.clear() | ||
|
|
||
|
|
||
| def test_is_initialized(): | ||
| client = get_instance() | ||
| assert client.is_initialized(), "Client should be initialized" | ||
|
|
||
|
|
||
| def test_get_bandit_action_bandit_does_not_exist(): | ||
| def test_get_bandit_action_flag_not_exist(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is really what the test is testing |
||
| client = get_instance() | ||
| result = client.get_bandit_action( | ||
| "nonexistent_bandit", | ||
| "nonexistent_flag", | ||
| "subject_key", | ||
| DEFAULT_SUBJECT_ATTRIBUTES, | ||
| {}, | ||
|
|
@@ -98,12 +108,52 @@ def test_get_bandit_action_bandit_does_not_exist(): | |
| assert result == BanditResult("default_variation", None) | ||
|
|
||
|
|
||
| def test_get_bandit_action_flag_without_bandit(): | ||
| def test_get_bandit_action_flag_has_no_bandit(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. have this use a real flag and get assigned a non-default (but non-bandit) variation |
||
| client = get_instance() | ||
| result = client.get_bandit_action( | ||
| "a_flag", "subject_key", DEFAULT_SUBJECT_ATTRIBUTES, {}, "default_variation" | ||
| "non_bandit_flag", | ||
| "subject_key", | ||
| DEFAULT_SUBJECT_ATTRIBUTES, | ||
| {}, | ||
| "default_variation", | ||
| ) | ||
| assert result == BanditResult("default_variation", None) | ||
| assert result == BanditResult("control", None) | ||
|
|
||
|
|
||
| @patch.object( | ||
| BanditEvaluator, "evaluate_bandit", side_effect=Exception("Mocked Exception") | ||
| ) | ||
| def test_get_bandit_action_bandit_error(mock_bandit_evaluator): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests save lives! (part I)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||
| client = get_instance() | ||
| client.set_is_graceful_mode(True) | ||
| actions = { | ||
| "adidas": ContextAttributes( | ||
| numeric_attributes={"discount": 0.1}, | ||
| categorical_attributes={"from": "germany"}, | ||
| ), | ||
| "nike": ContextAttributes( | ||
| numeric_attributes={"discount": 0.2}, categorical_attributes={"from": "usa"} | ||
| ), | ||
| } | ||
|
|
||
| result = client.get_bandit_action( | ||
| "banner_bandit_flag_uk_only", | ||
| "alice", | ||
| DEFAULT_SUBJECT_ATTRIBUTES, | ||
| actions, | ||
| "default_variation", | ||
| ) | ||
| assert result.variation == "banner_bandit" | ||
| assert result.action is None | ||
|
|
||
| # testing assignment logger | ||
| assignment_log_statement = mock_assignment_logger.assignment_events[-1] | ||
| assert assignment_log_statement["featureFlag"] == "banner_bandit_flag_uk_only" | ||
| assert assignment_log_statement["variation"] == "banner_bandit" | ||
| assert assignment_log_statement["subject"] == "alice" | ||
|
|
||
| # testing bandit logger | ||
| assert len(mock_assignment_logger.bandit_events) == 0 | ||
|
|
||
|
|
||
| def test_get_bandit_action_with_subject_attributes(): | ||
|
|
@@ -163,6 +213,35 @@ def test_get_bandit_action_with_subject_attributes(): | |
| ) | ||
|
|
||
|
|
||
| @patch.object( | ||
| MockAssignmentLogger, "log_bandit_action", side_effect=Exception("Mocked Exception") | ||
| ) | ||
| def test_get_bandit_action_bandit_logger_error(patched_mock_assignment_logger): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests save lives! (part II)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||
| client = get_instance() | ||
| actions = { | ||
| "adidas": ContextAttributes( | ||
| numeric_attributes={"discount": 0.1}, | ||
| categorical_attributes={"from": "germany"}, | ||
| ), | ||
| "nike": ContextAttributes( | ||
| numeric_attributes={"discount": 0.2}, categorical_attributes={"from": "usa"} | ||
| ), | ||
| } | ||
| result = client.get_bandit_action( | ||
| "banner_bandit_flag_uk_only", | ||
| "alice", | ||
| DEFAULT_SUBJECT_ATTRIBUTES, | ||
| actions, | ||
| "default_variation", | ||
| ) | ||
| assert result.variation == "banner_bandit" | ||
| assert result.action in ["adidas", "nike"] | ||
|
|
||
| # assignment should have still been logged | ||
| assert len(mock_assignment_logger.assignment_events) == 1 | ||
| assert len(mock_assignment_logger.bandit_events) == 0 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("test_case", test_data) | ||
| def test_bandit_generic_test_cases(test_case): | ||
| client = get_instance() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind updating the types in the doc block at line 259 for
subject_context? => type should beUnion[ContextAttributes, Attributes]