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

New action runInConsole to allow line by line execution of cell content #4330

Merged
merged 32 commits into from Jul 3, 2018

Conversation

@BoPeng
Copy link
Collaborator

@BoPeng BoPeng commented Apr 6, 2018

As discussed in #3453 and #4206, this PR adds a notebook command notebook:run-in-console and associated shortcut Ctrl Shift Enter that

  1. Opens a console panel for the notebook, create one if needed
  2. Injects the selected code, or current line to the console panel to execute. Move the cursor to the next line in the latter case.

This allows step by step execution of cell content, which is very useful for debugging purposes.

Implementation is straightforward, this PR

  1. Adds an option activate to console CommandIDs.open so that the notebook can keep its focus after sending the code to console window. Adding this option to CommandIDs.createConsoledoes not work, although activate=false option has been passed to createConsole implicitly by some other code.
  2. The command is implemented directly in notebook-extension without a corresponding function in notebook because the command ends up calling console:open and then console:inject.
@BoPeng BoPeng changed the title New action runInConsole to allow step by step execution of cell content New action runInConsole to allow line by line execution of cell content Apr 7, 2018
@jasongrout jasongrout added this to the Beta 3 milestone Apr 13, 2018
@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 10, 2018

@jasongrout A potential problem with the current implementation is that runInConsole is directly implemented in notebook-extension, with no runInConsole function in notebook. The reason behind this is because the runInConsole function ends up calling commands console:open and console:inject, and functions defined in notebook usually do not have access to commands. Please let me know if this is acceptable.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 10, 2018

Please let me know if this is acceptable.

I think that's great.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 10, 2018

OK. Then I will not try to define something like NotebookActions.runInConsole. This however means that I cannot add a "step" icon to notebook toolbar because toolbar icons trigger functions in NotebookActions (e.g. the run button) defined in packages/notebook.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 10, 2018

I'm generalizing document toolbars right now in my #4453 PR.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 10, 2018

because toolbar icons trigger functions in NotebookActions

That doesn't prevent the notebook extension from adding additional toolbar buttons. Especially with the generalized toolbars in #4453.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 10, 2018

OK, I will have a look at #4453. The problem was that runInConsole is a command and calls other two commands, but I do not see any reference to JupyterLab.commands in default-toolbar.ts. Perhaps there is some way to get hold of commands from panel? Like panel.context.session.commands (just guessing)?

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 10, 2018

More specifically, it is relatively easy to add an icon but I cannot figure out how to trigger command notebook:run-in-console when the icon is clicked here

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 14, 2018

I have assigned this to @jasongrout who can continued the review. A few comments that came up in a video call:

  • Not clear how common this command will be used (maybe just a command and no default shortcut?)
  • The command name should have the word "line" in it to make its purpose clear.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 14, 2018

@ellisonbg, I probably won't be able to review this well in the very near future, so if someone else can, please go ahead, i.e., don't block on me.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 14, 2018

Not clear how common this command will be used (maybe just a command and no default shortcut?)

I can say this shortcut is very important for my style of working (as implemented in SoS) and I can cite a few tickets in Jupyter and JupyterLab, but these are after all opinions from a small number of users. I can however argue that the shortcut is important for this particular feature because one tends to use this shortcut repeatedly to step through the content of a cell. Using a context menu can be tedious for this stepping-through action and will greatly reduce the usefulness of the command. A toolbar button would be better but less useful than a shortcut because the proposed command does not understand multi-line steps and users would need to move the mouse away from this button from time to time to select lines to execute.

The command name should have the word "line" in it to make its purpose clear.

run-line-in-console? The problem is that the command also executes selected code and advances cursor (in case of executing a line) so an accurate name can be quite long (e.g. run-current-line-or-selected-text-in-console ).

@h4gen
Copy link

@h4gen h4gen commented Jun 12, 2018

Would really love to see this feature in jupyterlab since it really would be a real improvement to get a real scientific-IDE-ish workflow. Nevertheless I would propose a run-cell-in-console, referring to my request.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jun 27, 2018

Since this is a new UI for the notebook, I can see it useful to implement this first as a third party extension. That gives users a way to try out the workflow and see if they like it, without including it in core. What do you think about this? Would this be possible or are there certain internal APIs that would need to be exposed to write this sort of extension?

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Jun 27, 2018

I feel that the console window feature is incomplete without a way to send contents to it. This window was perhaps designed initially as a "log" facility but after lots of discussions (#4424), we have all agreed that it is also, if not more, useful to use it as "scratch" areas for interactive data analysis. We have worked towards this goal by merging #4503 and I do not see why this PR should be implemented in an extension.

@jasongrout What do you think about this?

Edit: I noticed that #4453 is now merged. This should allow me to add the toolbar button.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

This window was perhaps designed initially as a "log" facility

It was actually originally designed as a counterpart to the ipython console, as a different way to interact with a kernel totally separate from a notebook or code file. Sending things to a console, and having it show other execution output, were nice things we got when we thought of how it could interact with other components in JLab. Since we are changing some of those workflows, it makes me more hesitant to introduce new workflows without testing them a bit more (for example, via an extension).

@jasongrout What do you think about this?

I also agree that experimenting with different workflows is a good idea. I think the question here is (a) should we experiment a bit more (through extensions) before committing to particular workflows, (b) should we take a step back and re-examine how the components are interacting (for example, we've talked about having an editor send text to consoles that the editor didn't create - should the same happen for notebooks?), or (c) should we go ahead and merge and iterate on this workflow? If it's just a command we are adding, I'd be okay merging the functionality into master (fyi: I haven't reviewed the implementation, though).

I think that even if we merge this PR into master (instead of making it a separate extension), I would vote against a toolbar button for this function by default in the notebook right now - we have been very careful and frugal using our toolbar space in stock jlab, and I'd like to see how the workflow plays out for many more people before using the toolbar space for it by default. For the same reason, I'd vote against adding a keyboard shortcut by default until it was clearer that this would be widely used. (People could define their own keyboard shortcut for the command, of course).

On the other hand, if this was a third party extension - by all means add a toolbar button and a keyboard shortcut, since then people are explicitly asking for this feature.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Jun 28, 2018

Sending things to a console, and having it show other execution output, were nice things we got when we thought of how it could interact with other components in JLab. Since we are changing some of those workflows, it makes me more hesitant to introduce new workflows without testing them a bit more (for example, via an extension).

Jupyter is not particularly friendly for interactive data analysis, lacking common IDE features such as stepping through the code and previewing variables. These were implemented in SoS Notebook and described in our blog post. All you need to do is go to our live server, open an example and run it to test the workflow.

I think the question here is (a) should we experiment a bit more (through extensions) before committing to particular workflows,

If you would like more people to experiement the workflow, isn't JLab beta a perfect place for it? This PR does not break or interfere with other parts of JLab so I do not see any problem with announcing it as an experimental feature and including/excluding it according to user feedbacks. I can implement the feature in the sos jupyterlab extension but I wonder how many people would test an alpha-stage extension at this point.

(b) should we take a step back and re-examine how the components are interacting (for example, we've talked about having an editor send text to consoles that the editor didn't create - should the same happen for notebooks?),

Is there a ticket for it? I would vote against it with only your description.

(c) should we go ahead and merge and iterate on this workflow? If it's just a command we are adding, I'd be okay merging the functionality into master (fyi: I haven't reviewed the implementation, though).

This is what I would strongly recommend.

I think that even if we merge this PR into master (instead of making it a separate extension), I would vote against a toolbar button for this function by default in the notebook right now

I was not fond of a toolbar button myself either because I much prefer a shortcut when stepping through the code. I have removed it.

For the same reason, I'd vote against adding a keyboard shortcut by default until it was clearer that this would be widely used. (People could define their own keyboard shortcut for the command, of course).

I did not add a menu item for this command so currently the shortcut is the only way to trigger it. Even if we add a context menu later, a shortcut is definitely the best way to use this feature because users tend to trigger the command continuously to step through the code. I have therefore kept the shortcut in the PR now but I am open to your suggestion on this.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

We talked more about this PR in our triage meeting this morning for the next beta, and brainstormed various other possibilities for having a notebook and kernel interact and ways to invoke such interactions, as well as talked about some of the consistency in keyboard shortcuts that we should have with other components in JupyterLab. We want to be pretty conservative about introducing new keyboard shortcuts because (a) there aren't that many keyboard shortcuts to go around, and (b) keyboard shortcuts pretty quickly become hard to change if we are exploring and iterating. We'd like to gain some more experience with this workflow before committing to a specific default keyboard shortcut.

For now, can you change the PR to just introduce the command, without the keyboard shortcut on by default? We can put a note in the changelog with a description of the command and instructions for adding a keyboard shortcut.

@idoDavid
Copy link

@idoDavid idoDavid commented Jul 31, 2018

@ian-r-rose @BoPeng is there any documentation regarding how to enable and use this feature? I just tried Ctrl Shift Enter and it's not sending to console

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jul 31, 2018

Hi @idoDavid, this coment gives an example for how to add a keybinding for this feature. Let me know if you need any extra information!

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Jul 31, 2018

Just to add that the definition should be added to the config file from within JupyterLab through the "Advanced Settings" menu item. A more user friendly shortcut is in the work and should be available soon.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 31, 2018

See #5014 for an addition to the changelog. I used Ctrl G for now since apparently we need to have a discussion about Ctrl Shift Enter.

@idoDavid
Copy link

@idoDavid idoDavid commented Aug 2, 2018

@ian-r-rose @BoPeng @jasongrout guys, it works superbly! The only problem I have is the need to copy paste this json every time I load the lab, as I'm working from a docker image. Do you think we will have a button to enable this feature anytime soon? btw, I think this should be enabled by default, super useful.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Aug 2, 2018

@idoDavid Thank you very much for your feedback. This is a new command so no shortcut will be assigned until it is proven to be useful (see this comment). SoS Notebook (my Jupyter extension to support multiple kernels in one notebook) was the first to implement this command and used shortcut C-S-Enter for it, but this particular shortcut was recently assigned to 'run-all'. There are some discussions on the shortcut assignment for run-all and run-in-console and you are welcome to comment in that thread.

@idoDavid
Copy link

@idoDavid idoDavid commented Aug 2, 2018

@BoPeng i'll take a look at the discussion later, but I can say that the Ctrl G shortcut works great for me and is shorter that the C-S-Enter

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 2, 2018

btw, I think this should be enabled by default, super useful.

Thank you very much for letting us know - that's exactly the sort of feedback we need.

@idoDavid
Copy link

@idoDavid idoDavid commented Nov 21, 2018

@BoPeng @jasongrout guys, I simply love this feature! I'm using jupyterlab via docker (to easily share dev env. with my colleagues) So I need to input the json enabling this feature every time I'm running the docker. Is there any way to set this shortcut via command line? i'll appreciate any other solution as well

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Nov 21, 2018

I do not know, perhaps you can try to change the shortcut configuration file with sed, or put the shortcuts in another file and start JupyterLab using jupyter lab --config=/path/to/config.

The current consensus is that JupyterLab should provide default shortcuts conservatively and allow users to add shortcuts using the new shortcut dialog. I however still prefer a default shortcut for this particular action because a shortcut is the only sensible way to use this feature.

@ellisonbg will you reconsider your objection on a default shortcut with @idoDavid 's input?

@idoDavid
Copy link

@idoDavid idoDavid commented Dec 18, 2018

I do not know, perhaps you can try to change the shortcut configuration file with sed, or put the shortcuts in another file and start JupyterLab using jupyter lab --config=/path/to/config.

The current consensus is that JupyterLab should provide default shortcuts conservatively and allow users to add shortcuts using the new shortcut dialog. I however still prefer a default shortcut for this particular action because a shortcut is the only sensible way to use this feature.

@ellisonbg will you reconsider your objection on a default shortcut with @idoDavid 's input?

@BoPeng I've tried launching jupyterlab with the --config flag, it's not working. Something similar was asked here as well. Do you have any proven method that allows for our run-in-console command to work? That would be great!

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Dec 19, 2018

@idoDavid . I am afraid that I do not know a good way to do this. Although a default shortcut was removed from #5151, I think you can try to add a default shortcut again by creating a PR with Ctrl Shift Enter added to this line of code. With good reasons from another user, maybe the proposal will be re-considered.

@JianghuiDu
Copy link

@JianghuiDu JianghuiDu commented Mar 14, 2019

This is a really useful feature. I edited the key shortcuts and it is working for me. But this line by line execution does not recognize code with line breaks (like a multi-line function). I have to manually select all the lines together. Is there a development on this?

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Mar 14, 2019

The problem here is that Jupyter does not know the language of the cell enough to identify multi-line statements. For example, it it perfectly ok for Python to be

a = [
1,
2]

or

if True:
    a = [1,
2]

so right now the only way to execute multi-line statements is for users to select it first.

@JianghuiDu
Copy link

@JianghuiDu JianghuiDu commented Mar 14, 2019

Understand. I'm used to this feature in Rstudio and it's really convenient. Hopefully Jupyter can have it in the future.

@atulkakrana
Copy link

@atulkakrana atulkakrana commented Jun 7, 2019

Yeah - we really need this feature in Jupyter.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 7, 2019

Jupyter can ask the kernel if a given block of text is a complete statement. Perhaps we just keep adding lines until the answer is yes? I imagine we'd need some indication of what would be executed before execution, though.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Jun 7, 2019

The following works correctly in RStudio:

a <- c(1, 2) 
b <- c(1,
       2)
c <- c(1,
2,

3)

It sends two lines and generates error for

b <- c(1
       2)

and

b <- c(1
2)

It sends the first line (but not the 2 line) of

b <- c(1,
       2,

and wait for user input in the console, which is something we do not currently support.

So the logic seems to be

  1. If the current line is "complete", submit and run.
  2. If the current line is not complete, read in more lines until a complete statement is found. submit and run.
  3. If no complete statement can be found, submit but not run.

Edit:

For case 3, it seems that the first line is sent to the console for evaluation, but the console window just asks for more because the statement is incomplete. Jupyter currently sents incomplete statements to the kernel and gets an error as output.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Jun 9, 2019

@JianghuiDu @atulkakrana I just submitted a PR (#6515) to select multi-line statements when stepping through code.

@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants