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 shutdown button option #3004

Merged
merged 6 commits into from Apr 4, 2018

Conversation

Projects
None yet
4 participants
@gnestor
Copy link
Contributor

gnestor commented Oct 31, 2017

Closes #1530

Introduces a new option --NotebookApp.shutdown_button which will add a button to the dashboard header that when clicked will shutdown the notebook server.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Nov 1, 2017

Aha, you beat me to it, thanks :-)

  • I'd say it should be enabled by default, otherwise basically no-one will ever know about it.
  • I don't entirely like 'shutdown' as a user-facing name, because that sounds like shutting down the computer. But I don't know how we make it clear that this is something more than closing the browser tab it's in.
  • As shutting down is a destructive action, should we give the user a chance to change their mind? I don't like 'are you sure' dialogs, but it could turn into a countdown: 'Shutting down in 10 seconds... Cancel'
  • How would this interact with the shutdown buttons in Jupyterhub? @minrk ?

This also reminds me that I don't think we should be showing a 'logout' button when the user is authenticated by a token, since from their perspective they're not 'logged in'.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Nov 1, 2017

How would this interact with the shutdown buttons in Jupyterhub?

From JupyterHub's perspective, this would mean that the server died and the Hub would take a finite (generally short) period of time to notice. It would probably be best in the future for SingleUserServer to override the method to trigger shutdown via the Hub API, so if it's easy to override that would be ideal.

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Nov 1, 2017

I don't entirely like 'shutdown' as a user-facing name, because that sounds like shutting down the computer. But I don't know how we make it clear that this is something more than closing the browser tab it's in.

How about "Disconnect"?

It would probably be best in the future for SingleUserServer to override the method to trigger shutdown via the Hub API, so if it's easy to override that would be ideal.

Seems like it should be simple to override. Here is the shutdown handler: https://github.com/jupyter/notebook/blob/master/notebook/services/shutdown.py

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Nov 1, 2017

I like disconnect even less ;-). Disconnect implies something will remain to which you can reconnect later.

@GadgetSteve

This comment has been minimized.

Copy link
Contributor

GadgetSteve commented Nov 2, 2017

Since this is not shutting down the machine and is not disconnecting how about "HALT JUPYTER" or "HALT SERVICE" as a label? Either should make it clear what is going on.

Regarding default enabling or not if the option is added to the configuration file options and included in the default generated by jupyter notebook --generate-config plus mentioned in the documentation then the user/maintainer of the installation can choose to show or hide it according to their preferences.

@gnestor gnestor modified the milestones: 5.3, 5.4 Jan 5, 2018

@takluyver takluyver modified the milestones: 5.4, 5.5 Jan 27, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 28, 2018

Returning to this, I think we need to:

  • Enable it by default
  • Make it configurable for Jupyterhub (@minrk what would this involve? pointing it to a different URL for the POST request?)
  • Show some indication on the page that the server has been shutdown, so it's clear that it worked.
  • Leave the text as 'Shutdown' for now. I still don't love it as a name, but so far I like all the proposed alternatives less.

@gnestor do you have time to come back to this?

@GadgetSteve

This comment has been minimized.

Copy link
Contributor

GadgetSteve commented Mar 29, 2018

Just a suggestion how about "Terminate" possibly with a hover of "Shutdown Jupyter Server Backend" this rather than "Shutdown" - it rightly sounds more drastic than close/exit/etc. but does not imply that the machine will shutdown. I like the idea of a countdown, (especially if the length of the countdown is a configurable item), but would suggest that it would be a good idea to have a "Now" button as well as a "Cancel" - everybody on Windows swears regularly at the "Applying Updates: Do not switch off...." message when they are in a hurry to get their machine shutdown so that they can go home or catch a bus/train/flight and I would like to avoid being in the same category.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 29, 2018

'Terminate' could work, though it sounds a bit violent ;-) Or maybe 'Quit'?

If we're trying to get this into 5.5, I'd probably leave the countdown for a future version to keep things simple.

@takluyver takluyver force-pushed the gnestor:issue-1530 branch from e27dcf1 to 9359a73 Mar 29, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 29, 2018

I've rebased this, enabled the button by default, changed the label to 'Quit', made sure all the human readable text is marked for translation, and made it display a dialog on successful shutdown. The dialog looks pretty ugly, but it's better than not showing anything. We can improve on it later.

Min and I agreed that Jupyterhub can simply disable this button and show its own buttons. Shutdown via the hub involves a HTTP DELETE request, so it would have to have config options for both the URL and the HTTP verb.

@gnestor

This comment has been minimized.

Copy link
Contributor Author

gnestor commented Apr 3, 2018

@takluyver Thanks for doing this. I was just about to dig into it. I'm not sure if "Quit" is better than "Shutdown"... We are using "shutdown_button" as the name for the module, so I guess that we should rename that as well if we go with "quit." You're call. Feel free to merge 👍

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 3, 2018

My rationale is that 'shutdown' is clearer for people who understand the idea of a local HTTP server, and 'quit' is clearer for people who don't. But on reflection, I think the config name should match what the button says. I'll update it.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 3, 2018

I've renamed the config option to quit_button, but left it as shutdown in the code, because the API endpoint is called shutdown too.

@takluyver takluyver merged commit 9d4a2da into jupyter:master Apr 4, 2018

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 76.08% (+<.01%) compared to faa0cab
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gnestor gnestor deleted the gnestor:issue-1530 branch Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.