Change progress bar instantiation for IPython >= 3 compatibility.

[jgosmann]

• Add RC option to set progress bar.
• Remove IPython notebook detection (cannot be done with IPython
  notebook >3 aka Jupyter)
• Add activate_ipynb_features() function.

Todo:

• Document new RC options
• Document activate_ipynb_features()
• Add activate_ipynb_features() to examples?
• Document how to run activate_ipynb_features() automatically when a kernel in a notebook is started.

We are still deploying the progress bar front-end code in IPython notebook in the non-recommended way:

If you distribute your custom widget, you may be using display and Javascript to publish the widget’s Javascript to the front-end. That is no longer the recommended way of distributing widget Javascript. Instead have the user install the Javascript to his/her nbextension directory or their profile’s static directory.

This is for backwards compatibility (the recommended way does not work prior to IPython 3). It would be possible to deploy the code in the recommended way for IPython 3 only and in the old way otherwise. But that might require some code duplication. Thus, I would vote to keep it how it is at the moment and switch over at a later point when we drop support for older IPython versions. The downsides of the old way are:

• Front-end code might not be loaded in time (and progress bar shows not up)
• Progress bar might break when the page is reloaded.

[tbekolay]

For interest's sake, SymPy's equivalent function for doing this is sympy.init_printing(). It used to be an IPython extension, loaded with %load_ext sympy.interactive.ipythonprinting, or %load_ext sympyprinting (from here; more details here). Their use case is different from ours, but it might be useful to use similar names.

[jgosmann]

In our case it is not really any printing functionality that gets initialized ...

[jgosmann]

Are we still aiming to support Python 2.6 in Nengo 2.1? I'm using the importlib module which does not exist in 2.6.

[tbekolay]

We haven't made any changes to drop support yet; ideally we would have at least one release of Nengo OCL that works on 2.6, but if Nengo OCL can target 2.0.1 then we can drop it.

[jgosmann]

Can we add importlib to the 2.6 dependencies? It should be pip installable.

[tbekolay]

Yeah, for sure; we do it for OrderedDict too.

[hunse]

Nengo OCL is working for most features on 2.0.1. You can take a look at which tests I'm skipping to see which features aren't supported. I think for the most part, they're pretty minor, so I think we can go ahead with a release.

In #633 I drop support of 2.6, basically just by removing the 2.6 tests.

[jgosmann]

@tbekolay Do you know why they moved from an extension to a function in SymPy? I was thinking about changing activate_ipynb_features() to an extension to be loaded with %load_ext nengo.ipynb.

The only way to automatically load the IPython progress bar (instead of the text version), seems to be to set an environment variable in ipython_notebook_config.py which is read out by Nengo. It would make things more convenient, but is somewhat magic and might cause non-obvious bugs? Otherwise activate_ipynb_features()/%load_ext nengo.ipynb has to be run in every notebook or the terminal progress bar is used (which looks not as nice and might blow up the notebook if a lot of those are created).

[jgosmann]

I dug around and found an answer to my question. As in our case it only ever makes sense to run the function within IPython, an extension should be fine.

[tbekolay]

Nice, I can finally close this tab ;) I didn't know off the top of my head so I'd have dug around as you did. That issue also linked to this one, so if they manage to figure that out then we can use their solution perhaps?

[jgosmann]

Nah, we can't. :(
They just print static stuff which is reasonably well supported. But there is no way to know whether widgets are supported or to provide a (non-static) fall back if widgets are not supported.

[tbekolay]

Fist shake! ✊ 🍶

[jgosmann]

This should be ready for review now.

[jgosmann]

bump

[jgosmann]

Note to self: Add changelog entries:

• IPython extension
• RC options
• breaks python 2.6 compatibility

[jgosmann]

Or do we prefer to add importlib to the requirements for Python 2.6 instead of breaking the compatibility (it's pip installable)?

[tbekolay]

I'd prefer adding importlib for now; we might as well keep Py 2.6 for 2.1 since it's still working. Break compatibility in 2.2.

[tcstewar]

I'd prefer adding importlib for now; we might as well keep Py 2.6 for 2.1 since it's still working. Break compatibility in 2.2.

That's my preference as well. :) It'd be nice to not require importlib as well for Py 2.6, but it's not too bad....

[jgosmann]

It'd be nice to not require importlib as well for Py 2.6, but it's not too bad....

The importlib code is just 38 lines of code. We could include it in Nengo?

[tbekolay]

Eh... we already have some Python 2.6 specific dependencies (ordereddict for example), and I don't think we have many Py 2.6 users anyhow. Mostly it's Travis CI running it in Python 2.6, and that's pretty trivial. In this case I think we're mostly trying to reduce programmer effort, and right now the least-effort solution is to just add importlib to the Py 2.6 dependency list.

[jgosmann]

Where is this list kept? There's requirements-test-py26.txt, but it is not just a test requirement.

[tbekolay]

Will let @jgosmann decide ;)

[jgosmann]

I prefer more and smaller commits.

[tbekolay]

So should we leave it at the 6 it is now then?

[jgosmann]

Would be my vote.

[tbekolay]

Yeah, I'm good with that. @hunse, acceptable?

[hunse]

The changelog commit should definitely be squashed, IMO.

Also, cfb5933 not only makes the change it purports to, but also does some refactoring. It's things like that which make me prefer fewer commits in some cases, because it seems less surprising to me that a commit changing the whole ProgressBar system would refactor things, versus a commit that says it changes when a warning is shown. Furthermore, 9a93fb8 unindents IPythonProgressWidget, and then cfb5933 moves it to another file. I feel like the diff would be cleaner if they're both in the same commit.

[tbekolay]

For the sake of concreteness, here are the two versions:

• less commits
• more commits

Separated, the three commits have 262 insertions and 221 deletions. Squashed, the one commit has 187 insertions and 152 deletions.

[hunse]

I think you forgot to squash the changelog commit in the fewer commits branch.

As a compromise, I'd be happy with more commits just with 6912d66 and 7abaa4a squashed, as I did here. (@jgosmann may want to edit the commit message to his liking, if that's what we want to go with.)

[tbekolay]

I think you forgot to squash the changelog commit in the fewer commits branch.

Right you are! Fixed.

I'd be happy with more commits just with 6912d66 and 7abaa4a squashed

That's fine with me too. The new commit message needs to be amended though, it's nengo/ipynb.py not utils/ipynb.py.

[jgosmann]

Also, cfb5933 not only makes the change it purports to, but also does some refactoring.

The refactoring is necessary to get rid of that warning mentioned in the commit message.

In general I find it easier to understand the changes in smaller commits than one big commit and smaller commits are also more helpful with tools like git bisect (as long as each commit in itself is consistent and leaves the repository in a working state).

I'm ok with squashing the changelog commit.

"Fixes to the IPythonProgressWidget" implies that IPythonProgressWidget was broken, but that was not the case I think. But the actual changes (detailed in the commit message) are more refactoring to keep the code simple and hide a warning.

[jgosmann]

btw: "Remove IPython notebook detection (cannot be done with IPython notebook >3 aka Jupyter)" is not 100% correct. It cannot be done with IPython == 3 (not yet jupyter) either.

[tbekolay]

My view is that the person making the PR has final say on the history, as they know better the intention behind each commit. That's why I like adding fixup / squash commits and getting a +1 before actually squashing / fixuping them (although admittedly I do a fair bit of squashing without an OK when a commit doesn't pass unit tests without a further commit). So, my vote would be to squash the changelog commit only.

It cannot be done with IPython == 3 (not yet jupyter) either.

I'll amend that commit message, whichever version we move forward with!

[jgosmann]

Also, I think it's better to have changes in code and movement of code to other places in separate commits as it makes for a more helpful diff for the changes. Though I admit, removing identation and moving code around might not be as critical.

The two commits I might squash in the long history are d81d2c7 and 6912d66 because the first of these directly caused the necessity of the second in a sense (to be able to set the IPython progress bar as a default in the config file).

[hunse]

I probably use the word "fixes" too much; I use it whenever I change something. As I said, feel free to amend the message.

I understand the reasoning behind smaller commits, but there are reasons for larger ones, too. I like them because they make the history easier to read, and easier to tell what's going on. If commits depend on commits around them for context, I think that's bad (not that that's the case here, per se). Obviously there's a balance; if commits are too small, then the history is too long and difficult to work with, and if they're too large, then any given commit is too hard to analyse. The question is where that balance is.

I think in general, we should always put changelog entries in the commits making the changes, not by themselves. I think that will help guide the size of commits, because if you're making a change that only requires one changelog entry, then it's more difficult to split it across multiple commits (though of course you could, if for example the first commit makes the change and then the following are just refactoring, etc.).

For this one, I'm fine with whatever.

[jgosmann]

If commits depend on commits around them for context, I think that's bad (not that that's the case here)

That's a case where non-fast-forward merges can make sense. (If the other option of squashing those commits together would end up with an unreasonable large commit.)

I think in general, we should always put changelog entries in the commits making the changes, not by themselves.

I agree.

[hunse]

That's a case where non-fast-forward merges can make sense.

Yeah, I would be open to something like that. I think that can still be considered a linear history, if the branch is always rebased to master before merging.

[tbekolay]

I did the squash @jgosmann suggested in the long history. Is that version OK with everyone? Looks like the best compromise to me. Will bring that over here and merge if everyone's good with it.

[hunse]

Fine by me.

[jgosmann]

LGTM 🍰