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

Use old-style (3.5 ad 3.5) for asyncio.run #3065

Closed
wants to merge 1 commit into from

Conversation

kinow
Copy link
Member

@kinow kinow commented Apr 2, 2019

Fix for #3063. Tested locally and I can run a suite in Python 3.7 and Python 3.6 with Linux terminal and PyCharm respectively.

Cheers
Bruno

@kinow kinow self-assigned this Apr 2, 2019
@kinow kinow added this to the cylc-8.0a1 milestone Apr 2, 2019
@@ -190,8 +190,13 @@ def __init__(self, host, port, encode_method, decode_method, secret_method,
raise ClientError(response['error'])

def serial_request(self, command, args=None, timeout=None):
return asyncio.run(
self.async_request(command, args, timeout))
# TODO: replace by asyncio.run when minimum req is py37
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this todo because the asyncio.run actually does a bit more. It checks whether the loop created is the only one, as it is not expected to use one loop while there is already an asyncio loop in the same thread. It also collects events when debugging, and on shutdown of the loop tries to cancel any remaining futures enqueued. So not exactly the same as old code used to do, but same plus good improvements that would be nice for us to use too IMO.

@kinow
Copy link
Member Author

kinow commented Apr 2, 2019

Hmmm, Travis utterly unhappy. All builds failing. I've probably missed something here. Let me take another look.

@kinow kinow force-pushed the use-old-style-asyncio-run branch from a4a0769 to 3ca0ba8 Compare April 3, 2019 00:04
@kinow
Copy link
Member Author

kinow commented Apr 3, 2019

Oh, so asyncio.run calls new_event_loop internally, not get_event_loop. It creates a new loop for each execution. Used new_event_loop and at least the test-battery tests that failed on Travis and I tried locally passed this time. Let's wait for Travis now 🍿

@oliver-sanders
Copy link
Member

Closed by #3069?

@kinow
Copy link
Member Author

kinow commented Apr 5, 2019

Yup! Forgot to close this one. Thanks!

@kinow kinow closed this Apr 5, 2019
@kinow kinow removed this from the cylc-8.0a1 milestone Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants