Skip to content

fix(client/async): replace asyncify with asyncio.to_thread (#1827) #1828

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix(client/async): replace asyncify with asyncio.to_thread (#1827) #1828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ filterwarnings = [
# there are a couple of flags that are still disabled by
# default in strict mode as they are experimental and niche.
typeCheckingMode = "strict"
pythonVersion = "3.7"
pythonVersion = "3.9.18"

exclude = [
"_dev",
Expand Down
4 changes: 2 additions & 2 deletions src/openai/_base_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
HttpxRequestFiles,
ModelBuilderProtocol,
)
from ._utils import is_dict, is_list, asyncify, is_given, lru_cache, is_mapping
from ._utils import is_dict, is_list, is_given, lru_cache, is_mapping
from ._compat import model_copy, model_dump
from ._models import GenericModel, FinalRequestOptions, validate_type, construct_type
from ._response import (
Expand Down Expand Up @@ -1549,7 +1549,7 @@ async def _request(
if self._platform is None:
# `get_platform` can make blocking IO calls so we
# execute it earlier while we are in an async context
self._platform = await asyncify(get_platform)()
self._platform = await asyncio.to_thread(get_platform)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't switch to just using asyncio.to_thread() itself as that was only added in Python 3.9 but we support Python 3.8+

Thankfully it looks like it mostly just delegates to loop.run_in_executor() under the hood which is available in older versions.

Can you instead change the asyncify function to use asyncio.to_thread() on 3.9 and fallback to a copied version of the source? or just always use the copied version? I don't feel strongly either way.

You can remove the other cancellable and limiter args from asyncify as I don't think they make sense anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

@RobertCraigie, since I don't have access to @jeongsu-an 's repository, I forked the repo and put up a PR for the changes you requested in #1853. Happy to iterate there if you have suggestions or changes you'd like.


# create a copy of the options we were given so that if the
# options are mutated later & we then retry, the retries are
Expand Down