From 5627ee6ecb60a7c72b52abc64d720c2d57d52fe4 Mon Sep 17 00:00:00 2001 From: M Pacer Date: Tue, 11 Sep 2018 12:22:51 -0700 Subject: [PATCH 1/6] add new find_kernel_name API for finding kernel names from nb metadata This is being introduced because we previously allowed users to pass "" to indicate that the kernel name should be exclusively inferred from the notebook itself. Doing so now overrides the default behaviour (which now defaults to infering the kernel name from the notebook itself if nothing is set). This is not a great pattern since it involves setting a parameter to a magic value to keep the default behaviour. This also issues a warning if you use the old way to do this. --- nbconvert/preprocessors/execute.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/nbconvert/preprocessors/execute.py b/nbconvert/preprocessors/execute.py index fe80c5d66..f03d83230 100644 --- a/nbconvert/preprocessors/execute.py +++ b/nbconvert/preprocessors/execute.py @@ -4,6 +4,8 @@ # Copyright (c) IPython Development Team. # Distributed under the terms of the Modified BSD License. +import warnings + from textwrap import dedent from contextlib import contextmanager @@ -161,7 +163,7 @@ class ExecutePreprocessor(Preprocessor): @default('kernel_name') def _kernel_name_default(self): try: - return self.nb.metadata.get('kernelspec', {}).get('name', 'python') + return find_kernel_name(self.nb) except AttributeError: raise AttributeError('You did not specify a kernel_name for ' 'the ExecutePreprocessor and you have not set ' @@ -248,6 +250,16 @@ def start_new_kernel(self, **kwargs): kc : KernelClient Kernel client as created by the kernel manager `km`. """ + + # Because of an old API, an empty kernel string should be interpreted as indicating + # that you want to use the notebook's metadata specified kernel. + # We use find_kernel_name to do this. + if self.kernel_name == u"": + self.kernel_name = find_kernel_name(self.nb) + warnings.warn("Setting kernel_name to '' as a way to use the kernel in a notebook metadata's is deprecated as of 5.4. \n" + "Instead, do not set kernel_name, this is now default behaviour.", DeprecationWarning, stacklevel=2) + + km = self.kernel_manager_class(kernel_name=self.kernel_name, config=self.config) km.start_kernel(extra_arguments=self.extra_arguments, **kwargs) @@ -516,6 +528,20 @@ def run_cell(self, cell, cell_index=0): return exec_reply, outs +def find_kernel_name(nb): + """This is a utility that finds kernel names from notebook metadata. + + Parameters + ---------- + nb : NotebookNode + The notebook that has a kernelspec defined in its metadata. + + Returns + ------- + kernel_name: str + The string representing the kernel name found in the metadata. + """ + return nb.metadata.get('kernelspec', {}).get('name', 'python') def executenb(nb, cwd=None, km=None, **kwargs): """Execute a notebook's code, updating outputs within the notebook object. From 07109c4a2be1706b910feca53d42f13cf9addd5e Mon Sep 17 00:00:00 2001 From: M Pacer Date: Tue, 11 Sep 2018 12:41:41 -0700 Subject: [PATCH 2/6] add test for find_kernel_name and notebook with kernel name metadata --- .../tests/files/UnicodePy3.ipynb | 32 +++++++++++++++++++ nbconvert/preprocessors/tests/test_execute.py | 11 ++++++- 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 nbconvert/preprocessors/tests/files/UnicodePy3.ipynb diff --git a/nbconvert/preprocessors/tests/files/UnicodePy3.ipynb b/nbconvert/preprocessors/tests/files/UnicodePy3.ipynb new file mode 100644 index 000000000..284c898fb --- /dev/null +++ b/nbconvert/preprocessors/tests/files/UnicodePy3.ipynb @@ -0,0 +1,32 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": 1, + "metadata": { + "collapsed": false + }, + "outputs": [ + { + "name": "stdout", + "output_type": "stream", + "text": [ + "\u2603\n" + ] + } + ], + "source": [ + "print('\u2603')" + ] + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python 3", + "language": "python", + "name": "python3" + } + }, + "nbformat": 4, + "nbformat_minor": 0 +} diff --git a/nbconvert/preprocessors/tests/test_execute.py b/nbconvert/preprocessors/tests/test_execute.py index 52af82508..2ccf7e52a 100644 --- a/nbconvert/preprocessors/tests/test_execute.py +++ b/nbconvert/preprocessors/tests/test_execute.py @@ -19,7 +19,7 @@ import pytest from .base import PreprocessorTestsBase -from ..execute import ExecutePreprocessor, CellExecutionError, executenb +from ..execute import ExecutePreprocessor, CellExecutionError, executenb, find_kernel_name from nbconvert.filters import strip_ansi from testpath import modified_env @@ -265,6 +265,15 @@ def test_custom_kernel_manager(self): for method, call_count in expected: self.assertNotEqual(call_count, 0, '{} was called'.format(method)) + def test_find_kernel_name(self): + current_dir = os.path.dirname(__file__) + filename = os.path.join(current_dir, 'files', 'UnicodePy3.ipynb') + + with io.open(filename) as f: + input_nb = nbformat.read(f, 4) + + assert find_kernel_name(input_nb) == "python3" + def test_execute_function(self): # Test the executenb() convenience API current_dir = os.path.dirname(__file__) From fc3dea10b699665bf9daafe61bb1afd513436741 Mon Sep 17 00:00:00 2001 From: M Pacer Date: Tue, 11 Sep 2018 13:03:21 -0700 Subject: [PATCH 3/6] add test for old behaviour where kernel_name="" uses file specified In order to not use the default "python" behvaiour, I set the kernel to be "python3". However, that means this can only work if we have a python3 kernel available. --- nbconvert/preprocessors/tests/test_execute.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/nbconvert/preprocessors/tests/test_execute.py b/nbconvert/preprocessors/tests/test_execute.py index 2ccf7e52a..ccd5076f6 100644 --- a/nbconvert/preprocessors/tests/test_execute.py +++ b/nbconvert/preprocessors/tests/test_execute.py @@ -21,6 +21,8 @@ from .base import PreprocessorTestsBase from ..execute import ExecutePreprocessor, CellExecutionError, executenb, find_kernel_name +from jupyter_client.kernelspec import KernelSpecManager + from nbconvert.filters import strip_ansi from testpath import modified_env from ipython_genutils.py3compat import string_types @@ -151,6 +153,19 @@ def test_empty_path(self): res['metadata']['path'] = '' input_nb, output_nb = self.run_notebook(filename, {}, res) self.assert_notebooks_equal(input_nb, output_nb) + + @pytest.mark.xfail("python3" not in KernelSpecManager().find_kernel_specs(), + reason="requires a python3 kernelspec") + def test_empty_kernel_name(self): + """Can kernel in nb metadata be found when an empty string is passed? + + Note: this pattern should be discouraged in practice. + Passing in no kernel_name to ExecutePreprocessor is preferable. + """ + filename = os.path.join(current_dir, 'files', 'UnicodePy3.ipynb') + res = self.build_resources() + input_nb, output_nb = self.run_notebook(filename, {"kernel_name": ""}, res) + self.assert_notebooks_equal(input_nb, output_nb) def test_disable_stdin(self): """Test disabling standard input""" From 6e725b9612cd1b92942f74d5bf8527d746acede0 Mon Sep 17 00:00:00 2001 From: M Pacer Date: Wed, 12 Sep 2018 10:25:20 -0700 Subject: [PATCH 4/6] Check for any falsy value, test `kernel_name=None` causes `TraitError` --- nbconvert/preprocessors/execute.py | 8 +++++--- nbconvert/preprocessors/tests/test_execute.py | 9 ++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/nbconvert/preprocessors/execute.py b/nbconvert/preprocessors/execute.py index f03d83230..3a1c807b1 100644 --- a/nbconvert/preprocessors/execute.py +++ b/nbconvert/preprocessors/execute.py @@ -254,13 +254,15 @@ def start_new_kernel(self, **kwargs): # Because of an old API, an empty kernel string should be interpreted as indicating # that you want to use the notebook's metadata specified kernel. # We use find_kernel_name to do this. - if self.kernel_name == u"": - self.kernel_name = find_kernel_name(self.nb) + if not self.kernel_name: + kernel_name = find_kernel_name(self.nb) warnings.warn("Setting kernel_name to '' as a way to use the kernel in a notebook metadata's is deprecated as of 5.4. \n" "Instead, do not set kernel_name, this is now default behaviour.", DeprecationWarning, stacklevel=2) + else: + kernel_name = self.kernel_name - km = self.kernel_manager_class(kernel_name=self.kernel_name, + km = self.kernel_manager_class(kernel_name=kernel_name, config=self.config) km.start_kernel(extra_arguments=self.extra_arguments, **kwargs) diff --git a/nbconvert/preprocessors/tests/test_execute.py b/nbconvert/preprocessors/tests/test_execute.py index ccd5076f6..29a98ddc0 100644 --- a/nbconvert/preprocessors/tests/test_execute.py +++ b/nbconvert/preprocessors/tests/test_execute.py @@ -21,11 +21,12 @@ from .base import PreprocessorTestsBase from ..execute import ExecutePreprocessor, CellExecutionError, executenb, find_kernel_name +from testpath import modified_env +from traitlets import TraitError from jupyter_client.kernelspec import KernelSpecManager +from ipython_genutils.py3compat import string_types from nbconvert.filters import strip_ansi -from testpath import modified_env -from ipython_genutils.py3compat import string_types addr_pat = re.compile(r'0x[0-9a-f]{7,9}') ipython_input_pat = re.compile(r'') @@ -160,12 +161,14 @@ def test_empty_kernel_name(self): """Can kernel in nb metadata be found when an empty string is passed? Note: this pattern should be discouraged in practice. - Passing in no kernel_name to ExecutePreprocessor is preferable. + Passing in no kernel_name to ExecutePreprocessor is recommended instead. """ filename = os.path.join(current_dir, 'files', 'UnicodePy3.ipynb') res = self.build_resources() input_nb, output_nb = self.run_notebook(filename, {"kernel_name": ""}, res) self.assert_notebooks_equal(input_nb, output_nb) + with pytest.raises(TraitError): + input_nb, output_nb = self.run_notebook(filename, {"kernel_name": None}, res) def test_disable_stdin(self): """Test disabling standard input""" From 0972813cc655db7adb58ecf03c7c53cce62d7bd3 Mon Sep 17 00:00:00 2001 From: M Pacer Date: Fri, 14 Sep 2018 23:06:24 -0700 Subject: [PATCH 5/6] switch to using validate on kernel name per Min's suggestion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because we cannot guarantee that self.nb exists when kernel_name is set, this requires some… finagling to handle the empty string correctly. It would be better if we could lazily query the value rather than need to specify at the time of setting. --- nbconvert/preprocessors/execute.py | 48 +++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/nbconvert/preprocessors/execute.py b/nbconvert/preprocessors/execute.py index 3a1c807b1..9c519544b 100644 --- a/nbconvert/preprocessors/execute.py +++ b/nbconvert/preprocessors/execute.py @@ -14,7 +14,7 @@ except ImportError: from Queue import Empty # Py 2 -from traitlets import List, Unicode, Bool, Enum, Any, Type, Dict, Integer, default +from traitlets import List, Unicode, Bool, Enum, Any, Type, Dict, Integer, default, validate from nbformat.v4 import output_from_msg @@ -163,13 +163,33 @@ class ExecutePreprocessor(Preprocessor): @default('kernel_name') def _kernel_name_default(self): try: - return find_kernel_name(self.nb) + return self.nb.metadata.get('kernelspec', {}).get('name', 'python') except AttributeError: raise AttributeError('You did not specify a kernel_name for ' 'the ExecutePreprocessor and you have not set ' 'self.nb to be able to use that to infer the ' 'kernel_name.') + + @validate('kernel_name') + def _valid_kernel_name(self, proposal): + # import pdb;pdb.set_trace() + if proposal.value ==u"" and hasattr(self, 'nb'): + response = self.nb.metadata.get('kernelspec', {}).get('name', 'python') + if response not in list('python', *self.kernelspec_manager_class(parent=self).find_kernel_specs()): + raise AttributeError(""" + You specified an empty kernel_name `''`. We inferred the kernel name `{}` + from your notebook metadata. The {} was unable to find + a kernel by that name.""".format(response, self.kernelspec_manager_class)) + elif proposal.value==u"": + response = 'python' + else: + response = proposal.value + if response not in list('python', *self.kernelspec_manager_class(parent=self).find_kernel_specs()): + raise AttributeError('You specified the kernel_name `{}`. The {} was unable ' + 'to find a kernel by that name'.format(response, self.kernelspec_manager_class)) + return response + raise_on_iopub_timeout = Bool(False, help=dedent( """ @@ -219,6 +239,19 @@ def _kernel_manager_class_default(self): except ImportError: raise ImportError("`nbconvert --execute` requires the jupyter_client package: `pip install jupyter_client`") return KernelManager + + kernelspec_manager_class = Type( + config=True, + help='The kernelspec manager class to use.' + ) + @default('kernelspec_manager_class') + def kernelspec_manager_class_default(self): + """Use a dynamic default to avoid importing jupyter_client at startup""" + try: + from jupyter_client.kernelspec import KernelSpecManager + except ImportError: + raise ImportError("`nbconvert --execute` requires the jupyter_client package: `pip install jupyter_client`") + return KernelSpecManager _display_id_map = Dict( help=dedent( @@ -254,16 +287,9 @@ def start_new_kernel(self, **kwargs): # Because of an old API, an empty kernel string should be interpreted as indicating # that you want to use the notebook's metadata specified kernel. # We use find_kernel_name to do this. - if not self.kernel_name: - kernel_name = find_kernel_name(self.nb) - warnings.warn("Setting kernel_name to '' as a way to use the kernel in a notebook metadata's is deprecated as of 5.4. \n" - "Instead, do not set kernel_name, this is now default behaviour.", DeprecationWarning, stacklevel=2) - else: - kernel_name = self.kernel_name - - km = self.kernel_manager_class(kernel_name=kernel_name, - config=self.config) + km = self.kernel_manager_class(kernel_name=self.kernel_name, + parent=self) km.start_kernel(extra_arguments=self.extra_arguments, **kwargs) kc = km.client() From 0d09e9dc6222a060330e9fc8cb443e56233441ec Mon Sep 17 00:00:00 2001 From: M Pacer Date: Fri, 14 Sep 2018 23:06:48 -0700 Subject: [PATCH 6/6] remove find_kernel_name and tests for it --- nbconvert/preprocessors/execute.py | 14 -------------- nbconvert/preprocessors/tests/test_execute.py | 11 +---------- 2 files changed, 1 insertion(+), 24 deletions(-) diff --git a/nbconvert/preprocessors/execute.py b/nbconvert/preprocessors/execute.py index 9c519544b..29d5f6605 100644 --- a/nbconvert/preprocessors/execute.py +++ b/nbconvert/preprocessors/execute.py @@ -556,20 +556,6 @@ def run_cell(self, cell, cell_index=0): return exec_reply, outs -def find_kernel_name(nb): - """This is a utility that finds kernel names from notebook metadata. - - Parameters - ---------- - nb : NotebookNode - The notebook that has a kernelspec defined in its metadata. - - Returns - ------- - kernel_name: str - The string representing the kernel name found in the metadata. - """ - return nb.metadata.get('kernelspec', {}).get('name', 'python') def executenb(nb, cwd=None, km=None, **kwargs): """Execute a notebook's code, updating outputs within the notebook object. diff --git a/nbconvert/preprocessors/tests/test_execute.py b/nbconvert/preprocessors/tests/test_execute.py index 29a98ddc0..026ce18d2 100644 --- a/nbconvert/preprocessors/tests/test_execute.py +++ b/nbconvert/preprocessors/tests/test_execute.py @@ -19,7 +19,7 @@ import pytest from .base import PreprocessorTestsBase -from ..execute import ExecutePreprocessor, CellExecutionError, executenb, find_kernel_name +from ..execute import ExecutePreprocessor, CellExecutionError, executenb from testpath import modified_env from traitlets import TraitError @@ -283,15 +283,6 @@ def test_custom_kernel_manager(self): for method, call_count in expected: self.assertNotEqual(call_count, 0, '{} was called'.format(method)) - def test_find_kernel_name(self): - current_dir = os.path.dirname(__file__) - filename = os.path.join(current_dir, 'files', 'UnicodePy3.ipynb') - - with io.open(filename) as f: - input_nb = nbformat.read(f, 4) - - assert find_kernel_name(input_nb) == "python3" - def test_execute_function(self): # Test the executenb() convenience API current_dir = os.path.dirname(__file__)