feat: simplify async functionality to provider

[leohoare]

This PR

Adds the ability for open feature providers to use async methods
Note: This version simplifies the implementation and doesn't change any of the hook implementation.

Related Issues

#284
#383

Notes

Just confirming the approach / POC for how this would work.

[beeme1mr]

Hey @leohoare, thanks for the pr. I'm at KubeCon this week so reviews may be a bit delayed but I'll take a look as soon as possible.

[leohoare]

No rush @beeme1mr, enjoy KubeCon.

[codecov]

Codecov Report

Attention: Patch coverage is 99.15966% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.84%. Comparing base (e6ada0f) to head (d90190c).

Files with missing lines	Patch %	Lines
openfeature/client.py	98.48%	1 Missing ⚠️
tests/provider/test_no_op_provider.py	96.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #385      +/-   ##
==========================================
+ Coverage   97.54%   97.84%   +0.29%     
==========================================
  Files          31       32       +1     
  Lines        1387     1622     +235     
==========================================
+ Hits         1353     1587     +234     
- Misses         34       35       +1     

Flag	Coverage Δ
unittests	97.84% <99.15%> (+0.29%)	⬆️

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.

[gruebel]

overall nice work 🍻

[gruebel]

maybe I'm wrong, but I think these two should also be async 🤔

[leohoare]

Do you mean more generally or specifically to the InMemoryProvider?

For this PR, I was limiting the scope of adding async functionality to the resolving calls.
This ensures we have backwards compatibility with current hooks / metadata calls and providers would just need to implement the async resolve calls.

I do agree support async hooks and metadata retrieval should be looked at in the future though.

[gruebel]

Yeah, meant it more generally, because currently those would block the call, which is not really relevant for the in-memory variant, but would be for remote calls.

But totally fine to change this in the future.

[leohoare]

The current approach wasn't very compatible with mypy, I've updated settings to ignore overrides.
i.e.

    def get_boolean_value(
        ...
    ) -> bool:

Can be overwritten to

    async def get_boolean_value(
        ...
    ) -> bool:

Also given we're not forking the typing protocol FeatureProvider the following line isn't recognised as awaitable.

resolution = await get_details_callable(*args)

So I've ignored the error for that line. # type: ignore[misc]

I'm not a heavy mypy user, let me know if there's a better or cleaner approach you'd prefer here.

[beeme1mr]

What's the expected behavior if a user registers an async provider but uses the synchronous client and vice versa? Could you please update the root readme examples for how to use this? If providers can't be used interchangeably, it may fragment the Python-related ecosystem in a confusing way.

[ChihweiLHBird]

Might be helpful to check whether get_details_callable is awaitable here in order to make sync provider compatible with async clients?

It might look like this, but I didn't test it

Suggested change

	resolution = await get_details_callable(*args) # type: ignore[misc]
	resolution = get_details_callable(*args) # type: ignore[misc]
	if isawaitable(resolution):
	resolution = await resolution

Typing can be a headache here, though.

[lukas-reining]

I actually think this has to be done. If not I think we would get type errors right?

[ChihweiLHBird]

@beeme1mr I think it's generally hard to perfectly support calling an async functions (from a provider) in a sync function (from a client) due to possibility of customized or already-started asyncio event loop, but calling sync function from async functions are much easier.

I would still recommend implementing the async support because the performance is MUCH better in I/O intensive applications in comparison with threading based blocking I/O. However, we do need some error checking and documentation about incompatibility between sync clients and async providers.

[beeme1mr]

Hey @ChihweiLHBird, I'm definitely in favor of adding async support. I'm just familiar enough with Python to provide a solid recommendation here. My main concern is putting too much burden on the end user to understand what provider and hook are compatible with a particular client.

@federicobond @gruebel @aepfli do you have any recommendations?

[beeme1mr]

Hey @leohoare, sorry for the delay and Happy New Year! This fell off my radar. Feel free to @ mention me if it happens again.

[ChihweiLHBird]

@beeme1mr Thanks! And I just realized we may still make the async provider be compatible with sync client. This pattern below may be helpful to determine if the current execution is in an asyncio event loop. It should be able to make it 2 ways compatible in most cases.

(Need more opinions on this pattern since I am not very familiar with it, I'm not sure if it's 100% reliable/secure)

import asyncio

def some_client_functions():
    try:
        asyncio.ensure_future(asyncio.get_running_loop().create_task(some_async_io_task()))
    except RuntimeError:
        asyncio.run(some_async_io_task())

However, an effort like this will make it extremely complicated, and it might be better and easier to just require asyncio clients for asyncio provider or sync clients for sync providers.

[leohoare]

Happy new year everyone! Apologies, this one has went on the back burner a bit. It's been quite a busy time for me personally.

@ChihweiLHBird I do like the idea of falling back to the synchronous function in terms of usability.
I'm not 100% of the pattern, but do we know of any alternative libraries that use a similar approach that we could review their implementation? The current implementation would force python 3.7+

With other projects, I generally opt for a more explicit implementations and the async suffix on functions. In my eyes it leads to less confusion around the execution and could raise an exception when not implemented.

flag = await get_boolean_details_async(...)

It would require refactoring to use the async client though (rather than just replacing the client itself).

It's mostly style preference though:

• libraries using async suffix: feast
• libraries using async client(/session): httpx, gcp, sqlalchemy
• discord for example uses a client, with async functions and no suffix. In my eyes, this leads to a poorer development experience. Another example of this is aioredis, which implements async based on the function.

[ChihweiLHBird]

@leohoare Most project I have seen implemented async stuff as you described, and I am also personally leaning to the style other projects used for our SDK here.

[aepfli]

Hey, sorry that i chime in here too. For me this seems like something, which we might should discuss even on a "global" open feature level. There are other languages, which also support async functionalities (like java with Futures) and maybe we should write a proper specification, on how we want to handle this. I think it could be problematic, if we do have mutliple different ways within our eco system to handle async. Hence it is worth to shortly discuss this on a bigger level. wdty? (@open-feature/technical-steering-committee fyi)

[beeme1mr]

I think it makes sense to include the async methods on the existing client. It should be possible for the async calls to be accessed via the async method (even if they're not truly async), meaning exiting providers would "just work." How would we want to support async-only providers? Keep in mind that the evaluation can't throw an error that bubbles up to the user.

[moredip]

there a lot of duplication of complex spec-compliance logic between the async and sync versions of evaluation_flag_details (~140 lines of code, with really the only difference being a single await when calling self._create_provider_evaluation).

Is there any way to share the implementations between the two?

[lukas-reining]

I am not a huge python expert, but I have a concern regarding the hard split between the sync and async world here.
If I am not seeing it wrong, a provider would either implement the sync or async world or both in this design. Is this correct?

If this is true: Do you see an option to have a sync provider and client while having an async client that wraps the sync client similar to:

class OpenFeatureClient:
    def _evaluate_flag_details(
        self,
        flag_type: FlagType,
        flag_key: str,
        default_value: Any,
        evaluation_context: Optional[EvaluationContext] = None,
        flag_evaluation_options: Optional[FlagEvaluationOptions] = None,
    ) -> FlagEvaluationDetails[Any]:
        # Dummy implementation
        return FlagEvaluationDetails()

    def get_boolean_details(self,
        flag_key: str,
        default_value: bool,
        evaluation_context: typing.Optional[EvaluationContext] = None,
        flag_evaluation_options: typing.Optional[FlagEvaluationOptions] = None) -> FlagEvaluationDetails[bool]:
        return self._evaluate_flag_details(FlagType.BOOLEAN, flag_key, default_value, evaluation_context, flag_evaluation_options)

    async def _async_wrap(self, func: Callable, *args, **kwargs) -> typing.Any:
        loop = asyncio.get_event_loop()
        return await loop.run_in_executor(None, partial(func, *args, **kwargs))

    async def get_boolean_details_async(self, 
        flag_key: str, 
        default_value: bool, 
        evaluation_context: typing.Optional[EvaluationContext] = None, 
        flag_evaluation_options: typing.Optional[FlagEvaluationOptions] = None) -> FlagEvaluationDetails[bool]:
        return await self._async_wrap(
            self.get_boolean_details, 
            flag_key, 
            default_value, 
            evaluation_context, 
            flag_evaluation_options
        )

The types and params might be a bit off, I just wanted to show the idea.
@open-feature/sdk-python-maintainers maybe you also have an opinion to this.

I would like to avoid "double" amount of work for implementers of providers if this is possible.

cc @gruebel @beeme1mr

[lukas-reining]

I actually think this has to be done. If not I think we would get type errors right?

[lukas-reining]

Hey, sorry that i chime in here too. For me this seems like something, which we might should discuss even on a "global" open feature level. There are other languages, which also support async functionalities (like java with Futures) and maybe we should write a proper specification, on how we want to handle this. I think it could be problematic, if we do have mutliple different ways within our eco system to handle async. Hence it is worth to shortly discuss this on a bigger level. wdty?

This conforms to what I also wrote in my comment I just wrote @aepfli.
E.g. async/await is really different between JS and Python in my view. But still I think we might be able to come up with some general things like "avoiding double implementation" if possible.

[lukas-reining]

I think it makes sense to include the async methods on the existing client. It should be possible for the async calls to be accessed via the async method (even if they're not truly async), meaning exiting providers would "just work." How would we want to support async-only providers? Keep in mind that the evaluation can't throw an error that bubbles up to the user.

I am totally with you with this @beeme1mr.

The only thing I think I would question is I think it makes sense to include the async methods on the existing client., as in the past I have seen several libs that implement explicit async and sync classes while sharing the implementation of the core logic. But tbh. I would accept whatever a more Python familiar person than me says is more common or "idiomatic".

[leohoare]

Agree with all the above conversation, to summarise:

• We should use single client and add _async methods
• Async methods should share common code with sync
• If a provider only has sync calls, async methods should "work" and use these methods

I'll have a think how to do this in python but I'm on the same page with the current code has way too much duplication.
It sounds like it somewhat needs to "check" if an async method exists, if not fallback to sync. At that point it would need to optionally await the call. I'll see if we could then "bubble" this up to separate async methods.

I'm not sure how this could be extended to support async hooks though.

[toddbaert]

I'm not sure how this could be extended to support async hooks though.

hmmm... we might be forced to make all hooks async in this case, which is the case for server-side JS: https://github.com/open-feature/js-sdk/blob/main/packages/server/src/hooks/hook.ts

That way they will work in either case... does that make sense? We are still pre-1.0 so now is the time if we need to make this sort of change.