Merge pull request #1206 from minrk/patch-session-kernel

Allow modifying kernel associated with a session via PATCH

notebook/services/sessions/handlers.py

@@ -86,19 +86,46 @@ class SessionHandler(APIHandler):
     @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:
+            if notebook.get('path') is not None:
                 changes['path'] = notebook['path']
+        if 'kernel' in model:
+            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 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)
+                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

notebook/services/sessions/sessionmanager.py

@@ -62,22 +62,29 @@ class SessionManager(LoggingConfigurable):
     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 @@ class SessionManager(LoggingConfigurable):
                 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,))

notebook/services/sessions/tests/test_sessionmanager.py

@@ -175,6 +175,8 @@ class TestSessionManager(TestCase):
         # 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
 

notebook/services/sessions/tests/test_sessions_api.py

@@ -44,10 +44,18 @@ class SessionAPI(object):
                            '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 @@ class SessionAPITest(NotebookTestBase):
         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])