Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: simplify async functionality to provider #385

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

leohoare
Copy link

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.

@leohoare leohoare force-pushed the feature/simplify_async_functionality_to_provider branch from df4fe5a to a6b8325 Compare November 13, 2024 04:20
@leohoare leohoare changed the title Feature/simplify async functionality to provider feat: simplify async functionality to provider Nov 13, 2024
@leohoare leohoare mentioned this pull request Nov 13, 2024
@beeme1mr
Copy link
Member

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
Copy link
Author

leohoare commented Nov 13, 2024

No rush @beeme1mr, enjoy KubeCon.

Copy link

codecov bot commented Nov 13, 2024

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.

Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall nice work 🍻

openfeature/api.py Outdated Show resolved Hide resolved
Comment on lines +128 to +132
def get_metadata(self) -> Metadata:
return InMemoryMetadata()

def get_provider_hooks(self) -> typing.List[Hook]:
return []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 leohoare force-pushed the feature/simplify_async_functionality_to_provider branch from 0bfde37 to 17acf42 Compare November 18, 2024 21:56
@leohoare leohoare marked this pull request as ready for review November 19, 2024 00:28
@leohoare leohoare force-pushed the feature/simplify_async_functionality_to_provider branch from 6740235 to 0d68182 Compare November 19, 2024 22:50
leohoare added 11 commits November 20, 2024 09:51
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
@leohoare
Copy link
Author

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.

…ider

Signed-off-by: Leo  <37860104+leohoare@users.noreply.github.com>
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants