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

Remove usage of QVersionNumber because it doesn't work as expected in PySide6 #568

Merged
merged 5 commits into from
Mar 7, 2023

Conversation

psobolewskiPhD
Copy link
Contributor

This PR closes #567
If using pyside6 backend, QVersionNumber.fromString('5.15.2') returns a PySide6.QtCore.QVersionNumber and as a result it is not subscriptable, so launching the console results in a TypeError.
Because the code refers to Qt5 versions, I've added a check that the backend isn't Qt6 prior to the version checking.
On my mac this console now works in pyside6 6.4.2, though there is a warning:

python3.10/site-packages/jupyter_client/threaded.py:73: RuntimeWarning: ZMQStream only supports the base zmq.Socket class.

                Use zmq.Socket(shadow=other_socket)
                or `ctx.socket(zmq.SUB, socket_class=zmq.Socket)`
                to create a base zmq.Socket object,
                no matter what other kind of socket your Context creates.
                
  self.stream = zmqstream.ZMQStream(self.socket, self.ioloop)

I think that is unrelated to this PR and probably requires another fix.

@psobolewskiPhD
Copy link
Contributor Author

Hmm, this PR shouldn't affect anything in the linux tests.
So I think the failed linux tests are likely because the ubuntu runners are now 22.04
actions/runner-images#5490
I guess something needs to be changed in the system apt install part of the workflow?

Copy link

@akirchhoff-modular akirchhoff-modular left a comment

Choose a reason for hiding this comment

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

As one data point, I was encountering this issue as well, and this PR fixes it. The change looks straightforward and makes sense. I hope this can be merged soon.

Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @psobolewskiPhD for your contribution!

qtconsole/qtconsoleapp.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Besides my small suggestion below, please add an entry for packaging in setup.py because this is a new dependency. Specifically, you need to add it to this list:

qtconsole/setup.py

Lines 68 to 77 in 55a8fb0

install_requires = [
'traitlets!=5.2.1,!=5.2.2',
'ipython_genutils',
'jupyter_core',
'jupyter_client>=4.1',
'pygments',
'ipykernel>=4.1', # not a real dependency, but require the reference kernel
'qtpy>=2.0.1',
'pyzmq>=17.1'
],

qtconsole/qtconsoleapp.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title Bugfix: Work around for QVersionNumber.fromString() return change in PySide6 Remove usage of QVersionNumber because it doesn't work as expected in PySide6 Mar 7, 2023
@ccordoba12 ccordoba12 added this to the 5.4.1 milestone Mar 7, 2023
psobolewskiPhD and others added 2 commits March 7, 2023 17:22
Co-authored-by: Carlos Cordoba <[email protected]>
add missing newline

Co-authored-by: Carlos Cordoba <[email protected]>
Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @psobolewskiPhD!

@ccordoba12 ccordoba12 merged commit 44dfb98 into jupyter:master Mar 7, 2023
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.

Crash on PySide6 due to version checking
3 participants