Implement delayed evaluation for the cell executions happening before a kernel is set up #1002

jasongrout · 2016-01-26T23:28:31Z

This is an attempt at taking care of #994. We implement two queues for cell executions. One is at the notebook level, to queue up executions when a kernel does not exist. Another is at the kernel level, to queue up messages when the kernel is not connected.

Edit: We scrapped the notebook-level queue, and now just have a kernel-level message queue for when the kernel isn't connected

jasongrout · 2016-02-02T18:18:12Z

The test error is a known/fixed error involving jpg images, which I don't think is related to this PR.

minrk · 2016-02-02T18:37:50Z

Poked tests now that IPython rc2 has fixed the test failures. I'm fine with this implementation. Did you want to tackle the two tasks you assigned yourself (test, better notification) in this PR?

jasongrout · 2016-02-02T20:37:15Z

Yes. When are we hoping to release a new version? I'd really like it in the next version.

minrk · 2016-02-02T20:43:20Z

The installation work going on in #879 is going to take some time, so I think it's a pretty safe bet that this can make it in.

jasongrout · 2016-02-02T21:40:01Z

Perhaps this can be in a quicker bugfix release, presuming I finish it quickly? I've seen it cause people confusion and problems.

minrk · 2016-02-04T21:14:43Z

4.2 seems like the right target. It should be pretty soon.

jasongrout · 2016-02-16T16:54:13Z

After thinking about this more, I think the HoldingKernel approach/kludge should be scrapped and instead we should implement proper support for the notebook to queue cell evaluations until a kernel is available.

jasongrout · 2016-03-15T19:23:52Z

Okay, new plan: implement buffering for kernel messages at the kernel level (eliminating the need to check to see if the kernel is connected), and then also implement cell execution buffering for when the kernel is not set in the notebook (if needed...)

jasongrout · 2016-03-16T21:53:48Z

@minrk, @SylvainCorlay, @Carreau - this is ready for review.

…ected.

blink1073 · 2016-03-16T22:09:23Z

notebook/static/notebook/js/notebook.js

+    Notebook.prototype.execute = function (cell) {
+        var executed = cell.execute();
+        if (!executed) {
+            this.pending_execution.push(cell)


Do we want to add the same cell more than once?

As in - if people execute the cell twice, it's added twice?

We could make the case that since the text is not retrieved until the cell is actually executed, we should just move the cell to the end when it is executed and already is pending. Does that sound better? (i.e., you can't execute, change text, then execute again in the current model if the kernel isn't set, but you can if the kernel is set and just not connected.)

I like that approach

jasongrout · 2016-03-16T23:55:56Z

@blink1073 - I can see the case for the 'move-to-the-end' behavior, and I can also see the case for it not being there. In the commit message, I give a case where the move-to-the-end behavior is surprising:

"This may still give surprising results, for example, if cell 1 defines a variable and cell 2 increments and prints the variable.
If the execution order is cell 1, cell 2, cell 1, then before this commit cell 2 would print fine, whereas now cell 2 will throw an error."

What do you think?

blink1073 · 2016-03-17T00:58:30Z

I think the right thing to do might be to clear the queue after itself if you rerun the same cell, and clear the prompt on remaining cells, so it is clear which cells are actually pending.
Another option is to update the queue in place, assuming no destructive chages have been made to the cell.

minrk · 2016-03-17T08:05:46Z

The kernel queue seems great. I'm less fond of the notebook queue, and adding a Notebook.execute(cell) method. Since the Kernel can now queue messages prior to the websocket connection being established, is it possible to get cell.kernel defined more promptly so the notebook queue doesn't need to exist?

jasongrout · 2016-03-17T14:48:09Z

I'm also less fond of the notebook queue as well - that's where the tricky logic is. The problem, as you point out, is that the cell.kernel can be null, and I'm not sure when exactly it is going to be null. The basic usecase I'm trying to address is starting up a notebook and immediately executing cells before a kernel is set up. I'll look into it more to see if it's clear when the cell.kernel is set, and if it can be set earlier.

Edit: I'm also tempted to go back to having a HoldingKernel as a good-enough solution, but I'd rather not go back to that.

jasongrout · 2016-03-17T15:29:00Z

but yeah, a long-running cell that was, perhaps, displaying continuously running output, would be a case to handle. We could support the user pressing the interrupt button to interrupt such a computation.

blink1073 · 2016-03-17T15:31:50Z

I would think pressing interrupt should clear the read-only and In [*] state of a cell (probably setting it to In [ ]).

jasongrout · 2016-03-17T15:38:20Z

@blink1073 - I agree. That said, making a cell read-only while executing is a pretty drastic change (possibly for the better, though).

The kinds of subtleties that we are discussing with the notebook cell queue probably don't come up all that often (i.e., executing cells, then re-executing some of them, possibly while editing them, all without a kernel/session started in the notebook).

To be consistent with how execution works when the kernel is running, we should do (3) above. Since it's difficult to guess when the notebook will have a kernel set vs. not set, it sounds like (3) above would be the least surprising alternative.

blink1073 · 2016-03-17T15:44:14Z

I can agree with (3), but we should discuss the read-only approach for JupyterLab.

jasongrout · 2016-03-17T15:46:23Z

@blink1073 - agreed.

minrk · 2016-03-17T17:13:27Z

(3) is only tricky for the case that the Kernel object hasn't yet been instantiated since the kernel queue ought to handle it without telling it anything about cells. A slow-starting kernel does not affect when the Kernel is instantiated, it only affects when the kernel_ready event fires and true execution can begin. Currently, the Kernel object is instantiated as soon as the session REST API returns, which is quite prompt. If we could do any of:

instantiate the Kernel object immediately, since it's useful immediately now, or
accept the longstanding behavior for the much shorter period between load and Kernel existing, or
disable execution (clearly) for the very brief time until the kernel exists

then the easy to follow queue on the kernel would be all we need.

minrk · 2016-03-17T17:21:32Z

I should say that (3) is actually the only version that makes sense to me. Asking to execute a cell shouldn't change behavior depending on how long it takes, so (2) doesn't seem right for me. And I would expect it to be hard to avoid annoying flashes when toggling read-only during execution from whatever we use for a visual indication that it has become read-only, even if for just a fraction of a second.

jasongrout · 2016-03-17T17:32:33Z

Okay. I'm just about finished with another take on the Holding kernel that we can set at the very start. If the rest api really does take a very short time to get back with a session/kernel object, then that's probably good enough for me. I'll check to see if it's okay.

Or - how about I slim down this PR to just have the kernel queue, and see how that works in practice. If we need to do something more for when the kernel is not set, I'll revisit this issue.

jasongrout · 2016-03-17T17:58:32Z

Currently, the Kernel object is instantiated as soon as the session REST API returns, which is quite prompt.

I don't think it's so prompt. I tried delaying kernel startup by adding the following patch to ipykernel:

diff --git a/ipykernel/__main__.py b/ipykernel/__main__.py
index d1f0cf5..adeb68a 100644
--- a/ipykernel/__main__.py
+++ b/ipykernel/__main__.py
@@ -1,3 +1,6 @@
 if __name__ == '__main__':
+    from time import sleep
+    print('sleeping')
+    sleep(5)
     from ipykernel import kernelapp as app
     app.launch_new_instance()

and the notebook now is unresponsive (even with the kernel message queue) for those 5 seconds.

jasongrout · 2016-03-17T18:14:30Z

Oh, wait, never mind. I was testing the wrong thing...

minrk · 2016-03-17T19:05:30Z

Or - how about I slim down this PR to just have the kernel queue, and see how that works in practice.

I like that idea a lot. It should be a big improvement no matter what, and then we can better see the tradeoff of the more complicated buffering on the cell or notebook as a separate change.

jasongrout · 2016-03-17T19:06:53Z

@minrk - ready for review. This seems to solve my use-case.

blink1073 · 2016-03-17T19:11:44Z

👍

minrk · 2016-03-17T19:34:47Z

Awesome, thanks @jasongrout!

Implement delayed evaluation for the cell executions happening before a kernel is set up

jasongrout · 2016-03-17T22:30:15Z

Thank you! That took a lot longer than I thought to get right.

…ns happening before a kernel is set up This is an attempt at taking care of #994. We implement two queues for cell executions. One is at the notebook level, to queue up executions when a kernel does not exist. Another is at the kernel level, to queue up messages when the kernel is not connected. *Edit: We scrapped the notebook-level queue, and now just have a kernel-level message queue for when the kernel isn't connected*

…xecutions happening before a kernel is set up This is an attempt at taking care of jupyter#994. We implement two queues for cell executions. One is at the notebook level, to queue up executions when a kernel does not exist. Another is at the kernel level, to queue up messages when the kernel is not connected. *Edit: We scrapped the notebook-level queue, and now just have a kernel-level message queue for when the kernel isn't connected*

jasongrout mentioned this pull request Jan 26, 2016

Run cells before notebook is live #994

Closed

minrk added this to the 4.2 milestone Feb 2, 2016

Carreau modified the milestones: 5.0, 4.2 Feb 7, 2016

jasongrout force-pushed the delayevaluation branch 2 times, most recently from 5944b00 to d8ff19d Compare February 15, 2016 18:39

jasongrout force-pushed the delayevaluation branch from d8ff19d to 5f56f30 Compare March 15, 2016 22:16

jasongrout changed the title ~~WIP Implement delayed evaluation for the cell executions happening before a kernel is set up~~ Implement delayed evaluation for the cell executions happening before a kernel is set up Mar 15, 2016

jasongrout force-pushed the delayevaluation branch 3 times, most recently from 17e6b4d to fc69067 Compare March 16, 2016 21:49

jasongrout modified the milestones: 4.2, 5.0 Mar 16, 2016

jasongrout added 2 commits March 16, 2016 17:55

Add tests for notebook cell execution buffering.

b38f747

Implement kernel message buffering for when the websocket is not conn…

4a05b1f

…ected.

jasongrout force-pushed the delayevaluation branch from fc69067 to e2f397a Compare March 16, 2016 21:57

blink1073 reviewed Mar 16, 2016
View reviewed changes

Add more kernel message buffer tests.

a7b122f

jasongrout force-pushed the delayevaluation branch from d0ddb98 to a7b122f Compare March 17, 2016 17:48

Fix code cell execution code in light of kernel message buffering.

60908dd

Reword console log message

ab35cf8

minrk added a commit that referenced this pull request Mar 17, 2016

Merge pull request #1002 from jasongrout/delayevaluation

3c4f6e9

Implement delayed evaluation for the cell executions happening before a kernel is set up

minrk merged commit 3c4f6e9 into jupyter:master Mar 17, 2016

blink1073 mentioned this pull request Mar 18, 2016

Add a pending message queue jupyter/services#113

Merged

takluyver mentioned this pull request Apr 22, 2016

Kernel not ready after restarting #1383

Closed

This was referenced Jan 30, 2017

Delayed evaluation of early-evaluated cells seems to cause ipywidgets "missing js issue" #2087

Closed

Widget Javascript not detected. It may not be installed or enabled properly. jupyter/help#131

Open

jasongrout mentioned this pull request Mar 18, 2017

widget javascript not installed message appears often jupyter-widgets/ipywidgets#1211

Closed

github-actions bot added the status:resolved-locked label Apr 17, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement delayed evaluation for the cell executions happening before a kernel is set up #1002

Implement delayed evaluation for the cell executions happening before a kernel is set up #1002

jasongrout commented Jan 26, 2016

jasongrout commented Feb 2, 2016

minrk commented Feb 2, 2016

jasongrout commented Feb 2, 2016

minrk commented Feb 2, 2016

jasongrout commented Feb 2, 2016

minrk commented Feb 4, 2016

jasongrout commented Feb 16, 2016

jasongrout commented Mar 15, 2016

jasongrout commented Mar 16, 2016

blink1073 Mar 16, 2016

jasongrout Mar 16, 2016

jasongrout Mar 16, 2016

blink1073 Mar 16, 2016

jasongrout commented Mar 16, 2016

blink1073 commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016

jasongrout commented Mar 17, 2016

blink1073 commented Mar 17, 2016

jasongrout commented Mar 17, 2016

blink1073 commented Mar 17, 2016

jasongrout commented Mar 17, 2016

minrk commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016

jasongrout commented Mar 17, 2016

jasongrout commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016

blink1073 commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016

Implement delayed evaluation for the cell executions happening before a kernel is set up #1002

Implement delayed evaluation for the cell executions happening before a kernel is set up #1002

Conversation

jasongrout commented Jan 26, 2016

jasongrout commented Feb 2, 2016

minrk commented Feb 2, 2016

jasongrout commented Feb 2, 2016

minrk commented Feb 2, 2016

jasongrout commented Feb 2, 2016

minrk commented Feb 4, 2016

jasongrout commented Feb 16, 2016

jasongrout commented Mar 15, 2016

jasongrout commented Mar 16, 2016

blink1073 Mar 16, 2016

Choose a reason for hiding this comment

jasongrout Mar 16, 2016

Choose a reason for hiding this comment

jasongrout Mar 16, 2016

Choose a reason for hiding this comment

blink1073 Mar 16, 2016

Choose a reason for hiding this comment

jasongrout commented Mar 16, 2016

blink1073 commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016

jasongrout commented Mar 17, 2016

blink1073 commented Mar 17, 2016

jasongrout commented Mar 17, 2016

blink1073 commented Mar 17, 2016

jasongrout commented Mar 17, 2016

minrk commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016

jasongrout commented Mar 17, 2016

jasongrout commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016

blink1073 commented Mar 17, 2016

minrk commented Mar 17, 2016

jasongrout commented Mar 17, 2016