Skip to content
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

Command-line interface: jupyter execute #165

Merged
merged 26 commits into from
Nov 2, 2021
Merged

Command-line interface: jupyter execute #165

merged 26 commits into from
Nov 2, 2021

Conversation

palewire
Copy link
Contributor

I've added here an experimental CLI for the purposes of spurring along the dicussion in #4. It can be tested by doing a quick editable install:

pip install --editable .

Then you can see the options

$ pipenv run nbclient --help 
Usage: nbclient [OPTIONS] COMMAND [ARGS]...

  Run Jupyter Notebooks from the command line

Options:
  --help  Show this message and exit.

Commands:
  execute  Execute a notebook

# palewire @ bunkerhill in ~/Code/nbclient on git:cli x [17:05:08] 
$ pipenv run nbclient execute --help
Usage: nbclient execute [OPTIONS] NOTEBOOK_PATH

  Execute a notebook

Options:
  -o, --output TEXT               Where to output the result
  -t, --timeout INTEGER           How long the script should run before it
                                  times out
  --allow-errors / --no-allow-errors
  --force-raise-errors / --no-force-raise-errors
  --help                          Show this message and exit.

The notebooks in the binder can be run with it like so:

pipenv run nbclient execute binder/empty_notebook.ipynb

@palewire palewire mentioned this pull request Sep 27, 2021
@davidbrochart
Copy link
Member

Thanks @palewire.
I am not opposed to having a CLI for nbclient, and I like click, but since nbclient can be configured through traitlets, maybe it would be better to use a traitlets-based CLI? That's also a common pattern for all Jupyter projects.

@palewire
Copy link
Contributor Author

Copy. You would like to aim for something similar in structure to this?

https://github.com/jupyter/nbconvert/blob/main/nbconvert/nbconvertapp.py

@davidbrochart
Copy link
Member

Maybe, yes. But it also depends how configurable we want the CLI to be. If we don't want to allow tuning nbclient's parameters, then it's not worth it. But if we do, then we should use traitlets' config.
I remember @MSeal was not in favor of having a CLI back in the days, how do you feel today about it?

@MSeal
Copy link
Contributor

MSeal commented Sep 27, 2021

I still feel nbconvert and papermill already have CLIs to achieve execution actions so it adds to maintenance burden and makes 3 ways to do things. But I'm not going to get in to way if there's a push and others are willing to support it over time.

@palewire
Copy link
Contributor Author

palewire commented Sep 28, 2021

@MSeal, thanks for the feedback. My view is that while nbconvert and papermill are great tools, there's still a gap.

nbconvert is framed as a conversion tool, which it's very good at. But the kitchen sink of conversion options is far beyond what anyone who is simply seeking to run a notebook needs. And the conversion focus of its marketing makes its "brand" difficult for newbies to grok.

papermill is impressive, but it's overkill for the use case I have in mind, which is a single hacker looking to run a single notebook. Maybe even schedule it with a cron or a GitHub Action. In my mind, that user wants a simple, obvious command to run a notebook. To me, that's something like:

jupyter execute my_notebook.ipynb

I feel like I can speak for this user, because this user is me. I also see this case crop up among my colleagues, who write a little web scraper or a data processor and want a quick way to schedule runs.

And let me add, if we can hammer out a CLI solution everyone is happy with, I am open to developing and supporting it.

@palewire
Copy link
Contributor Author

@davidbrochart I've pushed up a new draft that uses JupyterApp and traitlets as the spine of the command, which can now be run like so:

jupyter execute binder/empty_notebook.ipynb

The file here is still skeletal, of course. Before I try to trick anything out, I'd like to be sure you think it's on the right road.

@davidbrochart
Copy link
Member

Before I try to trick anything out, I'd like to be sure you think it's on the right road.

It looks great so far!

@palewire
Copy link
Contributor Author

palewire commented Sep 30, 2021

Okay, @davidbrochart, another couple steps.

I've added command-line options for client inputs. The full suite of args and kwargs seemed like overkill to me, so I cut it down to options that, to me, seemed crucial to terminal users. I welcome your feedback on how I did. Assuming you are onboard with only surfacing a subset, is there a way to allow setting the options for config file users without adding them to the CLI?

One key default, in my opinion, is that errors are raised without any additional options needed. In my view, the standard expectation of CLI users is, "If my code has an error, it will be raised." So that's been set here.

I've also taken the liberty of adding a new notebook with a single cell, 0/0. It can be used to test the error kwargs.

jupyter execute nbclient/tests/Error.ipynb
jupyter execute nbclient/tests/Error.ipynb --allow-errors

When you have a moment, take a look and let me know what else you'd like to see here. I'm happy to keep plugging.

@davidbrochart
Copy link
Member

The full suite of args and kwargs seemed like overkill to me, so I cut it down to options that, to me, seemed crucial to terminal users.

That seems reasonable, and we can support more options in the future if needed.

is there a way to allow setting the options for config file users without adding them to the CLI?

I'm not sure about that.

One key default, in my opinion, is that errors are raised without any additional options needed. In my view, the standard expectation of CLI users is, "If my code has an error, it will be raised." So that's been set here.

👍

When you have a moment, take a look and let me know what else you'd like to see here. I'm happy to keep plugging.

Maybe handle the case of jupyter execute with no argument so that it prints something like:

usage: jupyter-execute [-h] [--debug] [--show-config] [--show-config-json] [--generate-config] [-y] [--allow-errors] [--log-level NbClientApp.log_level]
                       [--config NbClientApp.config_file] [--timeout NbClientApp.timeout] [--startup_timeout NbClientApp.startup_timeout]
                       [--kernel_name NbClientApp.kernel_name]
                       [extra_args ...]
jupyter-execute: error: expected path to notebook

Also, --show-config, --show-config-json, --generate-config, --config... don't seem to have any effect.

@palewire
Copy link
Contributor Author

palewire commented Oct 1, 2021

Thanks for the feedback, @davidbrochart.

I've taken a quick stab at the "no input" message, but we are now running up against ignorance on my side about the internals of traitlets. As far as I can see in the Application class code there is no print_usage option similar to the one found in Python's built-in ArgumentParser. So, failing to find that, I've fallen back to using print_help, which achieves something similar to what you've requested, but much more verbose.

Here's how I did it. Let me know if there's something smarter I can put here.

        if not notebooks:
            self.print_help()
            print("jupyter-execute: error: expected path to notebook")
            sys.exit(-1)

On the second matter, I'm unsure how to properly support the base aliases and flags like --show-config. I've included them thus far out of deference to what I found in nbconvert's CLI, which I took to be an example. My assumption was that all JupyterApp commands were expected to include those default options, with the base class handling them where appropriate. If that's not the case and my copy pasta has introduced cruft, please educate me and I will tidy up the draft.

@davidbrochart
Copy link
Member

The usage message I posted above was from entering:

jupyter execute nbclient/tests/Error.ipynb --foo

Maybe that can help to show a similar message when no notebook is provided.
BTW maybe Error.ipynb can be moved to nbclient/tests/files?
For the config flags, if we don't know what they do, then maybe we shouldn't support them?

@palewire
Copy link
Contributor Author

palewire commented Oct 1, 2021

Okay. I've done the following:

  • Moved the error notebook into the proper subdirectory
  • Removed all the base flags and aliases from JupyterApp that I didn't have a reason for including
  • Restored a couple lines I had accidentally deleted

@davidbrochart, I'm still unable to find an API hook to do that shorter print_usage style output with your foo example. If I'm reading the traitlets internals right, it appears that it must be called several layers below the Application class via a loader class that appears to be a grandchild of the ArgumentParser. Unless i'm missing something, we'd need to subclass a big chunk of our new JupyterApplication to access that printer. If you think I'm missing a way to get it, let me know.

@davidbrochart
Copy link
Member

Sorry, I can't help you with that.
Maybe catching a missing notebook argument and printing something like "You must provide a notebook" is enough (as you did but without the whole help).

@palewire
Copy link
Contributor Author

palewire commented Oct 1, 2021

Roger, @davidbrochart. I've trimmed that bit to this:

        if not self.notebooks:
            print("jupyter-execute: error: expected path to notebook")
            sys.exit(-1)

You can test it with:

jupyter execute

@palewire
Copy link
Contributor Author

palewire commented Oct 1, 2021

Thanks for all the pointers, @davidbrochart. If there's nothing else on your list, I am going to mark this PR as ready for review. Are there others we should pull into the conversation?

@palewire palewire marked this pull request as ready for review October 1, 2021 17:30
@palewire palewire changed the title Experimental CLI Command-line interface: jupyter execute Oct 1, 2021
@choldgraf
Copy link
Contributor

Hey @palewire - thanks for this PR! It is super helpful to see a specific implementation and some of the discussion it has generated has been helpful for me. I provided some thoughts over in #4 since they are more high level than discussing just this implementation. Would love to hear what folks think

@palewire
Copy link
Contributor Author

I've started in on some modifications to the documentation, which appear in the most recent commits.

@davidbrochart davidbrochart merged commit 5056247 into jupyter:master Nov 2, 2021
@davidbrochart
Copy link
Member

Thanks @palewire !

@palewire palewire deleted the cli branch November 2, 2021 08:47
@palewire
Copy link
Contributor Author

palewire commented Nov 2, 2021

Excellent. Mission accomplished on #4. Thanks for all your guidance.

@choldgraf
Copy link
Contributor

Thanks for the contribution @palewire 🙂

@palewire
Copy link
Contributor Author

palewire commented Nov 3, 2021

Once we get this released here, I think the next front is to suggest it for inclusion in the jupyter package so the CLI can go out to the wider world.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyter-run-requires-notebook-to-be-previously-run/12250/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants