-
Notifications
You must be signed in to change notification settings - Fork 2
[ff-2397] Allow attributes to be specified as dicts #55
Changes from all commits
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 |
|---|---|---|
| @@ -1,17 +1,23 @@ | ||
| import datetime | ||
| import logging | ||
| import json | ||
| from typing import Any, Dict, Optional | ||
| from typing import Any, Dict, Optional, Union | ||
| from eppo_client.assignment_logger import AssignmentLogger | ||
| from eppo_client.bandit import BanditEvaluator, BanditResult, Attributes, ActionContexts | ||
| from eppo_client.bandit import ( | ||
| ActionContextsDict, | ||
| BanditEvaluator, | ||
| BanditResult, | ||
| Attributes, | ||
| ActionContexts, | ||
| ) | ||
| from eppo_client.configuration_requestor import ( | ||
| ExperimentConfigurationRequestor, | ||
| ) | ||
| from eppo_client.constants import POLL_INTERVAL_MILLIS, POLL_JITTER_MILLIS | ||
| from eppo_client.models import VariationType | ||
| from eppo_client.poller import Poller | ||
| from eppo_client.sharders import MD5Sharder | ||
| from eppo_client.types import SubjectAttributes, ValueType | ||
| from eppo_client.types import AttributesDict, ValueType | ||
| from eppo_client.validation import validate_not_blank | ||
| from eppo_client.eval import FlagEvaluation, Evaluator, none_result | ||
| from eppo_client.version import __version__ | ||
|
|
@@ -43,7 +49,7 @@ def get_string_assignment( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_attributes: SubjectAttributes, | ||
| subject_attributes: AttributesDict, | ||
| default: str, | ||
| ) -> str: | ||
| return self.get_assignment_variation( | ||
|
|
@@ -58,7 +64,7 @@ def get_integer_assignment( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_attributes: SubjectAttributes, | ||
| subject_attributes: AttributesDict, | ||
| default: int, | ||
| ) -> int: | ||
| return self.get_assignment_variation( | ||
|
|
@@ -73,7 +79,7 @@ def get_numeric_assignment( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_attributes: SubjectAttributes, | ||
| subject_attributes: AttributesDict, | ||
| default: float, | ||
| ) -> float: | ||
| # convert to float in case we get an int | ||
|
|
@@ -91,7 +97,7 @@ def get_boolean_assignment( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_attributes: SubjectAttributes, | ||
| subject_attributes: AttributesDict, | ||
| default: bool, | ||
| ) -> bool: | ||
| return self.get_assignment_variation( | ||
|
|
@@ -106,7 +112,7 @@ def get_json_assignment( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_attributes: SubjectAttributes, | ||
| subject_attributes: AttributesDict, | ||
| default: Dict[Any, Any], | ||
| ) -> Dict[Any, Any]: | ||
| json_value = self.get_assignment_variation( | ||
|
|
@@ -125,7 +131,7 @@ def get_assignment_variation( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_attributes: SubjectAttributes, | ||
| subject_attributes: AttributesDict, | ||
| default: Optional[ValueType], | ||
| expected_variation_type: VariationType, | ||
| ): | ||
|
|
@@ -149,7 +155,7 @@ def get_assignment_detail( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_attributes: SubjectAttributes, | ||
| subject_attributes: AttributesDict, | ||
| expected_variation_type: VariationType, | ||
| ) -> FlagEvaluation: | ||
| """Maps a subject to a variation for a given flag | ||
|
|
@@ -225,8 +231,8 @@ def get_bandit_action( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_context: Attributes, | ||
| actions: ActionContexts, | ||
| subject_context: Union[Attributes, AttributesDict], | ||
| actions: Union[ActionContexts, ActionContextsDict], | ||
| default: str, | ||
| ) -> BanditResult: | ||
| """ | ||
|
|
@@ -244,9 +250,11 @@ 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 (Attributes): The subject context | ||
| actions (Dict[str, Attributes]): The dictionary that maps action keys | ||
| subject_context (Attributes | AttributesDict): The subject context. | ||
| If supplying an AttributesDict, it gets converted to an Attributes instance | ||
| actions (ActionContexts | ActionContextsDict): The dictionary that maps action keys | ||
| to their context of actions with their contexts. | ||
| If supplying an AttributesDict, it gets converted to an Attributes instance. | ||
| default (str): The default variation to use if the subject is not part of the bandit. | ||
|
|
||
| Returns: | ||
|
|
@@ -264,7 +272,8 @@ def get_bandit_action( | |
| categorical_attributes={"country": "USA"}), | ||
| { | ||
| "action1": Attributes(numeric_attributes={"price": 10.0}, categorical_attributes={"category": "A"}), | ||
| "action2": Attributes.empty() | ||
| "action2": {"price": 10.0, "category": "B"} | ||
| "action3": Attributes.empty(), | ||
| }, | ||
| "default" | ||
| ) | ||
|
|
@@ -273,7 +282,6 @@ def get_bandit_action( | |
| else: | ||
| do_action(result.action) | ||
| """ | ||
|
|
||
| try: | ||
| return self.get_bandit_action_detail( | ||
| flag_key, | ||
|
|
@@ -292,14 +300,21 @@ def get_bandit_action_detail( | |
| self, | ||
| flag_key: str, | ||
| subject_key: str, | ||
| subject_context: Attributes, | ||
| actions: ActionContexts, | ||
| subject_context: Union[Attributes, AttributesDict], | ||
| actions: Union[ActionContexts, ActionContextsDict], | ||
| default: str, | ||
| ) -> BanditResult: | ||
| subject_attributes = convert_subject_context_to_attributes(subject_context) | ||
| action_contexts = convert_actions_to_action_contexts(actions) | ||
|
|
||
| # 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_context.categorical_attributes, default # type: ignore | ||
| flag_key, | ||
| subject_key, | ||
| subject_attributes.categorical_attributes | ||
| | subject_attributes.numeric_attributes, # type: ignore | ||
|
Comment on lines
+315
to
+316
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. minor: one thing that still bothers me is that: subject_attributes.categorical_attributes | subject_attributes.numeric_attributes != subject_contextIt's just seems very hacky and very easy to shoot oneself in the foot with this, as it may introduce subtle evaluation differences. (e.g., As one example, attributes that are not one of
Author
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. Thanks, this is a great comment!
But the attribute type covers all the accepted cases: Perhaps the problem is that we are not dropping none [str,int,float,bool] values when evaluating
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. The issue is that Python's typing is optional, so users are free to pass any type as attribute. It won't give any error and will be handled by applying So maybe another solution is to be more strict in what attributes we accept. i.e., add a runtime check for attribute type, error or filter out unexpected types. The issue is also slightly exaggerated for languages that don't have easy union types (e.g., Go accept any value for attributes). Maybe we should handle that in the respective language's implementation but it would be great for the python to set an example here, so this is not copied blindly
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. As another inconsistency:
Author
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. Not so worried about losing precision -- the loss in precision is orders of magnitude less than the statistical uncertainty in the bandit model |
||
| default, | ||
| ) | ||
|
|
||
| # if the variation is not the bandit key, then the subject is not allocated in the bandit | ||
|
|
@@ -318,8 +333,8 @@ def get_bandit_action_detail( | |
| evaluation = self.__bandit_evaluator.evaluate_bandit( | ||
| flag_key, | ||
| subject_key, | ||
| subject_context, | ||
| actions, | ||
| subject_attributes, | ||
| action_contexts, | ||
| bandit_data.model_data, | ||
| ) | ||
|
|
||
|
|
@@ -334,12 +349,12 @@ def get_bandit_action_detail( | |
| "modelVersion": bandit_data.model_version if evaluation else None, | ||
| "timestamp": datetime.datetime.utcnow().isoformat(), | ||
| "subjectNumericAttributes": ( | ||
| subject_context.numeric_attributes | ||
| subject_attributes.numeric_attributes | ||
| if evaluation.subject_attributes | ||
| else {} | ||
| ), | ||
| "subjectCategoricalAttributes": ( | ||
| subject_context.categorical_attributes | ||
| subject_attributes.categorical_attributes | ||
| if evaluation.subject_attributes | ||
| else {} | ||
| ), | ||
|
|
@@ -410,3 +425,20 @@ def check_value_type_match( | |
| if expected_type == VariationType.BOOLEAN: | ||
| return isinstance(value, bool) | ||
| return False | ||
|
|
||
|
|
||
| def convert_subject_context_to_attributes( | ||
| subject_context: Union[Attributes, AttributesDict] | ||
| ) -> Attributes: | ||
| if isinstance(subject_context, dict): | ||
| return Attributes.from_dict(subject_context) | ||
| return subject_context | ||
|
|
||
|
|
||
| def convert_actions_to_action_contexts( | ||
| actions: Union[ActionContexts, ActionContextsDict] | ||
| ) -> ActionContexts: | ||
| return { | ||
| k: Attributes.from_dict(v) if isinstance(v, dict) else v | ||
| for k, v in actions.items() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,5 +3,5 @@ | |
| ValueType = Union[str, int, float, bool] | ||
| AttributeType = Union[str, int, float, bool, None] | ||
| ConditionValueType = Union[AttributeType, List[AttributeType]] | ||
| SubjectAttributes = Dict[str, AttributeType] | ||
| AttributesDict = Dict[str, AttributeType] | ||
|
Author
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. Renamed this to re-use the type
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. Nice, I did the same in Java calling it "EppoAttributes" |
||
| Action = str | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ def init_fixture(): | |
| base_url=MOCK_BASE_URL, | ||
| api_key="dummy", | ||
| assignment_logger=mock_assignment_logger, | ||
| is_graceful_mode=False, | ||
|
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. oh yeah good catch! |
||
| ) | ||
| ) | ||
| sleep(0.1) # wait for initialization | ||
|
|
@@ -91,7 +92,7 @@ def test_get_bandit_action_bandit_does_not_exist(): | |
| "nonexistent_bandit", | ||
| "subject_key", | ||
| DEFAULT_SUBJECT_ATTRIBUTES, | ||
| [], | ||
|
Author
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. Did not catch that this test was broken because |
||
| {}, | ||
| "default_variation", | ||
| ) | ||
| assert result == BanditResult("default_variation", None) | ||
|
|
@@ -100,7 +101,7 @@ def test_get_bandit_action_bandit_does_not_exist(): | |
| def test_get_bandit_action_flag_without_bandit(): | ||
| client = get_instance() | ||
| result = client.get_bandit_action( | ||
| "a_flag", "subject_key", DEFAULT_SUBJECT_ATTRIBUTES, [], "default_variation" | ||
| "a_flag", "subject_key", DEFAULT_SUBJECT_ATTRIBUTES, {}, "default_variation" | ||
|
Author
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. Did not catch that this test was broken because |
||
| ) | ||
| assert result == BanditResult("default_variation", None) | ||
|
|
||
|
|
||
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.
Thanks for helping reduce friction for developers!