Skip to content

Conversation

@mrocklin
Copy link
Member

This replaces the use of Tornado gen.coroutines and the yield keyword with async def functions and the await keyword. Additionally, because these have slightly different semantics this also changes a few internal bits to smooth over this transition.

This is still a work in progress. The current implementation introduces a couple of small bugs and the conversion is not complete.

cc @jcrist (who requested this a while ago) and @jacobtomlinson who might find this easier to work with than gen.coroutine.

@mrocklin
Copy link
Member Author

This is getting close to mergeable, and includes fixes for issues in master. I'm going to try to clean things up here today or tomorrow and hopefully merge in shortly afterwards.

@mrocklin
Copy link
Member Author

There were some cases where I've decided to keep gen.coroutine semantics. In particular asyncio complains if we leave a task un-awaited in a way that it is bothersome for users. In a couple of places I didn't find a nice way to await every task (such as in the case of the TCP Comm.write method) so I left these in the tornado style.

@mrocklin mrocklin changed the title [WIP] Replace gen.coroutine with async/await Replace gen.coroutine with async/await in core Jul 23, 2019
@mrocklin
Copy link
Member Author

Tests are now starting to look good :)

@mrocklin
Copy link
Member Author

OK, I'm going to pronounce this as "good enough" and merge in. I'll probably do a follow-up in a couple of days.

@mrocklin mrocklin merged commit f16ee17 into dask:master Jul 24, 2019
@mrocklin mrocklin deleted the await branch July 24, 2019 16:02
# Stimuli #
###########

@gen.coroutine
Copy link
Contributor

Choose a reason for hiding this comment

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

async is not used here. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably? Is this causing some concrete issue for you? Does this function need to be asynchronous?

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