Skip to content

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

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

Merged
takluyver merged 8 commits into from
Apr 11, 2017
Merged

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

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions notebook/notebookapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import logging
import mimetypes
import os
import subprocess
import random
import re
import select
Expand Down Expand Up @@ -346,6 +347,45 @@ 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"
kill_cmd='kill'
kill_signal='-3'
Copy link
Member

@minrk minrk Apr 8, 2017

Choose a reason for hiding this comment

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

signal should be an integer from the signal module, e.g.signal.SIGTERM

Copy link
Author

@brookisme brookisme Apr 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

using signal.SIGQUIT since signal.SIGTERM requests confirmation
UPDATE: SIGQUIT works fine. See comment below.


flags = dict(
json=({'NbserverStopApp': {'json': True}},
"Produce machine-readable JSON output."),
Copy link
Member

@minrk minrk Apr 8, 2017

Choose a reason for hiding this comment

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

flags here are also unused and can be removed

)

json = Bool(True, config=True,
Copy link
Member

@minrk minrk Apr 8, 2017

Choose a reason for hiding this comment

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

json is unused and can be removed

help="If True, each line of output will be a JSON object with the "
"details from the server info fpyile.")

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

@takluyver takluyver Apr 11, 2017

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
Author

@brookisme brookisme Apr 11, 2017

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:
subprocess.Popen([self.kill_cmd,self.kill_signal,str(server.get('pid'))],stdout=subprocess.PIPE).communicate()
Copy link
Member

@minrk minrk Apr 8, 2017

Choose a reason for hiding this comment

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

Rather than Popen(['kill', ...]), it would be better to use os.kill here.

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

@takluyver takluyver Apr 11, 2017

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
Author

@brookisme brookisme Apr 11, 2017

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

@takluyver takluyver Apr 11, 2017

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

@minrk minrk Apr 8, 2017

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

@minrk minrk Apr 8, 2017

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
Author

@brookisme brookisme Apr 8, 2017

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

@takluyver takluyver Apr 11, 2017

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
Author

@brookisme brookisme Apr 11, 2017

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

@minrk minrk Apr 8, 2017

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.



class NbserverListApp(JupyterApp):
version = __version__
description="List currently running notebook servers."
Expand Down Expand Up @@ -449,6 +489,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