Update OFREP provider to use httpx and support async #311
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmontagu
changed the title
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
gruebel
reviewed
Oct 30, 2025
Comment on lines
+64
to
+65
| def initialize(self, evaluation_context: EvaluationContext) -> None: | ||
| self.client.__enter__() |
There was a problem hiding this comment.
why enter the client here? when you initialize it in the constructor then it should be already entered or not?
| self.client.__enter__() | ||
|
|
||
| def shutdown(self) -> None: | ||
| self.client.__exit__(None, None, None) |
There was a problem hiding this comment.
why not just call .close() on it
|
|
||
| try: | ||
| # TODO(someday): support non asyncio runtimes here | ||
| asyncio.get_running_loop().create_task(self.client_async.__aexit__(None, None, None)) |
Comment on lines
+258
to
+260
| if not self._client_async_is_entered: | ||
| await self.client_async.__aenter__() | ||
| self._client_async_is_entered = True |
There was a problem hiding this comment.
as far as I can tell from the official docs this is not needed
Comment on lines
+262
to
+268
| now = datetime.now(timezone.utc) | ||
| if self.retry_after and now <= self.retry_after: | ||
| raise GeneralError( | ||
| f"OFREP evaluation paused due to TooManyRequests until {self.retry_after}" | ||
| ) | ||
| elif self.retry_after: | ||
| self.retry_after = None |
There was a problem hiding this comment.
duplicated code, please create a small private method for it
Comment on lines
+282
to
+294
| try: | ||
| data = response.json() | ||
| except JSONDecodeError as e: | ||
| raise ParseError(str(e)) from e | ||
|
|
||
| _typecheck_flag_value(data["value"], flag_type) | ||
|
|
||
| return FlagResolutionDetails( | ||
| value=data["value"], | ||
| reason=Reason[data["reason"]], | ||
| variant=data["variant"], | ||
| flag_metadata=data.get("metadata", {}), | ||
| ) |
Comment on lines
+331
to
+333
| def __del__(self): | ||
| # Ensure clients get cleaned up | ||
| self.shutdown() |
There was a problem hiding this comment.
not sure, if this is really needed, but it is better to use weakref for this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
Updates the OFREP provider to use httpx and have proper async support.
I have not yet added tests etc. to this PR, because I wanted to get feedback on it before doing more work. But I have used this version of the provider code internally and it has worked fine, so I at least wanted to share.
Related Issues
Closes #310 if merged.
Follow-up Tasks
Need to update pyproject.toml, add tests, etc. Probably will take a decent effort so I don't want to undertake it if there's not consensus this is a good approach.
I'd be potentially willing to port all existing usage of requests to httpx if there was interest in expanding beyond just this OFREP provider (and with that, adding async support for all providers in this package). I'm not sure what the right replacement for requests-mock is with httpx (which I can see is what is used in tests) but I'm sure something reasonable exists.
How to test
This won't work yet unless you manually (uv) pip install httpx. I'm mostly looking to get confirmation of interest in the approach / replacing requests with httpx at this time.