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

Add preprocessor to execute notebooks #5639

Closed
wants to merge 19 commits into from
Closed

Add preprocessor to execute notebooks #5639

wants to merge 19 commits into from

Conversation

jvns
Copy link
Contributor

@jvns jvns commented Apr 16, 2014

This adds a preprocessor to execute all the code cells in a notebook.

I basically copied all of this code from @damianavila's gist here: https://gist.github.com/damianavila/5208296.

I don't totally understand all of the error handling. This probably wants more robust unit tests, but I'm not sure exactly what I should be testing.

@jvns
Copy link
Contributor Author

jvns commented Apr 16, 2014

I also shut down the kernel in __del__, and there may be a better place.

self.km = KernelManager()
# run %pylab inline, because some notebooks assume this
# even though they shouldn't
self.km.start_kernel(extra_arguments=['--pylab=inline'], stderr=open(os.devnull, 'w'))
Copy link
Member

Choose a reason for hiding this comment

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

it definitely should not start with --pylab=inline

Copy link

Choose a reason for hiding this comment

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

+1 for making it an option; it should handle --matplotlib=inline as well

Copy link
Member

Choose a reason for hiding this comment

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

No, it should handle neither.

Copy link

Choose a reason for hiding this comment

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

My mistake, I somehow misinterpreted your comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be an option, defaulting to no arguments. As much as we might like everyone to never run the notebook with --pylab inline, people do run it with arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Issues is in 3.0, starting notebook server with --pylab=inline will not work. So kernel started with --pylab=inline in nbconvert will make no sens. The only thing that will make sens is chosing the kernel you want to use to run the notebook.

Copy link
Member

Choose a reason for hiding this comment

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

It should not handle the --pylab option... I agree...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. What would be the best way to make it possible to run it with %pylab inline or %matplotlib inline that would work in 3.0?

@paulgb
Copy link

paulgb commented Apr 16, 2014

Thanks @Carreau for pointing me here.

@jvns feel free to test with the runipy test suite. It's small but covers basically everything that was broken in the original runipy and later fixed: https://github.com/paulgb/runipy/tree/master/tests

I'm happy to see this behaviour making its way into IPython!

@Carreau
Copy link
Member

Carreau commented Apr 16, 2014

hum, Travis seem unhappy because of pandoc. restarting it.

@Carreau
Copy link
Member

Carreau commented Apr 17, 2014

Travis still seem to get the wrong pandoc... wondering why... restarting again.

@jlstevens
Copy link
Contributor

Hi

I'm very glad to see this functionality making its way into IPython. I would just like to ask whether this design will allow me to batch run notebooks from the commandline?

So far I have not found a decent way to automatically run notebooks so that display hooks are run properly and that the resulting notebook is saved back to disk (with the fresh display output). This is very important to me as a Topographica developer as that project is now making heavy use of notebooks and we need this type of automation for testing purposes.

My unsatisfactory solution has been to hack my own copy of runipy (which was a very helpful starting point) to load our custom IPython profile, run a %load_ext command (with shell.execute) and then save the result on completion to a new .ipynb file.

I am eager to see that this execute preprocessor can help me to avoid such hackery in IPython 3.0!

Thank you

@Carreau
Copy link
Member

Carreau commented Apr 21, 2014

I'm very glad to see this functionality making its way into IPython. I would just like to ask whether this design will allow me to batch run notebooks from the commandline?

I have to check, but IIRC not yet, there are still a few missing pieces in nbconvert. In 3.0 I would say probably.

@jdfreder
Copy link
Member

I have to check, but IIRC not yet, there are still a few missing pieces in nbconvert. In 3.0 I would say probably.

#5691

@jvns
Copy link
Contributor Author

jvns commented Apr 24, 2014

Okay, I added the tests from runipy (thanks @paulgb!), and fixed the issues @minrk pointed out (thanks!).

Are there more issues blocking this? I'd like to make it possible in some way to run notebooks with %pylab inline and %matplotlib inline, but that could be a separate PR after this is merged, since there seems to be disagreement about that.

@jvns
Copy link
Contributor Author

jvns commented Apr 24, 2014

The Travis build seems to be failing but I think that's because of pandoc.

@minrk
Copy link
Member

minrk commented Apr 24, 2014

I'd like to make it possible in some way to run notebooks with %pylab inline and %matplotlib inline

This should already work as long as the %matplotlib magic is called in the notebook itself. If that is not in the notebook, then the notebook shouldn't be expecting inline figures.

@minrk
Copy link
Member

minrk commented Apr 24, 2014

The tests are failing because your test files aren't installed. I think you will need to add a glob to setupbase.find_package_data.

@jvns
Copy link
Contributor Author

jvns commented Apr 24, 2014

what does "your test files aren't installed" mean?

@jvns
Copy link
Contributor Author

jvns commented Apr 24, 2014

so I agree that it shouldn't be expecting inline figures, but in real life lots of people have notebooks that they've created with --pylab inline or --matplotlib inline. It seems like it would be more useful to people if it could handle legacy things (and maybe give a warning).

@minrk
Copy link
Member

minrk commented Apr 24, 2014

Sorry, files that don't end with '.py' in the source tree are not installed by default. You have to tell Python to install things things via a structure called 'package_data', which is populated in setupbase.find_package_data. You will need to add file globs for the new test files you have added here, in order for them to be included in an install of IPython.

@jvns
Copy link
Contributor Author

jvns commented Apr 24, 2014

I changed find_package_data(). We'll see if it fixes Travis! Thanks @minrk =)

import os
import sys

from Queue import Empty
Copy link
Member

Choose a reason for hiding this comment

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

This is queue on Python 3.

@jhamrick
Copy link
Contributor

jhamrick commented May 7, 2014

I'm really excited to see this becoming a preprocessor! 👍

I was playing around with it a bit, and found that the prompt number actually isn't being updated in the notebook. I think rather than checking if the message type is pyout and then setting the prompt number only for the output, you need to check if execution_count is in the content and then set the prompt number for both the cell and the output.

Happy to submit a pull request to your branch @jvns if you'd like!

@jvns
Copy link
Contributor Author

jvns commented May 7, 2014

yes please!
On May 7, 2014 2:50 AM, "Jessica B. Hamrick" notifications@github.com
wrote:

I'm really excited to see this becoming a preprocessor! [image: 👍]

I was playing around with it a bit, and found that the prompt number
actually isn't being updated in the notebook. I think rather than checking
if the message type is pyout and then setting the prompt number only for
the output, you need to check if execution_count is in the content and
then set the prompt number for both the cell and the output.

Happy to submit a pull request to your branch @jvnshttps://github.com/jvnsif you'd like!


Reply to this email directly or view it on GitHubhttps://github.com//pull/5639#issuecomment-42394866
.

@minrk minrk added this to the 3.0 milestone May 7, 2014
@jlstevens
Copy link
Contributor

Just to say I've just tested out this PR and it largely works as I hoped. Unfortunately, I notice that sometimes the ordering of output is wrong with output appearing underneath an unrelated code cell.

Otherwise, I'm looking forward to using this as soon as it is merged!

@minrk
Copy link
Member

minrk commented May 14, 2014

@jvns can you rebase the PR? There are going to need to be a few changes due to recent things in the message spec, but the PR needs to be based on the latest master, first. If not, I can do the rebase.

@minrk
Copy link
Member

minrk commented May 14, 2014

Sorry, that was for @jvns.

@jvns
Copy link
Contributor Author

jvns commented May 16, 2014

I'm a bit swamped right now and probably won't get to this for a bit =\

@tbekolay tbekolay mentioned this pull request May 19, 2014
10 tasks
@minrk minrk mentioned this pull request Jun 26, 2014
@minrk
Copy link
Member

minrk commented Jun 26, 2014

I've rebased this and added the necessary changes for msg spec 5 in #6053.

@minrk minrk closed this in f64a9ca Jul 1, 2014
@minrk minrk modified the milestones: no action, 3.0 Jul 1, 2014
@minrk
Copy link
Member

minrk commented Jul 1, 2014

Merged as #6053. Thanks!

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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.

None yet

10 participants