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: add support for async methods #65

Merged
merged 5 commits into from
Nov 5, 2024
Merged

Conversation

leohoare
Copy link
Contributor

@leohoare leohoare commented Oct 29, 2024

Introduce async support using httpx.

@leohoare leohoare changed the title add async methods feat: add support for async methods Oct 29, 2024
Copy link
Member

@nicklasl nicklasl left a comment

Choose a reason for hiding this comment

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

Thanks for the contributions!

IMO the async API make sense, unfortunately I don't know too much about Python and if there are different libraries to use here?
For example, is asyncio an alternative approach?

Please address the comment about OpenFeature stuff.

Other than that, there seems to be some opportunity to refactor to reuse some of the code in the evalate and resolve functions... I could live with that but the fruit is pretty low hanging.

@@ -121,6 +121,24 @@ def resolve_boolean_details(
flag_metadata=details.flag_metadata,
)

async def resolve_boolean_details_async(
Copy link
Member

Choose a reason for hiding this comment

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

The OpenFeature provider should not be used on it's own but rather together with the OpenFeature SDK so I would revert the changes in this class (and the test).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright this has been removed, I'll look into adding the feature into OpenFeature SDK.

Copy link
Member

Choose a reason for hiding this comment

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

I asked about it in their slack yesterday and was pointed to this issue. Federico that has commented on that issue is one of the main contributors to the OpenFeature Python SDK and he seems positive 👍

@leohoare
Copy link
Contributor Author

leohoare commented Nov 4, 2024

IMO the async API make sense, unfortunately I don't know too much about Python and if there are different libraries to use here?
For example, is asyncio an alternative approach?

httpx and aiohttp are built on top of asyncio and tend to be the go-to options for async requests due to their robustness. Implementing requests with asyncio directly tends to be pretty messy and error prone.
As for the choice between implementations:

  • httpx is a drop in replacement for requests, supports sync/async (if you wanted to refactor to use a single library), more "modern" approach.
  • aiohttp has a larger following and the standard for a relatively long time, a bit clunkier to implement.

This article has a good comparison.
image

Something to consider is the default configuration on the client e.g. timeout default is 5 seconds with httpx.

Either option is appropriate for the use case and there should be negligible differences in speed (not tested for the use case). If there's a preference, switching is straight forward.

Other than that, there seems to be some opportunity to refactor to reuse some of the code in the evalate and resolve functions... I could live with that but the fruit is pretty low hanging.

Happy to do this change, I was initially avoid refactoring. It's a common issue in python is supporting sync/async you end up with large chunks of forked code.

@nicklasl nicklasl merged commit a06a18b into spotify:main Nov 5, 2024
5 checks passed
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.

2 participants