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

ENH: add %gui support for Qt6 #1054

Merged
merged 21 commits into from Dec 26, 2022
Merged

Conversation

shaperilio
Copy link
Contributor

Addresses #775

  1. Support for Qt 4, 5, and 6, via PyQt or PySide. Replicated logic from IPython (e.g. prefer PyQt over PySide)
  2. Put Qt importing at the level of enable_gui so that any import errors show up in the client as opposed to in the kernel's console.
  3. Added checks for more helpful error messages / warnings when you can't / aren't going to get the version you're requesting.
  4. Included relevant parts of @tacaswell's work in ENH: add support for qt6 #782

 - Distinguish between specific version requests and the generic one.
 - Use a `QEventLoop` instance to properly keep windows open between event loop calls.

 This is "instrumented" with print statements to follow the flow
This way import errors show up in the client, not the kernel.
@blink1073
Copy link
Member

Thanks for working on this @shaperilio!

@ccordoba12, mind taking a look at this and/or testing against Spyder?

@shaperilio
Copy link
Contributor Author

shaperilio commented Dec 9, 2022

In case it helps, I've published a notebook I used to test functionality. Each "case" should be run on a fresh kernel.

It would be nice to turn these into unit tests, but I'm afraid setting those up is over my head. I'd be happy to take a stab at it if someone can point me in the right direction.

@shaperilio
Copy link
Contributor Author

I'm playing with this in Linux now and have become aware that IPython is not quite there. I guess I got mixed up yesterday while working on ipykernel. See this issue.

Effectively, the logic here is not in IPython, i.e. if you request %gui qt6 in straight IPython, it will never try to use PySide6. Since ipykernel defers to IPython to import Qt, I'm thinking the logic should be all in one place (and probably IPython). Any thoughts?

@tacaswell
Copy link
Contributor

https://github.com/matplotlib/matplotlib/blob/1d2abe7ab909ebca32908694fc3e3828c37cf11d/lib/matplotlib/tests/test_backends_interactive.py#L24-L68 this and matplotlib.testing.subprocess_run_helper may be a useful pattern to copy.

shaperilio added a commit to shaperilio/ipython that referenced this pull request Dec 12, 2022
@shaperilio
Copy link
Contributor Author

I wrote a bit of a unit test, but it won't run unless we make the CI install a couple of versions of either PyQt or PySide. How do I do that / do we want that?

Importing a second version of Qt is not allowed. `IPython`
silently ignores requests for different versions; we want
`enable_gui` to raise an exception so the user can see it.
@blink1073
Copy link
Member

We can create a coverage matrix:

diff --git a/pyproject.toml b/pyproject.toml
index 9904b54..05be2c2 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -62,6 +62,7 @@ cov = [
   "curio",
   "trio",
 ]
+pyqt5 = ["pyqt5"]

 [tool.hatch.version]
 path = "ipykernel/_version.py"
@@ -92,6 +93,15 @@ features = ["test", "cov"]
 test = "python -m pytest -vv --cov ipykernel --cov-branch --cov-report term-missing:skip-covered {args}"
 nowarn = "test -W default {args}"

+[[tool.hatch.envs.cov.matrix]]
+qt = ["qt5", "qt6"]
+
+[tool.hatch.envs.cov.overrides]
+matrix.qt.features = [
+  { value = "pyqt5", if = ["qt5"] },
+  { value = "pyqt6", if = ["qt6"] },
+]
+
 [tool.hatch.envs.typing]
 features = ["test"]
 dependencies = ["mypy>=0.990"]

So when you call hatch run cov:test, it would run with each variant.

@blink1073
Copy link
Member

blink1073 commented Dec 20, 2022

You can run just one variant locally as hatch run cov.qt5:test

@shaperilio
Copy link
Contributor Author

OK, I think this is ready! I've put code in this pull request to IPython which replicates the behavior here.

The only difference is that, in IPython, 'qt4' is an alias to 'qt', so you can never explicitly get a Qt4 / PySide event loop there. That's not the case here. I left the behavior in IPython because I don't know the history of that alias.

@shaperilio
Copy link
Contributor Author

@blink1073 sorry about that - I had removed the windows skips because those tests passed fine on my windows machine.

@blink1073
Copy link
Member

I had removed the windows skips because those tests passed fine on my windows machine.

No worries! Not sure why they hang in CI then, but things like that happen for other platforms too. 😄

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

This is great, thanks! I'll leave it open for a few days in case anyone else gets a chance to review.

@blink1073 blink1073 enabled auto-merge (squash) December 26, 2022 14:25
@blink1073 blink1073 merged commit fbea757 into ipython:main Dec 26, 2022
@ccordoba12
Copy link
Member

ccordoba12 commented Dec 27, 2022

@blink1073, this PR broke the %matplotlib qt magic in Spyder (detected by one of our tests). So, could you yank it on PyPI? I'm still investigating how to fix it (along with other problems I found with this approach).

@blink1073
Copy link
Member

Yanked

@ccordoba12
Copy link
Member

Thanks @blink1073! And sorry for not reviewing it on time, I'm finishing a Spyder release right now.

@blink1073
Copy link
Member

I'd love to add spyder as a downstream test, how feasible do you think that is?

@ccordoba12
Copy link
Member

That'd be great! Thanks for the offer. I think adding our Spyder-kernels test suite shouldn't be that hard.

As for the Spyder tests (which also exercise our kernel under different scenarios), we could only run a file called test_ipythonconsole.py (I could give you a hand with that one because it's harder to setup).

@ccordoba12
Copy link
Member

ccordoba12 commented Dec 27, 2022

BTW, I found a simple way to solve the regressions introduced by this PR. I hope to submit a fix in the next couple of days, after testing that change against our two test suites.

@blink1073
Copy link
Member

Great, thanks!

@shaperilio
Copy link
Contributor Author

Thanks y'all; @ccordoba12, let me know if I can be of any help.

Carreau pushed a commit to shaperilio/ipython that referenced this pull request Jan 3, 2023
Carreau added a commit to ipython/ipython that referenced this pull request Feb 14, 2023
Addresses #13859

Changes here parallel the changes in
[ipykernel](ipython/ipykernel#1054), i.e. prefer
`PyQt` over `PySide` and allow requesting explicit versions (e.g. 'qt5')
with one difference (see below).

I believe that, eventually, the Qt importing logic should be all here,
and `ipykernel` should defer to`ipython`. I chose not to do that at this
time since it would mean the latest `ipykernel` would require the latest
`IPython`; I'm happy to discuss further.

The only difference between `IPython` and `ipykernel`, after these two
pull requests, is that, in `IPython`, it's not possible to explicitly
request an event loop for Qt4. This is because an alias exists
[here](https://github.com/ipython/ipython/blob/5409de68d87ddd073a35111aca0cb8360ff63ca8/IPython/terminal/pt_inputhooks/__init__.py#L5)
which effectively makes "qt4" be "the latest Qt available". I did not
remove the alias because I don't know the history behind it.
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.

None yet

4 participants