Cast provided categorical attributes to strings. Still log and return variation if no actions by lisaah · Pull Request #59 · Eppo-exp/python-sdk

lisaah · 2024-07-08T13:24:29Z

Fixes: #FF-2573

Motivation and Context

Tests were modified, catching some edge cases.

Handle dynamic casting for categorical attributes
~~We want to return the default value for a bandit flag with no actions~~ Return the assigned variation, but None for action, if there were no actions provided or there was an error evaluating the bandit

Description

Converts provided categorical attributes to strings.
If bandit action selection is unsuccessful, return the variation if it has been successfully computed
Surrounded bandit logging with a try block

How has this been tested?

New unit tests in client_bandit_test.py

typotter

Thanks for continuing to tackle the missing features in this SDK, Lisa.

typotter · 2024-07-08T14:53:13Z

eppo_client/client.py

            )
            return BanditResult(variation, None)

+        # if no actions are given - a valid use case - return the default value


We don't want to compute the flag string assignment (because it will log the flag assignment) and then return the default value if no actions are passed. If no actions are passed and the flag is not associated with bandits, we can compute and return the string assignment.
This requires reading the bandits field of the UFC response.

This comment was correct at the time, but has since changed. See this Slack thread.

I've implemented the new plan, and have re-requesting a review

…ed categorical attributes to strings.

aarsilv · 2024-07-18T23:02:09Z

eppo_client/client.py

        )
-        if result.action is None:
-            do_variation(result.variation)
+        if result.action:


I think this is more representative of how we expect users to use our bandits for now (with the two variation template)

aarsilv · 2024-07-19T00:18:04Z

eppo_client/client.py

                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


Corrected the comment

Would you mind updating the types in the doc block at line 259 for subject_context? => type should be Union[ContextAttributes, Attributes]

aarsilv · 2024-07-19T00:18:15Z

eppo_client/client.py

                          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 (str | null): The key of the selected action if the subject was assigned one


returned actions can be None

Nit: should say None instead of null, or better: Optional[str] to keep it Pythonic

aarsilv · 2024-07-19T00:19:46Z

eppo_client/client.py

+        action = None
        try:
-            return self.get_bandit_action_detail(
+            subject_attributes = convert_context_attributes_to_attributes(subject_context)


get_bandit_action_detail was ensuring the attributes were passed in with context and then disaggregating them back down to Attributes.

Now that we pass these in before we try to ensure context, we want to do the opposite, which is to make sure they are disaggregated.

aarsilv · 2024-07-19T00:21:21Z

eppo_client/client.py

+            else:
+                raise e
+
+        return BanditResult(variation, action)


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)

aarsilv · 2024-07-19T00:24:27Z

eppo_client/client.py

+    stringified_categorical_attributes = {
+        key: str(value) for key, value in subject_context.categorical_attributes.items()
+    }


@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

Should we also ensure conversion to string when subject_context is a dictionary?

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.

ContextAttributes.from_dict() handles this... bucketing all numeric values numeric attributes and then the string values of strings and bools as categorical attributes

aarsilv · 2024-07-19T00:26:09Z

test/client_bandit_test.py



-def test_get_bandit_action_flag_without_bandit():
+def test_get_bandit_action_flag_has_no_bandit():


have this use a real flag and get assigned a non-default (but non-bandit) variation

aarsilv · 2024-07-19T00:26:24Z

test/client_bandit_test.py



-def test_get_bandit_action_bandit_does_not_exist():
+def test_get_bandit_action_flag_not_exist():


this is really what the test is testing

aarsilv · 2024-07-19T00:36:31Z

test/client_bandit_test.py

+    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):


Tests save lives! (part I)

aarsilv · 2024-07-19T00:36:39Z

test/client_bandit_test.py

    )

+@patch.object(MockAssignmentLogger, 'log_bandit_action', side_effect=Exception("Mocked Exception"))
+def test_get_bandit_action_bandit_logger_error(patched_mock_assignment_logger):


Tests save lives! (part II)

💯
feel free to file issues against the other SDKs to ensure we've got protection around logging in each.

schmit

Thanks for the changes, this looks good to me. I don't feel great that we can have 'null' actions after working so hard to removing the null variation in the UFC, but I also don't know how to better handle the empty actions list case.

schmit · 2024-07-19T04:45:16Z

eppo_client/client.py

                          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 (str | null): The key of the selected action if the subject was assigned one


Nit: should say None instead of null, or better: Optional[str] to keep it Pythonic

schmit · 2024-07-19T04:52:59Z

eppo_client/client.py

+    stringified_categorical_attributes = {
+        key: str(value) for key, value in subject_context.categorical_attributes.items()
+    }


Should we also ensure conversion to string when subject_context is a dictionary?

schmit · 2024-07-19T04:53:26Z

eppo_client/client.py

+    stringified_categorical_attributes = {
+        key: str(value) for key, value in subject_context.categorical_attributes.items()
+    }


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.

typotter · 2024-07-24T12:39:58Z

eppo_client/client.py

        )
-        if result.action is None:
-            do_variation(result.variation)
+        if result.action:


typotter · 2024-07-24T12:47:55Z

eppo_client/client.py

                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


Would you mind updating the types in the doc block at line 259 for subject_context? => type should be Union[ContextAttributes, Attributes]

typotter · 2024-07-24T12:50:35Z

test/client_bandit_test.py

+    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):


typotter · 2024-07-24T12:51:18Z

test/client_bandit_test.py

    )

+@patch.object(MockAssignmentLogger, 'log_bandit_action', side_effect=Exception("Mocked Exception"))
+def test_get_bandit_action_bandit_logger_error(patched_mock_assignment_logger):


💯
feel free to file issues against the other SDKs to ensure we've got protection around logging in each.

typotter · 2024-07-24T12:51:30Z

test/client_bandit_test.py

    )

+@patch.object(MockAssignmentLogger, 'log_bandit_action', side_effect=Exception("Mocked Exception"))
+def test_get_bandit_action_bandit_logger_error(patched_mock_assignment_logger):


💯
feel free to file issues against the other SDKs to ensure we've got protection around logging in each.

lisaah requested review from aarsilv, giorgiomartini0 and tbuffington7 July 8, 2024 13:24

typotter suggested changes Jul 8, 2024

View reviewed changes

lisaah added 2 commits July 16, 2024 10:27

Return the default value for a bandit flag if no actions. Cast provid…

b1313d1

…ed categorical attributes to strings.

Formatting

4441e50

aarsilv force-pushed the lisa/ff-2573-python-sdk-do-not-evaluate-string-assignment-or-bandit-if branch from ea4c247 to 4441e50 Compare July 18, 2024 22:52

update for revisted zero-action strategy

e13c9b3

aarsilv changed the title ~~Return the default value for a bandit flag if no actions. Cast provided categorical attributes to strings.~~ Cast provided categorical attributes to strings. Still log and return variation if no actions Jul 18, 2024

aarsilv added 3 commits July 18, 2024 18:17

return variation if that has been computed

f0b9fd3

changes from self-review of PR

13e466a

test for bandit logger error

19e5a28

aarsilv reviewed Jul 19, 2024

View reviewed changes

aarsilv requested review from schmit and typotter July 19, 2024 00:36

aarsilv assigned schmit, giorgiomartini0 and typotter Jul 19, 2024

schmit approved these changes Jul 19, 2024

View reviewed changes

schmit removed their assignment Jul 19, 2024

aarsilv added 5 commits July 19, 2024 17:27

fix flake8 lintint errors

4b0ef38

black formatter

80470e8

return type ignore hint

95356c9

fix type hint

a97e8aa

feedback from PR

726d8c5

typotter approved these changes Jul 24, 2024

View reviewed changes

update type in comment per PR suggestion

83ab617

aarsilv merged commit 70595d1 into main Jul 24, 2024

aarsilv deleted the lisa/ff-2573-python-sdk-do-not-evaluate-string-assignment-or-bandit-if branch July 24, 2024 15:12



		def test_get_bandit_action_flag_without_bandit():
		def test_get_bandit_action_flag_has_no_bandit():



		def test_get_bandit_action_bandit_does_not_exist():
		def test_get_bandit_action_flag_not_exist():

Conversation

lisaah commented Jul 8, 2024 • edited by aarsilv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How has this been tested?

Uh oh!

typotter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lisaah commented Jul 8, 2024 •

edited by aarsilv

Loading