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

async ward tests fail with piccolo 0.75.0, pass with piccolo 0.74.4 #536

Closed
backwardspy opened this issue Jun 8, 2022 · 9 comments · Fixed by #539
Closed

async ward tests fail with piccolo 0.75.0, pass with piccolo 0.74.4 #536

backwardspy opened this issue Jun 8, 2022 · 9 comments · Fixed by #539

Comments

@backwardspy
Copy link
Contributor

i've put together a small project here to help replicate the issue: https://github.com/backwardspy/piccolo-ward-async-repro

i have a project with around 100 ward tests. most of those tests fail on piccolo 0.75.0 with the following error:

FixtureError: Unable to resolve fixture '[redacted]'
    ↓ caused by ↓
RuntimeError: There is no current event loop in thread 'MainThread'.

running the tests on piccolo 0.74.4 works fine.

i believe this is (at least indirectly) caused by the changes to the async event loop handling in piccolo. that said, i'm not sure if this is actually an issue with piccolo, or just an issue with ward that has now been illuminated due to the aforementioned piccolo changes.

i appreciate any thoughts/suggestions. great work on piccolo!

@dantownsend
Copy link
Member

In the last version of Piccolo we slightly changed how run_sync works, because it was using a function in asyncio which was marked as deprecated in Python 3.10.

It sounds like there are some edge cases where the new one is failing.

Stay on Piccolo 0.74.4 for now (there aren't any other major changes in 0.75.0) until we can figure out a fix.

We're catching the RuntimeError in run_sync, so I wonder how that exception is escaping ... it's a strange one.

Screenshot 2022-06-08 at 17 12 37

@backwardspy
Copy link
Contributor Author

backwardspy commented Jun 8, 2022

i'm not actually using run_sync anywhere in my own code, though i suspect my fixtures are calling into some piccolo functionality that does use run_sync internally.

with respect to the exception escaping your catch - in my case this exception is actually originating in ward, specifically from this line:

fixture.resolved_val = asyncio.get_event_loop().run_until_complete(arg(**args_to_inject))

i must admit i don't fully understand how the various asyncio event loop functions fit together, but my guess is previously piccolo was setting up the event loop in a certain way and ward was able to piggyback off of that for its own purposes.

@dantownsend
Copy link
Member

Yeah, I think you're right. Piccolo was probably calling run_sync internally somewhere, which created the event loop, which ward was then using.

Are you using Python 3.10? I know Python 3.11 is out soon, and might be one cause.

@backwardspy
Copy link
Contributor Author

yeah i am using 3.10.5. i can try the latest 3.11 version to see if it helps at all

@dantownsend
Copy link
Member

Its OK - I'll test it with Python 3.10.

@dantownsend
Copy link
Member

I've dug into it a bit more.

I came across a suggestion in this thread: python/cpython#93453

If I use asyncio.get_event_loop_policy().get_event_loop() in run_sync then it works:

from __future__ import annotations

import asyncio
import typing as t
from concurrent.futures import ThreadPoolExecutor


def run_sync(coroutine: t.Coroutine):
    """
    Run the coroutine synchronously - trying to accommodate as many edge cases
    as possible.
     1. When called within a coroutine.
     2. When called from ``python -m asyncio``, or iPython with %autoawait
        enabled, which means an event loop may already be running in the
        current thread.
    """
    try:
        # This bit is new:
        loop = asyncio.get_event_loop_policy().get_event_loop()
    except RuntimeError:
        # No current event loop, so it's safe to use:
        return asyncio.run(coroutine)
    else:
        if not loop.is_running():
            return loop.run_until_complete(coroutine)

        # We're already inside a running event loop, so run the coroutine in a
        # new thread:
        new_loop = asyncio.new_event_loop()

        with ThreadPoolExecutor(max_workers=1) as executor:
            future = executor.submit(new_loop.run_until_complete, coroutine)
            return future.result()

I need to still wrap my head around it a bit more before pushing a fix.

@dantownsend
Copy link
Member

I had to take a different approach in the end.

Basically, the core problem is Piccolo's create_tables function is sync only, and wasn't working inside ward. I had a deep dive into the ward and asyncio codebases, and the best solution was just to make an async version of create_tables.

So in the next release, you'll be able to do this instead:

from piccolo.table import create_db_tables, drop_db_tables

@fixture
async def tables():
    table_classes = Finder().get_table_classes()
    await create_db_tables(*table_classes)
    yield
    await drop_db_tables(*table_classes)

@backwardspy
Copy link
Contributor Author

sounds perfect. :)

thanks a lot for taking a look at this. excellent work as always!

@dantownsend
Copy link
Member

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 a pull request may close this issue.

2 participants