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

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

Open
lbustelo opened this issue Dec 11, 2015 · 10 comments

Comments

@lbustelo
Copy link

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

@Carreau Carreau added this to the 5.0 milestone Dec 11, 2015
@SylvainCorlay
Copy link
Member

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

  • running two cells at once with the run-all action on the one hand
  • running the same code in a single cell on the other hand.

@lbustelo
Copy link
Author

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
Copy link
Member

minrk commented Dec 14, 2015

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

Maybe I'm misreading this code...

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

@lbustelo
Copy link
Author

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
Copy link
Contributor

  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
Copy link
Author

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.

rgbkrk added a commit to rgbkrk/roadmap that referenced this issue Apr 1, 2016
Libraries, and by extension kernels, have static assets that are required in frontends such as the notebook, thebe, hydrogen, and dashboards..

This issue continues to come up over and over without a total solution. At the very least I'd like for us to put it on the roadmap and acknowledge that we will solve it.

Initial (old) proposal/discussion: jupyter/notebook#116
Related: jupyter/notebook#839

As it applies to Thebe, they currently have to bundle

* all notebook js (not tied to kernel)
* ipywidgets js (tied to kernel)
* thebe itself

Each thebe release has to strictly specify the version of notebook, ipywidgets, in addition to thebe itself, and is incompatible with other versions.

Discussion from the Spring Jupyter Dev Meeting 2016: https://jupyter.hackpad.com/Spring-2016-Dev-Meeting-h0y1TIAWxz1#:h=Kernel-Static-Resources
@minrk minrk modified the milestone: 5.0 Jan 13, 2017
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

No branches or pull requests

5 participants