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

Revert latest changes to ThreadedZMQSocketChannel because they break Qtconsole #803

Merged
merged 3 commits into from
Jun 8, 2022

Conversation

ccordoba12
Copy link
Contributor

@ccordoba12 ccordoba12 commented Jun 8, 2022

  • Those changes were done in commit 0057186, released in version 7.3.2.
  • They break Qtconsole and hence Spyder, but I checked locally that a simple revert fixes the problem.
  • I added the qtconsole test suite here to avoid future breakages.

@ccordoba12 ccordoba12 changed the title Revert the latest changes done to ThreadedZMQSocketChannel because they break Qtconsole Revert latest changes done to ThreadedZMQSocketChannel because they break Qtconsole Jun 8, 2022
@ccordoba12 ccordoba12 force-pushed the fix-threaded-client branch 11 times, most recently from 100151f to f7c7673 Compare June 8, 2022 19:18
@ccordoba12 ccordoba12 force-pushed the fix-threaded-client branch from f7c7673 to 3c6fc38 Compare June 8, 2022 19:35
@ccordoba12
Copy link
Contributor Author

ccordoba12 commented Jun 8, 2022

@blink1073, this is ready for review. As you can see, the Qtconsole test suite passes with these changes (you can check that it dies with Jupyter-client 7.3.3).

Note: I don't know how to fix the MyPy error, so I need help with that.

@ccordoba12 ccordoba12 changed the title Revert latest changes done to ThreadedZMQSocketChannel because they break Qtconsole Revert latest changes to ThreadedZMQSocketChannel because they break Qtconsole Jun 8, 2022
@ccordoba12 ccordoba12 changed the title Revert latest changes to ThreadedZMQSocketChannel because they break Qtconsole Revert latest changes to ThreadedZMQSocketChannel because they break Qtconsole Jun 8, 2022
@blink1073 blink1073 added the bug label Jun 8, 2022
@blink1073
Copy link
Contributor

Hi @ccordoba12, I think this might be an error in the typings in pyzmq. I added a type ignore for now.

Copy link
Contributor

@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.

Thanks very much for adding the qtconsole test!

@blink1073
Copy link
Contributor

Docs failure is unrelated, I'll pick that up in a separate PR.

@ccordoba12
Copy link
Contributor Author

ccordoba12 commented Jun 8, 2022

I think this might be an error in the typings in pyzmq. I added a type ignore for now.

Great! Thanks for your help with that.

Thanks very much for adding the qtconsole test!

No prob. Jupyter-client is critical for us, so that's something we really needed to do (I'll do the same in IPykernel once I have some time).

@blink1073 blink1073 merged commit 2c54559 into jupyter:main Jun 8, 2022
@blink1073
Copy link
Contributor

I'll cut a release once I merge #804

@ccordoba12 ccordoba12 deleted the fix-threaded-client branch June 8, 2022 20:00
@davidbrochart
Copy link
Member

Thanks @blink1073 and @ccordoba12 for taking care of that.
Something is still not clear to me, is the callback passed to ZMQStream.on_recv an awaitable or not in pyzmq v23.0?
I raised that point in the original PR. Would love some feedback from @minrk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants