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

Add support for multiple cancellations in Notebook stop button #99203

Closed
DonJayamanne opened this issue Jun 3, 2020 · 9 comments
Closed

Add support for multiple cancellations in Notebook stop button #99203

DonJayamanne opened this issue Jun 3, 2020 · 9 comments
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@DonJayamanne
Copy link
Contributor

When user runs a notebook/cell, the cancel button can be clicked once.
This cancels the CancellationToken passed into the NotebookKernel.executeCell.

In Jupyter, clicking interrupt (stop button) doesn't necessarily work the first time the button is clicked. hence one can click the interrupt button multiple times.

We'd like the same behavior.

I.e. clicking the cancel button should trigger a cancellation (either the cancellation token or a cancellation callback).

It is my understanding that cancellation tokens will not trigger the onCancellationRequested event more than once. Hence the suggestion for a callback or we create a new construct that supports such a notification.

Note: Today we Python extension have a dedicated interrupt button on the toolbar that can be used to interrupt/stop the kernel at any point. We were planning on using the VS Code stop icon, however it doesn't trigger callbacks when clicked more than once.

@rebornix /cc

@DonJayamanne DonJayamanne changed the title Add support for multiple cancellations Add support for multiple cancellations in Notebook stop button Jun 3, 2020
@rebornix rebornix added feature-request Request for new features or functionality notebook labels Jun 5, 2020
@rebornix rebornix added this to the June 2020 milestone Jun 5, 2020
@roblourens
Copy link
Member

Is the interrupt also "per cell" or a global interrupt?

@roblourens
Copy link
Member

This also ties into another issue where I suggested a "cancel" method instead of the token so it can live independently of the "executeCell" method

@DonJayamanne
Copy link
Contributor Author

Is the interrupt also "per cell" or a global interrupt?

In the case of Jupyter kernel, its a global interrupt.
I.e. if you click cancel on any cell or use the command notebook.cancelExecution, it will interrupt the kernel and execution of all cells will be interrupted.

@roblourens
Copy link
Member

Does it make sense to still have the cancel button on each running cell, or would it be better to not have it on the cell and only have a global cancel action in the toolbar?

@DonJayamanne
Copy link
Contributor Author

I think it makes sense to have a cancel button on each running cell.
When running a cell, the mouse is closer to the cell, hence cancelling it would be easy to have a cancel right next to where they initiated the process.

Also, in the case of Jupyter we have a single cancellation for all executing cells. However in other contexts (another implementation) its possible each cell can run in parallel. E.g. if we have a polyglot solution or something like an SQL query, each cell could be completely independent of one another. Hence in such cases I'd like the cancel per cell, as its upto extension authors to handle cancellation.

@roblourens
Copy link
Member

roblourens commented Jun 6, 2020

Sounds good to me, thanks for the feedback

@roblourens roblourens modified the milestones: June 2020, Backlog Jun 15, 2020
@rebornix rebornix removed their assignment Jun 24, 2020
@roblourens
Copy link
Member

roblourens commented Jul 14, 2020

Hey @DonJayamanne do you still feel that this is ok?

I think it makes sense to have a cancel button on each running cell.

Also I was wondering, what sort of response do you get when you send the interrupt (eg a user clicks the "cancel" button)? Or is this documented somewhere? eg does the response tell you whether the running cell has stopped executing, or how do you determine whether it was "successful"? Or in other words, whether in vscode you should take the cell out of the "running" state.

Also, in Jupyter what is the difference between running a notebook vs running a single cell? If the user clicks the "run notebook" button, do you instruct the kernel to execute cells one by one? And will cancellation work any different in that case?

I am wondering whether we need a notebook-level execution state to match the cell-level execution state, since right now our model provides total flexibility. Another solution would be to just get rid of the notebook-level cancellation button.

We could have a quick call to discuss if that would be easier for you, sorry for throwing a pile of questions at you.

@DonJayamanne
Copy link
Contributor Author

g does the response tell you whether the running cell has stopped executing, or how do you determine whether it was "successful"?

We don't get a response indicating it was interrupted.
We wait for the original cell execution to stop.

Also, in Jupyter what is the difference between running a notebook vs running a single cell? If the user clicks the "run notebook" button, do you instruct the kernel to execute cells one by one? And will cancellation work any different in that case?

We run one cell at a time. Cancellation here is still the same.
If we cancel, then we stop the entire notebook execution (i.e. current & all remaining cells stop running)

Another solution would be to just get rid of the notebook-level cancellation button.

I just treat the notebook level and cell level cancellations as a trigger to cancel current execution. So having this at a notebook level IMHO is good. I.e. I'd like this to continue to exist.

We could have a quick call to discuss if that would be easier for you, sorry for throwing a pile of questions at you.

Anything works for me. No worries, please feel free to ping me on teams.

@roblourens
Copy link
Member

roblourens commented Jul 17, 2020

Notes from our call:

  • the API changes work for your scenarios
  • The document-level cancel button does the same thing as the cell-level cancel button. When the user executes a single cell, for Jupyter the extension would put the full document in the running state to get the document-level cancel button to show.
  • When the document is in the running state, we hide the "Run All Cells" button and show the document-level cancel button
  • A Queued state for cells where the cancel button does not appear could be useful for Jupyter, but it's not necessary

@roblourens roblourens modified the milestones: Backlog, July 2020 Jul 20, 2020
joaomoreno pushed a commit that referenced this issue Jul 21, 2020
@roblourens roblourens added verification-needed Verification of issue is requested verified Verification succeeded labels Aug 4, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Sep 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants
@roblourens @rebornix @DonJayamanne and others