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

ENH: Simple install_kicker() #807

Merged
merged 2 commits into from
Dec 15, 2017
Merged

ENH: Simple install_kicker() #807

merged 2 commits into from
Dec 15, 2017

Conversation

hhslepicka
Copy link
Contributor

Usually users need to call either install_qt_kicker or install_nb_kicker in order to get the live plots and other callbacks to work properly. The idea of this PR is to add a functionality that I added to LIX while working there to make it easier for the user by just installing the correct one based on how they are running the code.

Description

Added a method at utils.py named install_kicker which based on the environment will install either qt_kicker or nb_kicker.

Motivation and Context

It simplifies the setup of the kicker.

How Has This Been Tested?

Locally tested.

@codecov-io
Copy link

codecov-io commented Aug 29, 2017

Codecov Report

Merging #807 into master will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   85.98%   85.86%   -0.12%     
==========================================
  Files          18       18              
  Lines        4881     4890       +9     
==========================================
+ Hits         4197     4199       +2     
- Misses        684      691       +7
Impacted Files Coverage Δ
bluesky/tests/utils.py 75.69% <0%> (-0.91%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 770df4e...e88338e. Read the comment docs.

@danielballan
Copy link
Member

I like this thought. Forgive me for raining on this PR with some complications -- but then one positive suggestion.

  • Modern IPython recommends launching kernels with ipykernel but in the "old days" (~2 years ago) ipykernel didn't exist and kernels were launched from IPython directly. At NSLS-II, we have some old conda environments still in use on the floor that predate ipykernel and would thus take the wrong code path here.
  • The ipython console or ipython qtconsole invoke ipykernel (I think) but use Qt plotting. Again, they would take the wrong code path.
  • Worst of all, a given kernel may have multiple front-ends ("sessions") attached to it -- a notebook and a Qt console. A stated design principle of IPython is that a kernel is not allowed to know/care what kinds of front-ends it is talking to.

But let me suggest this solution: ask matplotlib which backend it is using and use that to differentiate which kicker to install. A kernel might have multiple front-ends, but matplotlib can only have one backend at a time, and that's the real determining factor as to which kicker we need.

@hhslepicka
Copy link
Contributor Author

@danielballan suggestions are always welcome! 👍

How do you guys have old IPython running since every deploy the environments are wiped out and recreated with the new cycle?

I will take a look at your suggestion and send modifications to the PR.

@danielballan
Copy link
Member

We don't wipe out the old conda environments (anymore, anyway). We keep them around so that old user notebooks can run on their original kernels without suffering from API changes.

@hhslepicka
Copy link
Contributor Author

I see! I will work on the suggested changes...
Invoking @tacaswell so he can tell me which backends go with qt_kicker and which ones goes with nb_kicker...
I saw nbAgg which seems a bit obvious but there was one very weird when I use %matplotlib inline

@danielballan
Copy link
Member

The "inline" magic isn't a real backend; it's sort of an IPython hack. It can't do live-updating plots anyway, so I don't think we have to worry about it for the purposes of the kicker.

@tacaswell
Copy link
Contributor

mpl.get_backend() is the right way to sort out how do do the dispatch.

@tacaswell
Copy link
Contributor

tacaswell commented Aug 30, 2017

This kicker may work for all GUI backends

def _kicker()
    Gcf.draw_all()
    # there is not a singleton exposed through Mpl, but all of the GUI backends have one (and 
    plt.gcf().canvas.flush_events()
    loop.call_later(0.03, _kicker)

but may not work for nbagg or ipympl

@danielballan
Copy link
Member

For the record, my wife has inquired why we are kicking backends. We work in a strange profession.

@tacaswell
Copy link
Contributor

Or I should just not be allowed to name things....

@danielballan
Copy link
Member

@hhslepicka I took the liberty of rebasing and force-pushing to this branch. This is the approach I was suggesting. Can you check that it works for you before we merge?

@danielballan danielballan added this to the 0.11 milestone Oct 18, 2017
@danielballan danielballan modified the milestones: 0.11, 1.x Oct 31, 2017
@danielballan
Copy link
Member

bump @hhslepicka

@hhslepicka
Copy link
Contributor Author

@danielballan sorry for the delay. I did some tests and it worked for my using iPython and the notebook.
I believe it is good to go. 👍 Thank you for finishing this!

@jrmlhermitte
Copy link
Contributor

I like this.
We should eventually add a note in the docs which is here.

Codecov seems to complain. Is there an easy way to test something like this or should we just ignore?

Maybe add this to .coveragerc in main dir?

exclude_lines =
    if __name__ == '__main__':
    def install_qt_kicker
    def install_nb_kicker
    def install_kicker

@danielballan
Copy link
Member

danielballan commented Dec 15, 2017

This one is hard to test programmatically but quite easy to test interactively. Since we use it regularly, the risk of an unnoticed regression seems low. I would be inclined to let coverage continue to shame us (that is, not silence it by modifying exclude_lines) but not worry about addressing this any time soon.

@jrmlhermitte
Copy link
Contributor

sounds good. If someone can confirm they've tested it for both ipython and jupyter I second on merging.
(I can test a little later on as well if no one else can get to it)

@danielballan
Copy link
Member

Looks like Hugo confirmed it since the most recent commit was added. Merging!

@danielballan danielballan merged commit f73b880 into bluesky:master Dec 15, 2017
@hhslepicka
Copy link
Contributor Author

@danielballan yes I did! Thank you for merging. 👍

@jrmlhermitte
Copy link
Contributor

oops yes thanks @hhslepicka ! useful addition!

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

5 participants