From 5161c90efde1282ef657c928035725a29025c462 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 6 Jun 2022 09:53:01 +1000 Subject: [PATCH 01/13] Fixes --- .../execution/cellExecutionMessageHandler.ts | 359 ++++++++++++++---- src/platform/common/refBool.ts | 14 - 2 files changed, 293 insertions(+), 80 deletions(-) delete mode 100644 src/platform/common/refBool.ts diff --git a/src/kernels/execution/cellExecutionMessageHandler.ts b/src/kernels/execution/cellExecutionMessageHandler.ts index a7cacf06094..3bcc4b8b12a 100644 --- a/src/kernels/execution/cellExecutionMessageHandler.ts +++ b/src/kernels/execution/cellExecutionMessageHandler.ts @@ -5,7 +5,7 @@ import * as fastDeepEqual from 'fast-deep-equal'; import type * as nbformat from '@jupyterlab/nbformat'; -import type * as KernelMessage from '@jupyterlab/services/lib/kernel/messages'; +import * as KernelMessage from '@jupyterlab/services/lib/kernel/messages'; import { NotebookCell, NotebookCellExecution, @@ -30,15 +30,20 @@ import { CellExecutionCreator } from './cellExecutionCreator'; import { IApplicationShell } from '../../platform/common/application/types'; import { disposeAllDisposables } from '../../platform/common/helpers'; import { traceError, traceWarning } from '../../platform/logging'; -import { RefBool } from '../../platform/common/refBool'; import { IDisposable, IExtensionContext } from '../../platform/common/types'; import { concatMultilineString, formatStreamText, isJupyterNotebook } from '../../platform/common/utils'; -import { traceCellMessage, cellOutputToVSCCellOutput, translateCellDisplayOutput } from './helpers'; +import { + traceCellMessage, + cellOutputToVSCCellOutput, + translateCellDisplayOutput, + CellOutputMimeTypes +} from './helpers'; import { swallowExceptions } from '../../platform/common/utils/decorators'; import { noop } from '../../platform/common/utils/misc'; import { ITracebackFormatter } from '../../kernels/types'; import { handleTensorBoardDisplayDataOutput } from './executionHelpers'; import { WIDGET_MIMETYPE } from '../../kernels/ipywidgets-message-coordination/constants'; +import isObject = require('lodash/isObject'); // Helper interface for the set_next_input execute reply payload interface ISetNextInputPayload { @@ -60,6 +65,26 @@ type DisplayData = nbformat.IDisplayData & { */ export const activeNotebookCellExecution = new WeakMap(); +function getParentHeaderMsgId(msg: KernelMessage.IMessage): string | undefined { + if (msg.parent_header && 'msg_id' in msg.parent_header) { + return msg.parent_header.msg_id; + } + return undefined; +} + +function canMimeTypeBeRenderedByWidgetManager(mime: string) { + if (mime == CellOutputMimeTypes.stderr || mime == CellOutputMimeTypes.stdout || mime == CellOutputMimeTypes.error) { + // These are plain text mimetypes that can be rendered by the Jupyter Lab widget manager. + return true; + } + if (mime.startsWith('application/vnd')) { + // Custom vendored mimetypes cannot be rendered by the widget manager, it relies on the output renderers. + return false; + } + // Everything else can be rendered by the Jupyter Lab widget manager. + return true; +} + /** * Responsible for handling of jupyter messages as a result of execution of individual cells. */ @@ -73,10 +98,12 @@ export class CellExecutionMessageHandler implements IDisposable { */ private completedExecution?: boolean; /** - * Listen to messages and update our cell execution state appropriately - * Keep track of our clear state + * Jupyter can sent a `clear_output` message which indicates the output of a cell should be cleared. + * If the flag `wait` is set to `true`, then we should wait for the next output before clearing the output. + * I.e. if the value for `wait` is false (default) then clear the cell output immediately. + * https://ipywidgets.readthedocs.io/en/latest/examples/Output%20Widget.html#Output-widgets:-leveraging-Jupyter's-display-system */ - private readonly clearState = new RefBool(false); + private clearOutputOnNextUpdateToOutput?: boolean; private execution?: NotebookCellExecution; private readonly _onErrorHandlingIOPubMessage = new EventEmitter<{ @@ -98,6 +125,25 @@ export class CellExecutionMessageHandler implements IDisposable { * Because if after the stream we have an image, then the stream is not the last output item, hence its cleared. */ private lastUsedStreamOutput?: { stream: 'stdout' | 'stderr'; text: string; output: NotebookCellOutput }; + private outputsAreSpecificToAWidget?: { + /** + * Comm Id (or model_id) of widget that will handle all messages and render them via the widget manager. + * This could be a widget in another cell. + */ + handlingCommId: string; + /** + * All messages that have a parent_header.msg_id = msg_id will be swallowed and handled by the widget with model_id = this.handlingCommId. + * These requests could be from another cell, ie messages can original from one cell and end up getting displayed in another. + * E.g. widget is in cell 1 and output will be redirected from cell 2 into widget 1. + */ + msgIdsToSwallow: string; + /** + * If true, then we should clear all of the output owned by the widget defined by the commId. + * By owned, we mean the output added after the widget widget output and not the widget itself. + */ + clearOutputOnNextUpdateToOutput?: boolean; + }; + private commIdsMappedToParentWidgetModel = new Map(); private readonly disposables: IDisposable[] = []; private readonly prompts = new Set(); /** @@ -105,11 +151,12 @@ export class CellExecutionMessageHandler implements IDisposable { * or for any subsequent requests as a result of outputs sending custom messages. */ private readonly ownedCommIds = new Set(); + private readonly outputsOwnedByWidgetModel = new Map>(); /** * List of msg_ids of requests sent either as part of request_execute * or for any subsequent requests as a result of outputs sending custom messages. */ - private readonly ownedRequestIds = new Set(); + private readonly ownedRequestMsgIds = new Set(); constructor( public readonly cell: NotebookCell, private readonly applicationService: IApplicationShell, @@ -122,7 +169,7 @@ export class CellExecutionMessageHandler implements IDisposable { cellExecution: NotebookCellExecution ) { this.executeRequestMessageId = request.msg.header.msg_id; - this.ownedRequestIds.add(request.msg.header.msg_id); + this.ownedRequestMsgIds.add(request.msg.header.msg_id); workspace.onDidChangeNotebookDocument( (e) => { if (!isJupyterNotebook(e.notebook)) { @@ -142,16 +189,16 @@ export class CellExecutionMessageHandler implements IDisposable { this.disposables ); this.execution = cellExecution; - request.onIOPub = (msg) => { + // We're in all messages. + // When using the `interact` function in Python, we can get outputs from comm messages even before execution has completed. + // See https://github.com/microsoft/vscode-jupyter/issues/9503 for more information on why we need to monitor anyMessage and iopubMessage signals. + this.kernel.anyMessage.connect(this.onKernelAnyMessage, this); + this.kernel.iopubMessage.connect(this.onKernelIOPubMessage, this); + + request.onIOPub = () => { // Cell has been deleted or the like. - if (this.cell.document.isClosed) { + if (this.cell.document.isClosed && !this.completedExecution) { request.dispose(); - return; - } - try { - this.handleIOPub(msg); - } catch (ex) { - this._onErrorHandlingIOPubMessage.fire(ex); } }; request.onReply = (msg) => { @@ -166,11 +213,6 @@ export class CellExecutionMessageHandler implements IDisposable { request.done .finally(() => { this.completedExecution = true; - // We're only interested in messages after execution has completed. - // See https://github.com/microsoft/vscode-jupyter/issues/9503 for more information. - // this.kernel.anyMessage.connect(this.onKernelAnyMessage, this); - // this.kernel.iopubMessage.connect(this.onKernelIOPubMessage, this); - this.endCellExecution(); }) .catch(noop); @@ -201,7 +243,7 @@ export class CellExecutionMessageHandler implements IDisposable { this.clearLastUsedStreamOutput(); this.execution = undefined; - if (this.ownedCommIds.size === 0 && this.completedExecution) { + if (this.cell.document.isClosed || (this.ownedCommIds.size === 0 && this.completedExecution)) { // If no comms channels were opened as a result of any outputs of this cell, // this means we don't have any widgets that can send comm message back to the kernel. // Hence no point listening to any of the iopub messages & the like, i.e. we can stop listening to everything in this class. @@ -209,6 +251,10 @@ export class CellExecutionMessageHandler implements IDisposable { } } private onKernelAnyMessage(_: unknown, { direction, msg }: Kernel.IAnyMessageArgs) { + if (this.cell.document.isClosed) { + return this.endCellExecution(); + } + // We're only interested in messages after execution has completed. // See https://github.com/microsoft/vscode-jupyter/issues/9503 for more information. if (direction !== 'send' || !this.completedExecution) { @@ -219,10 +265,13 @@ export class CellExecutionMessageHandler implements IDisposable { if (jupyterLab.KernelMessage.isCommMsgMsg(msg) && this.ownedCommIds.has(msg.content.comm_id)) { // Looks like we have a comm msg request sent by some output or the like. // See https://github.com/microsoft/vscode-jupyter/issues/9503 for more information. - this.ownedRequestIds.add(msg.header.msg_id); + this.ownedRequestMsgIds.add(msg.header.msg_id); } } private onKernelIOPubMessage(_: unknown, msg: KernelMessage.IIOPubMessage) { + if (this.cell.document.isClosed) { + return this.endCellExecution(); + } // We're only interested in messages after execution has completed. // See https://github.com/microsoft/vscode-jupyter/issues/9503 for more information. @@ -230,11 +279,9 @@ export class CellExecutionMessageHandler implements IDisposable { // comm message (requests) sent by an output widget. // See https://github.com/microsoft/vscode-jupyter/issues/9503 for more information. if ( - !this.completedExecution || !msg.parent_header || !('msg_id' in msg.parent_header) || - msg.parent_header.msg_id === this.executeRequestMessageId || - !this.ownedRequestIds.has(msg.parent_header.msg_id) || + !this.ownedRequestMsgIds.has(msg.parent_header.msg_id) || msg.channel !== 'iopub' ) { return; @@ -243,6 +290,11 @@ export class CellExecutionMessageHandler implements IDisposable { this.handleIOPub(msg); } catch (ex) { traceError(`Failed to handle iopub message as a result of some comm message`, msg, ex); + if (!this.completedExecution && !this.cell.document.isClosed) { + // If there are problems handling the execution, then bubble those to the calling code. + // Else just log the errors. + this._onErrorHandlingIOPubMessage.fire(ex); + } } } private clearLastUsedStreamOutput() { @@ -321,7 +373,7 @@ export class CellExecutionMessageHandler implements IDisposable { } else if (jupyterLab.KernelMessage.isCommOpenMsg(msg)) { // Noop. } else if (jupyterLab.KernelMessage.isCommMsgMsg(msg)) { - // Noop. + this.handleCommMsg(msg); } else if (jupyterLab.KernelMessage.isCommCloseMsg(msg)) { // Noop. } else { @@ -333,8 +385,76 @@ export class CellExecutionMessageHandler implements IDisposable { this.execution.executionOrder = msg.content.execution_count; } } + private handleCommMsg(msg: KernelMessage.ICommMsgMsg) { + const data = msg.content.data as Partial<{ + method: 'update'; + state: { msg_id: string } | { children: string[] }; + }>; + if (!isObject(data) || data.method !== 'update' || !isObject(data.state)) { + return; + } - private addToCellData(output: ExecuteResult | DisplayData | nbformat.IStream | nbformat.IError | nbformat.IOutput) { + if ('msg_id' in data.state && typeof data.state.msg_id === 'string') { + // When such a comm message is received, then + // the kernel is instructing the front end (UI widget manager) + // to handle all of the messages that would have other wise been handled as regular execution messages for msg_id. + const parentHeader = 'msg_id' in msg.parent_header ? msg.parent_header : undefined; + if ( + this.ownedRequestMsgIds.has(msg.content.comm_id) || + (parentHeader && this.ownedRequestMsgIds.has(parentHeader.msg_id)) + ) { + if (data.state.msg_id) { + // Any future messages sent from `parent_header.msg_id = msg_id` must be handled by the widget with the `mode_id = msg.content.comm_id`. + this.outputsAreSpecificToAWidget = { + handlingCommId: msg.content.comm_id, + msgIdsToSwallow: data.state.msg_id + }; + } else if (this.outputsAreSpecificToAWidget?.handlingCommId === msg.content.comm_id) { + // Handle all messages the normal way. + this.outputsAreSpecificToAWidget = undefined; + } + } + } else if ( + 'children' in data.state && + Array.isArray(data.state.children) && + this.ownedCommIds.has(msg.content.comm_id) + ) { + // This is the kernel instructing the widget manager that some outputs (comm_ids) + // are in fact children of another output (comm). + // We need to keep track of this so that we know who the common parent is. + const IPY_MODEL_PREFIX = 'IPY_MODEL_'; + data.state.children.forEach((item) => { + if (typeof item !== 'string') { + return traceWarning(`Came across a comm update message a child that isn't a string`, item); + } + if (!item.startsWith(IPY_MODEL_PREFIX)) { + return traceWarning( + `Came across a comm update message a child that start start with ${IPY_MODEL_PREFIX}`, + item + ); + } + const commId = item.substring(IPY_MODEL_PREFIX.length); + this.ownedCommIds.add(commId); + this.commIdsMappedToParentWidgetModel.set(commId, msg.content.comm_id); + }); + } + } + private clearOutputIfNecessary(execution: NotebookCellExecution | undefined): { + previousValueOfClearOutputOnNextUpdateToOutput: boolean; + } { + if (this.clearOutputOnNextUpdateToOutput) { + traceCellMessage(this.cell, 'Clear cell output'); + this.clearLastUsedStreamOutput(); + execution?.clearOutput().then(noop, noop); + this.clearOutputOnNextUpdateToOutput = false; + return { previousValueOfClearOutputOnNextUpdateToOutput: true }; + } + return { previousValueOfClearOutputOnNextUpdateToOutput: false }; + } + private addToCellData( + output: ExecuteResult | DisplayData | nbformat.IStream | nbformat.IError | nbformat.IOutput, + originalMessage: KernelMessage.IMessage + ) { if ( this.context.extensionMode === ExtensionMode.Test && output.data && @@ -358,12 +478,8 @@ export class CellExecutionMessageHandler implements IDisposable { } traceCellMessage(this.cell, 'Update output'); // Clear if necessary - if (this.clearState.value) { - this.clearLastUsedStreamOutput(); - this.execution?.clearOutput().then(noop, noop); - this.clearState.update(false); - } - // Keep track of the displa_id against the output item, we might need this to update this later. + this.clearOutputIfNecessary(this.execution); + // Keep track of the display_id against the output item, we might need this to update this later. if (displayId) { this.outputDisplayIdTracker.trackOutputByDisplayId(this.cell, displayId, cellOutput); } @@ -375,10 +491,99 @@ export class CellExecutionMessageHandler implements IDisposable { const task = this.execution || this.createTemporaryTask(); this.clearLastUsedStreamOutput(); traceCellMessage(this.cell, 'Append output in addToCellData'); - task?.appendOutput([cellOutput]).then(noop, noop); + // If the output belongs to a widget, then add the output to that specific widget (i.e. just below the widget). + let outputShouldBeAppended = true; + const parentHeaderMsgId = getParentHeaderMsgId(originalMessage); + if ( + this.outputsAreSpecificToAWidget && + this.outputsAreSpecificToAWidget?.msgIdsToSwallow === parentHeaderMsgId && + cellOutput.items.every((item) => canMimeTypeBeRenderedByWidgetManager(item.mime)) + ) { + // Plain text outputs will be displayed by the widget. + outputShouldBeAppended = false; + } else if ( + this.outputsAreSpecificToAWidget && + this.outputsAreSpecificToAWidget?.msgIdsToSwallow === parentHeaderMsgId + ) { + const result = this.updateWidgetOwnedOutput( + { commId: this.outputsAreSpecificToAWidget.handlingCommId, outputToAppend: cellOutput }, + task + ); + + if (result?.outputAdded) { + outputShouldBeAppended = false; + } + } + if (outputShouldBeAppended) { + task?.appendOutput([cellOutput]).then(noop, noop); + } this.endTemporaryTask(); } + private updateWidgetOwnedOutput( + options: { outputToAppend: NotebookCellOutput; commId: string } | { clearOutput: true }, + task?: NotebookCellExecution + ): { outputAdded: true } | undefined { + const commId = 'commId' in options ? options.commId : this.outputsAreSpecificToAWidget?.handlingCommId; + if (!commId) { + return; + } + const outputToAppend = 'outputToAppend' in options ? options.outputToAppend : undefined; + const expectedModelId = this.commIdsMappedToParentWidgetModel.get(commId) || commId; + const widgetOutput = this.cell.outputs.find((output) => { + return output.items.find((outputItem) => { + if (outputItem.mime !== WIDGET_MIMETYPE) { + return false; + } + try { + const value = JSON.parse(Buffer.from(outputItem.data).toString()) as { model_id?: string }; + return value.model_id === expectedModelId; + } catch (ex) { + traceWarning(`Failed to deserialize the widget data`, ex); + } + return false; + }); + }); + if (!widgetOutput) { + return; + } + const outputsOwnedByWidgetModel = this.outputsOwnedByWidgetModel.get(expectedModelId) || new Set(); + + // We have some new outputs, that need to be placed immediately after the widget and before any other output + // that doesn't belong to the widget. + const clearWidgetOutput = this.outputsAreSpecificToAWidget?.clearOutputOnNextUpdateToOutput === true; + if (this.outputsAreSpecificToAWidget) { + this.outputsAreSpecificToAWidget.clearOutputOnNextUpdateToOutput = false; + } + const newOutputs = this.cell.outputs.slice().filter((item) => { + if (clearWidgetOutput) { + // If we're supposed to clear the output, then clear all of the output that's + // specific to this widget. + // These are tracked further below. + return !outputsOwnedByWidgetModel.has(item.id); + } else { + return true; + } + }); + + const outputsUptoWidget = newOutputs.slice(0, newOutputs.indexOf(widgetOutput) + 1); + const outputsAfterWidget = newOutputs.slice(newOutputs.indexOf(widgetOutput) + 1); + + this.outputsOwnedByWidgetModel.set(expectedModelId, outputsOwnedByWidgetModel); + if (outputToAppend) { + // Keep track of the output added that belongs to the widget. + // Next time when we need to clear the output belonging to this widget, all we need to do is + // filter out (exclude) these outputs. + outputsOwnedByWidgetModel.add(outputToAppend.id); + } + + // Ensure the new output is added just after the widget. + const newOutput = outputToAppend + ? outputsUptoWidget.concat(outputToAppend).concat(outputsAfterWidget) + : outputsUptoWidget.concat(outputsAfterWidget); + task?.replaceOutput(newOutput).then(noop, noop); + return { outputAdded: true }; + } private async handleInputRequest(msg: KernelMessage.IStdinMessage) { // Ask the user for input if (msg.content && 'prompt' in msg.content) { @@ -405,14 +610,17 @@ export class CellExecutionMessageHandler implements IDisposable { // See this for docs on the messages: // https://jupyter-client.readthedocs.io/en/latest/messaging.html#messaging-in-jupyter private handleExecuteResult(msg: KernelMessage.IExecuteResultMsg) { - this.addToCellData({ - output_type: 'execute_result', - data: msg.content.data, - metadata: msg.content.metadata, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - transient: msg.content.transient as any, // NOSONAR - execution_count: msg.content.execution_count - }); + this.addToCellData( + { + output_type: 'execute_result', + data: msg.content.data, + metadata: msg.content.metadata, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + transient: msg.content.transient as any, // NOSONAR + execution_count: msg.content.execution_count + }, + msg + ); } private handleExecuteReply(msg: KernelMessage.IExecuteReplyMsg) { @@ -428,15 +636,18 @@ export class CellExecutionMessageHandler implements IDisposable { this.handleSetNextInput(payload as unknown as ISetNextInputPayload); } if (payload.data && payload.data.hasOwnProperty('text/plain')) { - this.addToCellData({ - // Mark as stream output so the text is formatted because it likely has ansi codes in it. - output_type: 'stream', - // eslint-disable-next-line @typescript-eslint/no-explicit-any - text: (payload.data as any)['text/plain'].toString(), - name: 'stdout', - metadata: {}, - execution_count: reply.execution_count - }); + this.addToCellData( + { + // Mark as stream output so the text is formatted because it likely has ansi codes in it. + output_type: 'stream', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + text: (payload.data as any)['text/plain'].toString(), + name: 'stdout', + metadata: {}, + execution_count: reply.execution_count + }, + msg + ); } }); } @@ -477,6 +688,14 @@ export class CellExecutionMessageHandler implements IDisposable { traceCellMessage(this.cell, `Kernel switching to ${msg.content.execution_state}`); } private handleStreamMessage(msg: KernelMessage.IStreamMsg) { + if ( + getParentHeaderMsgId(msg) && + this.outputsAreSpecificToAWidget?.msgIdsToSwallow == getParentHeaderMsgId(msg) + ) { + // Stream messages will be handled by the widget output. + return; + } + // eslint-disable-next-line complexity traceCellMessage(this.cell, 'Update streamed output'); // Possible execution of cell has completed (the task would have been disposed). @@ -485,13 +704,7 @@ export class CellExecutionMessageHandler implements IDisposable { const task = this.execution || this.createTemporaryTask(); // Clear output if waiting for a clear - const clearOutput = this.clearState.value; - if (clearOutput) { - traceCellMessage(this.cell, 'Clear cell output'); - this.clearLastUsedStreamOutput(); - task?.clearOutput().then(noop, noop); - this.clearState.update(false); - } + const { previousValueOfClearOutputOnNextUpdateToOutput } = this.clearOutputIfNecessary(task); // Ensure we append to previous output, only if the streams as the same & // If the last output is the desired stream type. if (this.lastUsedStreamOutput?.stream === msg.content.name) { @@ -524,7 +737,7 @@ export class CellExecutionMessageHandler implements IDisposable { }); traceCellMessage(this.cell, `Replace output items ${this.lastUsedStreamOutput.text.substring(0, 100)}`); task?.replaceOutputItems(output.items, this.lastUsedStreamOutput.output).then(noop, noop); - } else if (clearOutput) { + } else if (previousValueOfClearOutputOnNextUpdateToOutput) { // Replace the current outputs with a single new output. const text = formatStreamText(concatMultilineString(msg.content.text)); const output = cellOutputToVSCCellOutput({ @@ -558,14 +771,28 @@ export class CellExecutionMessageHandler implements IDisposable { // eslint-disable-next-line @typescript-eslint/no-explicit-any transient: msg.content.transient as any // NOSONAR }; - this.addToCellData(output); + this.addToCellData(output, msg); } private handleClearOutput(msg: KernelMessage.IClearOutputMsg) { - // If the message says wait, add every message type to our clear state. This will - // make us wait for this type of output before we clear it. - if (msg && msg.content.wait) { - this.clearState.update(true); + // Check if this message should be handled by a specific Widget output. + if ( + this.outputsAreSpecificToAWidget && + this.outputsAreSpecificToAWidget.msgIdsToSwallow === getParentHeaderMsgId(msg) + ) { + if (msg.content.wait) { + this.outputsAreSpecificToAWidget.clearOutputOnNextUpdateToOutput = true; + } else { + const task = this.execution || this.createTemporaryTask(); + this.updateWidgetOwnedOutput({ clearOutput: true }, task); + this.endTemporaryTask(); + } + return; + } + + // Regular output. + if (msg.content.wait) { + this.clearOutputOnNextUpdateToOutput = true; } else { // Possible execution of cell has completed (the task would have been disposed). // This message could have come from a background thread. @@ -590,7 +817,7 @@ export class CellExecutionMessageHandler implements IDisposable { traceback }; - this.addToCellData(output); + this.addToCellData(output, msg); this.cellHasErrorsInOutput = true; } diff --git a/src/platform/common/refBool.ts b/src/platform/common/refBool.ts deleted file mode 100644 index 22b0421b5e9..00000000000 --- a/src/platform/common/refBool.ts +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -export class RefBool { - constructor(private val: boolean) {} - - public get value(): boolean { - return this.val; - } - - public update(newVal: boolean) { - this.val = newVal; - } -} From 25e70cbf0a24ec53d3ffcd87b75d052623a94122 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 6 Jun 2022 09:56:10 +1000 Subject: [PATCH 02/13] Misc --- src/kernels/execution/cellExecutionMessageHandler.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/kernels/execution/cellExecutionMessageHandler.ts b/src/kernels/execution/cellExecutionMessageHandler.ts index 3bcc4b8b12a..fb9202b4b86 100644 --- a/src/kernels/execution/cellExecutionMessageHandler.ts +++ b/src/kernels/execution/cellExecutionMessageHandler.ts @@ -72,6 +72,10 @@ function getParentHeaderMsgId(msg: KernelMessage.IMessage): string | undefined { return undefined; } +/** + * The Output Widget in Jupyter can render multiple outputs. However some of them + * like ipywidgets and the like cannot be handled by it. + */ function canMimeTypeBeRenderedByWidgetManager(mime: string) { if (mime == CellOutputMimeTypes.stderr || mime == CellOutputMimeTypes.stdout || mime == CellOutputMimeTypes.error) { // These are plain text mimetypes that can be rendered by the Jupyter Lab widget manager. From 3a03608f395f56153478591092ea00ebb6fb890f Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 6 Jun 2022 10:47:12 +1000 Subject: [PATCH 03/13] Added tests --- .../execution/cellExecutionMessageHandler.ts | 2 +- .../notebook/executionService.vscode.test.ts | 60 ++++++++++++++++ .../notebooks/inteactive_function.ipynb | 27 ++++++++ .../notebooks/interactive_button.ipynb | 28 ++++++++ .../notebooks/nested_output_widget.ipynb | 69 +++++++++++++++++++ .../widgets/standard.vscode.common.test.ts | 68 ++++++++++++++++++ 6 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 src/test/datascience/widgets/notebooks/inteactive_function.ipynb create mode 100644 src/test/datascience/widgets/notebooks/interactive_button.ipynb create mode 100644 src/test/datascience/widgets/notebooks/nested_output_widget.ipynb diff --git a/src/kernels/execution/cellExecutionMessageHandler.ts b/src/kernels/execution/cellExecutionMessageHandler.ts index fb9202b4b86..0ccb50839f3 100644 --- a/src/kernels/execution/cellExecutionMessageHandler.ts +++ b/src/kernels/execution/cellExecutionMessageHandler.ts @@ -74,7 +74,7 @@ function getParentHeaderMsgId(msg: KernelMessage.IMessage): string | undefined { /** * The Output Widget in Jupyter can render multiple outputs. However some of them - * like ipywidgets and the like cannot be handled by it. + * like vendored mimetypes cannot be handled by it. */ function canMimeTypeBeRenderedByWidgetManager(mime: string) { if (mime == CellOutputMimeTypes.stderr || mime == CellOutputMimeTypes.stdout || mime == CellOutputMimeTypes.error) { diff --git a/src/test/datascience/notebook/executionService.vscode.test.ts b/src/test/datascience/notebook/executionService.vscode.test.ts index a549cd4ef38..5d479cb48e1 100644 --- a/src/test/datascience/notebook/executionService.vscode.test.ts +++ b/src/test/datascience/notebook/executionService.vscode.test.ts @@ -371,6 +371,66 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { await waitForExecutionCompletedSuccessfully(cell); }); + test('Clearing output immediately via code', async function () { + // Assume you are executing a cell that prints numbers 1-100. + // When printing number 50, you click clear. + // Cell output should now start printing output from 51 onwards, & not 1. + await insertCodeCell( + dedent` + from ipywidgets import widgets + from IPython.display import display, clear_output + import time + + display(widgets.Button(description="First Button")) + + time.sleep(2) + clear_output() + + display(widgets.Button(description="Second Button"))`, + { index: 0 } + ); + const cell = vscodeNotebook.activeNotebookEditor?.notebook.cellAt(0)!; + + await runAllCellsInActiveNotebook(); + + await Promise.all([ + waitForExecutionCompletedSuccessfully(cell), + waitForTextOutput(cell, 'Second Button', 1, false) + ]); + }); + test('Clearing output via code only when receiving new output', async function () { + // Assume you are executing a cell that prints numbers 1-100. + // When printing number 50, you click clear. + // Cell output should now start printing output from 51 onwards, & not 1. + await insertCodeCell( + dedent` + from ipywidgets import widgets + from IPython.display import display, clear_output + import time + + display(widgets.Button(description="First Button")) + + time.sleep(2) + clear_output(True) + + display(widgets.Button(description="Second Button"))`, + { index: 0 } + ); + const cell = vscodeNotebook.activeNotebookEditor?.notebook.cellAt(0)!; + + await runAllCellsInActiveNotebook(); + + // Wait for first button to appear then second. + await Promise.all([ + waitForExecutionCompletedSuccessfully(cell), + waitForTextOutput(cell, 'First Button', 0, false), + waitForTextOutput(cell, 'Second Button', 1, false) + ]); + + // Verify first is no longer visible and second is visible. + assert.notInclude(getCellOutputs(cell), 'First Button'); + assert.include(getCellOutputs(cell), 'Second Button'); + }); test('Shell commands should give preference to executables in Python Environment', async function () { if (IS_REMOTE_NATIVE_TEST()) { return this.skip(); diff --git a/src/test/datascience/widgets/notebooks/inteactive_function.ipynb b/src/test/datascience/widgets/notebooks/inteactive_function.ipynb new file mode 100644 index 00000000000..5d6e90a70b8 --- /dev/null +++ b/src/test/datascience/widgets/notebooks/inteactive_function.ipynb @@ -0,0 +1,27 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "from ipywidgets import widgets, interact\n", + "def do_something(filter_text):\n", + " print(f\"Executing do_something with '{filter_text}'\")\n", + " return filter_text\n", + " \n", + " \n", + "interact(do_something, filter_text=widgets.Text())" + ] + } + ], + "metadata": { + "language_info": { + "name": "python" + }, + "orig_nbformat": 4 + }, + "nbformat": 4, + "nbformat_minor": 2 +} diff --git a/src/test/datascience/widgets/notebooks/interactive_button.ipynb b/src/test/datascience/widgets/notebooks/interactive_button.ipynb new file mode 100644 index 00000000000..742dd5b15ab --- /dev/null +++ b/src/test/datascience/widgets/notebooks/interactive_button.ipynb @@ -0,0 +1,28 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "from IPython.display import display\n", + "from ipywidgets import widgets, interactive\n", + "button = widgets.Button(description=\"Click Me!\")\n", + "def on_button_clicked(b):\n", + " print(\"Button clicked\")\n", + "\n", + "display(button)\n", + "button.on_click(on_button_clicked)\n" + ] + } + ], + "metadata": { + "language_info": { + "name": "python" + }, + "orig_nbformat": 4 + }, + "nbformat": 4, + "nbformat_minor": 2 +} diff --git a/src/test/datascience/widgets/notebooks/nested_output_widget.ipynb b/src/test/datascience/widgets/notebooks/nested_output_widget.ipynb new file mode 100644 index 00000000000..ac83dcb4d59 --- /dev/null +++ b/src/test/datascience/widgets/notebooks/nested_output_widget.ipynb @@ -0,0 +1,69 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "from ipywidgets import interact, interactive, fixed, interact_manual\n", + "import ipywidgets as widgets\n", + "from IPython.display import display\n", + "\n", + "out = widgets.Output()\n", + "out" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "@out.capture()\n", + "def function_with_captured_output():\n", + " print('This goes into the first output widget')\n", + "\n", + "function_with_captured_output()" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "out2 = widgets.Output()\n", + "textBox = widgets.Text(value=\"Hello\")\n", + "\n", + "@out.capture()\n", + "def function_with_captured_output():\n", + " display(out2)\n", + " display(textBox)\n", + "\n", + "function_with_captured_output()" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "@out2.capture()\n", + "def function_with_captured_output2():\n", + " print('This goes into the second output widget which is nested in the first output widget')\n", + "\n", + "function_with_captured_output2()" + ] + } + ], + "metadata": { + "language_info": { + "name": "python" + }, + "orig_nbformat": 4 + }, + "nbformat": 4, + "nbformat_minor": 2 +} diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index cc3dec9a71b..0e76cbaf9c9 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -22,6 +22,7 @@ import { defaultNotebookTestTimeout, prewarmNotebooks, runCell, + waitForCellExecutionToComplete, waitForExecutionCompletedSuccessfully, waitForKernelToGetAutoSelected, waitForTextOutput, @@ -395,4 +396,71 @@ suite('IPyWisdget Tests', function () { await executeCellAndWaitForOutput(cell, comms); await assertOutputContainsHtml(cell, comms, ['66'], '.widget-readout'); }); + test.only('Nested Output Widgets', async () => { + const comms = await initializeNotebook({ templateFile: 'nested_output_widget.ipynb' }); + const [cell1, cell2, cell3, cell4] = vscodeNotebook.activeNotebookEditor!.notebook.getCells(); + await executeCellAndWaitForOutput(cell1, comms); + + // Run the second cell & verify we have output in the first cell. + await Promise.all([runCell(cell2), waitForCellExecutionToComplete(cell1)]); + await assertOutputContainsHtml(cell2, comms, ['First output widget'], '.widget-output'); + + // Run the 3rd cell to add a nested output. + // Also display the same nested output and the widget in the 3rd cell. + await Promise.all([runCell(cell3), waitForCellExecutionToComplete(cell3)]); + await assertOutputContainsHtml(cell1, comms, ['Widgets are linked an get updated<'], '.widget-output'); + await assertOutputContainsHtml(cell3, comms, ['>Widgets are linked an get updated<'], '.widget-output'); + }); + test.only('Interactive Button', async () => { + const comms = await initializeNotebook({ templateFile: 'interactive_button.ipynb' }); + const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); + + await executeCellAndWaitForOutput(cell, comms); + await assertOutputContainsHtml(cell, comms, ['Click Me!', ' { + const comms = await initializeNotebook({ templateFile: 'interactive_function.ipynb' }); + const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); + + await executeCellAndWaitForOutput(cell, comms); + await assertOutputContainsHtml( + cell, + comms, + ['Executing do_something with ''<", ">''<"], + '.widget-output' + ); + + // Update the textbox and confirm the output is updated accordingly. + await comms.setValue(cell, '.widget-text input', 'Updated First Time'); + await assertOutputContainsHtml( + cell, + comms, + [">Executing do_something with 'Updated First Time'<", ">'Updated First Time'<"], + '.widget-output' + ); + + // Update the textbox again and confirm the output is updated accordingly (should replace previous output). + await comms.setValue(cell, '.widget-text input', 'Updated Second Time'); + await assertOutputContainsHtml( + cell, + comms, + [">Executing do_something with 'Updated Second Time'<", ">'Updated Second Time'<"], + '.widget-output' + ); + }); }); From bc1398da6f3ef6d6e353db8826df03b402576ee9 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 6 Jun 2022 14:31:34 +1000 Subject: [PATCH 04/13] oops --- src/test/datascience/widgets/standard.vscode.common.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index 0e76cbaf9c9..948577cddd3 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -396,7 +396,7 @@ suite('IPyWisdget Tests', function () { await executeCellAndWaitForOutput(cell, comms); await assertOutputContainsHtml(cell, comms, ['66'], '.widget-readout'); }); - test.only('Nested Output Widgets', async () => { + test('Nested Output Widgets', async () => { const comms = await initializeNotebook({ templateFile: 'nested_output_widget.ipynb' }); const [cell1, cell2, cell3, cell4] = vscodeNotebook.activeNotebookEditor!.notebook.getCells(); await executeCellAndWaitForOutput(cell1, comms); @@ -422,7 +422,7 @@ suite('IPyWisdget Tests', function () { await assertOutputContainsHtml(cell1, comms, ['>Widgets are linked an get updated<'], '.widget-output'); await assertOutputContainsHtml(cell3, comms, ['>Widgets are linked an get updated<'], '.widget-output'); }); - test.only('Interactive Button', async () => { + test('Interactive Button', async () => { const comms = await initializeNotebook({ templateFile: 'interactive_button.ipynb' }); const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); @@ -433,7 +433,7 @@ suite('IPyWisdget Tests', function () { await click(comms, cell, 'button'); await assertOutputContainsHtml(cell, comms, ['Button clicked']); }); - test.only('Interactive Function', async () => { + test('Interactive Function', async () => { const comms = await initializeNotebook({ templateFile: 'interactive_function.ipynb' }); const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); From 099e84ae846331a29fe9a6242d6654a87974d707 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 8 Jun 2022 10:52:29 +1000 Subject: [PATCH 05/13] Misc --- .../execution/cellExecutionMessageHandler.ts | 78 ++++++++++++++---- .../notebook/executionService.vscode.test.ts | 4 +- .../notebooks/inteactive_function.ipynb | 3 +- .../widgets/notebooks/interactive_plot.ipynb | 34 ++++++++ .../interactive_plot_another_cell.ipynb | 82 +++++++++++++++++++ .../notebooks/nested_output_widget.ipynb | 11 ++- .../widgets/standard.vscode.common.test.ts | 40 ++++----- .../webview-side/ipywidgets/kernel/index.ts | 12 ++- 8 files changed, 219 insertions(+), 45 deletions(-) create mode 100644 src/test/datascience/widgets/notebooks/interactive_plot.ipynb create mode 100644 src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb diff --git a/src/kernels/execution/cellExecutionMessageHandler.ts b/src/kernels/execution/cellExecutionMessageHandler.ts index 0ccb50839f3..a860f2e43e2 100644 --- a/src/kernels/execution/cellExecutionMessageHandler.ts +++ b/src/kernels/execution/cellExecutionMessageHandler.ts @@ -81,6 +81,9 @@ function canMimeTypeBeRenderedByWidgetManager(mime: string) { // These are plain text mimetypes that can be rendered by the Jupyter Lab widget manager. return true; } + if (mime === WIDGET_MIMETYPE) { + return true; + } if (mime.startsWith('application/vnd')) { // Custom vendored mimetypes cannot be rendered by the widget manager, it relies on the output renderers. return false; @@ -93,6 +96,12 @@ function canMimeTypeBeRenderedByWidgetManager(mime: string) { * Responsible for handling of jupyter messages as a result of execution of individual cells. */ export class CellExecutionMessageHandler implements IDisposable { + /** + * Keeps track of the Models Ids (model_id) that is displayed in the output of a specific cell. + * model_id's maps to widget outputs. + * Hence here we're keeping track of the widgets displayed in outputs of specific cells. + */ + private static modelIdsOwnedByCells = new WeakMap>(); /** * The msg_id of the original request execute (when executing the cell). */ @@ -155,7 +164,7 @@ export class CellExecutionMessageHandler implements IDisposable { * or for any subsequent requests as a result of outputs sending custom messages. */ private readonly ownedCommIds = new Set(); - private readonly outputsOwnedByWidgetModel = new Map>(); + private static readonly outputsOwnedByWidgetModel = new Map>(); /** * List of msg_ids of requests sent either as part of request_execute * or for any subsequent requests as a result of outputs sending custom messages. @@ -459,14 +468,21 @@ export class CellExecutionMessageHandler implements IDisposable { output: ExecuteResult | DisplayData | nbformat.IStream | nbformat.IError | nbformat.IOutput, originalMessage: KernelMessage.IMessage ) { - if ( - this.context.extensionMode === ExtensionMode.Test && - output.data && - typeof output.data === 'object' && - WIDGET_MIMETYPE in output.data - ) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (output.data[WIDGET_MIMETYPE] as any)['_vsc_test_cellIndex'] = this.cell.index; + if (output.data && typeof output.data === 'object' && WIDGET_MIMETYPE in output.data) { + const widgetData = output.data[WIDGET_MIMETYPE] as { + version_major: number; + version_minor: number; + model_id: string; + }; + if (widgetData && this.context.extensionMode === ExtensionMode.Test) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (widgetData as any)['_vsc_test_cellIndex'] = this.cell.index; + } + if (widgetData && 'model_id' in widgetData) { + const modelIds = CellExecutionMessageHandler.modelIdsOwnedByCells.get(this.cell) || new Set(); + modelIds.add(widgetData.model_id); + CellExecutionMessageHandler.modelIdsOwnedByCells.set(this.cell, modelIds); + } } const cellOutput = cellOutputToVSCCellOutput(output); const displayId = @@ -509,7 +525,7 @@ export class CellExecutionMessageHandler implements IDisposable { this.outputsAreSpecificToAWidget && this.outputsAreSpecificToAWidget?.msgIdsToSwallow === parentHeaderMsgId ) { - const result = this.updateWidgetOwnedOutput( + const result = this.updateJupyterOutputWidgetWithOutput( { commId: this.outputsAreSpecificToAWidget.handlingCommId, outputToAppend: cellOutput }, task ); @@ -523,7 +539,21 @@ export class CellExecutionMessageHandler implements IDisposable { } this.endTemporaryTask(); } - private updateWidgetOwnedOutput( + /** + * Assume we have a Jupyter Output Widget, and we render that in Cell1. + * Now we add some output to that output from Cell2, general outputs will be automatically handled by the widget class. + * More complex outputs like plotly or the like, need to be handed by us (as Jupyter Output Widget expects Jupyter Notebooks/Jupyter Lab UI to handle that). + * To make this possible, we first need to find out the cell that owns the Output Widget, and then add that output to that particular cell. + * What we do in such a case is append this complex output just after the Output widget. + * + * In other cases, when appending such output, we need to clear the previous output that we added. + * + * This method is responsible for: + * - Finding the cell that owns the output widget + * - Clearing the custom output that was appended after the output widget. + * - Appending custom output after the output widget + */ + private updateJupyterOutputWidgetWithOutput( options: { outputToAppend: NotebookCellOutput; commId: string } | { clearOutput: true }, task?: NotebookCellExecution ): { outputAdded: true } | undefined { @@ -534,7 +564,15 @@ export class CellExecutionMessageHandler implements IDisposable { const outputToAppend = 'outputToAppend' in options ? options.outputToAppend : undefined; const expectedModelId = this.commIdsMappedToParentWidgetModel.get(commId) || commId; - const widgetOutput = this.cell.outputs.find((output) => { + // Find the cell that owns the widget (where its model_id = commId). + const cell = this.cell.notebook + .getCells() + .find((cell) => CellExecutionMessageHandler.modelIdsOwnedByCells.get(cell)?.has(expectedModelId)); + if (!cell) { + traceWarning(`Unable to find a cell that owns the model ${expectedModelId}`); + return; + } + const widgetOutput = cell.outputs.find((output) => { return output.items.find((outputItem) => { if (outputItem.mime !== WIDGET_MIMETYPE) { return false; @@ -551,7 +589,8 @@ export class CellExecutionMessageHandler implements IDisposable { if (!widgetOutput) { return; } - const outputsOwnedByWidgetModel = this.outputsOwnedByWidgetModel.get(expectedModelId) || new Set(); + const outputsOwnedByWidgetModel = + CellExecutionMessageHandler.outputsOwnedByWidgetModel.get(expectedModelId) || new Set(); // We have some new outputs, that need to be placed immediately after the widget and before any other output // that doesn't belong to the widget. @@ -559,7 +598,7 @@ export class CellExecutionMessageHandler implements IDisposable { if (this.outputsAreSpecificToAWidget) { this.outputsAreSpecificToAWidget.clearOutputOnNextUpdateToOutput = false; } - const newOutputs = this.cell.outputs.slice().filter((item) => { + const newOutputs = cell.outputs.slice().filter((item) => { if (clearWidgetOutput) { // If we're supposed to clear the output, then clear all of the output that's // specific to this widget. @@ -573,7 +612,7 @@ export class CellExecutionMessageHandler implements IDisposable { const outputsUptoWidget = newOutputs.slice(0, newOutputs.indexOf(widgetOutput) + 1); const outputsAfterWidget = newOutputs.slice(newOutputs.indexOf(widgetOutput) + 1); - this.outputsOwnedByWidgetModel.set(expectedModelId, outputsOwnedByWidgetModel); + CellExecutionMessageHandler.outputsOwnedByWidgetModel.set(expectedModelId, outputsOwnedByWidgetModel); if (outputToAppend) { // Keep track of the output added that belongs to the widget. // Next time when we need to clear the output belonging to this widget, all we need to do is @@ -585,7 +624,12 @@ export class CellExecutionMessageHandler implements IDisposable { const newOutput = outputToAppend ? outputsUptoWidget.concat(outputToAppend).concat(outputsAfterWidget) : outputsUptoWidget.concat(outputsAfterWidget); - task?.replaceOutput(newOutput).then(noop, noop); + if (outputsAfterWidget.length === 0 && newOutput) { + // No need to replace everything, just append what we need. + task?.appendOutput(newOutput, cell).then(noop, noop); + } else { + task?.replaceOutput(newOutput, cell).then(noop, noop); + } return { outputAdded: true }; } private async handleInputRequest(msg: KernelMessage.IStdinMessage) { @@ -788,7 +832,7 @@ export class CellExecutionMessageHandler implements IDisposable { this.outputsAreSpecificToAWidget.clearOutputOnNextUpdateToOutput = true; } else { const task = this.execution || this.createTemporaryTask(); - this.updateWidgetOwnedOutput({ clearOutput: true }, task); + this.updateJupyterOutputWidgetWithOutput({ clearOutput: true }, task); this.endTemporaryTask(); } return; diff --git a/src/test/datascience/notebook/executionService.vscode.test.ts b/src/test/datascience/notebook/executionService.vscode.test.ts index 5d479cb48e1..c90534925c7 100644 --- a/src/test/datascience/notebook/executionService.vscode.test.ts +++ b/src/test/datascience/notebook/executionService.vscode.test.ts @@ -395,7 +395,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { await Promise.all([ waitForExecutionCompletedSuccessfully(cell), - waitForTextOutput(cell, 'Second Button', 1, false) + waitForTextOutput(cell, 'Second Button', 0, false) ]); }); test('Clearing output via code only when receiving new output', async function () { @@ -424,7 +424,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { await Promise.all([ waitForExecutionCompletedSuccessfully(cell), waitForTextOutput(cell, 'First Button', 0, false), - waitForTextOutput(cell, 'Second Button', 1, false) + waitForTextOutput(cell, 'Second Button', 0, false) ]); // Verify first is no longer visible and second is visible. diff --git a/src/test/datascience/widgets/notebooks/inteactive_function.ipynb b/src/test/datascience/widgets/notebooks/inteactive_function.ipynb index 5d6e90a70b8..468744abf36 100644 --- a/src/test/datascience/widgets/notebooks/inteactive_function.ipynb +++ b/src/test/datascience/widgets/notebooks/inteactive_function.ipynb @@ -12,7 +12,8 @@ " return filter_text\n", " \n", " \n", - "interact(do_something, filter_text=widgets.Text())" + "interact(do_something, filter_text=widgets.Text(value='Foo'))\n", + "do_something('Hello World')" ] } ], diff --git a/src/test/datascience/widgets/notebooks/interactive_plot.ipynb b/src/test/datascience/widgets/notebooks/interactive_plot.ipynb new file mode 100644 index 00000000000..c314ba3e549 --- /dev/null +++ b/src/test/datascience/widgets/notebooks/interactive_plot.ipynb @@ -0,0 +1,34 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "import ipywidgets as widgets\n", + "from ipywidgets import interact\n", + "import pandas as pd\n", + "import plotly.express as px\n", + "pd.options.plotting.backend = \"plotly\"\n", + "\n", + "def update_graph(rng):\n", + " df = pd.DataFrame({\"x\": list(range(*rng)), \"y\": list(range(*rng))})\n", + " fig = df.plot.scatter(x=\"x\", y=\"y\")\n", + " fig.show()\n", + "\n", + "slider=widgets.IntRangeSlider(value=(0, 10), min=0, max=20, description='Hello')\n", + "interact(update_graph, rng=slider)\n", + "update_graph([0, 10])\n" + ] + } + ], + "metadata": { + "language_info": { + "name": "python" + }, + "orig_nbformat": 4 + }, + "nbformat": 4, + "nbformat_minor": 2 +} diff --git a/src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb b/src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb new file mode 100644 index 00000000000..82dbee880a4 --- /dev/null +++ b/src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb @@ -0,0 +1,82 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "import ipywidgets as widgets\n", + "from ipywidgets import interact\n", + "import pandas as pd\n", + "import plotly.express as px\n", + "pd.options.plotting.backend = \"plotly\"\n", + "\n", + "from ipywidgets import interact, interactive, fixed, interact_manual\n", + "import ipywidgets as widgets\n", + "from IPython.display import display\n", + "\n", + "out = widgets.Output()\n", + "out" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "@out.capture()\n", + "def function_with_captured_output():\n", + " print('First output widget')\n", + "\n", + "function_with_captured_output()" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "out2 = widgets.Output()\n", + "caption = widgets.Label(value='Label Widget')\n", + "textbox = widgets.Text()\n", + "widgets.link((caption, 'value'), (textbox, 'value'))\n", + "\n", + "@out.capture()\n", + "def function_with_captured_output():\n", + " display(out2)\n", + " display(caption)\n", + " display(textbox)\n", + " df = pd.DataFrame({\"x\": list(range(10)), \"y\": list(range(10))})\n", + " fig = df.plot.scatter(x=\"x\", y=\"y\")\n", + " fig.show()\n", + "\n", + "\n", + "function_with_captured_output()" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "@out2.capture()\n", + "def function_with_captured_output2():\n", + " print('Second output widget')\n", + "\n", + "function_with_captured_output2()" + ] + } + ], + "metadata": { + "language_info": { + "name": "python" + }, + "orig_nbformat": 4 + }, + "nbformat": 4, + "nbformat_minor": 2 +} diff --git a/src/test/datascience/widgets/notebooks/nested_output_widget.ipynb b/src/test/datascience/widgets/notebooks/nested_output_widget.ipynb index ac83dcb4d59..820adbbc1ba 100644 --- a/src/test/datascience/widgets/notebooks/nested_output_widget.ipynb +++ b/src/test/datascience/widgets/notebooks/nested_output_widget.ipynb @@ -22,7 +22,7 @@ "source": [ "@out.capture()\n", "def function_with_captured_output():\n", - " print('This goes into the first output widget')\n", + " print('First output widget')\n", "\n", "function_with_captured_output()" ] @@ -34,12 +34,15 @@ "outputs": [], "source": [ "out2 = widgets.Output()\n", - "textBox = widgets.Text(value=\"Hello\")\n", + "caption = widgets.Label(value='Label Widget')\n", + "textbox = widgets.Text()\n", + "widgets.link((caption, 'value'), (textbox, 'value'))\n", "\n", "@out.capture()\n", "def function_with_captured_output():\n", " display(out2)\n", - " display(textBox)\n", + " display(caption)\n", + " display(textbox)\n", "\n", "function_with_captured_output()" ] @@ -52,7 +55,7 @@ "source": [ "@out2.capture()\n", "def function_with_captured_output2():\n", - " print('This goes into the second output widget which is nested in the first output widget')\n", + " print('Second output widget')\n", "\n", "function_with_captured_output2()" ] diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index 948577cddd3..763637ea286 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -396,33 +396,33 @@ suite('IPyWisdget Tests', function () { await executeCellAndWaitForOutput(cell, comms); await assertOutputContainsHtml(cell, comms, ['66'], '.widget-readout'); }); - test('Nested Output Widgets', async () => { + test.only('Nested Output Widgets', async () => { const comms = await initializeNotebook({ templateFile: 'nested_output_widget.ipynb' }); const [cell1, cell2, cell3, cell4] = vscodeNotebook.activeNotebookEditor!.notebook.getCells(); await executeCellAndWaitForOutput(cell1, comms); // Run the second cell & verify we have output in the first cell. await Promise.all([runCell(cell2), waitForCellExecutionToComplete(cell1)]); - await assertOutputContainsHtml(cell2, comms, ['First output widget'], '.widget-output'); + await assertOutputContainsHtml(cell1, comms, ['First output widget'], '.widget-output'); // Run the 3rd cell to add a nested output. // Also display the same nested output and the widget in the 3rd cell. await Promise.all([runCell(cell3), waitForCellExecutionToComplete(cell3)]); await assertOutputContainsHtml(cell1, comms, ['Widgets are linked an get updated<'], '.widget-output'); - await assertOutputContainsHtml(cell3, comms, ['>Widgets are linked an get updated<'], '.widget-output'); + assert.strictEqual(cell3.outputs.length, 0, 'Cell 3 should not have any output'); }); - test('Interactive Button', async () => { + test.only('Interactive Button', async () => { const comms = await initializeNotebook({ templateFile: 'interactive_button.ipynb' }); const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); @@ -433,7 +433,7 @@ suite('IPyWisdget Tests', function () { await click(comms, cell, 'button'); await assertOutputContainsHtml(cell, comms, ['Button clicked']); }); - test('Interactive Function', async () => { + test.only('Interactive Function', async () => { const comms = await initializeNotebook({ templateFile: 'interactive_function.ipynb' }); const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); @@ -441,25 +441,27 @@ suite('IPyWisdget Tests', function () { await assertOutputContainsHtml( cell, comms, - ['Executing do_something with ''<", ">''<"], + [ + 'Executing do_something with 'Foo'<", + ">'Foo'<", + ">Executing do_something with 'Hello World'<", + ">'Hello World'<" + ], '.widget-output' ); // Update the textbox and confirm the output is updated accordingly. - await comms.setValue(cell, '.widget-text input', 'Updated First Time'); + await comms.setValue(cell, '.widget-text input', 'Bar'); await assertOutputContainsHtml( cell, comms, - [">Executing do_something with 'Updated First Time'<", ">'Updated First Time'<"], - '.widget-output' - ); - - // Update the textbox again and confirm the output is updated accordingly (should replace previous output). - await comms.setValue(cell, '.widget-text input', 'Updated Second Time'); - await assertOutputContainsHtml( - cell, - comms, - [">Executing do_something with 'Updated Second Time'<", ">'Updated Second Time'<"], + [ + ">Executing do_something with 'Bar'<", + ">'Bar'<", + ">Executing do_something with 'Hello World'<", + ">'Hello World'<" + ], '.widget-output' ); }); diff --git a/src/webviews/webview-side/ipywidgets/kernel/index.ts b/src/webviews/webview-side/ipywidgets/kernel/index.ts index 26c11a533af..b83310d6df7 100644 --- a/src/webviews/webview-side/ipywidgets/kernel/index.ts +++ b/src/webviews/webview-side/ipywidgets/kernel/index.ts @@ -81,7 +81,7 @@ class WidgetManagerComponent { const outputDisposables = new Map(); const htmlDisposables = new WeakMap(); -const renderedWidgets = new Set(); +const renderedWidgets = new Map(); /** * Called from renderer to render output. * This will be exposed as a public method on window for renderer to render output. @@ -109,9 +109,14 @@ export function renderOutput(outputItem: OutputItem, element: HTMLElement, logge } export function disposeOutput(outputId?: string) { if (outputId) { + const widget = renderedWidgets.get(outputId)?.widget; + renderedWidgets.delete(outputId); stackOfWidgetsRenderStatusByOutputId = stackOfWidgetsRenderStatusByOutputId.filter( (item) => !(outputId in item) ); + if (widget) { + widget.dispose(); + } } } function renderIPyWidget( @@ -140,9 +145,12 @@ function renderIPyWidget( ele.className = 'cell-output-ipywidget-background'; container.appendChild(ele); ele.appendChild(output); - renderedWidgets.add(outputId); + renderedWidgets.set(outputId, {}); createWidgetView(model, ele) .then((w) => { + if (renderedWidgets.has(outputId)) { + renderedWidgets.get(outputId)!.widget = w; + } const disposable = { dispose: () => { // What if we render the same model in two cells. From fc913d05538dba78ed7a2f950002ccd41085cfe3 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 8 Jun 2022 11:39:06 +1000 Subject: [PATCH 06/13] Misc --- .../execution/cellExecutionMessageHandler.ts | 4 +- .../widgets/notebooks/interactive_plot.ipynb | 26 +++++++++ .../widgets/standard.vscode.common.test.ts | 56 ++++++++++++++++++- 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/src/kernels/execution/cellExecutionMessageHandler.ts b/src/kernels/execution/cellExecutionMessageHandler.ts index a860f2e43e2..35e34f56492 100644 --- a/src/kernels/execution/cellExecutionMessageHandler.ts +++ b/src/kernels/execution/cellExecutionMessageHandler.ts @@ -624,9 +624,9 @@ export class CellExecutionMessageHandler implements IDisposable { const newOutput = outputToAppend ? outputsUptoWidget.concat(outputToAppend).concat(outputsAfterWidget) : outputsUptoWidget.concat(outputsAfterWidget); - if (outputsAfterWidget.length === 0 && newOutput) { + if (outputsAfterWidget.length === 0 && outputToAppend) { // No need to replace everything, just append what we need. - task?.appendOutput(newOutput, cell).then(noop, noop); + task?.appendOutput(outputToAppend, cell).then(noop, noop); } else { task?.replaceOutput(newOutput, cell).then(noop, noop); } diff --git a/src/test/datascience/widgets/notebooks/interactive_plot.ipynb b/src/test/datascience/widgets/notebooks/interactive_plot.ipynb index c314ba3e549..0401774694f 100644 --- a/src/test/datascience/widgets/notebooks/interactive_plot.ipynb +++ b/src/test/datascience/widgets/notebooks/interactive_plot.ipynb @@ -6,6 +6,32 @@ "metadata": {}, "outputs": [], "source": [ + "# Since we don't want to install plotly on the CI, we can test with a custom mime type.\n", + "# The mime type application/vnd.custom should end up getting rendered as an individual output in the cell (instead of letting the Output Widget handle it - which it cannot).\n", + "\n", + "import ipywidgets as widgets\n", + "from ipywidgets import interact\n", + "import IPython.display\n", + "\n", + "\n", + "def update_graph(txt):\n", + " print(f\"Text Value is {txt}\")\n", + " display({\"application/vnd.custom\": f\"Text Value is {txt}\"}, raw=True)\n", + "\n", + "\n", + "text = widgets.Text(value='Foo')\n", + "interact(update_graph, txt=text)\n", + "update_graph(\"Hello World\")\n" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "metadata": {}, + "outputs": [], + "source": [ + "# This is a real world example.\n", + "\n", "import ipywidgets as widgets\n", "from ipywidgets import interact\n", "import pandas as pd\n", diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index 763637ea286..9ff780b7dfb 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -7,7 +7,7 @@ import { assert } from 'chai'; /* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ import * as urlPath from '../../../platform/vscode-path/resources'; import * as sinon from 'sinon'; -import { commands, ConfigurationTarget, NotebookCell, Uri, workspace } from 'vscode'; +import { commands, ConfigurationTarget, NotebookCell, notebooks, Uri, workspace } from 'vscode'; import { IVSCodeNotebook } from '../../../platform/common/application/types'; import { traceInfo } from '../../../platform/logging'; import { IDisposable } from '../../../platform/common/types'; @@ -30,6 +30,7 @@ import { } from '../notebook/helper'; import { initializeWidgetComms, Utils } from './commUtils'; import { WidgetRenderingTimeoutForTests } from './constants'; +import { getTextOutputValue } from '../../../notebooks/helpers'; /* eslint-disable @typescript-eslint/no-explicit-any, no-invalid-this */ suite('IPyWisdget Tests', function () { @@ -465,4 +466,57 @@ suite('IPyWisdget Tests', function () { '.widget-output' ); }); + test.only('Interactive Plot', async () => { + const comms = await initializeNotebook({ templateFile: 'interactive_plot.ipynb' }); + const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); + + await executeCellAndWaitForOutput(cell, comms); + await assertOutputContainsHtml(cell, comms, ['Text Value is Foo'], '.widget-output'); + assert.strictEqual(cell.outputs.length, 4, 'Cell should have 4 outputs'); + + const [, output2, output3, output4] = cell.outputs; + // This cannot be displayed by output widget, hence we need to handle this. + assert.strictEqual(output2.items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(output2.items[0].data).toString(), 'Text Value is Foo'); + + assert.strictEqual(getTextOutputValue(output3), 'Text Value is Foo'); + + // This cannot be displayed by output widget, hence we need to handle this. + assert.strictEqual(output4.items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(output4.items[0].data).toString(), 'Text Value is Foo'); + + // Wait for the second output to get updated. + const outputUpdated = new Promise((resolve) => { + workspace.onDidChangeNotebookDocument( + (e) => { + const currentCellChange = e.cellChanges.find((item) => item.cell === cell); + if (!currentCellChange || !currentCellChange.outputs || currentCellChange.outputs.length < 4) { + return; + } + const secondOutput = currentCellChange.outputs[1]; + if (Buffer.from(secondOutput.items[0].data).toString() === 'Text Value is Bar') { + resolve(true); + } + }, + undefined, + disposables + ); + }); + // Update the textbox and confirm the output is updated accordingly. + await comms.setValue(cell, '.widget-text input', 'Bar'); + + // Wait for the output to get updated. + await waitForCondition(() => outputUpdated, 5_000, 'Second output not updated'); + + // This should have been updated. + assert.strictEqual(output2.items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(output2.items[0].data).toString(), 'Text Value is Bar'); + + // This should not have been updated. + assert.strictEqual(getTextOutputValue(output3), 'Text Value is Foo'); + + // This should not have been updated. + assert.strictEqual(output4.items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(output4.items[0].data).toString(), 'Text Value is Foo'); + }); }); From 9c74089a2ad9a7d031a9c28d994afe2f6aaccf55 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 8 Jun 2022 13:04:29 +1000 Subject: [PATCH 07/13] oops --- src/test/datascience/widgets/standard.vscode.common.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index 9ff780b7dfb..173288e28e5 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -7,7 +7,7 @@ import { assert } from 'chai'; /* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ import * as urlPath from '../../../platform/vscode-path/resources'; import * as sinon from 'sinon'; -import { commands, ConfigurationTarget, NotebookCell, notebooks, Uri, workspace } from 'vscode'; +import { commands, ConfigurationTarget, NotebookCell, Uri, workspace } from 'vscode'; import { IVSCodeNotebook } from '../../../platform/common/application/types'; import { traceInfo } from '../../../platform/logging'; import { IDisposable } from '../../../platform/common/types'; From 6dac2087edfd1b8667921c374e11059b223e1d95 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 9 Jun 2022 03:39:34 +1000 Subject: [PATCH 08/13] Fix tests --- ...ction.ipynb => interactive_function.ipynb} | 0 .../interactive_plot_another_cell.ipynb | 14 +-- .../widgets/standard.vscode.common.test.ts | 85 +++++++++---------- 3 files changed, 49 insertions(+), 50 deletions(-) rename src/test/datascience/widgets/notebooks/{inteactive_function.ipynb => interactive_function.ipynb} (100%) diff --git a/src/test/datascience/widgets/notebooks/inteactive_function.ipynb b/src/test/datascience/widgets/notebooks/interactive_function.ipynb similarity index 100% rename from src/test/datascience/widgets/notebooks/inteactive_function.ipynb rename to src/test/datascience/widgets/notebooks/interactive_function.ipynb diff --git a/src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb b/src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb index 82dbee880a4..9ec539dc577 100644 --- a/src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb +++ b/src/test/datascience/widgets/notebooks/interactive_plot_another_cell.ipynb @@ -8,9 +8,9 @@ "source": [ "import ipywidgets as widgets\n", "from ipywidgets import interact\n", - "import pandas as pd\n", - "import plotly.express as px\n", - "pd.options.plotting.backend = \"plotly\"\n", + "# import pandas as pd\n", + "# import plotly.express as px\n", + "# pd.options.plotting.backend = \"plotly\"\n", "\n", "from ipywidgets import interact, interactive, fixed, interact_manual\n", "import ipywidgets as widgets\n", @@ -49,9 +49,11 @@ " display(out2)\n", " display(caption)\n", " display(textbox)\n", - " df = pd.DataFrame({\"x\": list(range(10)), \"y\": list(range(10))})\n", - " fig = df.plot.scatter(x=\"x\", y=\"y\")\n", - " fig.show()\n", + " # Output a custom mime type instead of a plotly plot so that we don't have to install plotly on CI.\n", + " display({\"application/vnd.custom\": f\"Custom Mime Output under the `output widget`\"}, raw=True)\n", + " # df = pd.DataFrame({\"x\": list(range(10)), \"y\": list(range(10))})\n", + " # fig = df.plot.scatter(x=\"x\", y=\"y\")\n", + " # fig.show()\n", "\n", "\n", "function_with_captured_output()" diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index 173288e28e5..09f580330f2 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -397,7 +397,7 @@ suite('IPyWisdget Tests', function () { await executeCellAndWaitForOutput(cell, comms); await assertOutputContainsHtml(cell, comms, ['66'], '.widget-readout'); }); - test.only('Nested Output Widgets', async () => { + test('Nested Output Widgets', async () => { const comms = await initializeNotebook({ templateFile: 'nested_output_widget.ipynb' }); const [cell1, cell2, cell3, cell4] = vscodeNotebook.activeNotebookEditor!.notebook.getCells(); await executeCellAndWaitForOutput(cell1, comms); @@ -423,7 +423,7 @@ suite('IPyWisdget Tests', function () { await assertOutputContainsHtml(cell1, comms, ['>Widgets are linked an get updated<'], '.widget-output'); assert.strictEqual(cell3.outputs.length, 0, 'Cell 3 should not have any output'); }); - test.only('Interactive Button', async () => { + test('Interactive Button', async () => { const comms = await initializeNotebook({ templateFile: 'interactive_button.ipynb' }); const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); @@ -432,58 +432,56 @@ suite('IPyWisdget Tests', function () { // Click the button and verify we have output in other cells await click(comms, cell, 'button'); - await assertOutputContainsHtml(cell, comms, ['Button clicked']); + await waitForCondition( + () => { + assert.strictEqual(getTextOutputValue(cell.outputs[1]).trim(), 'Button clicked'); + return true; + }, + 5_000, + `Expected 'Button clicked' to exist in ${getTextOutputValue(cell.outputs[1])}` + ); }); - test.only('Interactive Function', async () => { + test('Interactive Function', async () => { const comms = await initializeNotebook({ templateFile: 'interactive_function.ipynb' }); const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); await executeCellAndWaitForOutput(cell, comms); - await assertOutputContainsHtml( - cell, - comms, - [ - 'Executing do_something with 'Foo'<", - ">'Foo'<", - ">Executing do_something with 'Hello World'<", - ">'Hello World'<" - ], - '.widget-output' - ); + await assertOutputContainsHtml(cell, comms, [ + 'Executing do_something with 'Foo'", + ">'Foo'" + ]); + await waitForCondition(() => cell.outputs.length >= 3, 5_000, 'Cell must have 3 outputs'); + assert.strictEqual(getTextOutputValue(cell.outputs[1]).trim(), `Executing do_something with 'Hello World'`); + assert.strictEqual(getTextOutputValue(cell.outputs[2]).trim(), `'Hello World'`); // Update the textbox and confirm the output is updated accordingly. await comms.setValue(cell, '.widget-text input', 'Bar'); - await assertOutputContainsHtml( - cell, - comms, - [ - ">Executing do_something with 'Bar'<", - ">'Bar'<", - ">Executing do_something with 'Hello World'<", - ">'Hello World'<" - ], - '.widget-output' - ); + await assertOutputContainsHtml(cell, comms, [ + 'Executing do_something with 'Bar'", + ">'Bar'" + ]); + assert.strictEqual(getTextOutputValue(cell.outputs[1]).trim(), `Executing do_something with 'Hello World'`); + assert.strictEqual(getTextOutputValue(cell.outputs[2]).trim(), `'Hello World'`); }); - test.only('Interactive Plot', async () => { + test('Interactive Plot', async () => { const comms = await initializeNotebook({ templateFile: 'interactive_plot.ipynb' }); const cell = vscodeNotebook.activeNotebookEditor!.notebook.cellAt(0); await executeCellAndWaitForOutput(cell, comms); - await assertOutputContainsHtml(cell, comms, ['Text Value is Foo'], '.widget-output'); + await assertOutputContainsHtml(cell, comms, ['Text Value is Foo']); assert.strictEqual(cell.outputs.length, 4, 'Cell should have 4 outputs'); - const [, output2, output3, output4] = cell.outputs; // This cannot be displayed by output widget, hence we need to handle this. - assert.strictEqual(output2.items[0].mime, 'application/vnd.custom'); - assert.strictEqual(Buffer.from(output2.items[0].data).toString(), 'Text Value is Foo'); + assert.strictEqual(cell.outputs[1].items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(cell.outputs[1].items[0].data).toString(), 'Text Value is Foo'); - assert.strictEqual(getTextOutputValue(output3), 'Text Value is Foo'); + assert.strictEqual(getTextOutputValue(cell.outputs[2]).trim(), 'Text Value is Hello World'); // This cannot be displayed by output widget, hence we need to handle this. - assert.strictEqual(output4.items[0].mime, 'application/vnd.custom'); - assert.strictEqual(Buffer.from(output4.items[0].data).toString(), 'Text Value is Foo'); + assert.strictEqual(cell.outputs[3].items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(cell.outputs[3].items[0].data).toString().trim(), 'Text Value is Hello World'); // Wait for the second output to get updated. const outputUpdated = new Promise((resolve) => { @@ -508,15 +506,14 @@ suite('IPyWisdget Tests', function () { // Wait for the output to get updated. await waitForCondition(() => outputUpdated, 5_000, 'Second output not updated'); - // This should have been updated. - assert.strictEqual(output2.items[0].mime, 'application/vnd.custom'); - assert.strictEqual(Buffer.from(output2.items[0].data).toString(), 'Text Value is Bar'); - - // This should not have been updated. - assert.strictEqual(getTextOutputValue(output3), 'Text Value is Foo'); + // The first & second outputs should have been updated + await assertOutputContainsHtml(cell, comms, ['Text Value is Bar']); + assert.strictEqual(cell.outputs[1].items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(cell.outputs[1].items[0].data).toString().trim(), 'Text Value is Bar'); - // This should not have been updated. - assert.strictEqual(output4.items[0].mime, 'application/vnd.custom'); - assert.strictEqual(Buffer.from(output4.items[0].data).toString(), 'Text Value is Foo'); + // The last two should not have changed. + assert.strictEqual(getTextOutputValue(cell.outputs[2]).trim(), 'Text Value is Hello World'); + assert.strictEqual(cell.outputs[3].items[0].mime, 'application/vnd.custom'); + assert.strictEqual(Buffer.from(cell.outputs[3].items[0].data).toString().trim(), 'Text Value is Hello World'); }); }); From 7cbba72f1a58992236cb3f546aaf23bb82475974 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 9 Jun 2022 04:47:37 +1000 Subject: [PATCH 09/13] News --- news/2 Fixes/9503.md | 1 + src/test/datascience/widgets/standard.vscode.common.test.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 news/2 Fixes/9503.md diff --git a/news/2 Fixes/9503.md b/news/2 Fixes/9503.md new file mode 100644 index 00000000000..f75ef677f70 --- /dev/null +++ b/news/2 Fixes/9503.md @@ -0,0 +1 @@ +Support displaying of complex outputs (such as Plots) in the Output Widget. diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index 09f580330f2..0c376a9b746 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -30,7 +30,7 @@ import { } from '../notebook/helper'; import { initializeWidgetComms, Utils } from './commUtils'; import { WidgetRenderingTimeoutForTests } from './constants'; -import { getTextOutputValue } from '../../../notebooks/helpers'; +import { getTextOutputValue } from '../../../kernels/execution/helpers'; /* eslint-disable @typescript-eslint/no-explicit-any, no-invalid-this */ suite('IPyWisdget Tests', function () { From ad9100930bdf6c71efa526437d2cc4de478b206e Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 9 Jun 2022 05:39:00 +1000 Subject: [PATCH 10/13] test From 30ba4647459893d099a8f1450b060d84889a8e2b Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 9 Jun 2022 06:29:24 +1000 Subject: [PATCH 11/13] Misc --- src/test/datascience/notebook/executionService.vscode.test.ts | 4 +++- src/test/datascience/widgets/standard.vscode.common.test.ts | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/datascience/notebook/executionService.vscode.test.ts b/src/test/datascience/notebook/executionService.vscode.test.ts index c90534925c7..07ce8eb400b 100644 --- a/src/test/datascience/notebook/executionService.vscode.test.ts +++ b/src/test/datascience/notebook/executionService.vscode.test.ts @@ -99,6 +99,8 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { await startJupyterServer(); await createEmptyPythonNotebook(disposables); assert.isOk(vscodeNotebook.activeNotebookEditor, 'No active notebook'); + // With less realestate, the outputs might not get rendered (VS Code optimization to avoid rendering if not in viewport). + await commands.executeCommand('workbench.action.closePanel'); traceInfo(`Start Test (completed) ${this.currentTest?.title}`); } catch (e) { await captureScreenShot(this.currentTest?.title || 'unknown'); @@ -863,7 +865,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { }); test('Handling of carriage returns', async () => { - await insertCodeCell('print("one\\r", end="")\nprint("two\\r", end="")\nprint("three\\r", end="")', { + await insertCodeCell('print("one\\r", e nd="")\nprint("two\\r", end="")\nprint("three\\r", end="")', { index: 0 }); await insertCodeCell('print("one\\r")\nprint("two\\r")\nprint("three\\r")', { index: 1 }); diff --git a/src/test/datascience/widgets/standard.vscode.common.test.ts b/src/test/datascience/widgets/standard.vscode.common.test.ts index 0c376a9b746..1931933f60a 100644 --- a/src/test/datascience/widgets/standard.vscode.common.test.ts +++ b/src/test/datascience/widgets/standard.vscode.common.test.ts @@ -75,6 +75,7 @@ suite('IPyWisdget Tests', function () { await startJupyterServer(); await closeNotebooks(); traceInfo(`Start Test (completed) ${this.currentTest?.title}`); + // With less realestate, the outputs might not get rendered (VS Code optimization to avoid rendering if not in viewport). await commands.executeCommand('workbench.action.closePanel'); }); teardown(async function () { From b2d9fc763f58bbb0bdc18741285bb34d49465235 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 9 Jun 2022 09:03:02 +1000 Subject: [PATCH 12/13] oops --- src/test/datascience/notebook/executionService.vscode.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/datascience/notebook/executionService.vscode.test.ts b/src/test/datascience/notebook/executionService.vscode.test.ts index 07ce8eb400b..4713da541dd 100644 --- a/src/test/datascience/notebook/executionService.vscode.test.ts +++ b/src/test/datascience/notebook/executionService.vscode.test.ts @@ -865,7 +865,7 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { }); test('Handling of carriage returns', async () => { - await insertCodeCell('print("one\\r", e nd="")\nprint("two\\r", end="")\nprint("three\\r", end="")', { + await insertCodeCell('print("one\\r", end="")\nprint("two\\r", end="")\nprint("three\\r", end="")', { index: 0 }); await insertCodeCell('print("one\\r")\nprint("two\\r")\nprint("three\\r")', { index: 1 }); From 0c981643861182e0ad65f47f7fe7adf3aec4cce7 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 16 Jun 2022 16:34:22 +1000 Subject: [PATCH 13/13] test