Uh oh!
There was an error while loading. Please reload this page.
feat: Add async functionality to providers - #413
Conversation
c003a56 to
4ec15beCompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #413 +/- ##
==========================================
+ Coverage 97.55% 97.71% +0.15%
==========================================
Files 31 32 +1 Lines 1393 1621 +228 ==========================================
+ Hits 1359 1584 +225 - Misses 34 37 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
beeme1mr
commented
Jan 13, 2025
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. 🍻 |
86c64df to
bb9a4e6Compareleohoare
commented
Jan 22, 2025
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. Is this clear enough from the documentation? |
5142300 to
5d34cd8Comparebeeme1mr
commented
Jan 23, 2025
Sorry, I didn't get a chance to look at this today. It's on my to-do for tomorrow. |
leohoare
commented
Jan 23, 2025
No rush, I'll be off grid over the weekend anyway. |
beeme1mr
left a comment
There was a problem hiding this comment.
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
commented
Jan 27, 2025
Do you mean explain the scenarios or update the tests?
If async evaluation is not implemented, it will fall back to calling the synchronous function.
a provider that implements async calls is forced to implement sync functions. If the provider chooses to only implement async functions and throw an error on the sync functions. 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. |
3db8642 to
e51451dCompareSigned-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
Missed auto format Signed-off-by: Leo <37860104+leohoare@users.noreply.github.com> Signed-off-by: leohoare <leo@insight.co>
e51451d to
72d69d5Compareleohoare
commented
Jan 27, 2025
Sorry keep forgetting to sign-off the commits -.- |
beeme1mr
commented
Jan 27, 2025
I've approved because I'm good with the approach. Hopefully others with more Python experience can also provide some thoughts. |
lukas-reining
commented
Jan 28, 2025
Sorry for the late reply, I have been on vacation. Will have a look in the next 1 or 2 days :) |
ChihweiLHBird
left a comment
There was a problem hiding this comment.
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.
Signed-off-by: leohoare <leo@insight.co>
…terhooks and update readme with async doco Signed-off-by: leohoare <leo@insight.co>
leohoare
commented
Feb 2, 2025
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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
chrfwow
left a comment
There was a problem hiding this comment.
The changes of my latest additions seem fine so far, but I have a few other questions
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gruebel
left a comment
There was a problem hiding this comment.
nice work, just added a few comments
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
beeme1mr
commented
Feb 4, 2025
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. |
lukas-reining
left a comment
There was a problem hiding this comment.
Looks good to me, left one question :)
Thank you very much!
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: leohoare <leo@insight.co>
Signed-off-by: leohoare <leo@insight.co>
beeme1mr
commented
Feb 5, 2025
I'll merge this PR tomorrow unless someone objects. |
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