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

Rich display progress bar #1375

Closed
wants to merge 8 commits into from
Closed

Rich display progress bar #1375

wants to merge 8 commits into from

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Oct 25, 2017

Motivation and context:
%load_ext nengo.ipynb and the IPython progress bar currently break in in recent Jupyter notebook/IPython versions (potentially completely crashing the notebook frontend). Also, the implementation based on widgets has proven to be not very robust as the widget API has been evolving quickly.

This PR implements a new IPython5ProgressBar compatible with IPython 5 and newer. The HTML code is based on the old IPython2ProgressBar, but instead of using widgets, the IPython rich display system is used which allows updates to displays since IPython 5. As we do not require to capture any user interaction that is completely sufficient. It also allows us to display the HTML progress bar in notebooks without %load_ext nengo.ipynb.

The notebook extension nengo.ipynb is deprecated with this PR as well as the IPython2ProgressBar and loading it will issue a warning. At some point %load_ext nengo.ipynb should become a no-op or deleted (but that would break backwards compatibility).

Interactions with other PRs:
none

How has this been tested?
Tested the following scenarios:

  • Progress bar in Jupyter notebook
  • Progress bar when executing with ipython on the terminal
  • Progress bar when executing with python on the terminal

Still to test:

  • What happens in ipython qtconsole?

How long should this take to review?

  • Average (neither quick nor lengthy)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • [n/a] I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

Still to do:

Based on the rich display system, supports IPython >= 5.

Addresses #1087.
Copy link
Contributor

@Seanny123 Seanny123 left a comment

Choose a reason for hiding this comment

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

Works for me on the latest Jupyter and IPython, after it hasn't worked for a really long time. Got some flake8 errors, but those should be trivial to fix. Thanks for taking the time to make this work.

@jgosmann jgosmann self-assigned this Oct 25, 2017
@jgosmann
Copy link
Collaborator Author

Does not work in the QtConsole (the qt-console requests the text/html conversion, but does not actually use it).

@Seanny123
Copy link
Contributor

Seanny123 commented Oct 25, 2017 via email

@jgosmann
Copy link
Collaborator Author

The QtConsole should have defaulted to the TerminalProgressBar.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Oct 25, 2017

Ok, so the current state is now:

  • python defaults to TerminalProgressBar
  • ipython defaults to IPython5ProgressBar which will direct calls to TerminalProgressBar. This is detected by the fact that _ipython_display_ will not be invoked in this case.
  • All ipython kernels connected to via Jupyter (jupyter console, jupyter qtconsole, jupyter notebook etc) will use IPython5ProgressBar, but direct calls to HtmlProgressBar.
    • If the frontend supports HTML display (only notebook at the moment), a the HTML progress bar will be displayed.
    • All other frontends will display a message to set the progress bar to TerminalProgressBar.

For some reason the updating of the progress bar does not work for me in Jupyter notebook, but it seems that my Jupyter notebook installation might be broken. Just had to update the notebook server, it was an old version from toying around earlier.

@jgosmann
Copy link
Collaborator Author

The choices in the comment are meant to reflect that most people (I assume) use either python to run scripts on the console or Jupyter notebook for interactive work. So these two cases work out of the box. But of course we could change it to the prior state: Always use the terminal progress bar and activate the HTML progress bar in the notebook with a special command (%load_ext nengo.ipynb).

@Seanny123
Copy link
Contributor

I think this is fine. Is there some other reason this is still tagged as WIP?

@jgosmann
Copy link
Collaborator Author

Not really, just forgot to remove the tag. Might add some more documentation maybe.

@jgosmann jgosmann removed their assignment Oct 27, 2017
@jgosmann
Copy link
Collaborator Author

So #1380 is based on this PR and rewrites some of the code. Not sure whether to close this PR and move all discussion and review to the new PR or whether to get this one merged first and then rebase the other PR?

@tbekolay
Copy link
Member

Hmm... if it rewrites code I'm inclined to close this one and move to the new PR, unless you have strong feelings for doing this one first @jgosmann

@jgosmann
Copy link
Collaborator Author

Sounds perfectly fine to me.

@jgosmann jgosmann closed this Nov 13, 2017
@tbekolay tbekolay deleted the rich-display-progress branch September 30, 2019 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants