feat: add async methods

[leohoare]

This PR

Adds the ability for open feature providers to use async methods

Related Issues

#284

Notes

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

Follow-up Tasks

• unit tests
• forked/copied code isn't ideal, happy to refactor if the approach is correct

How to test

TODO

[codecov]

Codecov Report

Attention: Patch coverage is 84.61538% with 42 lines in your changes missing coverage. Please review.

Project coverage is 95.16%. Comparing base (2d8fadf) to head (9275136).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
openfeature/provider/__init__.py	56.09%	18 Missing ⚠️
openfeature/client.py	82.27%	14 Missing ⚠️
openfeature/hook/__init__.py	54.54%	5 Missing ⚠️
openfeature/provider/in_memory_provider.py	77.77%	4 Missing ⚠️
openfeature/hook/_hook_support.py	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
- Coverage   97.41%   95.16%   -2.25%     
==========================================
  Files          26       27       +1     
  Lines        1239     1511     +272     
==========================================
+ Hits         1207     1438     +231     
- Misses         32       73      +41     

Flag	Coverage Δ
unittests	95.16% <84.61%> (-2.25%)	⬇️

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.

[federicobond]

Nice work @leohoare, thanks for putting this out! It's a great starting point for discussing async support in the Python SDK!

As I was reviewing it, a couple of things came to my mind, and I want to hear your thoughts:

1. Would it make sense to split the client into both sync and async versions? One advantage I see is that it would clean the method signatures and prevent accidental use of sync versions, but I haven't fully mapped out the implications of such a split. Example:

client = openfeature_api.get_async_client()
await client.get_boolean_value("flag-key", False)

This way you won't accidentally call a sync method inside async code, as long as you grabbed the correct client first.

2. Do we need special considerations for hooks in async code paths? async-enabled hooks?
3. How does async impact provider initialization/shutdown?
4. For providers that implement only async methods, I'm worried that they would be breaking the FeatureProvider protocol because the sync versions would not have an implementation. In fact, users will probably only interact with one set of methods, so perhaps it makes sense to split the types?

[leohoare]

Would it make sense to split the client into both sync and async versions? One advantage I see is that it would clean the method signatures and prevent accidental use of sync versions, but I haven't fully mapped out the implications of such a split. Example:
For providers that implement only async methods, I'm worried that they would be breaking the FeatureProvider protocol because the sync versions would not have an implementation. In fact, users will probably only interact with one set of methods, so perhaps it makes sense to split the types?

Yeah, I agree. Splitting the clients into async / sync would be a cleaner division.
Typically if you're implementing async methods, you'd have sync available so I was treating it like an extension.

Edit: The more I look at it, I somewhat more prefer the _async suffix, but either pattern is good.
Some libraries that use _async: feast, boto3...
Some libraries that use AsyncClient: httpx

Saying that, in our case, It'd be a bit messy to force people to implement the _sync methods whenever they want to use the _async.

Do we need special considerations for hooks in async code paths? async-enabled hooks?

If we're splitting the clients, I'd lean towards making all hooks async on that client.

[federicobond]

Edit: The more I look at it, I somewhat more prefer the _async suffix, but either pattern is good.

Can you elaborate a bit more on this? I'm leaning a bit towards splitting the two, particularly because they may involve different initialization/shutdown routines due to the different clients being used, but I want to make sure I understand the benefits and tradeoffs from the other side too.

[leohoare]

Can you elaborate a bit more on this? I'm leaning a bit towards splitting the two, particularly because they may involve different initialization/shutdown routines due to the different clients being used, but I want to make sure I understand the benefits and tradeoffs from the other side too.

I more mean as a style / how it'd used, when a function has the _async suffix you're reminded to await it and handle it appropriately. Where as the AsyncClient requires you to know upfront that all functions are async.
e.g. httpx

>>> async with httpx.AsyncClient() as client:
...     r = await client.get('https://www.example.com/')

For our use case given initialization/shutdown/hooks may want to be handled differently, so having separate clients is looking more appropriate. Another advantage is if someone wants to use async methods, we shouldn't force them to implement to the sync methods. Extending and only forcing 1 of the 2 methods would be messy.

[leohoare]

Note, the current code is not ready and needs more unit tests / evaluation for this amount of code change. What do you think about the change though?

[beeme1mr]

I think the async client is a good idea. Just for my own understanding, the async hooks can be used as a drop in a replacement anywhere a hook current is supported, right? If that's the case, I'm fine with the approach. I just want to make sure hook support isn't fragmented by this change.

[leohoare]

Apologies, been a bit busy the last few days and will pick off chunks when I get time.

Things still TODO:

• Unit tests for coverage, the unit tests are currently pretty coupled (i.e. usings hooks across tests etc). It gets a bit messy with async/sync sharing underlying global variables (e.g. hooks).
• Cleanup and ensure hooks are implemented correctly.

Just for my own understanding, the async hooks can be used as a drop in a replacement anywhere a hook current is supported, right?

@beeme1mr

The current behaviour is:

• Using the Async Client, hooks need to be converted from sync to async
• All hook calls have been converted to async
• AsyncHooks are a drop in replacement for the existing hooks

Do we want the AsyncClient to support both sync and async hooks?

[federicobond]

Ideally, we would like to support providers and hooks like the standard OTel one without duplicating implementations, but I'm not sure what's the best approach for that.

@leohoare are you on the #openfeature-python CNCF Slack channel? Would you mind joining?

[leohoare]

I'll have a play around with open telemetry hooks and the confidence provider and how they interact. Agree ideally we should support this hook out the box and it should support sync hooks. Perhaps a simpler PR for the first version would just be introducing async methods to the flag resolution and not consider async hooks.

Yep I've joined the slack channel.

[leohoare]

Going to close this PR in favour of #385.
It feels like a more straight forward change and natural first step to not touch the hook implementation.
I.e. the initialising, shutdown and hooks will all be run in the same fashion synchronously.
The changes will make the client a drop in replacement to resolve features async.