use pyzmq tools where appropriate #1030

Merged
merged 2 commits into from Nov 23, 2011

Conversation

Projects
None yet
2 participants
@minrk
Member

minrk commented Nov 22, 2011

ZMQStream is the right object to use for event-driven handling of messages, but instead we had a duplication of half of it in KernelManager.

This removes most of the duplicate code, in favor of using ZMQStream.

also use the pyzmq install() function for using the pyzmq eventloop with tornado, instead of copying its contents into notebookapp.

use pyzmq tools where appropriate
ZMQStream is the right object to use for event-driven handling of messages, but instead we chose to rewrite half of it in KernelManager.

This removes most of the duplicate code, in favor of using ZMQStream.

also use the pyzmq install() function for using pyzmq with tornado, instead of manually pasting its contents in notebook app.
@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 23, 2011

Member

Mmh, with this I can't start the notebook at all. Does it mean it changes the minimum tornado version required?

longs[junk]> ipnb
Traceback (most recent call last):
  File "/home/fperez/usr/bin/ipython", line 7, in <module>
    launch_new_instance()
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/terminal/ipapp.py", line 392, in launch_new_instance
    app.initialize()
  File "<string>", line 2, in initialize
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/terminal/ipapp.py", line 292, in initialize
    super(TerminalIPythonApp, self).initialize(argv)
  File "<string>", line 2, in initialize
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/core/application.py", line 325, in initialize
    self.parse_command_line(argv)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/terminal/ipapp.py", line 287, in parse_command_line
    return super(TerminalIPythonApp, self).parse_command_line(argv)
  File "<string>", line 2, in parse_command_line
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 413, in parse_command_line
    return self.initialize_subcommand(subc, subargv)
  File "<string>", line 2, in initialize_subcommand
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 349, in initialize_subcommand
    subapp = import_item(subapp)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/utils/importstring.py", line 40, in import_item
    module = __import__(package,fromlist=[obj])
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/html/notebook/notebookapp.py", line 35, in <module>
    ioloop.install()
AttributeError: 'module' object has no attribute 'install'
Member

fperez commented Nov 23, 2011

Mmh, with this I can't start the notebook at all. Does it mean it changes the minimum tornado version required?

longs[junk]> ipnb
Traceback (most recent call last):
  File "/home/fperez/usr/bin/ipython", line 7, in <module>
    launch_new_instance()
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/terminal/ipapp.py", line 392, in launch_new_instance
    app.initialize()
  File "<string>", line 2, in initialize
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/terminal/ipapp.py", line 292, in initialize
    super(TerminalIPythonApp, self).initialize(argv)
  File "<string>", line 2, in initialize
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/core/application.py", line 325, in initialize
    self.parse_command_line(argv)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/terminal/ipapp.py", line 287, in parse_command_line
    return super(TerminalIPythonApp, self).parse_command_line(argv)
  File "<string>", line 2, in parse_command_line
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 413, in parse_command_line
    return self.initialize_subcommand(subc, subargv)
  File "<string>", line 2, in initialize_subcommand
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 84, in catch_config_error
    return method(app, *args, **kwargs)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/config/application.py", line 349, in initialize_subcommand
    subapp = import_item(subapp)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/utils/importstring.py", line 40, in import_item
    module = __import__(package,fromlist=[obj])
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/frontend/html/notebook/notebookapp.py", line 35, in <module>
    ioloop.install()
AttributeError: 'module' object has no attribute 'install'
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 23, 2011

Member

Sorry, the install() function is new in pyzmq-2.1.7 (from May), so I presume yours is older than that. We currently depend on 2.1.4 (except on Windows), so it should now fallback if it isn't defined.

The parallel code on Windows already depends on fixes in pyzmq-2.1.7, so we could consider updating the baseline.

Member

minrk commented Nov 23, 2011

Sorry, the install() function is new in pyzmq-2.1.7 (from May), so I presume yours is older than that. We currently depend on 2.1.4 (except on Windows), so it should now fallback if it isn't defined.

The parallel code on Windows already depends on fixes in pyzmq-2.1.7, so we could consider updating the baseline.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 23, 2011

Member

I'd suggest not forcing unix users to update to > 2.1.4 if we don't really need it (in my case I can do it easily, but someone could be in a situation where they have for some reason a harder time), but emitting a warning if you have to use the fallback. That way we prod people to upgrade to avoid the warning. What do you think of that approach?

Member

fperez commented Nov 23, 2011

I'd suggest not forcing unix users to update to > 2.1.4 if we don't really need it (in my case I can do it easily, but someone could be in a situation where they have for some reason a harder time), but emitting a warning if you have to use the fallback. That way we prod people to upgrade to avoid the warning. What do you think of that approach?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 23, 2011

Member

Since it's just this one function that we need, which is literally two lines of code, I'd leave out the warning. There are very few changes of any significance to people not using pyzmq directly, after 2.1.4. Most of the changes to pyzmq since then are user-level sugar, or related to libzmq-3.x compatibility.

Member

minrk commented Nov 23, 2011

Since it's just this one function that we need, which is literally two lines of code, I'd leave out the warning. There are very few changes of any significance to people not using pyzmq directly, after 2.1.4. Most of the changes to pyzmq since then are user-level sugar, or related to libzmq-3.x compatibility.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 23, 2011

Member

Got it. In that case, merging now. I tested it and everything seemed OK, the code looks clean, and I'm glad to see this kind of cleanup. Thanks!

Member

fperez commented Nov 23, 2011

Got it. In that case, merging now. I tested it and everything seemed OK, the code looks clean, and I'm glad to see this kind of cleanup. Thanks!

fperez added a commit that referenced this pull request Nov 23, 2011

Merge pull request #1030 from minrk/pyzmq
Use pyzmq tools when available instead of duplicating functionality.

ZMQStream is the right object to use for event-driven handling of messages, but instead we had a duplication of half of it in KernelManager.

Also use the pyzmq install() function for using the pyzmq eventloop with tornado, instead of copying its contents into notebookapp.

@fperez fperez merged commit aa2337b into ipython:master Nov 23, 2011

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1030 from minrk/pyzmq
Use pyzmq tools when available instead of duplicating functionality.

ZMQStream is the right object to use for event-driven handling of messages, but instead we had a duplication of half of it in KernelManager.

Also use the pyzmq install() function for using the pyzmq eventloop with tornado, instead of copying its contents into notebookapp.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment