Add token ICellExecutor providing an entry to customize cell execution

[fcollonval]

References

Follow-up of the discussion jupyterlab/frontends-team-compass#229 (comment)
This will allow experimentation like #15448 to happen outside of core. It will also open the box for handling non-standard cell type execution for example (e.g. SQL cell).

Code changes

Provide the Private.runCell generic function as a plugin to allow customization.

User-facing changes

None

Backwards-incompatible changes

None

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[fcollonval]

Failure are not related.

cc @echarles for awareness

[krassowski]

What about console cells? These get executed as well and 90% of the code is shared with notebook. Since this is named ICellExecutor, should we make it handle either notebook or console (and then if in future another execution context comes along we can easily add it).

[fcollonval]

I thought about it. But the two are quite different (presumably because they are separated). So should I try to reconciliate them or should the cell executor has two methods: runNotebookCell and runConsoleCell?

For reference, the console code is there:

jupyterlab/packages/console/src/widget.ts

Line 724 in 946f18c

private _execute(cell: CodeCell): Promise<void> {

[krassowski]

So should I try to reconciliate them or should the cell executor has two methods: runNotebookCell and runConsoleCell?

I am not sure which would be better; I brought it up on the lab call yesterday but there were no strong opinions.

My thoughts:

• The disadvantage of reconciliating them into one method is that implementers might need to do a few conditional logic blocks depending on the execution context
• The disadvantage of two methods is that implementers would need to provide both, or we would need to have a default implementation as a fallback if we decided to make them optional (but then an interface with all methods optional risks that the implementer will fail to provide any if they make a typo and will be confused why nothing is working).

Thinking more about it, maybe there should be two separate plugins:

• @jupyterlab/notebook-extension:cell-executor providing INotebookCellExecutor
• @jupyterlab/console-extension:cell-executor providing IConsoleCellExecutor

[krassowski]

Separate plugins sounds better in the sense that additional execution context means just a new plugin, and that there is no need to have any fallbacks/optional methods, and no conditional logic.

The downside is that adding new execution context does require implementers to add a new plugin, but realistically this is something which does not happen frequently so I think this is a good trade-off.

[fcollonval]

Having two plugins seems a good path. Thanks for bringing this PR for discussion at the call.

[krassowski]

So I think the minimum required change for this PR would be renaming ICellExecutor to INotebookCellExecutor.

[fcollonval]

Ok I pushed this one forward; 🤞 for the CI.

For the reviewer I have a final wording question, should it be I...CellExector.runCell (current version) or I...CellExecutor.run?

[krassowski]

should it be I...CellExector.runCell (current version) or I...CellExecutor.run?

I like how runCell how gives us freedom to expand this interface to include something like runAllCells in the future if needed (not saying that this is a good idea, just saying that having more specific names might be more future-proof).

[krassowski]

Kicking CI after bot updates

[krassowski]

The test failure in js-console group appears relevant:

  ● console/widget › CodeConsole › #executed › should emit a date upon execution

    expect(received).toBeInstanceOf(expected)

    Expected constructor: Date

    Received value has no prototype
    Received value: null

      104 |         await (widget.sessionContext as SessionContext).initialize();
      105 |         await widget.execute(force);
    > 106 |         expect(called).toBeInstanceOf(Date);
          |                        ^
      107 |       });
      108 |     });
      109 |

      at Object.<anonymous> (test/widget.spec.ts:106:24)

[fcollonval]

Thanks @krassowski

I fixed the code and rename the console/src/executor.ts to console/src/cellexecutor.ts to align naming.

[krassowski]

Just one suggestion on renaming IExecutionOptions or nesting them in namespaces.

[fcollonval]

Thanks @krassowski I went for the namespacing as it is a common pattern in core and will be a good fit in case new API are added to the executors.

I used IRunCellOptions instead of IOptions as the later is usually used for constructor. Moreover in case of a API extension the generic IOptions for runCell may bring confusion.

[krassowski]

Thank you @fcollonval.

I would personally prefix the namespaces with I (which is already common in the codebase) to take advantage of declaration merging, but I know it was called confusing by some new contributors so I am equally happy to go with the current version.

[fcollonval]

I would personally prefix the namespaces with I (which is already common in the codebase) to take advantage of declaration merging, but I know it was called confusing by some new contributors so I am equally happy to go with the current version.

done in 71cc3c9

[krassowski]

The visual test failure is relevant. Maybe I should mock the descriptions, but for now updating it.

bot please update documentation snapshots

[github-actions]

Documentation snapshots updated.

[fcollonval]

Kicking the CI

[krassowski]

Flaky snapshot update (a kernel from another test was captured). I feel like we need to mock the kernels out for some tests.

[fcollonval]

Thanks for the help