Add prototype shutdown button to Notebook dashboard #1343

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
6 participants
Owner

takluyver commented Jan 29, 2012

This adds a shutdown button on the notebook dashboard, which is a prerequisite to be able to run it without a terminal to Ctrl-C it.

It's pretty rough, and there's still quite a bit of work to be done. I thought I'd get it in so people could start telling me better ways to do things.

Owner

takluyver commented Jan 29, 2012

In particular, what should we do with the browser? A bit of searching suggests that we can't close a tab that we didn't open in Javascript. Should we render a simple page "Thanks for using IPython Notebook. You can now close this tab."? (It's like Windows 95 all over again!)

Owner

fperez commented Jan 29, 2012

Great start! Some thoughts:

  • It needs a confirmation dialog, given how destructive it is.
  • I also think this shutdown action should be available from the file menu in each notebook. Eventually we should be able to type

ipython notebook foo.ipynb

and be taken directly to that notebook page, without the dashboard being opened explicitly (it's always available though, just not shown). I suspect that use pattern would feel very natural to people, since it would be almost like editing any other local file with a regular application.

  • Yes, the static shutdown page at the end should provide a message along the lines you indicate. Perhaps with a logo that links to the main ipython.org page? Just so it doesn't look so bland :)
Owner

takluyver commented Jan 30, 2012

Done the first of those things - I'll get to the others tomorrow. When the action can be invoked from either the dashboard or the notebook view, where's the best place to put the JS function for it?

Owner

fperez commented Jan 30, 2012

Since this is still about managing the whole server, I think it should go into the dashboard code. It's just that the notebooks have a few things that are in the dashboard view. We have precedent: the new notebook action is also normally started from the dashboard, but we also expose it in each nb for convenience. I think the shutdown falls in this same category.

Owner

minrk commented Jan 30, 2012

(no direct bearing on this particular PR, but responding to above comment:)

ipython notebook foo.ipynb

Ultimately, when we decouple the notebook server from a single project directory, this command should actually just open the notebook window within an existing notebook server (launching it only if one is not running). The normal local use case should be that a user only ever runs a single notebook server, and everything they ever do goes through there.

Owner

fperez commented Jan 30, 2012

Completely agreed, @minrk.

Owner

takluyver commented Jan 30, 2012

Ok, we've got a confirmation dialog, a shutdown page, a menu option in the notebook view. And a merge conflict. I'll rebase it.

I added the dashboard JS file to the notebook, but doesn't that mean that the onready code from the dashboard is being invoked on the notebook page as well? Will that cause any problems?

Owner

takluyver commented Jan 30, 2012

Rebased.

Owner

ellisonbg commented Jan 30, 2012

I am -1 on having this in the notebook server. The notebook is quickly moving in the direction of being a full multi-user web application. In that context, it absolutely does not make sense to allow one user to shutdown the entire server. I would even say this does not make sense for the existing single user app, given that multiple users can still connect. It is a brutal and final action that will not only cause data loss for the user that clicks this button w/o understanding what it really does (what is the difference between killing a notebook and killing the server) but also for for every user.

We need to have some big design discussions at PyCon on how we are going to support both of the following usage cases:

  • Notebook as single user app that is lightweight.
  • Fullblown multiuser web site.

Until we do that I don't want to add things that wed us to the single user model.

Owner

takluyver commented Jan 30, 2012

On the other hand, I do think we need a way to stop the server besides having a terminal to hit Ctrl-C in. I don't see any way to do that without it being in the server itself.

It seems like we need some sort of 'admin' level of control in addition to the basic 'read-only'/interactive distinction we have at present. Starting the notebook locally would give you all rights. In a multiuser environment, an admin would also be able to manage users.

Owner

ellisonbg commented Jan 30, 2012

On Mon, Jan 30, 2012 at 3:47 PM, Thomas
reply@reply.github.com
wrote:

On the other hand, I do think we need a way to stop the server besides having a terminal to hit Ctrl-C in. I don't see any way to do that without it being in the server itself.

I guess I think that killing the notebook is so brutal, we want for
force people back to the terminal to carry it out. On the other
hand....

It seems like we need some sort of 'admin' level of control in addition to the basic 'read-only'/interactive distinction we have at present. Starting the notebook locally would give you all rights. In a multiuser environment, an admin would also be able to manage users.

Yes, if we have the notion of admin users, I would be fine with an
admin interface having the ability to kill the notebook server
entirely. I think this is the direction we are headed in.


Reply to this email directly or view it on GitHub:
#1343 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

minrk commented Jan 30, 2012

If we run the notebook as a daemon (e.g. ipython notebook /path/to/stuff opens up a project/notebook page and returns immediately), then we would be able to have things like ipython notebook stop. We need to implement all of the stuff required for a single notebook server instance to fulfill a user's needs:

  • specify kernel launch arguments from the web interface (most commonly pylab/not)
  • different arguments per kernel
  • navigate filesystem to some degree
  • decouple notebook server from single project so it can serve multiple projects at once
  • reliable URLs from file paths

But I agree that since this is the direction we are going, shutdown from the web interface (other than possible admin panel) is probably not a good idea.

Owner

takluyver commented Feb 23, 2012

OK, closing this since we're not going to do it this way.

@takluyver takluyver closed this Feb 23, 2012

Owner

fperez commented Feb 23, 2012

Mmh, I'd still like to think about this for a minute. The current situation is really highly sub-optimal, and I actually think that a web shutdown with confirmation dialog is better than the Ctrl-C in a terminal.

Brian mentions it's such a brutal action that he wants people to have to go to a terminal to do it, but the problem is that you can do that accidentally. It's easy to have the wrong window focused and hit Ctrl-C, killing every single kernel you have open, whereas a button that triggers a confirmation dialog before shutdown is actually safer. This form of accidental shutdown has only happened once to me so far ever and it was a trivial test notebook, so I didn't care. But I can see a user easily thinking they're doing Ctrl-C for 'copy' and accidentally shutting down a notebook with a ton of important kernels open. Keep in mind that Ctrl-C in a terminal offers no confirmation or way to back off the action, so the current situation is actually worse and less safe than what this PR offers.

Furthermore, having this shutdown button in the main page makes it possible to have a sensible GUI terminal-less launcher, which right now can't really be done because there's no way to sensible shut down the process started by the GUI launcher. But with this, we could include for Windows users an 'IPython notebook' Start menu entry, and Mac users could use one of the AppleScript examples that are floating on the net for IPython Qt console starting.

I'm all for a long-term elegant solution with a full admin panel, but I still think this fixes a serious, important limitation we have right now, and doesn't introduce any problems whatsoever. So I'd like to see this moving forward...

@takluyver takluyver reopened this Feb 23, 2012

Owner

takluyver commented Feb 23, 2012

OK, reopened. I did put a confirmation dialog in, so feel free to check this out and test. I guess we'll just need to find a way to disable/hide the button under certain conditions.

Owner

ellisonbg commented Feb 24, 2012

I am still strongly -1 on this. Let continue the discussion at PyCon/PyData.

Owner

fperez commented Feb 24, 2012

Sure. I really think what we have is effectively broken and that this is at least a first step in the right direction, but I'd love to understand your reasons.

Owner

ellisonbg commented Apr 18, 2012

We never got to discussing this at PyCon, so let's continue here.

Fernando you argue for this capability using the current fact that using Ctrl-C it is currently too easy to accidentally kill the notebook and all its kernels. These are two completely separate issues. We should fix the Ctrl-C issue by allowing the notebook to run as a daemon as Min suggests earlier in this thread. But that has no bearing on whether or not we allow users to kill everything from the browser. Running the notebook as a daemon will also allow us to build GUI launchers and the "ipython notebook mynb.ipynb" syntax without worrying about adding the kill button in the notebook web UI.

Here is my thinking about this:

  • Whether we like it or not, the notebook is a multiuser server not a single user application.
  • Multiuser servers should only be started and stopped by a very special person that has appropriate access/privs.
  • Until we can distinguish who has such access/privs, we should not expose this capability in the browser.
  • The eventual implementation will have to be tightly integrated with how access/privs are exposed in the notebook web app.
Owner

fperez commented Apr 18, 2012

The Ctrl-C problem was largely remedied in the recent merge of #1609,
which now at least asks the user for confirmation. So at least on
that front we're in better shape now, and I'm not worried about a
stray C-C in the wrong window nuking 10 long-running notebooks for
some unsuspecting user.

On Wed, Apr 18, 2012 at 11:46 AM, Brian E. Granger
reply@reply.github.com
wrote:

  • Whether we like it or not, the notebook is a multiuser server not a single user application.

Well, the current notebook isn't really a multiuser application at
all. It's a single user application that allows a second user to join
in with the privileges of the first user, just like a shared
screen/tmux session works. There is still only one user.

  • Multiuser servers should only be started and stopped by a very special person that has appropriate access/privs.
  • Until we can distinguish who has such access/privs, we should not expose this capability in the browser.
  • The eventual implementation will have to be tightly integrated with how access/privs are exposed in the notebook web app.

My reasoning is that, until we have a true multiuser notebook,
adding a simple 'shutdown' widget is a very small price to pay for a
significant usability improvement. I realize some of this will have
to get reworked once there's a full-blown multiuser notebook in place,
but what we have now is terrible usability.

And since it would only require adding a small amount of code that is
not API, I'm not worried about this enshrining the 'wrong' pattern
in our code. This feature can be completely removed once a multiuser
system with daemon features and multidirectory support exists, and we
don't have to worry about any API breakage, as none of this needs to
be publicly exposed.

I think that we can, at very little cost today, provide a massive
improvement in usability. Users already love the notebook, they'll
love it even more when they can double-click on their files and stop
things cleanly, I'm sure.

And when we do have the full multiuser app running, none of this will
cause any problems in removal. I simply think we're doing a
disservice to the users by blocking the implementation of something
genuinely useful and that solves their problem today on the argument
that it's slightly inelegant against a design brief that is months
into the future. And I'd agree 100% with your arguments if the 'hack
of today' meant blocking doing the right thing later. But in this
case that won't happen, the quick hack can be removed later at no
pain, and in the meantime it will have made many people happy.

Think of it this way: IPython itself was a hack without clean
architecture until the 2009 cleanup. Would you have me rather not
have written IPython at the start because I couldn't do it cleanly? :)

If you show me an argument why implementing this today will be a
problem in the future, I'll be happy to go along. But I haven't seen
a valid argument in that direction so far.

Owner

takluyver commented Apr 19, 2012

In support of Fernando's claim that a shutdown button would improve
usability today: I recently answered a question on SO from a user who
was using Ctrl+C but felt that he must be missing a way to close it
'properly'.

http://stackoverflow.com/questions/10162707/how-to-close-ipython-notebook-properly

Owner

ellisonbg commented May 23, 2012

I am in favor of closing this PR. Maybe @fperez and I can arm wrestle over it...or hash it out on Skype sometime...

Owner

fperez commented May 23, 2012

Well, there are two questions: (i) do we want the feature at all, and (ii) is this PR the right implementation?

I still hold firm on my comment above: we're doing a huge disservice to our users by not providing a feature that:

  • would help things tremendously in usability today
  • is tiny and can easily be changed later if the overall design goes in a different direction.

Telling users "things will be different sometime in the future, so deal with a sucky UI until we figure that out" isn't a good solution.

I simply don't see what the problem is: low cost, huge benefit, easy to change later... Practicality has to beat purity sometimes...

Owner

ellisonbg commented May 24, 2012

On Tue, May 22, 2012 at 10:13 PM, Fernando Perez
reply@reply.github.com
wrote:

Well, there are two questions: (i) do we want the feature at all, and (ii) is this PR the right implementation?

I agree these are important questions, but I think our disagreement
comes from a more fundamental question: is the notebook a single user
application or a multiuser web application. To this question, I
answer that it is a multiuser web application. While you can
semi-fake it into looking like a single user application by only
listening on localhost, the entire design, from the ground up is that
of a web application. As we add the notion of users that can manage
their own notebooks and kernels, this aspect will only become more
apparent.

My opposition to this feature is rooted in this model. It is of
practical relevance. Let's say we add the shutdown button right now.
This summer or shortly thereafter, I am hoping to make the notebook
fully multiuser. I will immediately have to answer the question "to
whom should the shutdown button appear?" Surely the answer is not all
users. That would be insanity. But to only show the shutdown button
to some users, everything is made much more complex. We have to have
the notion of "superusers" and develop all server side and JavaScript
logic and UIs to support that. This will slow and complexify the
development of the multiuser notebook - I am not willing to pay that
price. Part of what allows me to develop the notebook quickly is the
lack of baggage. Everything is brand new so we can change almost
anything at any time without worrying about breaking features that
people have been using features for years. Obviously, as the Notebook
matures, we slowly loose that freedom (we already need to make changes
to the notebook format very carefully for example), but I remain very
weary of adding anything that I know will have to be removed later.

Furthermore, I would argue that in a multiuser notebook, it simply
doesn't make sense for any single user to kill everything from the web
browser. The only person who should be able to kill everything should
be required to have ssh access to the host where the notebook was
started.

I still hold firm on my comment above: we're doing a huge disservice to our users by not providing a feature that:

  • would help things tremendously in usability today
  • is tiny and can easily be changed later if the overall design goes in a different direction.

Telling users "things will be different sometime in the future, so deal with a sucky UI until we figure that out" isn't a good solution.

I simply don't see what the problem is: low cost, huge benefit, easy to change later... Practicality has to beat purity sometimes...

I feel like I am being extremely practical by not wanting to add
something I know will slow future notebook development down.

But my sense is that our arguments are missing each other at a
fundamental level - that we have differing base assumptions about the
notebook that are the real point of impasse.


Reply to this email directly or view it on GitHub:
#1343 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

Carreau commented May 24, 2012

How would you like to make the notebook multi-user aware ? If you spawn a process with the logged user ID that act as the webserver only for this user, then it makes sens to shutdown this webserver, the Master Server that deal with redirecting each user would only be controllable by admins.

Owner

minrk commented May 24, 2012

@ellisonbg I don't really see how having this shutdown available in the current UI impedes developing the multi-user model with an admin panel. That development should just take place as if this functionality simply doesn't exist, and then once it's in place, this should just be deleted.

@Carreau - there will not be several servers, one for each user. There will be one multi-user notebook server, and the user would own their notebooks and kernels. Thus, only admins would be able to shutdown the notebook server.

Owner

ellisonbg commented May 24, 2012

Let me approach this from a different perspective.

Imagine that I own a house. It is within my right to destroy me house and there are many valid reasons why I might want to do that (remodel, termite infested, asbestos, etc.). But, destroying my house is somewhat difficult. I have to hire contractors or rent equipment, I have to make plans to move all of my stuff out of the house, I have to find somewhere else to live in the meantime. Now imagine if I decided to put a big red button by the front door that said "DESTROY THIS HOUSE" and I wired up the button to explosives that would instantly level the house. You would say I say I was crazy. You would say that destroying a house should be difficult. You would say that it was really dangerous, that accidents happen. What if someone bumped it. What if a young child pushed the button. What if it was pushed by accident when there was a party going on - many people would be killed and I would loose all my belonging. Even if I put safety measures in place (made it difficult to push, asked for user confirmation) you would still say the button makes to too easy to destroy the house.

The shutdown button, even with safety measures put into place, is analogous to this. The entire conceptual foundation of the button (make it easy to shutdown the notebook server) is problematic. I can think of multiple scenarios where such a button could be pushed by accident (new user who is confused, young children pushing buttons on the computer randomely, etc.). If the notebook were purely a single user application, then it is not a big deal. An accidental shutdown affects just that user and is no different from killing it at the terminal by mistake. But when there are multiple users in the picture, there is the risk of serious problems. What if I was using that notebook server for a big presentation at the time you killed it? What if a number of users had unsaved work or had performed a time consuming computation?

I simple don't understand why would would want to open the door for these possibilities.

Owner

ellisonbg commented May 24, 2012

@minrk You are right that the shutdown button doesn't preclude the multiuser notebook having an admin panel. But that is my point - the shutdown button requires that the multiuser notebook have an admin panel, which is not something I want to commit to starting out.

Owner

takluyver commented May 24, 2012

@ellisonbg: While the long-term design may be for a multi-user server,
at present the default mode is a single-user application, and I
imagine that's how most of our users are running it. In that context,
the shutdown button makes a lot of sense, and it seems odd to deny
users that feature because of our future plans.

Also, when we do add a multi-user mode, I imagine we will still want a
single-user mode similar to what we have today - using it as a local
application is working nicely for many people, and we don't want to
push added complexity onto them. So, for the first pass, we can show
the shutdown button only when it's running in single-user mode. In the
multi-user mode, the button wouldn't be there at all, so there's no
need for the added complexity of an admin panel.

Owner

fperez commented May 24, 2012

On Thu, May 24, 2012 at 2:24 PM, Brian E. Granger
reply@reply.github.com
wrote:

But when there are multiple users in the picture, there is the risk of serious problems.

But that is the heart of the current discussion: there are NO
multiple users in the picture, period. The current notebook is very
much a single user application, that runs with the privileges of the
invoking user and executes all code as that user.

Now, we want a proper multiuser system eventually, that will launch
individual kernels on behalf of users, I totally agree. But that
doesn't exist today.

The position we are taking is that adding a shutdown mechanism to the
notebook as it exists today is both very useful and logically
consistent.

And additionally, that it doesn't impose any burden on the future:
when we build proper multiuser capabilities, the central notebook
server will certainly not be able to be shutdown by any random user
that is logged in...

I totally understand your concern in terms of the long-term plans, but
I think that the point I'm advocating (along with @minrk, @takluyver
and @Carreau) is that today we can improve things significantly
without any long-term cost or problems.

Owner

ellisonbg commented May 24, 2012

On Thu, May 24, 2012 at 2:31 PM, Thomas Kluyver
reply@reply.github.com
wrote:

@ellisonbg: While the long-term design may be for a multi-user server,
at present the default mode is a single-user application, and I
imagine that's how most of our users are running it. In that context,
the shutdown button makes a lot of sense, and it seems odd to deny
users that feature because of our future plans.

But many of us run the notebook on the web today and allow multiple
users to log into it.

Also, when we do add a multi-user mode, I imagine we will still want a
single-user mode similar to what we have today - using it as a local
application is working nicely for many people, and we don't want to
push added complexity onto them. So, for the first pass, we can show
the shutdown button only when it's running in single-user mode. In the
multi-user mode, the button wouldn't be there at all, so there's no
need for the added complexity of an admin panel.

It is not clear what all of this will look like when the notebook
becomes multiuser. I am hoping that 1) there is no "added complexity"
for running in multiuser mode and that 2) there is no distinction
between single user and multiuser modes. What I want to avoid is
having:

if multiuser_mode:
...
elif singleuser_mode:
...

Throughout the code base. We are already seeing this exact type of
complexity in the notebook in terms of tracking things like read_only
mode, authenticated vs guest, etc. One of the big challenges with the
multiuser notebook will be in identifying the minimal set of "modes"
that each page has to know about. What about read_only, single_user,
non_authenticated mode? Should the button appear then? In addition
to the existing modes we will need to add new ones to enable sharing
of notebooks. I am trying to be very conservative about these things
because I see an exponential complexity emerging.


Reply to this email directly or view it on GitHub:
#1343 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

ellisonbg commented May 24, 2012

On Thu, May 24, 2012 at 2:44 PM, Fernando Perez
reply@reply.github.com
wrote:

On Thu, May 24, 2012 at 2:24 PM, Brian E. Granger
reply@reply.github.com
wrote:

But when there are multiple users in the picture, there is the risk of serious problems.

But that is the heart of the current discussion: there are NO
multiple users in the picture, period.  The current notebook is very
much a single user application, that runs with the privileges of the
invoking user and executes all code as that user.

I don't understand. At the recent NIH workshop in Boulder, one of the
big features of the notebook that you emphasized, demonstrated and
leveraged was its multiuser capabilities running on Amazon EC2. A
group of people even wrote an article about that experience! We have
been using a shared notebook server for our DoD grant. Just in the
past week, you and I have run multiple public notebook servers and
shared them with others. I understand that most of the time people
are not doing this, but to say that there are NO multiple users in
the picture doesn't match up with what has been going on. Are we
talking about different things?

And additionally, that it doesn't impose any burden on the future:
when we build proper multiuser capabilities, the central notebook
server will certainly not be able to be shutdown by any random user
that is logged in...

I totally understand your concern in terms of the long-term plans, but
I think that the point I'm advocating (along with @minrk, @takluyver
and @Carreau) is that today we can improve things significantly
without any long-term cost or problems.


Reply to this email directly or view it on GitHub:
#1343 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

fperez commented May 24, 2012

On Thu, May 24, 2012 at 2:56 PM, Brian E. Granger
reply@reply.github.com
wrote:

Are we talking about different things?

No, let me clarify: those are not multiple users, they are multiple
people sharing a single user account. That's the distinction I'm
making, and I think that clarifying that helps us better understand
each other's concerns...

I see the current situation as one of a 'shared user' setup, which is
emphasized by the fact that the 'login' screen never asks for a
username, only a password.

Let's put it another way: the current capabilities are so strongly
single-user that you have to be sure you trust whoever logs in not to
do something destructive like "!rm -rf *". So having a 'shut down the
whole server' button seems to me infinitely more benign than the
'attack surface' that we already expose...

So my take has always been: because the current tool offers "trusted
shared single-user", then it's consistent and useful to give that user
the right to also stop the application. That user already has so much
power (and responsibility) that it seems odd and pointless to remove
this one bit of control (which ultimately they can do anyway if they
really want, by simply issuing !pkill ipython).

Does this help clarify things?

Owner

takluyver commented May 24, 2012

On 24 May 2012 22:45, Brian E. Granger
reply@reply.github.com
wrote:

What I want to avoid is having:

if multiuser_mode:
   ...
elif singleuser_mode:
   ...

I think having some amount of this is unavoidable. We've built
something that's useful in two very different scenarios, and there
will need to be some differences in the interface to account for those
scenarios.

Owner

ellisonbg commented May 24, 2012

@fperez has greatly clarified his thinking in his last post. Let me summarize. In our current "trusted single user mode" we already have a big "DESTROY THIS HOUSE" button on the wall, namely that any user has the full privs of the user that started the notebook. The argument is that adding a shutdown button to the notebook UI doesn't introduce any new risk that wasn't already there. I am buying this argument at the 85% level.

Here is my 15% reservation: my 6 year old regularly comes over to my laptop and clicks on things. If a notebook is open, he could easily click on a shutdown button and confirm. He would, on the other hand, have a very difficult time typing "!rm -rf ~/* into a cell and running that cell. I realize this is not too likely, which is why this is only a 15% effect for me.

I am OK going ahead with a shutdown button (we will have to finish reviewing this particular implementation) under two conditions:

  • If we ever find that the "shutdown button clicker" has more power than can be wielded by simply running code in notebook cells, the button has to go. That will happen in a full multiuser setting.
  • If the code to support the shutdown button in other contexts gets overly complex, we reconsider it.
Owner

fperez commented May 24, 2012

On Thu, May 24, 2012 at 3:28 PM, Brian E. Granger
reply@reply.github.com
wrote:

I am OK going ahead with a shutdown button (we will have to finish reviewing this particular implementation) under two conditions:

  • If we ever find that the "shutdown button clicker" has more power than can be wielded by simply running code in notebook cells, the button has to go.  That will happen in a full multiuser setting.
  • If the code to support the shutdown button in other contexts gets overly complex, we reconsider it.

Those two seem perfectly reasonable to me, and I'd add the following
design brief to the whole shutdown situation (which may mean this PR
isn't what we want, I'm not sure):

  • in each notebook page, there is no 'shutdown server' button, only
    a 'stop my kernel' one, at most. I could also not have this, but
    instead have a red 'Stop' button next to the 'Delete' one in the
    dashboard. I really think we need a way to stop individual kernels
    without shutting down the whole thing.
  • the shutdown button is only in the dashboard, to emphasize the fact
    that it affects all notebooks. If more than a single notebook is
    alive, it could even have an extra checkbox that must be manually
    ticked to emphasize the potentially wide destruction about to happen
    (like some browsers ask for extra confirmation when shutting down
    multiple tabs).

How does this sound?

Owner

ellisonbg commented May 24, 2012

On Thu, May 24, 2012 at 3:35 PM, Fernando Perez
reply@reply.github.com
wrote:

On Thu, May 24, 2012 at 3:28 PM, Brian E. Granger
reply@reply.github.com
wrote:

I am OK going ahead with a shutdown button (we will have to finish reviewing this particular implementation) under two conditions:

  • If we ever find that the "shutdown button clicker" has more power than can be wielded by simply running code in notebook cells, the button has to go.  That will happen in a full multiuser setting.
  • If the code to support the shutdown button in other contexts gets overly complex, we reconsider it.

Those two seem perfectly reasonable to me, and I'd add the following
design brief to the whole shutdown situation (which may mean this PR
isn't what we want, I'm not sure):

  • in each notebook page, there is no 'shutdown server' button, only
    a 'stop my kernel' one, at most.  I could also not have this, but
    instead have a red 'Stop' button next to the 'Delete' one in the
    dashboard.  I really think we need a way to stop individual kernels
    without shutting down the whole thing.

We have a very nice open PR that adds a shutdown kernel button to the
notebook dashboard. It works very nicely. As time goes on I think
that we want to enforce the following constraint: if a notebook is
open by one or more users, it always has a running kernel. Having a
notebook open without a kernel puts things in a unknown state that I
think we want to avoid. This means two things:

  • We should no longer prompt the user if the kernel dies. We should
    just restart it a notify the user.
  • The individual notebook pages should not have a "stop" button.
  • the shutdown button is only in the dashboard, to emphasize the fact
    that it affects all notebooks.  If more than a single notebook is
    alive, it could even have an extra checkbox that must be manually
    ticked to emphasize the potentially wide destruction about to happen
    (like some browsers ask for extra confirmation when shutting down
    multiple tabs).

Sounds good.

How does this sound?


Reply to this email directly or view it on GitHub:
#1343 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

fperez commented May 24, 2012

On Thu, May 24, 2012 at 3:42 PM, Brian E. Granger
reply@reply.github.com
wrote:

We have a very nice open PR that adds a shutdown kernel button to the
notebook dashboard.  It works very nicely.  As time goes on I think
that we want to enforce the following constraint: if a notebook is
open by one or more users, it always has a running kernel.  Having a
notebook open without a kernel puts things in a unknown state that I
think we want to avoid.  This means two things:

  • We should no longer prompt the user if the kernel dies.  We should
    just restart it a notify the user.
  • The individual notebook pages should not have a "stop" button.

Sounds good to me...

Owner

Carreau commented May 25, 2012

  • We should no longer prompt the user if the kernel dies. We should
    just restart it a notify the user.

Then if a user have a "forgotten" notebook opened on a remote computer (let's say at home) and wan't to shut it down from the dashboard (from work) he can't ... the kernel won't stop restarting. That would be weird.

  • The individual notebook pages should not have a "stop" button.

That makes sens, we could even have the dashboard button active only if all the kernels are stopped.

Owner

ellisonbg commented May 26, 2012

On Fri, May 25, 2012 at 9:39 AM, Bussonnier Matthias
reply@reply.github.com
wrote:

  • We should no longer prompt the user if the kernel dies.  We should
    just restart it a notify the user.

Then if a user have a "forgotten" notebook opened on a remote computer (let's say at home)  and wan't to shut it down from the dashboard (from work) it can't ... the kernel won't stop restarting. That would be weird.

I don't follow. The heartbeating that detects a dead kernel and
triggers the kernel restart logic is handled on the server side. This
logic is disabled when a kernel is shutdown cleanly (like the
dashboard button does). This means that if you shutdown a kernel from
the dashboard while the notebook is running somewhere else, that other
notebook will simply stop being able to run code. We should probably
make sure a message is sent to that other notebook (it might be
already) and that the user is at least notified "someone else shutdown
the kernel for this notebook"

  • The individual notebook pages should not have a "stop" button.

That makes sens, we could even have the dashboard button active only if all the kernels are stopped.


Reply to this email directly or view it on GitHub:
#1343 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

Carreau commented May 26, 2012

  • We should no longer prompt the user if the kernel dies. We should
    just restart it a notify the user.

Then if a user have a "forgotten" notebook opened on a remote computer (let's say at home) and wan't to shut it down from the dashboard (from work) it can't ... the kernel won't stop restarting. That would be weird.

I don't follow. The heartbeating that detects a dead kernel and
triggers the kernel restart logic is handled on the server side. This
logic is disabled when a kernel is shutdown cleanly (like the
dashboard button does). This means that if you shutdown a kernel from
the dashboard while the notebook is running somewhere else, that other
notebook will simply stop being able to run code. We should probably
make sure a message is sent to that other notebook (it might be
already) and that the user is at least notified "someone else shutdown
the kernel for this notebook"

Ok, my bad, I thought the "The kernel has died, would you like to restart it?" message was done by polling in JS.
If it's pushed by the webserver then it's ok for me.
We should then push 2 kinds of messages :

  • kernel has died and been automatically restarted
  • kernel has been automatically shutdown

fperez added a commit that referenced this pull request Jun 13, 2012

Merge pull request #1759 from Carreau/mpr
Add git-mpr, to merge PR(s) from github just by number(s).

Inspired by git-mrb and test_pr, I thougth it would be usefull to be able to merge PR only by giving their number. Hence `git merge-pull-request`or `git-mpr`.

usage: 
```bash
$ git mpr -h                                                                          ~/ipython/tools
usage: git-mpr [-h] [-l | -a | -m [pr-number [pr-number ...]]]

Merge (one|many) github pull request by their number. If pull request can't be
merge as is, cancel merge, and continue to the next if any.

optional arguments:
  -h, --help            show this help message and exit
  -l, --list            list PR, their number and their mergeability
  -a, --merge-all       try to merge as many PR as possible, one by one
  -m [pr-number [pr-number ...]], --merge [pr-number [pr-number ...]]
                        The pull request numbers
```

examples :
```bash
$ git mpr --list  
* #1758 [√]:  test_pr, fallback on http if git protocol fail, and SSL errors...
* #1755 [√]:  test for pygments before running qt tests
* #1715 [√]:  Fix for #1688, traceback-unicode issue
[...]
* #1493 [√]:  Selenium web tests proof-of-concept
* #1471 [ ]:  simplify IPython.parallel connections and enable Controller Resume
* #1343 [ ]:  Add prototype shutdown button to Notebook dashboard
* #1285 [√]:  Implementation of grepping the input history using several patterns
* #1215 [√]:  updated %quickref to show short-hand for %sc and %sx
```
PR number, mergeability and title
Quite slow, as it does 1 api call by PR, since api does not give mergeability anymore if you ask for the list of all PRs at once.

merge one or more PR (skip the ones with conflict and present a nice list to copy and past to do it by hand) 

```bash
$ git mpr --merge [pr-number [pr-number ...]]]
[...]
*************************************************************************************
the following branch have not been merged automatically, considere doing it by hand :
PR 1630: git pull https://github.com/minrk/ipython.git mergekernel
PR 1343: git pull https://github.com/takluyver/ipython.git notebook-shutdown
PR 1715: git pull https://github.com/jstenar/ipython.git ultratb-pycolorize-unicode
PR 1732: git pull https://github.com/fperez/ipython.git cellmagics
PR 1471: git pull https://github.com/minrk/ipython.git connection
PR 1674: git pull https://github.com/mdboom/ipython.git notebook-carriage-return
*************************************************************************************
```

And last, 
```
git mpr --merge-all
```

That is pretty self explainatory
Contributor

bfroehle commented Jul 26, 2012

I haven't managed to read the entire thread here, but a shutdown button seems appropriate enough. However, this pull request will need to be rebased before it can be merged.

Owner

Carreau commented Aug 24, 2012

Do we revive this or move close and open an issue for it ?

Owner

Carreau commented Sep 28, 2012

ping again.

Owner

Carreau commented Sep 30, 2012

Hi,
This PR is without activities for 2 month now,
I'm going to close it and open an issue to reference it.
This does not mean than we refuse the code that is here, but that we try to keep the number of opened Pull request as low as possible to have better bird view of active code.

Fell free to reopen this PR whenever you want or contact us if you have any questions.

Thanks for contributing.

@Carreau Carreau closed this Sep 30, 2012

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1759 from Carreau/mpr
Add git-mpr, to merge PR(s) from github just by number(s).

Inspired by git-mrb and test_pr, I thougth it would be usefull to be able to merge PR only by giving their number. Hence `git merge-pull-request`or `git-mpr`.

usage: 
```bash
$ git mpr -h                                                                          ~/ipython/tools
usage: git-mpr [-h] [-l | -a | -m [pr-number [pr-number ...]]]

Merge (one|many) github pull request by their number. If pull request can't be
merge as is, cancel merge, and continue to the next if any.

optional arguments:
  -h, --help            show this help message and exit
  -l, --list            list PR, their number and their mergeability
  -a, --merge-all       try to merge as many PR as possible, one by one
  -m [pr-number [pr-number ...]], --merge [pr-number [pr-number ...]]
                        The pull request numbers
```

examples :
```bash
$ git mpr --list  
* #1758 [√]:  test_pr, fallback on http if git protocol fail, and SSL errors...
* #1755 [√]:  test for pygments before running qt tests
* #1715 [√]:  Fix for #1688, traceback-unicode issue
[...]
* #1493 [√]:  Selenium web tests proof-of-concept
* #1471 [ ]:  simplify IPython.parallel connections and enable Controller Resume
* #1343 [ ]:  Add prototype shutdown button to Notebook dashboard
* #1285 [√]:  Implementation of grepping the input history using several patterns
* #1215 [√]:  updated %quickref to show short-hand for %sc and %sx
```
PR number, mergeability and title
Quite slow, as it does 1 api call by PR, since api does not give mergeability anymore if you ask for the list of all PRs at once.

merge one or more PR (skip the ones with conflict and present a nice list to copy and past to do it by hand) 

```bash
$ git mpr --merge [pr-number [pr-number ...]]]
[...]
*************************************************************************************
the following branch have not been merged automatically, considere doing it by hand :
PR 1630: git pull https://github.com/minrk/ipython.git mergekernel
PR 1343: git pull https://github.com/takluyver/ipython.git notebook-shutdown
PR 1715: git pull https://github.com/jstenar/ipython.git ultratb-pycolorize-unicode
PR 1732: git pull https://github.com/fperez/ipython.git cellmagics
PR 1471: git pull https://github.com/minrk/ipython.git connection
PR 1674: git pull https://github.com/mdboom/ipython.git notebook-carriage-return
*************************************************************************************
```

And last, 
```
git mpr --merge-all
```

That is pretty self explainatory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment