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

[MRG] Notebook CLI #148

Merged
merged 9 commits into from Nov 27, 2016

Conversation

Projects
None yet
2 participants
@Titan-C
Member

Titan-C commented Sep 24, 2016

This is a command line utility to expose sphinx gallery notebook generator.
I manually tested it on bash and zsh.

calling

sphx_ex2nb.py *.py

will convert all python files into notebooks

@Titan-C Titan-C added the enhancement label Oct 8, 2016

@Titan-C Titan-C changed the title from Notebook CLI to [MRG] Notebook CLI Oct 8, 2016

@Titan-C

This comment has been minimized.

Member

Titan-C commented Oct 10, 2016

@GaelVaroquaux Any comments on this one?

@Titan-C Titan-C force-pushed the Titan-C:notebookcli branch from e3e2d87 to 67db005 Nov 7, 2016

@lesteve

This comment has been minimized.

Contributor

lesteve commented Nov 8, 2016

Just curious, are you using the sphx_ex2nb.py script yourself?

setup.py Outdated
@@ -30,7 +30,7 @@
packages=find_packages(),
package_data={'sphinx_gallery': ['_static/gallery.css', '_static/no_image.png',
'_static/broken_example.png']},
scripts=['bin/copy_sphinxgallery.sh'],
scripts=['bin/copy_sphinxgallery.sh', 'bin/sphx_ex2nb.py'],

This comment has been minimized.

@lesteve

lesteve Nov 15, 2016

Contributor

Maybe something less cryptic like sphx_glr_example_to_notebook? Not 100% convinced so better suggestions welcome! My point is that trying to shorten names as much as possible does not make too much sense since you have tab-completion these days ...

args = parser.parse_args()
for src_file in args.file:
file_name = os.path.basename(src_file)

This comment has been minimized.

@lesteve

lesteve Nov 15, 2016

Contributor

For consistency I think this should be src_filename and filename. I tend to use filename for file names and file for object files (i.e. as returned from open) to make it easier to differentiate between the two.

@@ -118,6 +126,8 @@ def __init__(self, file_name, target_dir):
self.write_file = os.path.join(target_dir, self.file_name)
self.work_notebook = ipy_notebook_skeleton()
self.add_code_cell("%matplotlib inline")
self.fill_notebook(script_blocks)
self.save_file()

This comment has been minimized.

@lesteve

lesteve Nov 15, 2016

Contributor

It's generally considered best practice to have minimal logic in the constructor. In this case I would rather have an API like:

nb = Notebook(source_filename)
nb.to_notebook(target_filename)

The parsing of the blocks from source_filename could happen in the constructor.

@Titan-C Titan-C force-pushed the Titan-C:notebookcli branch from d711d71 to 2d0fb66 Nov 19, 2016

@Titan-C

This comment has been minimized.

Member

Titan-C commented Nov 19, 2016

Comments addressed. Notebook generation is simpler now as well. And I have included test for the command line utility

@Titan-C Titan-C force-pushed the Titan-C:notebookcli branch from 2d0fb66 to 0d28919 Nov 20, 2016

@lesteve

A few comments:

def __init__(self, file_name, target_dir):
"""Declare the skeleton of the notebook
work_notebook = ipy_notebook_skeleton()

This comment has been minimized.

@lesteve

lesteve Nov 22, 2016

Contributor

ipy_notebook -> jupyter_notebook here and everywhere else.

"""Test that written ipython notebook file corresponds to python object"""
blocks = sg.split_code_and_text_blocks('tutorials/plot_parse.py')
example_nb = jupyter_notebook(blocks)
nb_filename = "/tmp/testnb.ipynb"

This comment has been minimized.

@lesteve

lesteve Nov 22, 2016

Contributor

This is not going to work on Windows. Use tempfile.NamedTemporaryFile

FileNotFoundError = IOError
class CommandLineTest(TestCase):

This comment has been minimized.

@lesteve

lesteve Nov 22, 2016

Contributor

What's the point of using a test class with methods rather than just functions?

This comment has been minimized.

@Titan-C

Titan-C Nov 27, 2016

Member

I need this class with methods because of unittest. This methods are specific to test exceptions are raised, and come inside unittest.TestCase.
To use functions I need to explicitly rely on nosetest or pytest own functions to test exceptions.

Convert Python scripts into Jupyter Notebooks
=============================================
Sphinx Gallery exposes its python source to Jupyter notebook convert

This comment has been minimized.

@lesteve

lesteve Nov 22, 2016

Contributor

converter

.travis.yml Outdated
@@ -68,6 +68,7 @@ before_script:
script:
- if [ "$DISTRIB" == "ubuntu" ]; then python setup.py nosetests; fi
- if [ "$DISTRIB" == "conda" ]; then nosetests; fi
- nosetests bin # to test jupyter notebook generator CLI

This comment has been minimized.

@lesteve

lesteve Nov 22, 2016

Contributor

not very conventional ... maybe it would be great if the scripts in bin, was something super simple like:

from sphinx_gallery.notebook import main

if __name__ == '__main__':
    main()

This way you can still test the parser in sphinx_gallery/tests.

This comment has been minimized.

@Titan-C

Titan-C Nov 27, 2016

Member

I put this line because I could not get nosetest to scan the bin folder. Pytest did not have issues on that. But indeed I'll move everything to the notebook parsing file.

@Titan-C Titan-C force-pushed the Titan-C:notebookcli branch 2 times, most recently from 64276d9 to 5fb17e3 Nov 27, 2016

Titan-C added some commits Sep 22, 2016

Fill notebook function
text2string into notebook to avoid circular dependence
Python to Jupyter converter script
- Converter script takes multiple files & wildcards
- Install notebook converter script with setup.py
Documentation on Utilities
Includes the script converter and moves the gallery downloader
Refactor and test notebook generation
- Jupyter notebook generator function name is explicit
Test CLI script to notebook
CLI inside notebook file, this allows for testing directly on the
python module responsible for the rendering of the notebooks.

@Titan-C Titan-C force-pushed the Titan-C:notebookcli branch from 5fb17e3 to c881f3d Nov 27, 2016

@Titan-C

This comment has been minimized.

Member

Titan-C commented Nov 27, 2016

Rebased to master and comments addressed. Cleaned the commit history too

class CommandLineTest(TestCase):
"""Test the Sphinx-Gallery python to Jupyter notebook converter CLI"""
def test_with_empty_args(self):

This comment has been minimized.

@lesteve

lesteve Nov 27, 2016

Contributor

why still the class methods rather than functions?

This comment has been minimized.

@lesteve

lesteve Nov 27, 2016

Contributor

I push a change in your branch because apart from this it is good for merging I think.

lesteve added some commits Nov 27, 2016

@lesteve

This comment has been minimized.

Contributor

lesteve commented Nov 27, 2016

Merging this one, thanks!

@Titan-C

This comment has been minimized.

Member

Titan-C commented Nov 27, 2016

Thanks for the changes. I'm not sure if you got the comment about using the class methods for test. My idea behind it is that that is how unittest does in the standard library tests for exceptions. And since we plan to move to pytest. I did not wanted to include more use to nosetest.

@lesteve lesteve merged commit f1e089f into sphinx-gallery:master Nov 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lesteve

This comment has been minimized.

Contributor

lesteve commented Nov 27, 2016

Oops sorry, I missed your reply somehow. In order not to have camelCase in our tests I would either the same method as in scikit-learn so that we can have from sphinx_gallery.testing import assert_raises_regex or it seems like pytest has its own way of checking exception message (from http://doc.pytest.org/en/latest/assert.html):

def test_match():
    with pytest.raises(ValueError) as excinfo:
        myfunc()
    excinfo.match(r'.* 123 .*')
@lesteve

This comment has been minimized.

Contributor

lesteve commented Nov 28, 2016

Cleaned the commit history too

Don't worry about that too much, to be honest I tend to use the "Squash and merge" button these days and edit the commit message if there were too many commits and/or too many useless commit messages.

@Titan-C Titan-C deleted the Titan-C:notebookcli branch Dec 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment