From 02d1554f8f5bc5cd88b6a9df5c3257fa203b912e Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 14 Mar 2016 15:57:46 +0100 Subject: [PATCH 1/3] Allow modifying kernel associated with a session via PATCH - PATCH to kernel.id attaches to other kernel - PATCH to kernel.name starts new kernel with given name Previous kernel is shutdown after the change --- notebook/services/sessions/handlers.py | 29 ++++++++++- notebook/services/sessions/sessionmanager.py | 25 +++++---- .../sessions/tests/test_sessions_api.py | 51 +++++++++++++++++-- 3 files changed, 92 insertions(+), 13 deletions(-) diff --git a/notebook/services/sessions/handlers.py b/notebook/services/sessions/handlers.py index f70c17a584..d538b1b091 100644 --- a/notebook/services/sessions/handlers.py +++ b/notebook/services/sessions/handlers.py @@ -86,19 +86,46 @@ def get(self, session_id): @json_errors @gen.coroutine def patch(self, session_id): - # Currently, this handler is strictly for renaming notebooks + """Patch updates sessions: + + - notebook.path updates session to track renamed notebooks + - kernel.name starts a new kernel with a given kernelspec + """ sm = self.session_manager + km = self.kernel_manager model = self.get_json_body() if model is None: raise web.HTTPError(400, "No JSON data provided") + + # get the previous session model + before = yield gen.maybe_future(sm.get_session(session_id=session_id)) + changes = {} if 'notebook' in model: notebook = model['notebook'] if 'path' in notebook: changes['path'] = notebook['path'] + if 'kernel' in model: + if 'name' in model['kernel']: + kernel_name = model['kernel']['name'] + kernel_id = yield sm.start_kernel_for_session( + session_id, kernel_name=kernel_name, path=before['notebook']['path']) + changes['kernel_id'] = kernel_id + if 'id' in model['kernel']: + kernel_id = model['kernel']['id'] + if kernel_id not in km: + raise web.HTTPError(400, "No such kernel: %s" % kernel_id) + changes['kernel_id'] = kernel_id yield gen.maybe_future(sm.update_session(session_id, **changes)) model = yield gen.maybe_future(sm.get_session(session_id=session_id)) + + if model['kernel']['id'] != before['kernel']['id']: + # kernel_id changed because we got a new kernel + # shutdown the old one + yield gen.maybe_future( + km.shutdown_kernel(before['kernel']['id']) + ) self.finish(json.dumps(model, default=date_default)) @web.authenticated diff --git a/notebook/services/sessions/sessionmanager.py b/notebook/services/sessions/sessionmanager.py index 1259404083..11c810f3d0 100644 --- a/notebook/services/sessions/sessionmanager.py +++ b/notebook/services/sessions/sessionmanager.py @@ -62,22 +62,29 @@ def session_exists(self, path): def new_session_id(self): "Create a uuid for a new session" return unicode_type(uuid.uuid4()) - + @gen.coroutine def create_session(self, path=None, kernel_name=None): """Creates a session and returns its model""" session_id = self.new_session_id() - # allow nbm to specify kernels cwd - kernel_path = self.contents_manager.get_kernel_path(path=path) - kernel_id = yield gen.maybe_future( - self.kernel_manager.start_kernel(path=kernel_path, kernel_name=kernel_name) - ) + kernel_id = yield self.start_kernel_for_session(session_id, path, kernel_name) result = yield gen.maybe_future( self.save_session(session_id, path=path, kernel_id=kernel_id) ) # py2-compat raise gen.Return(result) - + + @gen.coroutine + def start_kernel_for_session(self, session_id, path, kernel_name): + """Start a new kernel for a given session.""" + # allow contents manager to specify kernels cwd + kernel_path = self.contents_manager.get_kernel_path(path=path) + kernel_id = yield gen.maybe_future( + self.kernel_manager.start_kernel(path=kernel_path, kernel_name=kernel_name) + ) + # py2-compat + raise gen.Return(kernel_id) + def save_session(self, session_id, path=None, kernel_id=None): """Saves the items for the session with the given session_id @@ -211,9 +218,9 @@ def list_sessions(self): pass return result + @gen.coroutine def delete_session(self, session_id): """Deletes the row in the session database with given session_id""" - # Check that session exists before deleting session = self.get_session(session_id=session_id) - self.kernel_manager.shutdown_kernel(session['kernel']['id']) + yield gen.maybe_future(self.kernel_manager.shutdown_kernel(session['kernel']['id'])) self.cursor.execute("DELETE FROM session WHERE session_id=?", (session_id,)) diff --git a/notebook/services/sessions/tests/test_sessions_api.py b/notebook/services/sessions/tests/test_sessions_api.py index 1a2dc80030..e824522810 100644 --- a/notebook/services/sessions/tests/test_sessions_api.py +++ b/notebook/services/sessions/tests/test_sessions_api.py @@ -44,10 +44,18 @@ def create(self, path, kernel_name='python'): 'kernel': {'name': kernel_name}}) return self._req('POST', '', body) - def modify(self, id, path): + def modify_path(self, id, path): body = json.dumps({'notebook': {'path':path}}) return self._req('PATCH', id, body) + def modify_kernel_name(self, id, kernel_name): + body = json.dumps({'kernel': {'name': kernel_name}}) + return self._req('PATCH', id, body) + + def modify_kernel_id(self, id, kernel_id): + body = json.dumps({'kernel': {'id': kernel_id}}) + return self._req('PATCH', id, body) + def delete(self, id): return self._req('DELETE', id) @@ -115,10 +123,47 @@ def test_delete(self): with assert_http_error(404): self.sess_api.get(sid) - def test_modify(self): + def test_modify_path(self): newsession = self.sess_api.create('foo/nb1.ipynb').json() sid = newsession['id'] - changed = self.sess_api.modify(sid, 'nb2.ipynb').json() + changed = self.sess_api.modify_path(sid, 'nb2.ipynb').json() self.assertEqual(changed['id'], sid) self.assertEqual(changed['notebook']['path'], 'nb2.ipynb') + + def test_modify_kernel_name(self): + before = self.sess_api.create('foo/nb1.ipynb').json() + sid = before['id'] + + after = self.sess_api.modify_kernel_name(sid, before['kernel']['name']).json() + self.assertEqual(after['id'], sid) + self.assertEqual(after['notebook'], before['notebook']) + self.assertNotEqual(after['kernel']['id'], before['kernel']['id']) + + # check kernel list, to be sure previous kernel was cleaned up + r = requests.get(url_path_join(self.base_url(), 'api/kernels')) + r.raise_for_status() + kernel_list = r.json() + self.assertEqual(kernel_list, [after['kernel']]) + + def test_modify_kernel_id(self): + before = self.sess_api.create('foo/nb1.ipynb').json() + sid = before['id'] + + # create a new kernel + r = requests.post(url_path_join(self.base_url(), 'api/kernels')) + r.raise_for_status() + kernel = r.json() + + # Attach our session to the existing kernel + after = self.sess_api.modify_kernel_id(sid, kernel['id']).json() + self.assertEqual(after['id'], sid) + self.assertEqual(after['notebook'], before['notebook']) + self.assertNotEqual(after['kernel']['id'], before['kernel']['id']) + self.assertEqual(after['kernel']['id'], kernel['id']) + + # check kernel list, to be sure previous kernel was cleaned up + r = requests.get(url_path_join(self.base_url(), 'api/kernels')) + r.raise_for_status() + kernel_list = r.json() + self.assertEqual(kernel_list, [kernel]) From e9957ddb35f1f3c8569506078a90851daefe289b Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 14 Mar 2016 16:49:25 +0100 Subject: [PATCH 2/3] Handle coroutine possibility in test_sessionmanager --- notebook/services/sessions/tests/test_sessionmanager.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/notebook/services/sessions/tests/test_sessionmanager.py b/notebook/services/sessions/tests/test_sessionmanager.py index 0b31d97a4e..14bda028b5 100644 --- a/notebook/services/sessions/tests/test_sessionmanager.py +++ b/notebook/services/sessions/tests/test_sessionmanager.py @@ -175,6 +175,8 @@ def test_bad_delete_session(self): # try to delete a session that doesn't exist ~ raise error sm = self.sm self.create_session(path='/path/to/test.ipynb', kernel_name='python') - self.assertRaises(TypeError, sm.delete_session, bad_kwarg='23424') # Bad keyword - self.assertRaises(web.HTTPError, sm.delete_session, session_id='23424') # nonexistant + with self.assertRaises(TypeError): + self.loop.run_sync(lambda : sm.delete_session(bad_kwarg='23424')) # Bad keyword + with self.assertRaises(web.HTTPError): + self.loop.run_sync(lambda : sm.delete_session(session_id='23424')) # nonexistent From 23b1beaabc316e3a728937429d877126e2baf2c0 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 15 Mar 2016 13:56:26 +0100 Subject: [PATCH 3/3] treat null values as unspecified in session PATCH --- notebook/services/sessions/handlers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/notebook/services/sessions/handlers.py b/notebook/services/sessions/handlers.py index d538b1b091..987255d9c0 100644 --- a/notebook/services/sessions/handlers.py +++ b/notebook/services/sessions/handlers.py @@ -103,15 +103,15 @@ def patch(self, session_id): changes = {} if 'notebook' in model: notebook = model['notebook'] - if 'path' in notebook: + if notebook.get('path') is not None: changes['path'] = notebook['path'] if 'kernel' in model: - if 'name' in model['kernel']: + if model['kernel'].get('name') is not None: kernel_name = model['kernel']['name'] kernel_id = yield sm.start_kernel_for_session( session_id, kernel_name=kernel_name, path=before['notebook']['path']) changes['kernel_id'] = kernel_id - if 'id' in model['kernel']: + if model['kernel'].get('id') is not None: kernel_id = model['kernel']['id'] if kernel_id not in km: raise web.HTTPError(400, "No such kernel: %s" % kernel_id)