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

Better names & types for OutputItems like 'application/x.notebook.error-traceback' #120063

Closed
DonJayamanne opened this issue Mar 28, 2021 · 8 comments
Assignees
Labels
api feature-request Request for new features or functionality notebook verified Verification succeeded
Milestone

Comments

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Mar 28, 2021

Today when we create an output item for application/x.notebook.error-traceback there is no type information for the data provided for these output items.
As a new extension user I dont know what's supported for these mime types.
As I was involved in the design of some of the API I now the that this is expected for application/x.notebook.error-traceback:

{
    ename: string;  
	evalue: string;
	traceback: string;
}

When working on a new extension, i need to go into the Jupyter extension for VS Code to see what' supported.

Similarly whats the structure of the data passed into the following output items?:

  • application/x.notebook.stdout
  • application/x.notebook.stderr
  • application/x.notebook.stream

Would it be better to have separate output items, e.g. NotebookCellErrorOutputItem, NotebookCellStreamOutputItem

export class NotebookCellErrorOutputItem {
	constructur(value: string, error: {message?: string; name: string; stack?: string})
}

export class NotebookCellStreamOutputItem {
	constructur(value: string, name?: 'stdout' : 'stderr')
}

Finally, not sure why properties are named ename/evalue/traceback.
These names seem too tighly coupled with the Jupyter protocol. I'm ok with this, however when creating a kernel for non-jupyter or other, these don't seem right and doesn't feel like a good set of names in for the error object nodejs world.

FYI - Please consider this feedback from someone new to Notebooks (missing types/information/docs), or someone working on non-Jupyter notebooks (ename, evalue, etc seem like alient terms).

@jrieken /cc

@jrieken jrieken added api feature-request Request for new features or functionality notebook labels Mar 29, 2021
@jrieken jrieken added this to the Backlog milestone Mar 29, 2021
@jrieken
Copy link
Member

jrieken commented Mar 29, 2021

My plan is to have factory functions for this. Some rough sketches are here:

// add factory functions for common mime types
// static textplain(value:string): NotebookCellOutputItem;
// static errortrace(value:any): NotebookCellOutputItem;

@jrieken jrieken modified the milestones: Backlog, May 2021 Apr 21, 2021
@jrieken
Copy link
Member

jrieken commented Apr 21, 2021

Finally, not sure why properties are named ename/evalue/traceback.

Yeah, I believe that's 100% jupyter leaking through. I think better type would be a normal Error object. It has the same properties (name, message, stack) and is typical for JavaScript

@firelizzard18
Copy link

@jrieken Until this is solved, would you add comments somewhere specifying what values are expected for the special-cased mime types?

// code specific mime types
// application/x.notebook.error-traceback
// application/x.notebook.stdout
// application/x.notebook.stderr
// application/x.notebook.stream
export class NotebookCellOutputItem {

Maybe:

    // application/x.notebook.error-traceback: NotebookErrorTraceback
    // application/x.notebook.stdout: string
    // application/x.notebook.stdout: string
    // application/x.notebook.stderr: string
    // application/x.notebook.stream: ???
    // text/json, application/json: any (will be stringified)

    // ...

    interface NotebookErrorTraceback {
        ename: string;
        evalue: string;
        traceback: string;
    }

In general { mime: string, value: any } seems weird to me, as in almost any other context it would be { mime: string, value: string | byte[] }. I see the utility of allowing structured data for things like x.notebook.error-traceback or x.notebook.diagnostic. But in general, I expect to provide bytes or a string. This is especially weird for JSON, as I expect to provide raw JSON as a string or bytes, which does not do what I expect (#119936).

@jrieken
Copy link
Member

jrieken commented May 12, 2021

But in general, I expect to provide bytes or a string. This is especially weird for JSON, as I expect to provide raw JSON as a string or bytes, which does not do what I expect (#119936).

Yeah, I really like that idea and I think we should make this change.

fyi @DonJayamanne @colombod the type NotebookCellOutputItem#value is very likely changing to string | Unit8Array. At first we can make this an API-only change but sooner or later renders (ours and others) should follow

@RandomFractals
Copy link

yes. those two types for value passing to renderers would be good for starters. my comments on that and mime types here: #123884 (comment)

@jrieken
Copy link
Member

jrieken commented May 26, 2021

closing - NotebookCellOutputItem has now factory function for built-in types, esp for stdout/stderr and JS errors

@jrieken jrieken closed this as completed May 26, 2021
@jrieken jrieken added the verification-needed Verification of issue is requested label Jun 2, 2021
@jrieken
Copy link
Member

jrieken commented Jun 2, 2021

Verify that NotebookCellOutputItem has factory functions for text, json etc

@aeschli aeschli added verified Verification succeeded and removed verification-needed Verification of issue is requested labels Jun 3, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality notebook verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants
@RandomFractals @firelizzard18 @jrieken @DonJayamanne @aeschli and others