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

Stop using deprecated recv_multipart when using in-process socket. #762

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Sep 1, 2021

Found while working on napari/napari#3314

I'm not too sure this is the right fix, as BackgroundSocket is used only
in inprocess kernel, and that in general io_pub socket looks like the
can be Any() and that therefore may not have a io_thread.

But if that's the case, then should we maybe un-deprecate recv_multipart on background socket ?

the recv in jupyter_client side (which is called by the line I change
here) is

def recv(self, socket, mode=zmq.NOBLOCK, content=True, copy=True):
    """Receive and unpack a message.

    Parameters
    ----------
    socket : ZMQStream or Socket
        The socket or stream to use in receiving.

    Returns
    -------
    [idents], msg
        [idents] is a list of idents and msg is a nested message dict of
        same format as self.msg returns.
    """
    if isinstance(socket, ZMQStream):
        socket = socket.socket
    try:
        msg_list = socket.recv_multipart(mode, copy=copy) # this will trigger deprecation warning
    except zmq.ZMQError as e:
        ...

And I doubt we want to make that aware of background socket.

@Carreau
Copy link
Member Author

Carreau commented Sep 1, 2021

I believe @minrk and or @SylvainCorlay might have more experience around this area.

@minrk
Copy link
Member

minrk commented Sep 2, 2021

Fine with undeprecating recv_multipart, or adding a check here that does an isinstance if it's a socket itself or a BackgroundSocket.

@Carreau
Copy link
Member Author

Carreau commented Sep 6, 2021

or adding a check here that does an isinstance if it's a socket itself or a BackgroundSocket.

Actually while doing that I just realized that

class InProcessKernel(IPythonKernel):

    ...

    iopub_socket = Instance(BackgroundSocket)

So it can only be a BackgroundSocket or subclass so the isinstance seem unnecessary and this is likely the right fix.

@Carreau Carreau marked this pull request as ready for review September 6, 2021 20:59
Found while working on napari/napari#3314

This should be the right fix, as BackgroundSocket is used only
in inprocess kernel, and while in general iopub_socket looks like it
can be `Any()` for this particular class we have a trait saying
iopub_socket has to be a BackgroundSocket

The recv in jupyter_client side (which is called by the line I change
here) is

    def recv(self, socket, mode=zmq.NOBLOCK, content=True, copy=True):
        """Receive and unpack a message.

        Parameters
        ----------
        socket : ZMQStream or Socket
            The socket or stream to use in receiving.

        Returns
        -------
        [idents], msg
            [idents] is a list of idents and msg is a nested message dict of
            same format as self.msg returns.
        """
        if isinstance(socket, ZMQStream):
            socket = socket.socket
        try:
            msg_list = socket.recv_multipart(mode, copy=copy) # this will trigger deprecation warning
        except zmq.ZMQError as e:
            ...

And I doubt we want to make that aware of background socket.
@Carreau
Copy link
Member Author

Carreau commented Sep 6, 2021

FOrce pushed identical commit but with above note added to the text.

@minrk minrk merged commit c810559 into ipython:master Sep 7, 2021
@blink1073 blink1073 added this to the 6.4 milestone Sep 9, 2021
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.

3 participants