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.

Sign up for GitHub

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

Jump to bottom

NbserverStopApp: stop notebooks through cli - jupyter notebook stop <… #2388

Merged
merged 8 commits into from Apr 11, 2017
Merged

NbserverStopApp: stop notebooks through cli - jupyter notebook stop <… #2388

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cd3233f
NbserverStopApp: stop notebooks through cli - jupyter notebook stop <…
brookisme Apr 8, 2017
219c762
remove unused props
brookisme Apr 8, 2017
b6ac73c
use os.kill
brookisme Apr 8, 2017
8b20dcc
exit if server not found
brookisme Apr 8, 2017
277d000
dont cast to string
brookisme Apr 8, 2017
ed4de77
remove unused kill_cmd/signal prop
brookisme Apr 8, 2017
b2f63b4
SIGTERM not SIGQUIT
brookisme Apr 10, 2017
44df51a
always exit(1) if not server found. break up long list comprehension
brookisme Apr 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions notebook/notebookapp.py
View file
Open in desktop
Expand Up @@ -346,6 +346,32 @@ def start(self):
self.log.info("Wrote hashed password to %s" % self.config_file)


class NbserverStopApp(JupyterApp):
version = __version__
description="Stop currently running notebook server for a given port"

port = Integer(8888, config=True,
help="Port of the server to be killed. Default 8888")

def parse_command_line(self, argv=None):
super(NbserverStopApp, self).parse_command_line(argv)
if self.extra_args:
self.port=int(self.extra_args[0])

def start(self):
server=next((server for server in list_running_servers(self.runtime_dir) if server.get('port')==self.port),None)
Copy link
Member

Choose a reason for hiding this comment

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

Can this line be broken up a bit? We're not strict about any particular code style, but this isn't super readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i pulled list_running_servers out of the list comprehension so its a little bit shorter but i'm not sure if you'd like more.

if server: os.kill(server.get('pid'), signal.SIGQUIT)
Copy link
Member

Choose a reason for hiding this comment

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

I think SIGTERM is probably the right signal here, instead of SIGQUIT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So strange... I could swear my initial tests requested confirmation after sending sigterm (which is why I switched to sigquit) but I just double checked and sigterm worked fine. Anyway its fixed now.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible you tested with SIGINT initially? IIRC that's the one that asks for confirmation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably something like that. apologies for confusion though - glad it worked in the end :)

else:
ports=[s.get('port') for s in list_running_servers(self.runtime_dir)]
Copy link
Member

Choose a reason for hiding this comment

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

You can now reuse the servers variable here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha - yeah, that was the very first thing i did, back before my initial commit, but list_running_servers returns an iterator so it doesn't work...

In [1]: from notebook.notebookapp import *

In [2]: nbstop=NbserverStopApp()

In [3]: servers=list_running_servers(nbstop.runtime_dir)

In [4]: [s for s in servers]
Out[4]: 
[{u'base_url': u'/',
  u'hostname': u'localhost',
  u'notebook_dir': u'/Users/brook/code/jupyter/notebook',
  u'password': False,
  u'pid': 27105,
  u'port': 8888,
  u'secure': False,
  u'token': u'48019c94f11b76fa4690c1a7d431bf58e4b2fb2d0cd65ae5',
  u'url': u'http://localhost:8888/'},
 {u'base_url': u'/',
  u'hostname': u'localhost',
  u'notebook_dir': u'/Users/brook/code/jupyter/notebook',
  u'password': False,
  u'pid': 27213,
  u'port': 8889,
  u'secure': False,
  u'token': u'4c1588c1139d214b9d1eb432fd000270e2645a226633fde5',
  u'url': u'http://localhost:8889/'},
 {u'base_url': u'/',
  u'hostname': u'localhost',
  u'notebook_dir': u'/Users/brook/code/jupyter/notebook',
  u'password': False,
  u'pid': 27238,
  u'port': 8890,
  u'secure': False,
  u'token': u'db40ac85ddd5694cc4f67d9c4a18f4c066b63887f6d9fb9b',
  u'url': u'http://localhost:8890/'}]

In [5]: [s for s in servers]
Out[5]: []

Copy link
Member

Choose a reason for hiding this comment

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

Aha, OK then. Thanks, merging :-)

if ports:
print("There is currently no server running on port {}.".format(self.port))
Copy link
Member

Choose a reason for hiding this comment

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

This print statement can be moved up one level, and only print ports below.

Copy link
Member

Choose a reason for hiding this comment

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

These print statements should also go on stderr, since they are error output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It we move this above if ports: the output for no running servers will be

There is currently no server running on port 8888
There are currently no running servers

Its seems the

There are currently no running servers

might be better. Thoughts?

print("Ports currently in use:")
for port in ports: print("\t* {}".format(port))
Copy link
Member

Choose a reason for hiding this comment

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

This branch should have exit code 1 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

else:
print("There are currently no running servers")
Copy link
Member

Choose a reason for hiding this comment

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

This should also end with self.exit(1) when no server can be found to stop.

self.exit(1)


class NbserverListApp(JupyterApp):
version = __version__
description="List currently running notebook servers."
Expand Down Expand Up @@ -449,6 +475,7 @@ class NotebookApp(JupyterApp):

subcommands = dict(
list=(NbserverListApp, NbserverListApp.description.splitlines()[0]),
stop=(NbserverStopApp, NbserverStopApp.description.splitlines()[0]),
password=(NotebookPasswordApp, NotebookPasswordApp.description.splitlines()[0]),
)

Expand Down