jupyter / notebook

Jupyter Interactive Notebook
https://jupyter-notebook.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
11.56k stars 4.84k forks source link

Run All implementation does not account for COM channel communication that may be dependencies of cells below #839

Open lbustelo opened 8 years ago

lbustelo commented 8 years ago

The code below is what used for Run All Cells.

Notebook.prototype.execute_cells = function (indices) {
        if (indices.length === 0) {
            return;
        }

        var cell;
        for (var i = 0; i < indices.length; i++) {
            cell = this.get_cell(indices[i]);
            cell.execute();
        }

        this.select(indices[indices.length - 1]);
        this.command_mode();
        this.set_dirty(true);
    };

The basic for loop ends up generating a queue of execute_requests calls. The problem that I'm seeing with this code is that it does not give the responses from cell execution the opportunity to inject additional execute_requests in the queue prior to the following cells. On the contrary, these new execute_requests are appended to the queue behind the other cells in the document.

An example of execute_requests that might need to be injected into the queue at appropriate locations would be COMM channel communication that might be needed to bootstrap a widget before the following cell might make use of it.

This problem is exposed in dashboards with declarativewidgets (this issue provides some details), but there might be other cases (i.e. ipywidgets).

A potential solution might be to rather than using a for loop, cell execution is done in a chain of promises. If all kernel.send() messages are done sync (at least queued sync), then we might have a chance perform COMM channel communication before the next cell gets a chance to execute.

Note that this COMM channel does not necessarily need to be initiated browser side. The code in the cell might actually have Python code that does some COMM chatter. Without a fix, any additional COMM sent from browser would be appended to the end of the queue and might be too late.

/cc @parente @jdfreder @SylvainCorlay

SylvainCorlay commented 8 years ago

I encountered this issue before but I am not sure whether of what is the right way do address this. Indeed, if we do what you propose, a user may also be rightfully surprised that the behavior differs when

lbustelo commented 8 years ago

I think I'm mostly concerned about seeing differences in behavior if user selects Run All vs executing the same set of cells but one by one manually very quickly.

The latter would then need to be smart to know that there is a cell in execution, and maybe others waiting, and place itself in a promise.

minrk commented 8 years ago

I view the immediate submission of requests as a very important feature. We mustn't wait for the previous execution to complete before sending the next request, so chaining futures in Run All doesn't sound good to me. It also seems like a violation that code in a subsequent cell requires frontend interaction after the result of a previous cell.

If you depend on things like this, only you can know what needs to be done before subsequent executions are allowed, so you would really need to disable or override the Run All action to behave differently.

What if Cell.execute returns a Future that resolves immediately, and your custom UI overrides that to resolve the Future at a later time? Would that give you the hook you need?

lbustelo commented 8 years ago

I view the immediate submission of requests as a very important feature. We mustn't wait for the previous execution to complete before sending the next request, so chaining futures in Run All doesn't sound good to me. It also seems like a violation that code in a subsequent cell requires frontend interaction after the result of a previous cell.

I would agree with you completely if we were just talking about notebooks where all cells are eval/response type. Meaning, once the response reaches the notebook, that cell is really done.

HTML and JS magics, the COMM channel and Widgets introduce the likelihood of that more has to be executed before a cell could be consider fully done.

Consider ipywidgets as another example. I don't have concrete proof, but I think that it currently by-passes this issue all-together because it preloads all its widgets JS code ahead of time on notebook page load. If ipywidgets did not do this, there would be no guarantees that the widgets would be available for next cell's result due to async AMD loads. Since it is a close-set of widgets, it is able to implement the work-around, although it does so by impacting load times of all notebook pages regardless if they have widgets or not (for the record, declarativewidgets does the same).

Belief me that I understand that this is not a simple issue. Changing the way cells are executed is changing the core of the notebook. There is also the question of how do you really know when a cell is really done. Even harder if the cell involves async loading of JS code.

I think your suggestion of making Cell.execute return a promise and allow overriding might be a start, but it also is punting on the issue. I personally don't have a good solution to offer either, but do think that more discussion should take place.

jdfreder commented 8 years ago

If ipywidgets did not do this, there would be no guarantees that the widgets would be available for next cell's result due to async AMD loads

Even the preloaded Javascript is reloaded asynchronously.

I agree with @minrk that the immediate submission is important. I think the rate of execution is something that should be controlled by the kernel, unimpeded by the client. If you need the client to have a say in the rate at which executions occur, why not manually halt the executions while waiting for a message from the front-end?

Alternatively, alter the client to execute differently, as Min suggests.

lbustelo commented 8 years ago

Maybe I'm misreading this code... https://github.com/ipython/ipywidgets/blob/master/ipywidgets/static/widgets/js/utils.js#L79 but it seems to me that pre-registered widgets (which all ipywidgets are) causes the utils.loadClass to resolve immediately.

If you need the client to have a say in the rate at which executions occur, why not manually halt the executions while waiting for a message from the front-end?

If you tell me how I can do that... it would solve all my problems! If you are referring to putting some code on the Python side to block until a message is received... I tried that approach once. Does not work for several reasons:

  1. The kernel is blocked busy and the shell socket is not processed
  2. Even if we could un-block it, the frontend message could still be behind other cells in the request queue.
jdfreder commented 8 years ago

Maybe I'm misreading this code...

Yes I think you are, because resolving immediately != synchronous execution. See this jdfiddle

lbustelo commented 8 years ago

So it is more of a timing problem then. Since the code is preloaded, the callback is sure going to get invoked sooner than if we had to load the files remotely. And that might be fast enough to not see the issues that I'm bringing up here.

jdfreder commented 8 years ago
  1. Even if we could un-block it, the frontend message could still be behind other cells in the request queue.

Good point, but that would only apply to messages that are subject to the execution queue, right? So if you were able to hold the kernel, by blocking, and you manually listened for comm messages (which I don't even know if this is possible right now, but if it is, it's probably ugly), the comm messages wouldn't be subject to that queue and could be received immediately, right?

lbustelo commented 8 years ago

As far as I know... both cell execution and comm.send messages go through kernel.send_shell_message. What I observed using Chrome's WebSocket inspector was my frontend comm messages all after the other cell execution.