FF-1677 Python SDK UFC update by schmit · Pull Request #28 · Eppo-exp/python-sdk

schmit · 2024-03-05T01:37:14Z

Fixes: #issue

Motivation and Context

Migrating the Python SDK to the UFC to enable more flexible feature flag evaluation and advanced use cases.

Description

Leverage UFC to evaluate flags
Create explicit integer and float variation types

How has this been tested?

Updated unit tests
Updated cross-SDK UFC test cases

test/rules_test.py

eppo_client/client.py

schmit · 2024-03-05T01:43:18Z

eppo_client/eval.py

+    sharder: Sharder
+
+    def evaluate_flag(
+        self, flag, subject_key, subject_attributes


Also decided to pass along subject_key so we can include it in rule evaluation if we want (so you don't also have to add id to subject attributes), which has been a pet-peeve. We could just inject "id": subject_key into subject_attributes somewhere

Nice quality of life improvement!

giorgiomartini0

Love how fast this is coming together! Partial review for now

eppo_client/client.py

eppo_client/eval.py

giorgiomartini0 · 2024-03-05T17:02:26Z

eppo_client/eval.py

+    sharder: Sharder
+
+    def evaluate_flag(
+        self, flag, subject_key, subject_attributes


Nice quality of life improvement!

eppo_client/eval.py

giorgiomartini0

A few more comments - haven't started on the tests yet.

giorgiomartini0 · 2024-03-05T22:14:27Z

eppo_client/eval.py

+        allocation_key=None,
+        variation=None,
+        extra_logging={},
+        do_log=False,


Thinking out loud: the more we see the none/default outcome as an error/abnormal, the more I'd like to log it:

when a flag doesn't exist in the RAC, logging the attempted evaluation will help to discover mismatches between the flag key here and what was inputted in the UI; or help with autocomplete at flag creation time

when no allocations matched (not even the default one, somehow), it's basically an error and we would want to keep track of it

This would be benefit of having complex return type that includes evaluation reason.

You can see us passing around a "variation reason" for when we hacked a version with that for Palta: Eppo-exp/js-sdk-common@v2.1.0...v2.1.1-alpha.0

I think we can include the reason IN the FAC itself for most cases.

I think of the FlagEvaluation object as capturing the evaluation reason (in particular, the allocation key: we returned variation X because we matched allocation Y)

Seems like the main thing we should consider adding is an "error" attribute to flag evaluation that we can set

eppo_client/models.py

eppo_client/config.py

eppo_client/rules.py

eppo_client/client.py

aarsilv

This is looking good!

aarsilv · 2024-03-08T20:46:39Z

eppo_client/eval.py

+        allocation_key=None,
+        variation=None,
+        extra_logging={},
+        do_log=False,


This would be benefit of having complex return type that includes evaluation reason.

You can see us passing around a "variation reason" for when we hacked a version with that for Palta: Eppo-exp/js-sdk-common@v2.1.0...v2.1.1-alpha.0

I think we can include the reason IN the FAC itself for most cases.

eppo_client/models.py

eppo_client/eval.py

aarsilv · 2024-03-14T16:32:53Z

eppo_client/eval.py

+    subject_attributes: Dict[str, Union[str, float, int, bool]]
+    allocation_key: str
+    variation: Variation
+    extra_logging: Dict[str, str]


is extra_logging where the evaluation reasoning would work its way in?

No, this is a generic object that allows us to add extra logging fields. The immediate use case is to log holdout assignments

eppo_client/__init__.py

schmit · 2024-03-15T22:10:04Z

eppo_client/client.py

            raise e

-    def get_assignment_variation(
+    def get_assignment_detail(


@aarsilv this function returns the FlagEvaluation object, which I currently consider to be the "detailed view of assignment".

Is this part of the "public" API then? Developers who want the full return object call this one instead of get__assignment?

Yes that is my thought -- but open to feedback.

For example: should we have typed versions of the detailed version?

eppo_client/client.py

schmit · 2024-03-15T22:12:47Z

eppo_client/client.py

-                "[Eppo SDK] No assigned variation. Subject attributes do not match targeting rules: {0}".format(
-                    subject_attributes
-                )
+        if not check_type_match(expected_variation_type, flag.variation_type):


Check this before we check whether the flag is enabled so that we can notify developers even for disabled flags.

leoromanovsky · 2024-03-14T17:15:35Z

eppo_client/configuration_requestor.py

-            configs = cast(dict, self.__http_client.get(RAC_ENDPOINT).get("flags", {}))
-            for exp_key, exp_config in configs.items():
-                configs[exp_key] = ExperimentConfigurationDto(**exp_config)
+            configs = cast(dict, self.__http_client.get(UFC_ENDPOINT).get("flags", {}))


It's official 🥳

I would think that not finding flags in the response should trigger an exception, not silently return an empty config

I think that's fair, but this is how we have been doing things and I think it may be better to change this in a separate PR

eppo_client/__init__.py

eppo_client/client.py

leoromanovsky · 2024-03-21T02:51:36Z

eppo_client/client.py

+        if subject_attributes is None:
+            subject_attributes = {}


So that from that point onwards we can assume subject_attributes is a dictionary and can avoid the "null" case

leoromanovsky · 2024-03-21T02:52:39Z

eppo_client/eval.py

+class FlagEvaluation:
+    flag_key: str
+    subject_key: str
+    subject_attributes: Dict[str, Union[str, float, int, bool]]


We always return a dict here -- it might just be empty. I imagine that's easier to deal with downstream than having either nulls or dictionaries with values. Open to feedback

eppo_client/rules.py

eppo_client/sharding.py

eppo_client/configuration_requestor.py

giorgiomartini0 · 2024-03-21T21:55:39Z

eppo_client/configuration_requestor.py

-            configs = cast(dict, self.__http_client.get(RAC_ENDPOINT).get("flags", {}))
-            for exp_key, exp_config in configs.items():
-                configs[exp_key] = ExperimentConfigurationDto(**exp_config)
+            configs = cast(dict, self.__http_client.get(UFC_ENDPOINT).get("flags", {}))


I would think that not finding flags in the response should trigger an exception, not silently return an empty config

eppo_client/configuration_requestor.py

eppo_client/client.py

giorgiomartini0 · 2024-03-21T22:24:42Z

eppo_client/rules.py

+from eppo_client.types import AttributeType, ConditionValueType, SubjectAttributes


 class OperatorType(Enum):


Would you be open to adding "IS_NULL", "IS_NOT_NULL" here or would you prefer that to be a minor version bump once we support it in the frontend?

I think we should add it, and we can do that before publishing this version of the SDK, but let's move it to a different PR

giorgiomartini0 · 2024-03-21T22:41:15Z

test/configuration_store_test.py

+    assert store.get_configuration("flag") == mock_flag


 def test_evicts_old_entries_when_max_size_exceeded():


What's this? Do we have a max size concept elsewhere?

Not sure -- I did not change the logic here

test/eval_test.py

eppo_client/rules.py

test/rules_test.py

eppo_client/client.py

* Pass new tests * mypy + flake

eppo_client/sharders.py

test/rules_test.py

leoromanovsky · 2024-04-19T01:45:25Z

eppo_client/client.py

+            logger.error(
+                "[Eppo SDK] Variation value does not have the correct type for the flag: "
+                f"{flag_key} and variation key {result.variation.key}"
+            )


when does this situation happen?

This should never happen, but can theoretically happen if the backend sends a value that does not have the correct type

wip

72d1ee5

schmit commented Mar 5, 2024

View reviewed changes

test/rules_test.py Outdated Show resolved Hide resolved

schmit commented Mar 5, 2024

View reviewed changes

eppo_client/client.py Outdated Show resolved Hide resolved

add todo on timestamps

35b6151

schmit commented Mar 5, 2024

View reviewed changes

Sven Schmit added 3 commits March 4, 2024 21:41

fix linting errors

a3a4833

wip

aed5425

wip

8ba3330

giorgiomartini0 reviewed Mar 5, 2024

View reviewed changes

wip

26006ec

giorgiomartini0 reviewed Mar 5, 2024

View reviewed changes

address comments

36a3e18

schmit requested review from aarsilv and leoromanovsky March 7, 2024 17:21

aarsilv reviewed Mar 8, 2024

View reviewed changes

wip

0df2809

schmit force-pushed the allocation-revamp branch from 5c98d88 to 0df2809 Compare March 14, 2024 03:51

aarsilv reviewed Mar 14, 2024

View reviewed changes

Sven Schmit added 7 commits March 14, 2024 15:51

update tests

571c2c7

update tests and json

c698818

update tests

209bfea

move value type to flag

bdcbb61

fixes

7cae3d5

update makefile to point to ufc tests

a95e0d8

update version

4252e78

schmit changed the title ~~United RAC revamp [wip]~~ Python SDK UFC update Mar 15, 2024

schmit commented Mar 15, 2024

View reviewed changes

eppo_client/__init__.py Show resolved Hide resolved

schmit commented Mar 15, 2024

View reviewed changes

eppo_client/client.py Outdated Show resolved Hide resolved

schmit commented Mar 15, 2024

View reviewed changes

Sven Schmit added 2 commits March 20, 2024 16:28

eval always returns a FlagEvaluation

b231589

add type signatures

502f8f3

leoromanovsky reviewed Mar 21, 2024

View reviewed changes

more typing

2ba14c1

giorgiomartini0 approved these changes Mar 21, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

Sven Schmit added 5 commits March 21, 2024 15:52

use numeric instead of float

6a0f3d3

more typing

6ec21dd

more careful about rules and tests

e55e88f

Address Giorgio's comments

5cb9830

fix typing error in none_result

7897a67

sameerank approved these changes Mar 22, 2024

View reviewed changes

eppo_client/rules.py Show resolved Hide resolved

test/rules_test.py Show resolved Hide resolved

eppo_client/client.py Show resolved Hide resolved

eppo_client/client.py Outdated Show resolved Hide resolved

Sven Schmit added 9 commits March 22, 2024 16:08

add NOT_MATCHES operator

0506aaa

get_integer_assignment should return an integer

de969b0

update types in client.py

dcbe36e

use numeric instead of float

b0ad9c1

Fix UFC tests, force default value

8248353

add metadata logging and is_initialized

9fda450

update endpoint

2c0b6cf

Python UFC SDK updates (#29)

1b830d8

* Pass new tests * mypy + flake

Update function signatures

3931550

leoromanovsky reviewed Apr 19, 2024

View reviewed changes

eppo_client/sharders.py Show resolved Hide resolved

leoromanovsky reviewed Apr 19, 2024

View reviewed changes

test/rules_test.py Outdated Show resolved Hide resolved

leoromanovsky reviewed Apr 19, 2024

View reviewed changes

Sven Schmit added 4 commits April 22, 2024 10:50

added semver tests

6f35c5a

return noneresult more often

82bfa59

add sharders test

4864085

🧹

67c439f

leoromanovsky mentioned this pull request Apr 23, 2024

Initial implementation Eppo-exp/eppo-multiplatform#2

Merged

schmit merged commit d7cba6b into main Apr 24, 2024

		from eppo_client.types import AttributeType, ConditionValueType, SubjectAttributes


		class OperatorType(Enum):

		assert store.get_configuration("flag") == mock_flag


		def test_evicts_old_entries_when_max_size_exceeded():

Conversation

schmit commented Mar 5, 2024 • edited 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giorgiomartini0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

giorgiomartini0 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aarsilv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmit commented Mar 5, 2024 •

edited

Loading