Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

oakbani
Copy link
Contributor

@oakbani oakbani commented Oct 26, 2018

Summary

This updates project config and the audience evaluator to finish the implementation of typed audience evaluation:

  • In audience_evaluator, allow evaluation to continue when no user attributes are passed (previously we were immediately returning false when no attributes were passed)
  • Update project_config to update audience_id_map with typed_audiences
  • Added unit tests checking that all top-level methods that accept user attributes can bucket users into experiments or rollouts that use typed audiences.

Test Plan

  • New & existing unit tests
  • Ran compatibility suite tests for typed audiences

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.608% when pulling ee451fb on oakbani/audience-match-types-project-config into 3c8f6f0 on oakbani/audience-match-type-condition-evaluator-2.

@coveralls
Copy link

coveralls commented Oct 26, 2018

Coverage Status

Coverage increased (+0.001%) to 99.693% when pulling 2ca253d on oakbani/audience-match-types-project-config into 992ce35 on oakbani/audience-match-type-condition-evaluator-2.

… oakbani/audience-match-types-project-config
@@ -174,6 +174,10 @@ def is_finite(self, value):
# numbers.Integral instead of int to accomodate long integer in python 2
return False

if isinstance(value, bool):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this belong in #146?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but this showed up only when I as able to run new audiences compat suite tests with this PR. Will move it into #146.

tests/base.py Outdated
{
'id': '3988293898',
'name': 'substringString',
'conditions': '["and", ["or", ["or", '
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After recent discussions, conditions will not be stringified but instead object (or list of objects and boolean operators)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Let me revise it.

Copy link
Contributor

@nchilada nchilada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to review #146 which this is based on, but this looks pretty good!

# Conditions of audiences in typedAudiences are not expected
# to be string-encoded as they are in audiences.
for typed_audience in self.typed_audiences:
typed_audience['conditions'] = json.dumps(typed_audience['conditions'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda weird that we're calling json.dumps on typedAudiences instead of just calling json.loads on audiences, but it looks like ConditionDecoder is the wrench in the works. Hopefully we can reconsider something like #145 in the future!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: might be clearer to do this during the initial assignment to self.typedAudiences (currently on line 50) instead of mutating self.typedAudiences later. Not a big deal though

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aliabbasrizvi aliabbasrizvi merged commit 88159da into oakbani/audience-match-type-condition-evaluator-2 Dec 18, 2018
@oakbani oakbani deleted the oakbani/audience-match-types-project-config branch December 19, 2018 04:28
@oakbani oakbani restored the oakbani/audience-match-types-project-config branch December 19, 2018 04:35
@oakbani oakbani deleted the oakbani/audience-match-types-project-config branch December 19, 2018 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants