-
Notifications
You must be signed in to change notification settings - Fork 5k
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
NbserverStopApp: stop notebooks through cli - jupyter notebook stop <… #2388
Conversation
Thanks @brookisme! I tested and it's working for me! @minrk Can you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this makes sense in principle. I've made some comments in-line for updating the PR.
If you would like any help addressing the comments, feel free to ask.
notebook/notebookapp.py
Outdated
"Produce machine-readable JSON output."), | ||
) | ||
|
||
json = Bool(True, config=True, |
There was a problem hiding this comment.
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
notebook/notebookapp.py
Outdated
version = __version__ | ||
description="Stop currently running notebook server for a given port" | ||
kill_cmd='kill' | ||
kill_signal='-3' |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
notebook/notebookapp.py
Outdated
def start(self): | ||
server=next((server for server in list_running_servers(self.runtime_dir) if server.get('port')==self.port),None) | ||
if server: | ||
subprocess.Popen([self.kill_cmd,self.kill_signal,str(server.get('pid'))],stdout=subprocess.PIPE).communicate() |
There was a problem hiding this comment.
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.
notebook/notebookapp.py
Outdated
|
||
flags = dict( | ||
json=({'NbserverStopApp': {'json': True}}, | ||
"Produce machine-readable JSON output."), |
There was a problem hiding this comment.
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
notebook/notebookapp.py
Outdated
else: | ||
ports=[s.get('port') for s in list_running_servers(self.runtime_dir)] | ||
if ports: | ||
print("There is currently no server running on port {}.".format(self.port)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) | ||
else: | ||
print("There are currently no running servers") |
There was a problem hiding this comment.
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.
@minrk thanks for the feedback. This should be complete apart from the minor no servers running issue commented on above. |
Should to docs be updated to reflect this new subcommand? |
notebook/notebookapp.py
Outdated
|
||
def start(self): | ||
server=next((server for server in list_running_servers(self.runtime_dir) if server.get('port')==self.port),None) | ||
if server: os.kill(server.get('pid'), signal.SIGQUIT) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than these things, this looks good to me.
notebook/notebookapp.py
Outdated
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ports: | ||
print("There is currently no server running on port {}.".format(self.port)) | ||
print("Ports currently in use:") | ||
for port in ports: print("\t* {}".format(port)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
server=next((server for server in servers if server.get('port')==self.port),None) | ||
if server: os.kill(server.get('pid'), signal.SIGTERM) | ||
else: | ||
ports=[s.get('port') for s in list_running_servers(self.runtime_dir)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]: []
There was a problem hiding this comment.
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 :-)
starting from which version of jupyter is this feature available?
|
It should be available from notebook 5.1. |
jupyter notebook stop <PORT>