Skip to content

Conversation

johnzhou721
Copy link
Contributor

No description provided.

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.

Remove myself from credits, obviously.

@hosaka
Copy link
Collaborator

hosaka commented Jul 14, 2025

This breaks qasync.run, for example with a simple test:

def test_run_with_contextmanager():
    async def coro():
        event_loop = asyncio.get_event_loop()
        assert type(event_loop).__name__ == "QSelectorEventLoop"
        await asyncio.sleep(0)

    qasync.run(coro())

    event_loop = asyncio.get_event_loop()
    assert type(event_loop).__name__ != "QSelectorEventLoop"

While not used internally, or in any example, we probably shouldn't break the run() interface. You're also not passing the argv to QApplication.

It's also worth pointing out that we currently support python 3.8 - 3.12. The event loop policies will be deprecated in 3.14 and removed in 3.16. This doesn't concern qasync currently, until we decided to support 3.14+, by adding it to CI test targets.

@johnzhou721
Copy link
Contributor Author

@hosaka I've made the requested changes, please review again. Thank you!

@johnzhou721
Copy link
Contributor Author

@hosaka if we want to support the older event loop policy, I think we can add a version check. Let me know if that is desired.

@johnzhou721
Copy link
Contributor Author

Hi @hosaka, could you please check any errors with this impl? Ty!

@hosaka
Copy link
Collaborator

hosaka commented Jul 21, 2025

Hi, thanks for your suggestions. I think this change should come together/post adding tests and support for 3.14. What is it solving on its own, just removing the "deprecated" warnings?

@johnzhou721
Copy link
Contributor Author

it’s getting removed in latest stable python in a year and a few months. I will add test and support later.

@johnzhou721
Copy link
Contributor Author

@hosaka Could you please approve CI for me? I don't have the required disk space to install more versions of Python.

@hosaka
Copy link
Collaborator

hosaka commented Jul 22, 2025

There are no CI tests that exercise run() function btw, which should be added. We can rename this PR into "Add support for Python 3.14" if that's okay with you. According to PEP 745 release date for 3.14 is Tuesday, 2025-10-07 and this is only deprecating loop policies. It will be removed in 3.16 which doesn't have a release date yet, AFAIK.

If you could add the CI changes that add 3.14 target to the matrix, add tests for the run() function to make sure it behaves the same as it does now (either using a version check or otherwise wrapping the asyncio.run() interface) and add the changes you've already made removing the policies, I will be happy to merge it.

@johnzhou721
Copy link
Contributor Author

@hosaka Sorry, read Python 3.15 and not 3.16.

PySide2 has no macOS arm64 wheels -- should we skip these tests on that platform, or use a macOS x86_64 runner instead? That'll be the first issue to fix such that the CI passes every run.

I'd also need tests covering run() that passes on main as well.

@johnzhou721 johnzhou721 changed the title Remove references to event loop policies. Suppport Python 3.14 Jul 22, 2025
@johnzhou721
Copy link
Contributor Author

@hosaka May I get another CI approval? Thank you!

@hosaka
Copy link
Collaborator

hosaka commented Jul 22, 2025

@johnzhou721 I think using macOS x86_64 runner would be preferred, we should support platforms where pyside/pyqt is available. As for the run() test, you can pretty much copy-paste my code from #131 (comment) or expand it further.

@johnzhou721
Copy link
Contributor Author

@hosaka I have done the latter. The former would be done later, however, I get FAILED tests/test_run.py::test_run_with_contextmanager - RuntimeError: There is no current event loop in thread 'MainThread'. if I run the whole test suite but no failures if pytest tests/test_run.py. Can you think of any reason? Thank you!

@johnzhou721
Copy link
Contributor Author

@hosaka I have fixed that; but rather than rerunning a workflow, could you please just give me approval? Thanks!

@hosaka
Copy link
Collaborator

hosaka commented Jul 22, 2025

@johnzhou721 I don't have the permissions to modify github actions settings. The only thing I can do is to approve the workflow.

@johnzhou721
Copy link
Contributor Author

that is good.

@hosaka
Copy link
Collaborator

hosaka commented Jul 22, 2025

@johnzhou721 Looking at the pipeline here, I think we better to avoid trying to prematurely implement this and wait for (at least) the following to happen:

  • 3.14 release (instead of adding 3.14-dev target)
  • PySide6 release that supports 3.14 (current package explicitly uses Python <3.14, >=3.9)

Without digging further into why other test cases fail, the above two are enough to put a hold on this, considering very little benifit from removing the event loop policies right now. Would you agree?

@johnzhou721
Copy link
Contributor Author

@hosaka Would you mind if I hold this PR for now and open another PR for adding the test case for run()?

@hosaka
Copy link
Collaborator

hosaka commented Jul 22, 2025

Sure thing, a test case against existing implementation with policies would be great in a separate PR. Also note, I've opened another PR that adds 3.11 to the test matrix, as well as properly excludes pyside2 on macos-arm64.

There is a small change to an old test case that needs some explanation. I'll comment on it before merging it.

@hosaka hosaka assigned hosaka and unassigned hosaka Jul 23, 2025
@johnzhou721
Copy link
Contributor Author

@hosaka however eventually this must be done; BeeWare is tracking this as an actual issue, and they’ve done the steps for macOS and iOS already, so I think we need this solved.

@hosaka
Copy link
Collaborator

hosaka commented Jul 23, 2025

I'm sorry I'm not following that toga project closely, can you link to where this is being tracked/treated as an issue for future visibility? This should be worked on after python 3.14 release, as well as pyside/pyqt support for 3.14, and we have a LOT of time until 3.16 when this deprecation becomes actual removal.

I assume you're referring to beeware/toga#1142 (comment)?

@johnzhou721
Copy link
Contributor Author

beeware/toga#3531

@hosaka
Copy link
Collaborator

hosaka commented Jul 23, 2025

I'm assuming you first need to have a working Qt backend for toga before worrying about an upcoming deprecation triggered by qasync? I'm going to keep #130 open and we can deal with it when 3.14 is out.

@johnzhou721
Copy link
Contributor Author

@hosaka Yes, but by the point I finish the Qt backend, Python 3.14 might as well already be out, so I was trying to get this resolved sooner than later. Looking at the CI fails, it seems that the 3.11 PR already solved most of the issues; with the other adding of tests, the only required changes are just 2 to 3 files to support Python 3.14; would you mind if I opened a Draft PR with these changes, just for the sake of keeping track of them?

@hosaka
Copy link
Collaborator

hosaka commented Jul 23, 2025

Sure thing, you can open a new PR and keep it in the draft state. Either way, the PR would only make sense when we get to 3.14.

@johnzhou721
Copy link
Contributor Author

Refs #140.

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