python-sdk: Work in progress initial commit of a python sdk #1

ajhelsby · 2022-05-12T12:12:36Z

Work in progress: Initial commit of all the features discussed in the open feature specification

matthewelwell

I have a feeling the modules with hyphens in their name (e.g. flag-evaluation and evaluation-context) will need to be renamed to either remove the hyphen or replace it with an underscore. Python doesn't like hyphens in module names.

readme.md

.pre-commit-config.yaml

src/client.py

matthewelwell

A few more comments.

(Sorry, got trigger happy on my previous review)

src/open_feature.py

matthewelwell · 2022-05-27T16:25:03Z

src/provider/flagsmith/flagsmith_provider.py

+    def get_boolean_value(self, key: str, defaultValue: bool) -> bool:
+        value = self.flagsmith_provider.get_value(key)
+        if not isinstance(value, bool):
+            raise Exception()


Does the spec have a defined behaviour for exception management anywhere? For example, should we have our own exceptions defined in provider.flagsmith.exceptions - maybe these should inherit from some base exception in the client?

Also, interestingly, we have the concept of enabled and a boolean value. I think @dabeeeenster raised this but I'm not sure what the outcome was?

The spec says no Exceptions should be raised so we should come up with a solution for what needs to be returned.

@ajhelsby Correct, exceptions should never bubble up, the only possible situation where that's not the case is exceptions thrown in user-defined-code in error() and finally() hook stages, which is something we're hoping to clarify a bit more soon.

@matthewelwell The "enabled" state in Flagsmith is a bit of an outlier relative to most providers. We struggled with it for a while and decided not to reflect an equivalent concept in the spec.

It will be up to the FlagSmith provider implementation how to handle enabled vs getting a boolean value. It might be something that can be controlled/specified with an option to the FlagSmith provider.

Understood, thanks @toddbaert.

Yes, I think something along the lines of a configuration option that allows the user to control not returning the value if the feature is not 'enabled' would work. We will have a think about it on our side and implement it in the provider as required.

In the java side of things, we allow providers to throw exceptions, but they are caught by the client itself.

In the java side of things, we allow providers to throw exceptions, but they are caught by the client itself.

Great point, this is how the TS/JS SDK works as well.

Regarding exceptions, are we ok with creating a base Exception class in the openfeature SDK and then expecting that all providers to subclass that or do we need to catch all Exceptions in the client? My preference would definitely be the former but it puts a responsibility on the providers to implement their exceptions correctly.

We need to catch all exceptions in the client. No one is going to implement anything correctly ;) . The primary design principal I've been using is: "We should never throw for the user if we can avoid it, and certainly not surprisingly at runtime". Like, I want people to be able to write code like:

if client.get_boolean('show_redesign', False): return redesign() return design()

and not this:

try: if client.get_boolean('show_redesign', False): return redesign() return design() except: return design()

They'd have to do the second if they want to catch stuff like connection timeouts, etc. We should assume an imperfect, but well intentioned provider.

src/provider/provider.py

… style

# Conflicts: # readme.md

toddbaert · 2022-05-31T17:13:34Z

src/provider/flagsmith/flagsmith_provider.py

@@ -0,0 +1,83 @@
+import typing


I could be misunderstanding, but we want the SDK to be low (preferably no) dependency, and vendor neutral.

The SDK repo itself isn't the place for any particular vendor provider. Providers should likely exist in a "python-contrib" repo, which would have useful hooks and other open-source providers, or perhaps in repositories belonging to the vendor in question.

The idea is, even 3rd party libraries could include open-feature, which would be a very-low profile dependency, and then the consuming application registers whatever provider they want to use. This way, the SDK the 3rd party need to implement is very light and 100% vendor agnostic.

Thanks for this @toddbaert. I just discussed it with @ajhelsby. Is the expectation therefore that the build process (perhaps via a github workflow) would pull in this python-contrib repo and build as a single package to pypi still?

We were thinking it would be quite cool to use the pypi subpackages functionality similar to e.g. boto so you'd end up with something like:

pip install openfeature[flagsmith]

Sorry for missing this for so long...

I'm the farthest thing from a python expert, but this seems like a reasonable solution. The goal is not to pull in deps for things you aren't using, and to even allow 3rd party libraries to include OpenFeature generically in their libraries, without any concern over any particular provider/backend. The first party application could then leverage these flags after registering their provider globally.

We basically want to package the evaluation separate from any provider implementation.

justinabrahms · 2022-05-31T17:30:02Z

pyproject.toml

+[tool.black]
+line-length = 88
+target-version = ['py37']
+include = 'src.*py$|tests.*py$'


Exclude src files? What will those be?

justinabrahms · 2022-05-31T17:30:23Z

readme.md

@@ -1,5 +1,9 @@
-# OpenFeature SDK for Python 
+# Open Feature Python SDK


Add a note about experimental status? :)

Agreed I'll add a comment in on the next commit

justinabrahms · 2022-05-31T17:31:24Z

requirements.in

@@ -0,0 +1 @@
+flagsmith


For the java sdk, I've been expecting that the provider code would live in a separate repository. We should discuss this as a broader group.

Ya, I feel the same, I made this comment: #1 (comment)

justinabrahms · 2022-05-31T17:33:07Z

src/client.py

+            return self.provider.get_number_value(
+                key, default_value, evaluation_context, flag_evaluation_options
+            )
+        if flag_type is FlagType.STRING:


You'll want a fallthrough case in the event nothing matches.

justinabrahms · 2022-05-31T17:33:45Z

src/client.py

+        evaluation_context: typing.Any = None,
+        flag_evaluation_options: typing.Any = None,
+    ):
+        return self.provider.get_number_details(


What's the thinking around running the value ones through evaluate_flag but the detail ones directly calling provider?

justinabrahms · 2022-05-31T17:35:33Z

src/provider/flagsmith/flagsmith_provider.py

+    def get_boolean_value(self, key: str, defaultValue: bool) -> bool:
+        value = self.flagsmith_provider.get_value(key)
+        if not isinstance(value, bool):
+            raise Exception()


In the java side of things, we allow providers to throw exceptions, but they are caught by the client itself.

justinabrahms · 2022-05-31T17:39:06Z

src/provider/provider.py

+        default_value: bool,
+        evaluation_context: typing.Any = None,
+        flag_evaluation_options: typing.Any = None,
+    ):


probably want return value types here.

src/open_feature_api.py

src/open_feature_client.py

matthewelwell · 2022-06-16T13:26:37Z

src/provider/provider.py

+
+class AbstractProvider:
+    @abstractmethod
+    def get_name(self) -> str:


This could probably just be an attribute on the class itself, right? name = None. Maybe this is better though.

…valuation on client

justinabrahms · 2022-06-20T05:15:55Z

src/exception/exceptions.py

+        self.error_code = error_code
+
+
+class FlagNotFoundError(OpenFeatureError):


Not sure if you want to file an issue for it, but we should put docstrings on these.

justinabrahms · 2022-06-20T05:17:40Z

src/flag_evaluation/error_code.py

+from enum import Enum
+
+
+class ErrorCode(Enum):


Requirement 1.4.7
In cases of abnormal execution, the evaluation details structure's error code field MUST contain a string identifying an error occurred during flag evaluation and the nature of the error.

String is specified for this type, fyi.

I've changed all the enums to strings now

justinabrahms · 2022-06-20T05:22:28Z

src/open_feature_client.py

+            if flag_type is FlagType.BOOLEAN:
+                return self.provider.get_boolean_details(
+                    key,
+                    default_value,
+                    evaluation_context,
+                    flag_evaluation_options,
+                )
+
+            elif flag_type is FlagType.NUMBER:
+                return self.provider.get_number_details(
+                    key,
+                    default_value,
+                    evaluation_context,
+                    flag_evaluation_options,
+                )


Suggested change

if flag_type is FlagType.BOOLEAN:

return self.provider.get_boolean_details(

key,

default_value,

evaluation_context,

flag_evaluation_options,

)

elif flag_type is FlagType.NUMBER:

return self.provider.get_number_details(

key,

default_value,

evaluation_context,

flag_evaluation_options,

)

args = (key, default_value, evaluation_context, flag_evaluation_options)

if flag_type is FlagType.BOOLEAN:

return self.provider.get_boolean_details(*args)

elif flag_type is FlagType.NUMBER:

return self.provider.get_number_details(*args)

# or..

method = TYPE_TO_METHOD_NAME[flag_type]

return getattr(self.provider, method)(key, default_value, evaluation_context, flag_evaluation_options)

Possible things to reduce code duplication that I'm sure you probably already know. Not sure if you feel like it's a maintenance headache, but sharing in case you think it's worth it.

Yes agreed that's much neater, I've made the change

justinabrahms · 2022-06-20T05:26:23Z

tests/provider/test_no_op_provider.py

+
+def test_should_get_boolean_flag_from_no_op():
+    # Given
+    open_feature = OpenFeatureAPI()


Seems odd that you're using an instance here to call the static method. I had to go look twice to see if we were hitting the OFAPI = global singleton requirement. Not sure how that's typically done in python

Ah, ok, so we have the provider as a global singleton (https://github.com/open-feature/python-sdk/blob/python-sdk/src/open_feature_api.py#L4) but not the OFAPI object itself - this is currently just a class with static methods which I guess somewhat behaves like a singleton but isn't strictly a singleton. What's the thinking behind ensure that is a singleton? Is there something in the spec you can point us towards?

Perhaps @ajhelsby, we should create a global variable for the OFAPI itself instead and the provider lives within that?

The spec is intentionally vague in some areas to support many languages.

The important part is that there's state stored in the OpenFeature top level object, such as the provider, global hooks, etc. There's an assumption this data is global/singleton.

Understood, thanks @toddbaert & @justinabrahms.

In the latest commit, we've ensured that the API itself and the provider are singletons, just in slightly different ways.

The usage from a client perspective should look something like:

import open_feature_api open_feature_api.set_provider(NoOpProvider()) client = open_feature_api.get_client()

There is an alternative that uses an OpenFeatureAPI class to keep it more in key with the Java implementation, however, this module / function based implementation is definitely the most pythonic way of doing it.

Please don't make it java-y. ;)

matthewelwell · 2022-06-21T09:38:46Z

src/open_feature_client.py

@@ -154,37 +153,23 @@ def evaluate_flag_details(
        flag_evaluation_options: typing.Any = None,
    ) -> FlagEvaluationDetails:
        try:
+            args = (


You could probably simplify this by doing something like:

def evaluate_flag_details(self, flag_type, *args, **kwargs) -> FlagEvaluationDetails: try: if flag_type is FlagType.BOOLEAN: return self.provider.get_boolean_details(*args, **kwargs)

In that case, though, you'd need to create an accurate docstring for sure.

As discussed, let's leave this as it is for now.

matthewelwell · 2022-06-21T09:43:12Z

tests/provider/test_no_op_provider.py

+
+def test_should_get_boolean_flag_from_no_op():
+    # Given
+    open_feature = OpenFeatureAPI()


Ah, ok, so we have the provider as a global singleton (https://github.com/open-feature/python-sdk/blob/python-sdk/src/open_feature_api.py#L4) but not the OFAPI object itself - this is currently just a class with static methods which I guess somewhat behaves like a singleton but isn't strictly a singleton. What's the thinking behind ensure that is a singleton? Is there something in the spec you can point us towards?

Perhaps @ajhelsby, we should create a global variable for the OFAPI itself instead and the provider lives within that?

…, modify flake8 config

matthewelwell · 2022-06-21T14:45:44Z

@toddbaert @justinabrahms I think this initial implementation (without hooks and evaluation context) is now ready for final review.

If you're ok with the approach, we'd like to get this merged and add the hooks and context in separate PRs.

Let me know if you'd rather keep it all to a single PR though and we can do that instead.

justinabrahms · 2022-06-21T14:49:44Z

I support merging this as is. :)

toddbaert · 2022-06-21T14:50:54Z

I support merging this as is. :)

As do I.

ajhelsby added 4 commits May 12, 2022 16:11

python-sdk: Work in progress initial commit of a python sdk

8010bef

python-sdk: Update client and flag type enum

5779497

python-sdk: Update flagsmith implementation

940846c

python-sdk: Add formatters to pre commit

029801f

matthewelwell reviewed May 27, 2022

View reviewed changes

matthewelwell mentioned this pull request May 27, 2022

Two spec-compliant SDKs open-feature/spec#86

Closed

2 tasks

ajhelsby added 4 commits May 30, 2022 15:13

python-sdk: Fix issues raised in code review to match a more pythonic…

d15c141

… style

python-sdk: Fix issues raised in code review to match a more pythonic…

cd19e7b

… style

python-sdk: Fix broken provider unit tests

0480749

Merge branch 'main' into python-sdk

41da399

# Conflicts: # readme.md

toddbaert reviewed May 31, 2022

View reviewed changes

toddbaert requested a review from justinabrahms May 31, 2022 17:29

justinabrahms reviewed May 31, 2022

View reviewed changes

ajhelsby added 3 commits June 9, 2022 16:57

python-sdk: Fix issues raised in code review

5e640a8

python-sdk: Remove dependency on provider

bee4d21

python-sdk: Move exceptions to a single file

b7b675a

matthewelwell reviewed Jun 16, 2022

View reviewed changes

python-sdk: Remove global from top level provider, fix default flag e…

2d5d01d

…valuation on client

justinabrahms reviewed Jun 20, 2022

View reviewed changes

python-sdk: Fix issues raised in code review

af90605

matthewelwell reviewed Jun 21, 2022

View reviewed changes

python-sdk: Use a singleton for the api, move number enums to strings…

abe82c0

…, modify flake8 config

toddbaert self-requested a review June 21, 2022 14:51

toddbaert approved these changes Jun 21, 2022

View reviewed changes

ajhelsby merged commit 0b30681 into main Jun 22, 2022

ajhelsby deleted the python-sdk branch October 13, 2022 10:44

		@@ -1,5 +1,9 @@
		# OpenFeature SDK for Python
		# Open Feature Python SDK

		self.error_code = error_code


		class FlagNotFoundError(OpenFeatureError):

python-sdk: Work in progress initial commit of a python sdk #1

python-sdk: Work in progress initial commit of a python sdk #1

Conversation

ajhelsby commented May 12, 2022

matthewelwell left a comment

Choose a reason for hiding this comment

matthewelwell 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

matthewelwell Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert Jun 21, 2022 • edited Loading

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

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

toddbaert Jun 21, 2022 • edited Loading

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

matthewelwell commented Jun 21, 2022

justinabrahms commented Jun 21, 2022

toddbaert commented Jun 21, 2022

matthewelwell Jun 9, 2022 •

edited

Loading

toddbaert May 31, 2022 •

edited

Loading

toddbaert Jun 21, 2022 •

edited

Loading

toddbaert Jun 21, 2022 •

edited

Loading