Output ID passed in experimental_registerHtmlRenderingHook is sometimes incorrect

[pwang347]

Does this issue occur when all extensions are disabled?: Yes (requires Jupyter/Python)

• VS Code Version: 1.80.2
• OS Version: Windows_NT x64 10.0.22621

Steps to Reproduce:

1. Clone https://github.com/pwang347/vscode-renderer-test/tree/pawang/outputIdBug
2. Build and run the extension (should be able to just press the play button, may need to npm install first)
3. Open the provided example.ipynb file in the repo
4. Open the debug tools for console logs (or check debug console)
5. Observe that for some cells, the output ID is sometimes incorrect when executing multiple times (the first time is correct, but not subsequent runs)

[pwang347]

fyi @mattbierner

[pwang347]

FYI @DonJayamanne, seems like downgrading Jupyter to v2023.4.1011241018 also fixes it (I am on VS Code 1.81.0-insider with Python v2023.15.12161007)

[DonJayamanne]

Sorry, I haven't had a chance to look at this, will look at this tomorrow

[DonJayamanne]

Looks like this is a duplicate of microsoft/vscode-jupyter#14161
This is an issue in VS Code renderers, the previous output item is not disposed.

The fault is either here or elsewhere
https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/notebook/common/model/notebookCellTextModel.ts#L282

spliceNotebookCellOutputs(splice: NotebookCellOutputsSplice): void {
    // 
}

In this method we have code to dispose the previous outputs when

• We delete output items
• Or when we end up adding new outputs that is less than previous outputs
  However we do not have any code to dispose previous outputs when they are replaced
  At least thats what I think.
  https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellOutput.ts#L585

private _renderNow(splice: NotebookCellOutputsSplice, context: CellOutputUpdateContext) {

[DonJayamanne]

The only reason it works in previous version of Jupyter is because the code in teh previous version was fractionally slower,
te new version ends up clearing previous output and replacing it with new output in the same operation.
Previously we would end up with clearing output and then sending that and then inserting the new output (which happened one after the other but with a few ms of delay, enough for the two actions to run separately).

Now things are much faster in Jupyter and both operations are happening together, but we seem to have hit an issue in existing code of VS Code.

[DonJayamanne]

I can now confirm this is a duplicate of microsoft/vscode-jupyter#14161

[pwang347]

Great, thanks for confirming!

[DonJayamanne]

Not a duplicate of microsoft/vscode-jupyter#14161

[DonJayamanne]

@pwang347 I'm closing this issue as @rebornix mentioned that this is by design
Basically if you run a cell, and the output mime type is the same then the renderer is called again with the same output item, instead of generating a whole new output item.

This explains why the post render is called again but with the same output item.

The reason it used to work in older versions of Jupyter extension is because we'd first clear the output and then render the new output in two separate steps, as a result when the new item is added we need to create a whole new outputitem object.
Now the Jupyter extension is more performant (faster) and the clear and update output operations happen to get batched up and processed in VS Code at the same time.

[pwang347]

@DonJayamanne @rebornix I see - so in general, we should not expect that the rendered item's ID is up-to-date (whereas the one in the extension side will have a new ID).

Would you have any recommendations on associating a particular output renderer to a cell? Would it be safe to cache the matching cell in the renderer assuming that on the first render both the renderer and the extension side will always have the same ID?

Thanks!

[rebornix]

Would you have any recommendations on associating a particular output renderer to a cell
Would it be safe to cache the matching cell in the renderer assuming that on the first render both the renderer and the extension side will always have the same ID

It's safe to cache the matching cell, the renderer won't get a render request from another cell on a re-render.

[pwang347]

Would you have any recommendations on associating a particular output renderer to a cell
Would it be safe to cache the matching cell in the renderer assuming that on the first render both the renderer and the extension side will always have the same ID

It's safe to cache the matching cell, the renderer won't get a render request from another cell on a re-render.

Thinking some more, caching won't work because the cache will be broken whenever the output is actually re-rendered into something else (e.g. loading a different dataset). Unless we also check the content, which seems to be very over-engineered for just checking which cell the output originated from.

Would it be safe to depend on the execution count in the output item metadata to figure out which cell the output came from at least as a temporary measure, and could we eventually have some other way to identify the cell in core? (Having it in core would also remove our dependency in #146831)

[pwang347]

@DonJayamanne @rebornix small ping on the above - thanks!

[DonJayamanne]

Would it be safe to depend on the execution count in the output item metadata to figure out which cell the output came from

yes