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

fix(asyncify): avoid hanging process under certain conditions #1853

Conversation

spokeydokeys
Copy link
Contributor

  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

This is a follow on PR for #1828 that updates the asyncify method to use asyncio.to_thread for python 3.9+ or a copy of the source code for the same for python 3.8. This was done since to_thread was only added to asyncio in python 3.9.

This addresses an issue where the code would hand with a left-over, non-daemon thread from anyio in certain contexts.

Additional context & links

#1827
#1828

@RobertCraigie RobertCraigie changed the title asyncify to use asycnio.to_thread or local implementation for execution of blocking code fix(asyncify): avoid hanging process under certain conditions Nov 7, 2024
Copy link
Collaborator

@RobertCraigie RobertCraigie left a comment

Choose a reason for hiding this comment

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

Thanks! I assume getting a test setup for this would be difficult?

src/openai/_utils/_sync.py Outdated Show resolved Hide resolved
src/openai/_utils/_sync.py Outdated Show resolved Hide resolved
src/openai/_utils/_sync.py Outdated Show resolved Hide resolved
@spokeydokeys spokeydokeys force-pushed the asyncify-to-use-asyncio.to_thread-or-implementation branch from 1515486 to e166164 Compare November 7, 2024 17:50
@spokeydokeys
Copy link
Contributor Author

Thanks! I assume getting a test setup for this would be difficult?

I added a test that I can confirm fails on main, but works on the fixed branch.

I was concerned that since nest_asyncio.apply() is global and that I couldn't guarantee any particular test execution order, it might end up impacting other tests. For that reason I used a separate process and made sure that process closed in a timely manner. I used 2 seconds, which is quite long really, but the nature of the failure is that it hangs for much longer than that and I didn't want to introduce a flaky test if I cut the timeout too tight.

Let me know what you think.

tests/test_client.py Outdated Show resolved Hide resolved
@RobertCraigie
Copy link
Collaborator

looks like the test is failing is CI, I'm surprised there isn't any output from the subprocess? 🤔

@spokeydokeys spokeydokeys force-pushed the asyncify-to-use-asyncio.to_thread-or-implementation branch from 545f4c2 to a5e8a57 Compare November 8, 2024 22:37
@spokeydokeys spokeydokeys force-pushed the asyncify-to-use-asyncio.to_thread-or-implementation branch from a5e8a57 to 61f95b3 Compare November 8, 2024 22:40
@spokeydokeys
Copy link
Contributor Author

looks like the test is failing is CI, I'm surprised there isn't any output from the subprocess? 🤔

I think it was because I foolishly added the nest_asyncio requirement to the lock file, rather than the pyproject.toml file so it was missing an import.

@RobertCraigie
Copy link
Collaborator

ahhh good catch, looks like it's not in the lock file anymore, you'll need to update it with rye sync

@spokeydokeys
Copy link
Contributor Author

@RobertCraigie, rye sync done!

@spokeydokeys
Copy link
Contributor Author

@RobertCraigie is there anything else that you need for this?

This fix enables us to run our automated AI tasks in github workflows and we are holding off on deploying until this is merged. I'm trying to gauge how much effort to put into workarounds based on timelines...

@RobertCraigie RobertCraigie changed the base branch from main to next November 18, 2024 12:41
@RobertCraigie RobertCraigie merged commit 3d23437 into openai:next Nov 18, 2024
2 checks passed
@RobertCraigie
Copy link
Collaborator

Thanks!

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