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

notebook outputs api #115817

Merged
merged 55 commits into from Feb 11, 2021
Merged

notebook outputs api #115817

merged 55 commits into from Feb 11, 2021

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Feb 5, 2021

TODOs:

  • Unify exthost and main thread output types
    • First step is reusing rich output for rendering always. Text and Error output now have special mimetypes. We will only have rich renderers (some defined in the core, some are contributed by extensions).
      • Legacy CellOutput and the new NotebookCellOutput are converted to IOutputDtoWithId before transferred to main thread.
      • CellOutputViewModel holds reference to the raw output and also the transformed one (stream to { 'application/x-notebook-stream': '' ) .
      • Renderers will only know about ITransformedDisplayOutputDto, which has outputId and [mimeType: string]: any.
    • Introduce NotebookCell.outputs2: NotebookCellOutput[]
      • We can probably do all the transform/addOutputId in the extension host, and then in the main thread we work with the rich output only ( { id: string;outputs: NotebookCellOutputItem[]; })
      • When we broadcast outputs changes to multiple extension host, we will need to create NotebookCellOutput objects in extension host.
    • Drop NotebookCell.outputs: ICellOutput[]
  • Drop direct edit of metadata and outputs
    • Cell outputs
    • Cell metadata
    • Cell language
    • Document metadata
  • Replace cell output data (mime type)
  • Append cell output data (mime type)
  • Adopt changes in diff editor
  • Test
  • Make sure all integration tests are enabled

@jrieken jrieken mentioned this pull request Feb 5, 2021
3 tasks
@jrieken jrieken marked this pull request as ready for review February 9, 2021 09:21
@jrieken jrieken added this to the February 2021 milestone Feb 9, 2021
@DonJayamanne
Copy link
Contributor

@jrieken @roblourens
The current API might need some tweeking.

export class NotebookCellOutputItem {
    readonly metadata?: Record<string, string | number | boolean>;
}

The metadata only supports a flat keyvalue pair. However We don't control the metadata returned by Jupyter and it could (we don't know) contain complex data.
Please can we support a true dictionary (with support for nested items such as Record<string, any>)
As mentioned, we don't really control what metadata is returned & stored in these by Jupyter.

@jrieken jrieken merged commit 0d7c8ec into master Feb 11, 2021
@jrieken jrieken deleted the notebook/outputs branch February 11, 2021 15:38
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants