[IPython-dev] Code review of html frontend

Brian Granger ellisonbg at gmail.com
Fri Oct 29 13:44:22 EDT 2010


James,

Here is a longer code review of the html frontend.  Overall, this is
really nice.  Great job!

====================
HTML Notebook Review
====================

General
=======

* Add IPython standard copyright to each file and put an
  author section with yourself in the module level docstrings.
* Use a space after the "#" in comments.
* We should refactor the main KernelManager to consist of two
  classes, one that handles the starting of the kernel process
  and a second that handles the channels. In this case, there
  is not reason to have all the channel stuff, as I think
  we can simple use comet or websockets to pass the raw
  ZMQ JSON messages between the client and the kernel. The
  only reason we might not want to do this is to allow us
  to validate the messages in the web server, but really we
  should be doing that in the kernel anyways. This would
  make the webserver stuff even thinner.
* Let's document clearly the URL structure we are using. It
  will be much easier to do this if we move to tornado.
* Let's make sure we develop the html/js stuff in a way that
  will be independent of the webserver side of things, that
  way, in the future, we can easily swap out either component
  if we want.
* Please add brief docstrings to the important methods.
* We will probably have multiple html frontends, so we should
  probably put your stuff in something like html/basic or
  html/basicnb. We did this for the qt frontend (qt/console).
* For now I would mostly focus on the Javascript and UI side of
  things, as I think we probably want to move to Tornado
  even for simple servers like this.

UI
==

* The results of ? and ?? only go to one client.

ipythonhttp.py
==============

* We should discuss the possibility of using Tornado as our
  web server. It will be a much better match for working
  with zmq and many things will be much easier. We are
  already shipping a good part of Tornado with pyzmq and
  could possibly ship all of it. Using the stdlib for now
  is fine though. Tornado also has websocket support that
  would work extremely well with pyzmq.
* Remove command line options that you don't support yet.
* Move defer to the top level and rename something like
  "start_browser". Move import threading to top level as well.


kernelmanager.py
================

* In do_GET and do_POST, document what each elif clause is
  handling.
* I see that you are using time.time for the client id. Would
  it be better to use a real uuid? If you want to also have
  the time you could still pass that to manager.register as
  well.
* When a kernel dies, the client should note that and there
  should be an option to restart the kernel.
* We should probaby have top level URLs for the different
  ZMQ sockets like /XREQ, /SUB, etc. so that the GET and POST
  traffic has different URLS.

CometManager.py
================

* Change filename to cometmanager.py.


-- 
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
bgranger at calpoly.edu
ellisonbg at gmail.com



More information about the IPython-dev mailing list