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

Create new eventloop to run coroutines #1055

Closed
wants to merge 2 commits into from
Closed

Conversation

ejguan
Copy link
Contributor

@ejguan ejguan commented Feb 27, 2023

Prevent calling asyncio.run within __iter__ function. New event loop would be created rather than reusing the global event loop.
Using policy for the case of custom policy is used as suggested in https://docs.python.org/3.8/library/asyncio-eventloop.html#event-loop

Address the comment

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 27, 2023
@facebook-github-bot
Copy link
Contributor

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ejguan
Copy link
Contributor Author

ejguan commented Feb 27, 2023

I will add a test to iterate over two asyncio DataPipe at the same time to make sure it works.

Copy link

@dracifer dracifer left a comment

Choose a reason for hiding this comment

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

thanks

@ejguan ejguan mentioned this pull request Feb 27, 2023
10 tasks
@facebook-github-bot
Copy link
Contributor

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

LGTM! This should fix the situation where asyncio.run() function cannot be called when another asyncio event loop is running in the same thread, such as in Jupyter notebook.

Non-blocking, it would be nice if we have some tests related to that.

@facebook-github-bot
Copy link
Contributor

@ejguan merged this pull request in f30281b.

@ejguan
Copy link
Contributor Author

ejguan commented Feb 28, 2023

LGTM! This should fix the situation where asyncio.run() function cannot be called when another asyncio event loop is running in the same thread, such as in Jupyter notebook.

Non-blocking, it would be nice if we have some tests related to that.

I just looked deeper on asyncio. I think creating new eventloop is redundant because in theory we should only keep a single eventloop within a process rather than multiple ones or nested one.
The case of Jupyter notebook is special though. If users want to use it, they need to rely on module like nest_asyncio to achieve iterating over this DataPipe. If they want to do multiprocessing, then it's not a problem at all as each sub-process would launch new eventloop excluding jupyter eventloop to execute self.process_batch.

In thoery, users should never make DataLoader2 inside a asyncio.run function as DataLoader2 doesn't provide any async API to users and it gives no benefit at all. This DataPipe should be the place that converts async calls to sync. And, even though I use new loop within AsyncMapper, it's not supported by asyncio.

@dracifer @wenleix @NivekT

@ejguan
Copy link
Contributor Author

ejguan commented Feb 28, 2023

@NivekT also suggests to capture the exception to raise suggestion for users who want to use it in notebook.

@ejguan ejguan added the topic: bug fixes topic category label Mar 20, 2023
@NivekT NivekT added this to the 0.6.1 milestone Mar 20, 2023
@ejguan ejguan removed this from the 0.6.1 milestone Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged topic: bug fixes topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants