[ff-2397] Allow attributes to be specified as dicts#55
Conversation
| "nonexistent_bandit", | ||
| "subject_key", | ||
| DEFAULT_SUBJECT_ATTRIBUTES, | ||
| [], |
There was a problem hiding this comment.
Did not catch that this test was broken because is_graceful_mode was set to True
| 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" |
There was a problem hiding this comment.
Did not catch that this test was broken because is_graceful_mode was set to True
| AttributeType = Union[str, int, float, bool, None] | ||
| ConditionValueType = Union[AttributeType, List[AttributeType]] | ||
| SubjectAttributes = Dict[str, AttributeType] | ||
| AttributesDict = Dict[str, AttributeType] |
There was a problem hiding this comment.
Nice, I did the same in Java calling it "EppoAttributes"
| subject_attributes = convert_subject_context_to_attributes(subject_context) | ||
| action_contexts = convert_actions_to_action_contexts(actions) |
There was a problem hiding this comment.
Thanks for helping reduce friction for developers!
| AttributeType = Union[str, int, float, bool, None] | ||
| ConditionValueType = Union[AttributeType, List[AttributeType]] | ||
| SubjectAttributes = Dict[str, AttributeType] | ||
| AttributesDict = Dict[str, AttributeType] |
There was a problem hiding this comment.
Nice, I did the same in Java calling it "EppoAttributes"
| base_url=MOCK_BASE_URL, | ||
| api_key="dummy", | ||
| assignment_logger=mock_assignment_logger, | ||
| is_graceful_mode=False, |
eppo_client/version.py
Outdated
| @@ -1 +1 @@ | |||
| __version__ = "3.2.0" | |||
| __version__ = "3.2.1" | |||
There was a problem hiding this comment.
nit: 3.2.0 seems to be non-released yet so there's no reason to bump? (If it were released, the change here should be a minor bump, not patch.)
| __version__ = "3.2.1" | |
| __version__ = "3.2.0" |
There was a problem hiding this comment.
Good catch, didn't realize I forgot to release 3.2.0 -- will undo the version bump and add this to the 3.2.0 release
| subject_attributes.categorical_attributes | ||
| | subject_attributes.numeric_attributes, # type: ignore |
There was a problem hiding this comment.
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., get_string_assignment() returns a different value from here when called by the user).
As one example, attributes that are not one of [str,int,float,bool] are silently dropped for bandits. (In normal flag evaluation, they are convertible to string.)
- We can cover this with a lot of tests
- Or maybe refactor the code so that we perform similar transformation (float-promotion and converting to string) on all flag evaluation before attributes enter rules evaluation—so that flag evaluation engine only deals with floats and strings.
- Alternatively, we can make
get_string_assignment()acceptUnion[Attributes, AttributesDict]as well—this way we can pass user-passed attributes unmodified to flag evaluation. - At the very least, this deserves a caution comment here.
There was a problem hiding this comment.
Thanks, this is a great comment!
As one example, attributes that are not one of [str,int,float,bool] are silently dropped for bandits
But the attribute type covers all the accepted cases: AttributeType = Union[str, int, float, bool, None]; Yes None gets dropped but it indicates absence anyway and would evaluate the same way whether we set the value to None or remove the key, value pair altogether.
Perhaps the problem is that we are not dropping none [str,int,float,bool] values when evaluating get_string_assignment?
There was a problem hiding this comment.
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 json.dumps.
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
There was a problem hiding this comment.
As another inconsistency: ints may loose precision.
There was a problem hiding this comment.
Not so worried about losing precision -- the loss in precision is orders of magnitude less than the statistical uncertainty in the bandit model
|
lgtm 👍 |
schmit
force-pushed
the
ff-2397/attributes-in-dict
branch
from
51c4aaa to
fda0003
Compare
5 participants
Fixes: #ff-2397
Motivation and Context
Currently, users need to specify numerical and categorical attributes explicitly. This adds an option to supply a dictionary and we do the conversion to numerical and categorical attributes within the SDK based on value types
Description
How has this been tested?
All tests pass