Skip to content

Conversation

johnzhou721
Copy link
Contributor

No description provided.

@johnzhou721
Copy link
Contributor Author

I've merged the code of #134 into here as well, so we can CI-test on this branch if it takes a long time for #134 to get merged. When #134 get merged, I will do a rebase such that the changes are no longer applied.

@hosaka: Please approve CI for this and also #134.

@johnzhou721 johnzhou721 force-pushed the py316-deprecate branch 3 times, most recently from 6511cd5 to 930c53a Compare July 23, 2025 18:53
@johnzhou721
Copy link
Contributor Author

FYI: I've made this such that it's stacked on top of #134 so we can test here before #134 gets merged; will rebase after that gets merged.

Copy link
Contributor Author

@johnzhou721 johnzhou721 left a comment

Choose a reason for hiding this comment

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

Please approve CI as well, I want to see how it turns out, that way we can simply merge this day after py314 is released. Thanks!

with _set_event_loop_policy(DefaultQEventLoopPolicy()):
return asyncio.run(*args, **kwargs)
# A run function matching the signature of asyncio.run
def run(main_coro, *, debug=None, loop_factory=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hosaka I tried to mimic the official asyncio.run interface, but is loop_factory sort of... extra? I mean purpose of using qasync.run is to run with the qasync event loop, not any other ones (use asyncio.run for that, I guess).

@johnzhou721
Copy link
Contributor Author

@hosaka It seems that the flaky test for x86_64 3.12 qt6 need to be solved, else it will plague more CI runs.

@johnzhou721
Copy link
Contributor Author

My CI is approved, but I repent for wasting so much resources because I can't get CI configs right, but there's no way to test CI offline... there's act but I can't seem to sset it up wright.

@hosaka
Copy link
Collaborator

hosaka commented Jul 24, 2025

I think rather than mimicing asyncio.run (admittedly I also thought we should do it this way), we should adopt using asyncio.run(coro(), loop_factory=QEventLoop) or asyncio.Runner as suggested in the policies deprecation warning and asyncio.run() docs.

The goal is to preserve the current behaviour of qasync.run() with policies removed or version guarded so it doesn't break existing code and move forward to using asyncio.run instead.

@hosaka
Copy link
Collaborator

hosaka commented Jul 24, 2025

So for >3.12 qasync.run can be a simple wrapper for

asyncio.run(coro, *args, **kwargs, loop_factory=QEventLoop(QApplication.instance() or QApplication(sys.argv))

@johnzhou721
Copy link
Contributor Author

@hosaka Let's do your proposition for >=3.14.

@hosaka
Copy link
Collaborator

hosaka commented Jul 24, 2025

I think something like this would be enough, removing all loop policy related code, keeping qasync.run backwards compatible and using asyncio.run for >3.12:

def _get_qevent_loop():
    return QEventLoop(QApplication.instance() or QApplication(sys.argv))


@contextlib.contextmanager
def _use_event_loop():
    old_loop = None
    try:
        old_loop = asyncio.get_event_loop()
    except RuntimeError:
        pass
    loop = _get_qevent_loop()
    asyncio.set_event_loop(loop)
    try:
        yield
    finally:
        loop.close()
        asyncio.set_event_loop(old_loop)


def run(*args, **kwargs):
    if sys.version_info >= (3, 12):
        return asyncio.run(*args, **kwargs, loop_factory=_get_qevent_loop)
    else:
        with _use_event_loop():
            return asyncio.run(*args, **kwargs)

This passes all the tests, with the exception of adding a skip to test_qasync_run_restores_policy which needs to look like:

@pytest.mark.skipif(sys.version_info < (3, 12), reason="Requires Python 3.12+")
def test_qasync_run_restores_policy(get_event_loop_coro):

@johnzhou721
Copy link
Contributor Author

FWIW I'm getting this wwarning

collecting: 8/10 workers/opt/anaconda3/lib/python3.10/site-packages/pytest_asyncio/plugin.py:217: PytestDeprecationWarning: The configuration option "asyncio_default_fixture_loop_scope" is unset.
The event loop scope for asynchronous fixtures will default to the fixture caching scope. Future versions of pytest-asyncio will default the loop scope for asynchronous fixtures to function scope. Set the default fixture loop scope explicitly in order to avoid unexpected behavior in the future. Valid fixture loop scopes are: "function", "class", "module", "package", "session"

  warnings.warn(PytestDeprecationWarning(_DEFAULT_FIXTURE_LOOP_SCOPE_UNSET))
10 workers [53 items]   

@hosaka
Copy link
Collaborator

hosaka commented Jul 24, 2025

We don't use pytest-asyncio

@johnzhou721
Copy link
Contributor Author

@hosaka Good to know. I think this is ready, macOS-x86_64 is an intermittent failure that appears quite often that need to get resolved separately; otherwise, let's just wait for Python 3.14 release to and Let's Get This Merged.

Would you agree?

@hosaka
Copy link
Collaborator

hosaka commented Jul 25, 2025

The version check should be agains 3.12, not 3.14. Since loop_factory is available from 3.12, we should be using that and fallback to policies. I shuffled your code a bit to reflect that. The test_qasync_run_restore_policy also needs to be skipped.

Seeing how little these changes actually connected to 3.14, please remove all your changes to main.yaml and we can merge this now without waiting for 3.14 release. When it is released, we can include it in the CI.

@johnzhou721
Copy link
Contributor Author

@hosaka Done, also since CI is failing a lot on TimeoutError, I increased all the 3.0 and 5.0 second timeouts to 10.0 seconds.

@hosaka hosaka changed the title Support Python 3.14 (DRAFT) Use asyncio loop_factory instead of policies for python>=3.12 Jul 25, 2025
@hosaka hosaka marked this pull request as ready for review July 25, 2025 15:06
@hosaka hosaka merged commit 16f3f7e into CabbageDevelopment:master Jul 25, 2025
73 checks passed
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