[WIP] Initial support for the interrupt button #460

brendan0powers · 2022-01-28T00:14:13Z

References

#459

Code changes

Implemented interrupt route handler
Added interrupt API to the kernel interface
Added interrupt support to the pyolite kernel, using the method described here: https://pyodide.org/en/stable/usage/keyboard-interrupts.html

User-facing changes

No user visible changes. The only effect should be the interrupt button working as epxected.

Backwards-incompatible changes

None

Remaining Issues

The current implementation fails to interrupt the kernel if it is awaiting a promise. More research is needed to figure out what's going on.
The current implementation only works in recent Chrome, Firefox, or Safari browsers, and requires the correct HTTP headers be set. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#security_requirements See Can't Interrupt Cell Execution in Python Kernel #459 for a more detailed discussion.
When interrupt is not available the interrupt button does nothing, and there is no feedback to the user. Should we display an error dialog, or gray the button out?
What do we do about the Javascript kernel?

github-actions · 2022-01-28T00:14:27Z

👈 Try it on ReadTheDocs

jtpio · 2022-01-28T12:38:32Z

Thanks @brendan0powers for starting this!

bollwyvl · 2022-01-30T16:41:22Z

packages/pyolite-kernel/src/kernel.ts

+declare global {
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  interface Window {
+    crossOriginIsolated: boolean | undefined;


can we find a more canonical source for this type? or at least document where this magic variable comes from. At the very least, we'd want to move it out of the implementation, and probably in a dedicated .d.ts somewhere...

I'll move this to the declarations.d.ts for now. Types for crossOriginIsolated were added in typescript 4.4. microsoft/TypeScript-DOM-lib-generator#1029

I'm happy to find another way to do this as well, if you have any suggestions.

Looks like upstream is on:

"typescript": "~4.5.2",

This would probably be separate PR, and we should have a look at what other goodies we can get any new from static analysis tooling.

bollwyvl · 2022-01-30T16:46:23Z

packages/pyolite-kernel/src/kernel.ts

+    if (!window.crossOriginIsolated) {
+      console.warn(
+        'JupyterLite is not running in a cross origin isolated context. The interrupt button will not function. Details: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#security_requirements'
+      );


I'd probably use a tick-delimited string and break it up over multiple lines) so it can pass the linter... and it (along with many other constants) should probably be made internationalizable (but I wouldn't worry about it on this PR).

One approach might be to link to our some of our own, perhaps-not-yet-written documentation for this, as we can put it in a little more context.

Again, the average user is not going to open up the console to see this, so we may want to put this someplace else as well, e.g. the in-app kernel-focused log console... and potentially open it here.

Hm... it's still rather a deep dive to figure out how this might be effectively set. Some ideas:

add something to the docs (maybe in deploying.md) about how one might set the appropriate headers

update serve.py to set the appropriate headers so we can evaluate something happening on e.g. binder

probably put it behind a flag, e.g. --enable-shared-buffer

if this won't work over HTTP (even/especially on localhost), we can figure out some way to make it https with e.g. trustme

I'll add some documentation to deployment.md, and look into adding a flag as you suggest. Logging to the log console seems like a good idea. Should the log be printed on kernel startup, or when the stop button is pressed?

bollwyvl · 2022-01-30T16:50:19Z

packages/pyolite-kernel/src/worker.ts

@@ -412,6 +419,10 @@ self.onmessage = async (event: MessageEvent): Promise<void> => {
      results = commClose(messageContent);
      break;

+    case 'set-interrupt-buffer':


hm... i don't know if the intent of this API was to closely map to the kernel messaging spec. Ideally, there would be some way to model this on top of jupyter primitives instead of adding a new message type.

This is pretty low level, and i'm not sure it's well suited for the comms API. Especially since I'm not sure what would happen if you passed a SharedArrayBuffer to python and back. I can certainly test that though, if you think we should.

Right, there are some limits to what will work (and across what boundaries, see #463) and not everything is going to map perfectly, but as the first "custom" message on this pipe, it's worth taking a look. If we could use these buffers directly, that might be great for e.g. #195.

Perhaps we need to further differentiate the types of messages... it would be lovely for us to be able to: first distinguish between:

"well-known" kernel message types from JupyterLab

... and be able to use more typescript types from upstream, and maybe (ab)use totality checks in the switch

"custom" jupyterlite ones

there might be a class of ones that all lite kernels might need, e.g. what is the root of the lite site

per-kernel ones...

I'm not sure where to go from here. Do you have any concrete suggestions on an approach?

brendan0powers · 2022-01-31T23:52:48Z

@bollwyvl Do you have any thoughts on how we could make the CORS errors more visible to the user? My preferred approach would be to gray out the stop button, remove it entirely, or show a dialog. Putting the error message in the kernel log would be better than the browser console, but I'm still not sure users would notice it.

Also, I'm not sure how much freedom we have to alter the Jupyter UI.

bollwyvl · 2022-02-01T12:40:44Z

CORS errors more visible to the user

yeah, there's a whole class of issues in these deployment scenarios that are... hard to describe in user-relatable terms. As you say, the ideal is removing things that don't work...

gray out the stop button

...It may be possible... some command control surfaces (e.g. command palette, menu items) support an isEnabled callback... but i don't know about buttons. Also, a big chunk of stuff around buttons is landing upstream in JupyterLab 3.3... and the perhaps might still be time to sneak in some more features.

how much freedom we have

ha, we basically have infinite freedom: it's just a question of how long we want to maintain something that's divergent from upstream.

brendan0powers · 2022-02-07T21:42:02Z

@bollwyvl I'm waiting on pyodide/pyodide#2142, but in the meantime, I'd like to push this forward. Do you have an idea of what you'd like to see in order for this PR to be merged?

bollwyvl · 2022-02-20T19:12:12Z

packages/pyolite-kernel/src/declarations.d.ts

@@ -14,3 +14,12 @@ declare let _pipliteUrls: string[];
 declare let _disablePyPIFallback: boolean;
 declare let pyodide: any;
 declare let loadPyodide: any;
+
+// eslint-disable-next-line @typescript-eslint/naming-convention
+interface WindowOrWorkerGlobalScope {


We synced the typescript version with the upstream, so you should be able to remove the extra typing.

bollwyvl · 2022-03-31T13:47:17Z

Perhaps this will help: https://github.com/pyodide/pyodide/pull/2332/files

…t update

bollwyvl · 2022-04-09T20:01:27Z

If throwing an "info" is below the status bar threshold, maybe that at start up and then a warning? You may also want to see if anything is different with the pyodide 0.20 pr... I haven't been tracking what actually got merged before/after that alpha.

brendan0powers · 2022-04-23T14:03:31Z

Perhaps this will help: https://github.com/pyodide/pyodide/pull/2332/files

It's great to see these patches making their way upstream, unfortunately, it doesn't change the way interrupts work.

…few corner cases.

brendan0powers · 2022-04-23T18:33:52Z

After fiddling with a few different ways of displaying an error, I've settled on simply printing to the notebook when the interrupt button is clicked.

This has the advantage of notifying the user that something is wrong immediately upon clicking the button. The user may not be in a position to resolve the issue, but they will at least have a useful error message to forward to the maintainer of site.

While implementing this, a few things came up:

Clicking the interrupt button while the kernel is starting up will cause kernel startup to fail. This silently breaks the session, and the user cannot execute code. I've worked around this by adding a new _kernelStarted promise, as well as a new type of event to go with it. This adds a second new message type, which is less than ideal.
I had to modify the message for setting the interrupt buffer so that it did not return a reply from the worker, otherwise it would cause the first cell to finish executing early in some cases.

Like you mentioned earlier, it seems like we might need a more elegant way to handle the "out of band" messages. I don't think using comms for this would be appropriate, since some of these actions need to happen before the kernel is fully ready.

Oh, also I tested setting the headers on localhost, and it works. I must have done something incorrectly with my previous tests. This means you don't need special Chrome flags or SSL to test, just the headers. I wonder if it would make sense to set the by default when using the jupyterlite CLI tool?

bollwyvl · 2022-06-22T13:27:10Z

There is now a service worker available as of #655, which might enable more of this.

brendan0powers mentioned this pull request Jan 28, 2022

Can't Interrupt Cell Execution in Python Kernel #459

Open

jtpio added the enhancement New feature or request label Jan 28, 2022

bollwyvl reviewed Jan 30, 2022

View reviewed changes

bollwyvl mentioned this pull request Feb 13, 2022

Bump TypeScript to 4.5.x #499

Merged

3 tasks

bollwyvl reviewed Feb 20, 2022

View reviewed changes

bollwyvl mentioned this pull request Mar 17, 2022

Update development server options #563

Merged

11 tasks

brendan0powers added 2 commits April 9, 2022 15:32

Initial support for the interrupt button

0002924

Address some PR feedback

d142894

brendan0powers force-pushed the interrupt-kernel branch from ad442f9 to 58ede67 Compare April 9, 2022 19:46

Remove type defenitions for WindowOrWorkerGlobalScope after typescrip…

232f408

…t update

brendan0powers force-pushed the interrupt-kernel branch from 58ede67 to 232f408 Compare April 9, 2022 19:53

Print error message in the notebook if headers are not set. Handle a …

62ebf1d

…few corner cases.

bollwyvl mentioned this pull request May 31, 2022

Implement a custom Emscripten File System which communicates with the JupyterLab Content Manager, giving file access to pyolite #655

Merged

jtpio mentioned this pull request Jan 19, 2024

Make the service worker an opt-in, not an opt-out #1294

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Initial support for the interrupt button #460

[WIP] Initial support for the interrupt button #460

brendan0powers commented Jan 28, 2022

github-actions bot commented Jan 28, 2022

jtpio commented Jan 28, 2022

bollwyvl Jan 30, 2022

brendan0powers Jan 31, 2022

bollwyvl Feb 2, 2022

bollwyvl Jan 30, 2022

bollwyvl Feb 20, 2022

brendan0powers Apr 9, 2022

bollwyvl Jan 30, 2022

brendan0powers Jan 31, 2022

bollwyvl Feb 2, 2022

brendan0powers Feb 7, 2022

brendan0powers commented Jan 31, 2022

bollwyvl commented Feb 1, 2022

brendan0powers commented Feb 7, 2022

bollwyvl Feb 20, 2022

bollwyvl commented Mar 31, 2022

bollwyvl commented Apr 9, 2022 via email

brendan0powers commented Apr 23, 2022

brendan0powers commented Apr 23, 2022

bollwyvl commented Jun 22, 2022

[WIP] Initial support for the interrupt button #460

Are you sure you want to change the base?

[WIP] Initial support for the interrupt button #460

Conversation

brendan0powers commented Jan 28, 2022

References

Code changes

User-facing changes

Backwards-incompatible changes

Remaining Issues

github-actions bot commented Jan 28, 2022

jtpio commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendan0powers commented Jan 31, 2022

bollwyvl commented Feb 1, 2022

brendan0powers commented Feb 7, 2022

Choose a reason for hiding this comment

bollwyvl commented Mar 31, 2022

bollwyvl commented Apr 9, 2022 via email

brendan0powers commented Apr 23, 2022

brendan0powers commented Apr 23, 2022

bollwyvl commented Jun 22, 2022