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

Changed the default command to jupyter run, but left in execute as an alias with a deprecation warning #173

Merged
merged 3 commits into from
Nov 12, 2021

Conversation

palewire
Copy link
Contributor

No description provided.

…otebook, but left in as an alias with a deprecation warning
@palewire palewire mentioned this pull request Nov 10, 2021
nbclient/cli.py Outdated Show resolved Hide resolved
@fperez
Copy link
Member

fperez commented Nov 10, 2021

Tiny fix suggested to print stmt, otherwise I'd go for it. Thanks so much @palewire!

@fperez
Copy link
Member

fperez commented Nov 10, 2021

Great, thx! I've now made the IPython PR above to match this functionality in the IPython docs.

@davidbrochart davidbrochart merged commit 635cd62 into jupyter:master Nov 12, 2021
@davidbrochart
Copy link
Member

Thanks @palewire !

@bnavigator
Copy link

This conflicts with the already existing jupyter-run from jupyter_client:

https://github.com/jupyter/jupyter_client/blob/f453b51eeeff9e905c583b7da3905c0e35cfbdf0/setup.py#L65

@palewire
Copy link
Contributor Author

Thanks for flagging that. I'd defer here to @fperez and @davidbrochart

@davidbrochart
Copy link
Member

I didn't know this command. Looks like it executes some code from a file in a kernel, e.g.:

jupyter run foo.py -f kernel-foo.json

I don't know if it's still used, @minrk ?

@fperez
Copy link
Member

fperez commented Nov 15, 2021

Mmh, I wasn't aware of it either, though at this point if it's been in real-world use for a while, we don't want to break that!

Question is whether the functionalities could be subsumed together, I suspect they are similar enough that they could fall under a single umbrella.

@minrk
Copy link
Member

minrk commented Nov 15, 2021

I'm fine with deprecating the one in juptyer_client to be an alias to nbclient if available. I don't know if anyone uses it.

@davidbrochart
Copy link
Member

Do you mean that we would use nbclient if the file is a notebook, and jupyter_client otherwise?

@minrk
Copy link
Member

minrk commented Nov 15, 2021

Ah, I misread. I thought it did the same thing, but was moving to a new package. jupyter-run was added in jupyter/jupyter_client#184 I'm not sure how much adoption it gets, but there are going to be tricky issues with the fact that the name is already taken, unless this package adds a dependency on a version of jupyter-client that fully removes it.

The least disruptive would be to use an available name, e.g. jupyter nbrun and leave run alone, since it's taken, even if not widely used. A perhaps more desirable final situation would be for this to take over jupyter run and have run-script or run-cell to do the current jupyter-run behavior, which is more analogous to running a single cell with a kernel. I don't think that can be done without breaking any existing calls to jupyter run out there, because for jupyter_client to install a deprecated jupyter-run necessarily conflicts with nbclient providing jupyter-run. So it has to be a hard-break upgrade path for any env which has jupyter-client but not nbclient.

I don't have particularly strong feelings, but usually lean toward not breaking things, even if I don't use them.

@davidbrochart
Copy link
Member

Question is whether the functionalities could be subsumed together, I suspect they are similar enough that they could fall under a single umbrella.

There is a problem with having two packages with the same console-script entry point (see #179), so I think we should just remove jupyter-run in nbclient and keep jupyter-execute.

@davidbrochart
Copy link
Member

Thinking about it, as @minrk mentioned, jupyter-nbrun would be well aligned with jupyter-nbconvert.

@palewire
Copy link
Contributor Author

palewire commented Nov 18, 2021

Whatever the solution, I would argue for avoiding prefixes like nb for these public facing verbs. They aren't a crime or anything, but they do introduce jargon that can be off-putting to noobs.

My take is that a dis-ambiguation strategy is the best long term solution. Even if we stick with execute, we are left with two similar verbs, execute and run, that do two slightly different things. That could be a new source of confusion.

For that reason, I am sympathetic to an approach that either merges the two commands, or changes the existing one to a more specific namespace like run-cell.

@davidbrochart
Copy link
Member

I see merging the two commands as difficult because jupyter-run would still need to be in both jupyter_client and nbclient, and that is already a problem for some package managers (they check for conflicting commands).

@palewire
Copy link
Contributor Author

Gotcha. How does the scope of these two commands compare to the IPython run command from which we are drawing inspiration? I wonder if the overlap there might help us devise harmonious language.

@davidbrochart
Copy link
Member

IPython's %run can run a .py file or a .ipynb file, so it basically does jupyter_client's jupyter-run or nbclient's jupyter-run depending on the file extension (or some more clever thing).

@bnavigator
Copy link

jupyter-run would still need to be in both jupyter_client and nbclient, and that is already a problem for some package managers (they check for conflicting commands).

This is how I noticed and is the reason why we currently don't ship nbclient's jupyter-run in openSUSE.

@palewire
Copy link
Contributor Author

palewire commented Nov 18, 2021

Hmm. So how could we harmonize, with the generic Jupyter package end user in mind? Write a unified command and limit it to one of these two packages? Create a new simple package just for the cli?

@davidbrochart
Copy link
Member

Write a unified command and limit it to one of these two packages?

jupyter_client can be installed without nbclient, so it must have the command, but this means nbclient cannot have it.

Creat a new simple package just for the cli?

And remove it from jupyter_client and nbclient? Since this is a breaking change, we could as well replace jupyter_client's jupyter run with jupyter run-cell.

@palewire
Copy link
Contributor Author

palewire commented Nov 18, 2021

Gotcha. So just to think it through, here are those two routes to a unified command that mirrors the IPython %run functionality fleshed out. I'm not advocating one or the other, just talking it out. I suspect you are 10 steps ahead of me here, so thank you in advance for your patience.

Route 1: Pick a package

  • Remove the cli from nbclient
  • Add nbclient as a dependency of jupyter_client
  • Expand the existing run command in jupyter_client to import nbclient, determine what type of file the CLI user is attempting to execute (.py or .ipynb), and then run the correct execution pattern.
  • Rerelease both packages
  • nbclient no longer has a CLI, but the end user of jupyter doesn't see a difference

Route 2: Spin off a CLI package

  • Pull out the run bits from both nbclient and jupyter_client
  • Create a new package
  • Write a CLI there that supports both .py and .ipynb execution
  • Set that as a dependency of both nbclient and jupyter_client to maintain backwards compatibility
  • Rerelease both packages

I pencil that out right? You like one or the other? Hate them both?

@bnavigator
Copy link

bnavigator commented Nov 18, 2021

Route 1 can be simplified, as nbclient already depends on jupyter_client:

Route 1b: Pick a package

  • Remove the cli from nbclient
  • Add nbclient as a dependency of jupyter_client
  • Expand the existing run command in jupyter_client to import nbclient, determine what type of file the CLI user is attempting to execute (.py or .ipynb), and then run the correct execution pattern.
  • if the user is attempting to execute a notebook, try to import nbclient and verbosely fail if it is not present.
  • Rerelease both packages
  • nbclient no longer has a CLI, but the end user of jupyter doesn't see a difference
  • nbclient can still provide the already released jupyter-execute for backwards compatibility

@davidbrochart
Copy link
Member

Route 1 makes nbclient a dependency of jupyter_client, I'm not sure we want that.
Route 2 also makes jupyter_client depend on nbclient, right?

@palewire
Copy link
Contributor Author

palewire commented Nov 18, 2021

@bnavigator, thanks for the ideas. One thing I'm not clear on in your sketch: How would jupyter_client run a notebook file without using the nbclient tool we've written here?

You are right that jupyter_client is one of the dependencies here in nbclient. One thing I could imagine is moving all of the jupyter_client CLI features here, to nbclient, and then release the all-in-one run command in this repo's setup.py.

We could then remove the console script from jupyter_client. The obvious downside is that you'd need nbclient for any run CLI at all. However, if jupyter_client is a private-ish internal package that doesn't get installed directly much, it would all be the same for an end user of pip install jupyter.

@bnavigator
Copy link

@bnavigator, thanks for the ideas. One thing I'm not clear on in your sketch: How would jupyter_client run a notebook file without using the nbclient tool we've written here?

# in jupyter-client
if jupyter_run_was_called_with_a_notebook_argument:
    try:
       from nbclient import cli as nbclientcli
    except ImportError:
        output_some_error_message("We need nbclient for this funcitionality")
        sys.exit(ERROR_CODE)
    nbclientcli.run(*sys.argv)

You can add nbclient to an extras_require[nbclient] or similar.

You are right that jupyter_client is one of the dependencies here in nbclient. One thing I could imagine is moving all of the jupyter_client CLI features here, to nbclient, and then release the all-in-one run command in this repo's setup.py.

That's the worst case. You add a dependency that was not there before.

We could then remove the console script from jupyter_client. The obvious downside is that you'd need nbclient for any run CLI at all.

Exactly.

However, if jupyter_client is a private-ish internal package that doesn't get installed directly much, it would all be the same for an end user of pip install jupyter.

As far as I understand jupyter_client is supposed to be an abstraction away from being just for ipython notebooks. Adding nbclient as dependency contradicts that.

@palewire
Copy link
Contributor Author

palewire commented Nov 18, 2021

I definitely better understand the challenges of merging the two commands. Thanks!

I guess I'm looking for some higher level package strategy to help me understand things. I worry that these noob questions might irritate maintainers who are miles and miles ahead of me in the thinking here, but I come back to: Why are nbclient and jupyter_client separate packages? Maybe there's something we can find there that would help us sort this out, or come up with a naming convention that everyone felt would be durable.

@davidbrochart
Copy link
Member

Why are nbclient and jupyter_client separate packages?

jupyter_client doesn't know about notebooks.

@davidbrochart
Copy link
Member

I removed jupyter run in #180, please use jupyter execute instead. Version 0.5.9 has been released with this change.
Versions 0.5.6 and 0.5.8 have been yanked in PyPI, and version 0.5.8 has been marked as broken in conda-forge.

@palewire
Copy link
Contributor Author

palewire commented Nov 19, 2021

Thanks for patching the bug. Mostly for myself, here's an attempt to redraft "Route 1" with @bnavigator's input.

  1. Add a switch in the initialize function in jupyter_client's RunApp class that evaluates the filenames provided by the user to identify if any of the file names end in ".ipynb".
  2. If any notebook files are present, the initialize method would attempt to import nbclient, as sketched by @bnavigator above:
if jupyter_run_was_called_with_a_notebook_argument:
    try:
       from nbclient import cli as nbclientcli
    except ImportError:
        output_some_error_message("We need nbclient for this funcitionality")
        sys.exit(ERROR_CODE)
  1. The notebook paths would then be executed using logic similar to what we have released as an execute command here. That could be done via a call to the CLI code stored here, or by porting the existing code over to jupyter_client. If we store it here, we'd need to decide if we're going to continue release it as a command via this setup.py, or simply "hide" it here for usage in jupyter_client.
  2. Smooth out an inconsistencies between the args and kwargs to the two CLIs
  3. Add nbclient as an "extras" require to the jupyter_client setup.py
  4. Figure out how we want to handle the documentation. Where does it go, etc?

I get that right? If we can all agree on a path, I'd be happy to do the work of slapping it in.

@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

@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/3

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.

None yet

6 participants