feat: Add async functionality to providers

[leohoare]

This PR

Adds the ability for open feature providers to use async methods
It extends the single client and attempts to refactor some code

Related Issues
#284
#383
#385

Follow-up Tasks & TODOS

• Add tests for async provider and implementation
• Add documentation and how to implement an async hook

[codecov]

Codecov Report

Attention: Patch coverage is 97.80564% with 7 lines in your changes missing coverage. Please review.

Project coverage is 97.72%. Comparing base (c2d1402) to head (e7b951e).

Files with missing lines	Patch %	Lines
tests/provider/test_provider_compatibility.py	90.54%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   97.55%   97.72%   +0.16%     
==========================================
  Files          31       32       +1     
  Lines        1393     1629     +236     
==========================================
+ Hits         1359     1592     +233     
- Misses         34       37       +3     

Flag	Coverage Δ
unittests	97.72% <97.80%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[beeme1mr]

Hey @leohoare, this looks good so far. Could you please add tests covering async providers with sync client calls and vise versa? Thanks for your hard work on this. 🍻

[leohoare]

Thanks @beeme1mr, I've added some tests and addressed the coverage issues.

One thing to note is sync methods are always enforced on async providers.
you can't implement an async only provider, although, you can work around this unless using NotImplementedError on the sync methods. It was implemented on the AbstractProvider to keep current functionality the same.

Is this clear enough from the documentation?

[beeme1mr]

Thanks @beeme1mr, I've added some tests and addressed the coverage issues.

One thing to note is sync methods are always enforced on async providers.
you can't implement an async only provider, although, you can work around this unless using NotImplementedError on the sync methods. It was implemented on the AbstractProvider to keep current functionality the same.

Is this clear enough from the documentation?

Sorry, I didn't get a chance to look at this today. It's on my to-do for tomorrow.

[leohoare]

No rush, I'll be off grid over the weekend anyway.

[beeme1mr]

Hey @leohoare, this looks good from what I can tell but I wouldn't consider myself a Python expert.

I see you have tests but would you mind also enumerating the expected behavior for the following scenarios?

• performing an async evaluation on a synonymous provider
• performing a sync evaluation on an async provider that implements the AbstractProvider

I believe I understand how everything will behave but I'd like to confirm.

Also, could someone with more Python experience please weigh in when you have a moment? FYI, @aepfli @guidobrei @federicobond @jamescarr @lukas-reining @toddbaert

[leohoare]

I see you have tests but would you mind also enumerating the expected behavior for the following scenarios?

Do you mean explain the scenarios or update the tests?

performing an async evaluation on a synonymous provider

If async evaluation is not implemented, it will fall back to calling the synchronous function.
I.e. the call will be async but the code itself will be blocking and not truly async.

performing a sync evaluation on an async provider that implements the AbstractProvider

a provider that implements async calls is forced to implement sync functions.
If calls on the AbstractProvider are implemented in the sync function, then the calls will be like client previously functioned.

    def resolve_boolean_details(
        self,
        flag_key: str,
        default_value: bool,
        evaluation_context: Optional[EvaluationContext] = None,
    ) -> FlagResolutionDetails[bool]:
        # do some thing things...
        return FlagResolutionDetails(value=True)

If the provider chooses to only implement async functions and throw an error on the sync functions.
Then the client will throw an error when attempting to resolve values.

        def resolve_boolean_details(
            self,
            flag_key: str,
            default_value: bool,
            evaluation_context: Optional[EvaluationContext] = None,
        ) -> FlagResolutionDetails[bool]:
            raise NotImplementedError("Use the async method")

We're essentially offloading the decision to the provider on how to handle async/sync calls. Implementing the async calls is optional and defaults to sync when not implemented.

[leohoare]

Sorry keep forgetting to sign-off the commits -.-

[beeme1mr]

I've approved because I'm good with the approach. Hopefully others with more Python experience can also provide some thoughts.

[lukas-reining]

Sorry for the late reply, I have been on vacation. Will have a look in the next 1 or 2 days :)

[ChihweiLHBird]

This looks good to me! I think we should mention the fallback mechanism in the documentation to avoid confusion. Users might expect asynchronous execution when calling those async functions, but they actually get synchronous function execution.

[leohoare]

I've updated the readme to include the suggestion @ChihweiLHBird, as well a general usage code block.

@chrfwow due to the refactoring required in this PR, some of your recent merge had to be moved.
Could you please also review this PR (particularly the last two commits)?

[chrfwow]

The changes of my latest additions seem fine so far, but I have a few other questions

[chrfwow]

From the function name _assert_provider_status I would expect the function to throw an error when the provider is not ready. I would not expect the function to have any side effects like invoking hooks.

[leohoare]

How about _validate_provider_status? or do you have any other suggestions?

[chrfwow]

Wouldn't it be possible to just throw the proper exception (both ProviderNotReadyError and ProviderFatalError are instances of OpenFeatureError, so the exception handling should work)? This function is called inside the try catch block, so the error hooks will also be called this way.
If not, I would go for a name like _handle_provider_not_ready, but I don't like that name either

[chrfwow]

I'm not a python expert. So purely out of interest, why do we need to box the args?

[leohoare]

The way it's used is just syntactic sugar to get around strict typing.
We call it like so:

args = ...
resolution = await get_details_callable(*args)

The alternative is passing in the arguments by key.
This option likely has a typing error as we're dynamically resolving the function call.

resolution = await get_details_callable(flag_key=flag_key,...)

Another option is arguments by order.
With this option, we lose the ability to have multiple defaults (unless you pass in arguments in the exact order) and order must match exactly, so it's not often recommended.

resolution = await get_details_callable(flag_key, default_value, evaluation_context)

When you pass it in like *args, it's a way of dynamically doing option 1.
This pattern was also copied from the sync function.

[gruebel]

this is incorrect. passing *args to get_details_callable() unpacks them as positional arguments and therefore you can directly set them there. you are mistaken it with **kwargs. Feel free to try it out and switch the position in the args tuple and you will see how it messes up the function call.

Nevertheless I need to check, if it is possible to properly type the dynamic resolving, but not important in this PR, because it is done the same way in the sync variant as you mentioned.

[leohoare]

Ah true, misinterpreted this sorry.
Happy to update this to **kwargs or if you want to avoid scope creep, I can do this in a follow up PR.

[gruebel]

Nono, leave as is and feel free to give it a try in a separate PR, because the type checker won't be so nice to you

[chrfwow]

It seems to me that this function shares a lot of code with evaluate_flag_details. Does the async prevent the extraction into a common function, or is there another reason for the duplication?

[leohoare]

Does the async prevent the extraction into a common function, or is there another reason for the duplication?

Somewhat yes, because the async call is in the middle of the try catch if we wanted to split it into common code we could only do something like:

• calls at the start of try in common call

            error_code = self._assert_provider_status(
                flag_type,
                hook_context,
                reversed_merged_hooks,
                hook_hints,
            )
            if error_code:
                flag_evaluation = FlagEvaluationDetails(
                    flag_key=flag_key,
                    value=default_value,
                    reason=Reason.ERROR,
                    error_code=error_code,
                )
                return flag_evaluation

            merged_context = self._before_hooks_and_merge_context(
                flag_type,
                hook_context,
                merged_hooks,
                hook_hints,
                evaluation_context,
            )

• calls after the async call in common code

            after_hooks(
                flag_type,
                hook_context,
                flag_evaluation,
                reversed_merged_hooks,
                hook_hints,
            )

            return flag_evaluation

• A helper call for each of the exception

I didn't really deem this worth it and would lead to passing around a lot of variables to common functions.
What are your thoughts?

[chrfwow]

Let's leave it as is 👍

[chrfwow]

Couldn't this map be global instead of a local variable?

[leohoare]

Yep, followed the pattern from the sync call but it could be global.

Edit: this would be pretty messy to do

• provider is dynamically evaluated each call
• functions from this provider are mapped when creating this dictionary

It means we'd need to push the logic into the AbstractProvider init, which feels wrong.
If we wanted it to be statically called once, we'd have to require calling super() on provider implementations.

[gruebel]

this is a perfect use case for structural pattern matching, which we can use in 1-2 years, when the minimum supported Python version reaches 3.10 😅 but till then this is good enough.

[chrfwow]

This function also shares most of its code with _create_provider_evaluation

[leohoare]

Yeah, they need to be separate functions as this is needs to propagate the async function.
One big downside of python's handling of async is it often leads to forked/duplicated code.

One option is to convert the logic before/after the _get_details_callable for both functions into helper functions, however, they're already only a few calls to helpers.

I'll see what I can do to clean this up

[chrfwow]

I see... Then let's leave it as is 👍

[gruebel]

nice work, just added a few comments

[gruebel]

this is incorrect. passing *args to get_details_callable() unpacks them as positional arguments and therefore you can directly set them there. you are mistaken it with **kwargs. Feel free to try it out and switch the position in the args tuple and you will see how it messes up the function call.

Nevertheless I need to check, if it is possible to properly type the dynamic resolving, but not important in this PR, because it is done the same way in the sync variant as you mentioned.

[gruebel]

this is a perfect use case for structural pattern matching, which we can use in 1-2 years, when the minimum supported Python version reaches 3.10 😅 but till then this is good enough.

[beeme1mr]

Greets everyone! I'd like to get this merged by the end of the week if possible. Please leave you feedback ASAP if you have any concerns. Thanks!

@leohoare, thanks for you hard work and patience. It's important that we have consensus when making changes to the public APIs. So changes like this tend to take a while.