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

Allow renderers to communicate with the extension host #123160

Closed
DonJayamanne opened this issue May 6, 2021 · 15 comments
Closed

Allow renderers to communicate with the extension host #123160

DonJayamanne opened this issue May 6, 2021 · 15 comments
Assignees
Labels
notebook under-discussion Issue is under discussion for relevance, priority, approach

Comments

@DonJayamanne
Copy link
Contributor

Use case - custom traceback renderer that provides links in output, which when clicked would:

  • Go to relevant line/cell in notebook
  • Provide user with option to install missing dependencies
  • Extension to send message to renderer (with details of settings)
    • E.g. i had an extension that displays only the first n lines of the traceback.
    • Now extension can no longer pass this information to the renderer

I know there are other extension authors (Plotly) that are interested in looking at renderers with ability to communicate with extension host (not necessarily the Kernel).

I noticed yesterday that this was removed, and a few of my personal extensions no longer worked. Shipped a stripped version of the extsnion so we can see how I'm using some of the existing API (https://marketplace.visualstudio.com/items?itemName=donjayamanne.jupyterpowertools)

@connor4312 @rebornix /cc

@connor4312
Copy link
Member

It is very intentional that renderers cannot communicate directly with the extension host. This is how we can guarantee that renderers can work across VS Code and aznb. No developer is going to (or, today, is able to) test their renderer in environments other than VS Code. If an renderers depend on the extension host, the marketplace for aznb to consume would be difficult because there would be no way to know whether an extension would actually work in that environment.

@connor4312 connor4312 self-assigned this May 6, 2021
@connor4312 connor4312 added notebook under-discussion Issue is under discussion for relevance, priority, approach labels May 6, 2021
@rebornix
Copy link
Member

rebornix commented May 6, 2021

Go to relevant line/cell in notebook
Provide user with option to install missing dependencies

This two can be done by command links without using comm, I got a prototype working in https://github.com/rebornix/better-jupyter-errors/blob/main/src/client/render.ts

@DonJayamanne
Copy link
Contributor Author

environments other than VS Code. If an renderers depend on the extension host, the marketplace for aznb to consume would

What if someone doesn't want to build a renderer that doesn't necessarily work in the aznb space.
Right now we're limiting the functionality here.

Today in Jupyter Notebooks/Labs, renderers can communicate with the rest of the extension eco system (in classic Jupyter).

It sounds like we're telling extension authors, sorry, your extension cannot work in Azure Notebook, hence we're not going to support that model. I would have thought it should be the other way around, if extnsion authros build extensions that donn't necessarily work in AzNb, then we need to find out why its being done and if its popular then adapt that into the other areas (such as AzNb).

I would have expected renderer communication to be optional feature and renderers support graceful degradation.
E.g. extension need not support mult-root workspace, they need to suppport custom file systems, they need not suppor virtual worksapces. They can still build it, and its limited in some enviromnets.

Opeiontally Azure Notebooks could just hide extensions from VS Code marketpalce that don't support some capabilities.

@connor4312
Copy link
Member

connor4312 commented May 6, 2021

What if someone doesn't want to build a renderer that doesn't necessarily work in the aznb space....
I would have expected renderer communication to be optional feature and renderers support graceful degradation.... Opeiontally Azure Notebooks could just hide extensions from VS Code marketpalce that don't support some capabilities.

That was the purpose of #119899. Likewise the way to declare a renderer that doesn't work in aznb, is to have it depend on preloads which aznb doesn't provide. Most extension authors--especially today where its visibility is limited--will not know about aznb, so we want to ensure that the obvious path is one that will lead them to the right place in building cross-compatible renderers.

We discussed this problem quite a bit before and is what lead to the current design. Initially there were thoughts around running renderers with some subset of VS Code extension APIs. However the idea of pure webview-only renderers solved this in a better way to create a 'closed-by-default' instead of an open-by-default world.

if extnsion authros build extensions that donn't necessarily work in AzNb, then we need to find out why its being done and if its popular then adapt that into the other areas (such as AzNb).

Yes, there is no reason why we cannot expose additional API methods and calls to renderers. So far I haven't had requests for any, but the possibility is there.

However the idea of a generic messaging API that requires arbitrary extension code running in the VS Code extension host is impossible to replicate in a standardized way -- that's why the possibility of having this in a renderer alone (without controller preloads, which we now track as dependencies strings and can map against in both Aznb and VS Code) is removed.

@DonJayamanne
Copy link
Contributor Author

So far I haven't had requests for any, but the possibility is there.

Can we consider this issue as a request.

extension host is impossible to replicate in a standardized way

By standardized way do you mean in a way that works in AzNb and VS Code?

@connor4312
Copy link
Member

I talked with Kai with this in my 1:1. I think we agree that we should do something to make this scenario work. Some ideas:

  • Simple approach: expose some acquireCommand that is typed to return ((...args: unknown[]) => any) | undefined. In aznb this would return undefined and we'd expect extensions to deal with it. This would also map somewhat decently onto existing command links.
  • A mixed approach where extensions declare what commands they need in their package.json, which vscode/aznb could match against. This works very well with command links.
  • Some kind of side-loaded dependency aside from controllers. For example maybe there's an vscode.notebook.provideRendererPreload(provides: string, urlOrScriptSource: Uri | string): { postMessage(...); onDidReceiveMessage<event> } and corresponding activation event. This would 'just work' with the current approach of renderers and dependencies, since it's just a dependency that aznb doesn't fulfill. However this is bulky and doesn't solve command links.

Will consider more over the next couple days. Feedback welcome.

@DonJayamanne
Copy link
Contributor Author

vscode.notebook.provideRendererPreload(provides: string, urlOrScriptSource: Uri | string

This option sounds perfect at least for one scenario in Jupyter extension.
We need require.js available in the renderers, i can see this being injected in this manner rather than shipping require.js as part of our renderers (today we don't & don't think we should either).

bulky and doesn't solve command links.

Do we need a global capabilities exposed?
So extension authors know what's supported in the hosting environment or a way to determine what kind of hosting environment they are in (for graceful degradation).
Much like env.UIKind in VSCode.

@DonJayamanne
Copy link
Contributor Author

Found another use case, here https://raw.githubusercontent.com/notebookPowerTools/vscode-plotly-chart-editor/master/images/demo.gif
Basically I want to open a cell output outside the renderer area. currently the only way to do that is via a message (a command is not sufficient as I need to pass all of the data).
Rendering outputs in output is not the best place for editors & the like.

The screenshot shows the editor in the output, but it doesn't work well (i have'nt displayed that), basically cells overlaps outputs due to overflowing/floating elements) - known issue in Notebook renderers.

@rebornix
Copy link
Member

rebornix commented May 7, 2021

Found another use case, here https://raw.githubusercontent.com/notebookPowerTools/vscode-plotly-chart-editor/master/images/demo.gif
Basically I want to open a cell output outside the renderer area. currently the only way to do that is via a message (a command is not sufficient as I need to pass all of the data).

IMHO this doesn't necessarily need to be solved by using the comm, we can try to find the output in the notebook document and then have it opened in a separate custom editor. The challenge here is there isn't any way to know which cell an output belongs to, even with command links. For example, in order to insert a cell above the cell containing the output, I found myself have to search outputs in all cells https://github.com/notebookPowerTools/better-jupyter-errors/blob/main/src/extension/extension.ts#L33-L43 (and it's not 100% correct). If we can provide Cell Info in onDidCreateOutput(({ element, mimeType }) API, we can unblock quite a few interesting features (majorly interact between renderer and other VS Code components).

@connor4312
Copy link
Member

This is the info we provide, can certainly add more data.

@jrieken
Copy link
Member

jrieken commented May 10, 2021

I would go further and have full support for renderer IPC, the classy tuple of postMessage and onmessage. I think that's the most natural way, esp since we use this pattern already. In aznb this wouldn't be available and renderers need to signal if IPC is optional or not

@connor4312
Copy link
Member

I would go further and have full support for renderer IPC, the classy tuple of postMessage and onmessage. I think that's the most natural way, esp since we use this pattern already. In aznb this wouldn't be available and renderers need to signal if IPC is optional or not

The thing with that is that aznb would never have any compatibility at all with that model of side-effects. Using commands, certain things like opening files could work in both aznb and vscode.

@jrieken
Copy link
Member

jrieken commented May 11, 2021

I don't think that commands and IPC are mutually exclusive. Supporting certain commands makes sense but an (optional) IPC channel would be of great benefit.

@connor4312
Copy link
Member

I opened a proposal for this following discussion this morning: #123601

@github-actions github-actions bot locked and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
notebook under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

5 participants
@rebornix @jrieken @DonJayamanne @connor4312 and others