feat (audience match types): Update condition evaluator for new audience match types #146

oakbani · 2018-10-25T09:37:00Z

Summary

This adds support for new audience match type conditions to the condition evaluator:

exact, exists, gt, lt, and substring conditions
Abort and return null when appropriate in leaf evaluators
Null handling in and/or/not evaluators
Assume implicit "or" when operator is missing

Test Plan

Added new unit tests.
Ran compatibility suite tests for typed audiences

oakbani · 2018-10-25T09:39:42Z

tests/base.py

+
+  def assertStrictFalse(self, to_assert):
+    self.assertIs(to_assert, False)
+


assertTrue and assertFalse check for truthy and falsy values.
Added these to ensure that we don't mistake null for false

coveralls · 2018-10-25T09:41:40Z

Coverage increased (+0.02%) to 99.693% when pulling 88159da on oakbani/audience-match-type-condition-evaluator-2 into e5ec118 on oakbani/audience-match-types.

…ience-match-type-condition-evaluator-2

msohailhussain · 2018-11-19T20:53:18Z

.github/pull_request_template.rst

@@ -0,0 +1,15 @@
+Summary


why this changes are coming in this PR? @oakbani

Updated base branch with master

optimizely/helpers/condition.py

aliabbasrizvi · 2018-11-27T20:53:34Z

I think compat tests are now available. You can update the test plan.

aliabbasrizvi

This is sort of a big change. Will try and finish review today.

aliabbasrizvi · 2018-11-27T20:58:25Z

optimizely/helpers/condition.py

+    result = self.evaluate(conditions[0], leaf_evaluator)
+    return None if result is None else not result
+
+  DEFAULT_OPERATOR_TYPES = [


nit. Put these dicts after line 33.

@aliabbasrizvi that might work better if these methods are all declared with @staticmethod.

Hmm, since ConditionTreeEvaluator actually doesn't contain any state, I wonder if we should just break it out into a separate module with top-level functions. As in the JS SDK. This might also be a preferable way to namespace when we add support for other audience condition types, other condition trees (e.g. audience combinations), ...

@oakbani what do you think?

nit. I don't think DEFAULT_... needs to be in the name of this enum.

And arguably we could get rid of this enum in favor of EVALUATORS_BY_OPERATOR_TYPE!

I was wondering if we should create condition as a separate module and keep each of these classes in separate files for better organization. @nchilada Yeah we can make them static.
@aliabbasrizvi What do you suggest? The changes will propagate to project config and combinations work(which has been done). I hope we are not in an urgency to merge this

My preference is to make them separate modules with their own top-level functions. From a code organization standpoint I am ok if all classes stay in this file itself.

nchilada

Nice work @oakbani! Thanks for waiting so long 😬

I've spent too much time in JavaScript-land but this seems like really good Python. I only left minor feedback, the most obnoxious being a potential reorganization to reduce the amount of class-based namespacing and more closely match the JS SDK.

I'll look at the unit tests in my next pass of review!

optimizely/helpers/audience.py

optimizely/helpers/condition.py

nchilada · 2018-11-27T22:59:52Z

optimizely/helpers/condition.py

@@ -159,7 +392,12 @@ def _audience_condition_deserializer(obj_dict):
  Returns:
    List consisting of condition key and corresponding value.


Out of date!

optimizely/helpers/condition.py

nchilada · 2018-11-27T23:17:52Z

optimizely/helpers/condition.py

+    result = self.evaluate(conditions[0], leaf_evaluator)
+    return None if result is None else not result
+
+  DEFAULT_OPERATOR_TYPES = [


@aliabbasrizvi that might work better if these methods are all declared with @staticmethod.

Hmm, since ConditionTreeEvaluator actually doesn't contain any state, I wonder if we should just break it out into a separate module with top-level functions. As in the JS SDK. This might also be a preferable way to namespace when we add support for other audience condition types, other condition trees (e.g. audience combinations), ...

@oakbani what do you think?

nchilada · 2018-11-28T00:47:07Z

optimizely/helpers/condition.py

+
+    # Compare types if one of the values is bool because bool is a subclass on Integer
+    if isinstance(first_val, bool) or isinstance(second_val, bool):
+      return first_val_type == second_val_type


Simpler, I think, and matches the structure of the numeric block:

if isinstance(first_val, bool) and isinstance(second_val, bool): return True

In that case if only 1 of values is a bool type and the other is float or int
it will return true in the following if block as bool is a subclass of int

Oh wow, you're right. So tricky! I haven't looked at the unit tests yet but hopefully we have test cases for this. The compatibility suite can't test the specifics of Python data types

nchilada · 2018-11-28T00:50:32Z

optimizely/helpers/condition.py

+    if isinstance(first_val, string_types) and isinstance(second_val, string_types):
+      return True
+
+    # Compare types if one of the values is bool because bool is a subclass on Integer


Nit: I think the key detail here is that we have to check bool before we check for arbitrary numbers. Could we find a way to mention this in the comment?

nchilada · 2018-11-28T00:59:35Z

optimizely/helpers/condition.py

+
+    return condition_value in user_value
+
+  EVALUATORS_BY_MATCH_TYPE = {


I'm guessing @aliabbasrizvi would want this, too, to be declared above?

I suppose can't declare it before defining evaluator methods

optimizely/helpers/condition.py

aliabbasrizvi

Overall looks good. Left some comments.

aliabbasrizvi · 2018-11-28T16:53:03Z

optimizely/helpers/condition.py

+      Boolean:
+        - True if all operands evaluate to True
+        - False if a single operand evaluates to False
+      None: if conditions couldn't be evaluated


nit. Might be useful to leave a comment here explaining when this would be the case. Thoughts on example @nchilada ?

Not sure. Would definitely get verbose. Perhaps we could just force ourselves to be diligent in documenting the new, collective behavior somewhere else? E.g. in the KB or at the very top of the condition tree module (rather than separately for each operator's evaluator)

aliabbasrizvi · 2018-11-28T16:54:17Z

optimizely/helpers/condition.py

+        - True if any operand evaluates to True
+        - False if all operands evaluate to False
+      None: if conditions couldn't be evaluated
+


nit. Remove this extraneous line.

aliabbasrizvi · 2018-11-28T16:54:36Z

optimizely/helpers/condition.py

+        - True if the operand evaluates to False
+        - False if the operand evaluates to True
+      None: if conditions is empty or condition couldn't be evaluated
+


nit. Remove this extraneous line.

optimizely/helpers/condition.py

aliabbasrizvi · 2018-11-28T17:08:10Z

optimizely/helpers/condition.py

+    result = self.evaluate(conditions[0], leaf_evaluator)
+    return None if result is None else not result
+
+  DEFAULT_OPERATOR_TYPES = [


My preference is to make them separate modules with their own top-level functions. From a code organization standpoint I am ok if all classes stay in this file itself.

aliabbasrizvi · 2018-11-28T17:16:45Z

optimizely/helpers/condition.py

+    condition_value = self.condition_data[index][1]
+    user_value = self.attributes.get(self.condition_data[index][0])
+
+    if not self.is_finite(condition_value) or not self.is_finite(user_value):


If self.is_finite(condition_value) is False, then that suggests a potential problem in the way the audience is set up.
@nchilada do we have log messages to help customers identify the problem?

Yes, not self.is_finite(condition_value) is degenerate (though still worth checking).

We're refining our log messages as a separate initiative...it's looking like we're more concerned about logging problems that customers are actually expected to encounter, but we'll see. Can I add you as a reviewer on the other doc that I'm talking about?

aliabbasrizvi · 2018-11-28T17:17:45Z

optimizely/helpers/condition.py

+
+    return condition_value in user_value
+
+  EVALUATORS_BY_MATCH_TYPE = {


aliabbasrizvi · 2018-11-28T17:22:10Z

tests/helpers_tests/test_condition.py

+    'name': 'browser_type',
+    'value': 'safari',
+    'type': 'custom_attribute',
+  }


nit. This indentation seems off.

optimizely/helpers/condition_tree_evaluator.py

…uator-2

nchilada

Looking good! I'll try to skim through the tests late today.

optimizely/helpers/condition_tree_evaluator.py

optimizely/helpers/condition.py

optimizely/helpers/validator.py

nchilada · 2018-11-29T19:05:23Z

optimizely/helpers/validator.py

+    if math.isnan(value) or math.isinf(value):
+      return False
+
+  return True


Thanks for extracting this! As mentioned on a now-obsolete commit, can we

enforce an absolute size limit of 1e53 if isinstance(value, numbers.Integral)

update helpers.validator.is_attribute_valid such that it calls helpers.validator.is_finite_number

?

And @aliabbasrizvi's addendum:

Random. I am wondering if we should extend this validation i.e. the is_finite check to revenue and quantity values?

+1, is worth considering

Will do this in a separate commit.

Sounds good.

I assume by "separate commit" you mean "separate PR"? That's fine, thanks! Tracking in 3654

nchilada · 2018-11-29T19:07:50Z

optimizely/helpers/condition.py

+
+    # Compare types if one of the values is bool because bool is a subclass on Integer
+    if isinstance(first_val, bool) or isinstance(second_val, bool):
+      return first_val_type == second_val_type


Oh wow, you're right. So tricky! I haven't looked at the unit tests yet but hopefully we have test cases for this. The compatibility suite can't test the specifics of Python data types

nchilada · 2018-11-29T19:15:54Z

optimizely/helpers/condition.py

+      Boolean:
+        - True if all operands evaluate to True
+        - False if a single operand evaluates to False
+      None: if conditions couldn't be evaluated


Not sure. Would definitely get verbose. Perhaps we could just force ourselves to be diligent in documenting the new, collective behavior somewhere else? E.g. in the KB or at the very top of the condition tree module (rather than separately for each operator's evaluator)

nchilada · 2018-11-29T19:17:41Z

optimizely/helpers/condition.py

+    condition_value = self.condition_data[index][1]
+    user_value = self.attributes.get(self.condition_data[index][0])
+
+    if not self.is_finite(condition_value) or not self.is_finite(user_value):


Yes, not self.is_finite(condition_value) is degenerate (though still worth checking).

We're refining our log messages as a separate initiative...it's looking like we're more concerned about logging problems that customers are actually expected to encounter, but we'll see. Can I add you as a reviewer on the other doc that I'm talking about?

optimizely/helpers/condition.py

optimizely/helpers/audience.py

optimizely/helpers/condition_tree_evaluator.py

nchilada

Thanks @oakbani! Code looks good. I just have a couple minor requests for the test cases

nchilada · 2018-11-30T19:12:16Z

tests/helpers_tests/test_condition.py

-class ConditionEvaluatorTests(base.BaseTest):
+exists_condition_list = [['input_value', None, 'custom_attribute', 'exists']]
+exact_string_condition_list = [['favorite_constellation', 'Lacerta', 'custom_attribute', 'exact']]
+exact_number_condition_list = [['lasers_count', 9000, 'custom_attribute', 'exact']]


Can we also have a test for exact number match where the condition value is a float? I see the doubleCondition above, but it looks like that's used for something else

nchilada · 2018-11-30T19:21:03Z

tests/helpers_tests/test_condition.py

+  def test_exact_number__returns_true__when_user_provided_value_is_equal_to_condition_value(self):
+
+    evaluator = condition_helper.CustomAttributeConditionEvaluator(
+      exact_number_condition_list, {'lasers_count': 9000}


Can we also find a way to test with 9000L in the case of Python 2?

Actually...do we run our unit tests in both Python 2 and Python 3?? @aliabbasrizvi

nchilada · 2018-11-30T19:22:48Z

tests/helpers_tests/test_condition.py

+
+    self.assertIsNone(evaluator.evaluate(0))
+
+  def test_exact_number__returns_true__when_user_provided_value_is_equal_to_condition_value(self):


We should have two versions of this:

test_exact_int_returns_true__when_user_provided_value_is_equal_to_condition_value

test_exact_float_returns_true__when_user_provided_value_is_equal_to_condition_value
with the same assertions but different condition value types.

Same for most or all of the other exact_number, greater_than, and less_than tests!

nchilada · 2018-11-30T19:24:14Z

tests/helpers_tests/test_condition.py

+exact_bool_condition_list = [['did_register_user', False, 'custom_attribute', 'exact']]
+substring_condition_list = [['headline_text', 'buy now', 'custom_attribute', 'substring']]
+gt_condition_list = [['meters_travelled', 48.2, 'custom_attribute', 'gt']]
+lt_condition_list = [['meters_travelled', 48.2, 'custom_attribute', 'lt']]


Can we also have int versions of the above two conditions?

nchilada

Stellar work. Thanks @oakbani!

tests/helpers_tests/test_condition.py

Co-Authored-By: oakbani <owais.akbani92@gmail.com>

aliabbasrizvi

Looks good. Requesting some minor changes.

aliabbasrizvi · 2018-12-10T21:47:06Z

optimizely/helpers/condition.py


-class ConditionalOperatorTypes(object):
+from .validator import is_finite_number, are_values_same_type


nit. I personally prefer doing something like:

from . import validator and then call the respective method.

aliabbasrizvi · 2018-12-10T21:47:36Z

optimizely/helpers/condition.py


    Args:
-      condition: Integer representing the index of condition_data that needs to be used for comparison.
+      value: Value to validate


nit. Period (.) at the end of line. Please make sure to do so in all messages in this PR.

aliabbasrizvi · 2018-12-10T21:47:48Z

optimizely/helpers/condition.py


    Returns:
-      Boolean indicating the result of comparing the condition value against the user attributes.
+      Boolean: True if value is a string type, or a boolean, or is finite. Otherwise False


nit. Same as above.

aliabbasrizvi · 2018-12-10T21:49:32Z

optimizely/helpers/condition.py


    Args:
-      conditions: List of conditions ex: [operand_1, operand_2]
+      index: Index of the condition to be evaluated


index is a built in Python method I think. Please rename to something else.

Objects have a special method named __index__, but no global index variable, I don't think. See the global functions reference in 2 and 3.

aliabbasrizvi · 2018-12-10T21:50:00Z

optimizely/helpers/condition.py

+    """ Evaluate the given exists match condition for the user attributes
+
+      Args:
+        index: Index of the condition to be evaluated


Same comment as above. Please update all instances of index in this PR to be something different.

I think this is actually clean after all!

aliabbasrizvi · 2018-12-10T21:54:27Z

tests/test_config.py

@@ -120,13 +120,13 @@ def test_init(self):
        '11154', 'Test attribute users 1',
        '["and", ["or", ["or", {"name": "test_attribute", "type": "custom_attribute", "value": "test_value_1"}]]]',
        conditionStructure=['and', ['or', ['or', 0]]],
-        conditionList=[['test_attribute', 'test_value_1']]
+        conditionList=[['test_attribute', 'test_value_1', 'custom_attribute', None]]


We haven't updated project_config.py code. How is this getting set to None?

We have edited def _audience_condition_deserializer(obj_dict): to add type and match to attribute node so that we use it in CustomAttributeConditionEvaluator - evaluate()
https://github.com/optimizely/python-sdk/pull/146/files#diff-54739e7fb9eb3054932a2f39734e8676R218-R232

…nfig for new audience match types (#147)

…nce match types (#146) (#153)

🖊️ Audience Match Type Condition Evaluator

3c8f6f0

oakbani commented Oct 25, 2018

View reviewed changes

oakbani requested review from mikeproeng37 and nchilada October 25, 2018 09:40

mikeproeng37 requested a review from aliabbasrizvi October 29, 2018 16:59

fix(exact_evaluator): Treat ints and floats as same type

b519b34

aliabbasrizvi mentioned this pull request Nov 15, 2018

feat (audience match types): Update audience evaluator and project config for new audience match types #147

Merged

oakbani added 2 commits November 19, 2018 16:34

fix: detect bool from int class

7a038a9

Merge remote-tracking branch 'remotes/origin/master' into oakbani/aud…

9c8ae5b

…ience-match-type-condition-evaluator-2

msohailhussain reviewed Nov 19, 2018

View reviewed changes

optimizely/helpers/condition.py Outdated Show resolved Hide resolved

tests: boolean check

2262ddc

aliabbasrizvi reviewed Nov 27, 2018

View reviewed changes

nchilada suggested changes Nov 28, 2018

View reviewed changes

aliabbasrizvi reviewed Nov 28, 2018

View reviewed changes

oakbani added 2 commits November 29, 2018 17:35

refact and comments addressed

96088c1

keep operator_types

2a8e931

oakbani commented Nov 29, 2018

View reviewed changes

optimizely/helpers/condition_tree_evaluator.py Outdated Show resolved Hide resolved

Merge branch 'master' into oakbani/audience-match-type-condition-eval…

aa2030c

…uator-2

nchilada reviewed Nov 29, 2018

View reviewed changes

refact and added a new test case

9fcf5af

nchilada suggested changes Nov 30, 2018

View reviewed changes

oakbani added 2 commits December 3, 2018 16:18

tests: Separate test cases for int, float and longs

88fa25d

Workaround - Python 3 syntax error on L suffix

e6bddbb

nchilada approved these changes Dec 3, 2018

View reviewed changes

tests/helpers_tests/test_condition.py Outdated Show resolved Hide resolved

raise NotImplementedError if 'long' called in PY3

992ce35

Co-Authored-By: oakbani <owais.akbani92@gmail.com>

oakbani mentioned this pull request Dec 10, 2018

feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 #151

Merged

aliabbasrizvi suggested changes Dec 10, 2018

View reviewed changes

nits

e9d3b48

yasirfolio3 mentioned this pull request Dec 12, 2018

feat(attribute_value): Don't target NAN, INF, -INF and Doubles > 1e53… optimizely/objective-c-sdk#348

Merged

nchilada mentioned this pull request Dec 18, 2018

Parses revenue value and logger integration #134

Closed

feat (audience match types): Update audience evaluator and project co…

88159da

…nfig for new audience match types (#147)

aliabbasrizvi merged commit 8e596c5 into oakbani/audience-match-types Dec 18, 2018

aliabbasrizvi deleted the oakbani/audience-match-type-condition-evaluator-2 branch December 18, 2018 19:00

msohailhussain mentioned this pull request Dec 18, 2018

feat (audience match types): Condition evaluator and Project Config PRs consolidation. #153

Merged

aliabbasrizvi pushed a commit that referenced this pull request Dec 18, 2018

feat (audience match types): Update condition evaluator for new audie…

abb6723

…nce match types (#146) (#153)

oakbani mentioned this pull request Dec 20, 2018

feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 optimizely/php-sdk#147

Merged

mfahadahmed mentioned this pull request Dec 20, 2018

feat(attribute_value): Don't target NaN, infinity and value greater than 2^53. optimizely/csharp-sdk#119

Closed


		def assertStrictFalse(self, to_assert):
		self.assertIs(to_assert, False)

		@@ -159,7 +392,12 @@ def _audience_condition_deserializer(obj_dict):
		Returns:
		List consisting of condition key and corresponding value.


		return condition_value in user_value

		EVALUATORS_BY_MATCH_TYPE = {


		self.assertIsNone(evaluator.evaluate(0))

		def test_exact_number__returns_true__when_user_provided_value_is_equal_to_condition_value(self):


		class ConditionalOperatorTypes(object):
		from .validator import is_finite_number, are_values_same_type

feat (audience match types): Update condition evaluator for new audience match types #146

feat (audience match types): Update condition evaluator for new audience match types #146

Conversation

oakbani commented Oct 25, 2018 • edited Loading

Summary

Test Plan

Choose a reason for hiding this comment

coveralls commented Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliabbasrizvi commented Nov 27, 2018

aliabbasrizvi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchilada 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

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

Choose a reason for hiding this comment

aliabbasrizvi 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

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

Choose a reason for hiding this comment

nchilada 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

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

nchilada 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchilada left a comment

Choose a reason for hiding this comment

aliabbasrizvi 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

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

Choose a reason for hiding this comment

oakbani commented Oct 25, 2018 •

edited

Loading

coveralls commented Oct 25, 2018 •

edited

Loading