-
Notifications
You must be signed in to change notification settings - Fork 18
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
🤩 Inline expressions #92
Changes from 8 commits
6215bbd
b83e308
5e33162
ffa600e
fa73593
5037a5d
36fda0b
fbfc0b6
13c86d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,4 +18,6 @@ dependencies: | |
- pip | ||
- wheel | ||
# additional packages for demos | ||
# - ipywidgets | ||
- numpy | ||
- matplotlib | ||
- ipywidgets |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { IRenderMimeRegistry } from '@jupyterlab/rendermime'; | ||
import React, { createContext, useContext } from 'react'; | ||
import { IUserExpressionMetadata } from './metadata'; | ||
|
||
type MetadataState = { | ||
metadata?: IUserExpressionMetadata[]; | ||
trusted?: boolean; | ||
rendermime?: IRenderMimeRegistry; | ||
}; | ||
|
||
const MetadataContext = createContext<MetadataState | undefined>(undefined); | ||
|
||
// Create a provider for components to consume and subscribe to changes | ||
export function CellMetadataProvider({ | ||
metadata, | ||
trusted, | ||
rendermime, | ||
children | ||
}: { | ||
metadata: IUserExpressionMetadata[] | undefined; | ||
trusted: boolean; | ||
rendermime: IRenderMimeRegistry; | ||
children: React.ReactNode; | ||
}): JSX.Element { | ||
return ( | ||
<MetadataContext.Provider value={{ metadata, trusted, rendermime }}> | ||
{children} | ||
</MetadataContext.Provider> | ||
); | ||
} | ||
|
||
export function useCellMetadata(): MetadataState { | ||
const state = useContext(MetadataContext) ?? {}; | ||
return state; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,11 +17,28 @@ import { useParse } from 'myst-to-react'; | |||||
import { parseContent } from './myst'; | ||||||
import { IMySTMarkdownCell } from './types'; | ||||||
import { linkFactory } from './links'; | ||||||
import { selectAll } from 'unist-util-select'; | ||||||
|
||||||
import { PromiseDelegate } from '@lumino/coreutils'; | ||||||
import { metadataSection, IUserExpressionMetadata } from './metadata'; | ||||||
import { CellMetadataProvider } from './CellMetadataProvider'; | ||||||
import { IRenderMimeRegistry } from '@jupyterlab/rendermime'; | ||||||
|
||||||
export class MySTMarkdownCell | ||||||
extends MarkdownCell | ||||||
implements IMySTMarkdownCell | ||||||
{ | ||||||
private _doneRendering = new PromiseDelegate<void>(); | ||||||
|
||||||
private __rendermime: IRenderMimeRegistry; | ||||||
|
||||||
constructor(options: MarkdownCell.IOptions, parent: StaticNotebook) { | ||||||
super(options); | ||||||
// Note we cannot clone this, and it must be the parents (the notebooks) | ||||||
this.__rendermime = parent.rendermime; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! I didn't get this far in my reading, nice spot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not totally sure why this happens, I feel like the clone should work!? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't break anything on my local test build, but I don't think we should be cloning in any case. I think I probably did that in
Suggested change
|
||||||
// this.__rendermime.addFactory(textRendererFactory); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add or improve the inline rendering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @agoose77, looking more at the text renderer -- I think we might actually just want to do this in react even? I think we are going to have to have a bunch of custom renderers, that are specific to the inline execution, but that can probably all come later! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, so before defering to the standard rendermime renderers, take a different rendering route based on For other things like widgets, we can maybe look at redefining/overriding the styles in CSS? we'd have to figure out how that plays with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think rendering plain-text in MyST might be reasonable. There's a big question over MyST integration in JLab as to how much we take over e.g. Markdown rendering currently goes through the default marked implementation. Perhaps we want to recognise a particular flavour of Markdown, so that users can opt-in to displaying rich text via outputs. Also, I wonder what myst-nb does here — how do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The downside of this is now other JupyterLab extensions can't override e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, agree with all of that! Will pick it up in a future PR! |
||||||
} | ||||||
|
||||||
myst: { | ||||||
pre?: GenericParent; | ||||||
post?: GenericParent; | ||||||
|
@@ -34,13 +51,34 @@ export class MySTMarkdownCell | |||||
const node = document.createElement('div'); | ||||||
this.myst = { node }; | ||||||
} | ||||||
|
||||||
this._doneRendering = new PromiseDelegate<void>(); | ||||||
const notebook = this.parent as StaticNotebook; | ||||||
this.myst.pre = undefined; | ||||||
parseContent(notebook); | ||||||
const parseComplete = parseContent(notebook); | ||||||
const widget = new Widget({ node: this.myst.node }); | ||||||
widget.addClass('myst'); | ||||||
this.addClass('jp-MySTMarkdownCell'); | ||||||
this.inputArea.renderInput(widget); | ||||||
if (parseComplete) { | ||||||
parseComplete.then(() => this._doneRendering.resolve()); | ||||||
} else { | ||||||
// Something went wrong, reject the rendering promise | ||||||
this._doneRendering.reject('Unknown error with parsing MyST Markdown.'); | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* Whether the Markdown renderer has finished rendering. | ||||||
*/ | ||||||
get doneRendering(): Promise<void> { | ||||||
return this._doneRendering.promise; | ||||||
} | ||||||
|
||||||
get expressions(): string[] { | ||||||
const { post: mdast } = this.myst ?? {}; | ||||||
const expressions = selectAll('inlineExpression', mdast); | ||||||
return expressions.map(node => (node as any).value); | ||||||
Comment on lines
+69
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gather all of the expressions to be evaluated. This is just the body of the role. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So much nicer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any thoughts on what this should be called on the spec side for the actual node? We could also have it code with an executable flag on it? That is closer to what JATS does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming that we're referring to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess there is some precedence in the spec at the moment. https://myst-tools.org/docs/spec/commonmark#inline-code I am inclined to leave it as is in this PR, and revisit when we start to standardize this in the spec?! My thinking is that we may want a difference in inline vs block-level expressions. e.g. an interactive figure inline maybe has a different renderer than the block level renderer. That could mean different mimebundles in the future. These are all ephemeral at the moment, so it shouldn't matter if we want to change it! |
||||||
} | ||||||
|
||||||
mystRender(): void { | ||||||
|
@@ -56,19 +94,30 @@ export class MySTMarkdownCell | |||||
const { references, frontmatter } = notebook.myst; | ||||||
|
||||||
const children = useParse(mdast as any, renderers); | ||||||
|
||||||
const metadata = this.model.metadata.get( | ||||||
metadataSection | ||||||
) as IUserExpressionMetadata[]; | ||||||
render( | ||||||
<ThemeProvider | ||||||
theme={Theme.light} | ||||||
Link={linkFactory(notebook)} | ||||||
renderers={renderers} | ||||||
> | ||||||
<TabStateProvider> | ||||||
<ReferencesProvider references={references} frontmatter={frontmatter}> | ||||||
{isFirstCell && <FrontmatterBlock frontmatter={frontmatter} />} | ||||||
{children} | ||||||
</ReferencesProvider> | ||||||
</TabStateProvider> | ||||||
<CellMetadataProvider | ||||||
metadata={metadata} | ||||||
trusted={this.model.trusted} | ||||||
rendermime={this.__rendermime} | ||||||
> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We add a cell metadata provider to give access to these values in the inline renderers. |
||||||
<TabStateProvider> | ||||||
<ReferencesProvider | ||||||
references={references} | ||||||
frontmatter={frontmatter} | ||||||
> | ||||||
{isFirstCell && <FrontmatterBlock frontmatter={frontmatter} />} | ||||||
{children} | ||||||
</ReferencesProvider> | ||||||
</TabStateProvider> | ||||||
</CellMetadataProvider> | ||||||
</ThemeProvider>, | ||||||
this.myst.node | ||||||
); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
import { ISessionContext } from '@jupyterlab/apputils'; | ||
import { Cell, IMarkdownCellModel } from '@jupyterlab/cells'; | ||
import { KernelMessage } from '@jupyterlab/services'; | ||
import { JSONObject } from '@lumino/coreutils'; | ||
import { IExpressionResult } from './userExpressions'; | ||
import { IUserExpressionMetadata, metadataSection } from './metadata'; | ||
import { | ||
Notebook, | ||
NotebookPanel, | ||
INotebookTracker | ||
} from '@jupyterlab/notebook'; | ||
import { IMySTMarkdownCell } from './types'; | ||
|
||
function isMarkdownCell(cell: Cell): cell is IMySTMarkdownCell { | ||
return cell.model.type === 'markdown'; | ||
} | ||
|
||
/** | ||
* Load user expressions for given XMarkdown cell from kernel. | ||
* Store results in cell attachments. | ||
*/ | ||
export async function executeUserExpressions( | ||
cell: IMySTMarkdownCell, | ||
sessionContext: ISessionContext | ||
): Promise<void> { | ||
// Check we have a kernel | ||
const kernel = sessionContext.session?.kernel; | ||
if (!kernel) { | ||
throw new Error('Session has no kernel.'); | ||
} | ||
|
||
const model = cell.model as IMarkdownCellModel; | ||
const cellId = { cellId: model.id }; | ||
|
||
// Can simplify with `Object.fromEntries` here | ||
// requires ts compiler upgrade! | ||
|
||
// Build ordered map from string index to node | ||
const namedExpressions = new Map( | ||
cell.expressions.map((expr, index) => [`${index}`, expr]) | ||
); | ||
|
||
// Extract expression values | ||
const userExpressions: JSONObject = {}; | ||
namedExpressions.forEach((expr, key) => { | ||
userExpressions[key] = expr; | ||
}); | ||
|
||
// Populate request data | ||
const content: KernelMessage.IExecuteRequestMsg['content'] = { | ||
code: '', | ||
user_expressions: userExpressions | ||
}; | ||
Comment on lines
+50
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. user_expressions are a Jupyter thing, that map is provided and executed. |
||
|
||
// Perform request | ||
console.debug('Performing kernel request', content); | ||
const future = kernel.requestExecute(content, false, { | ||
...model.metadata.toJSON(), | ||
...cellId | ||
}); | ||
|
||
// Set response handler | ||
future.onReply = (msg: KernelMessage.IExecuteReplyMsg) => { | ||
console.debug('Handling kernel response', msg); | ||
// Only work with `ok` results | ||
const content = msg.content; | ||
if (content.status !== 'ok') { | ||
console.error('Kernel response was not OK', msg); | ||
return; | ||
} | ||
|
||
console.debug('Clear existing metadata'); | ||
// Clear metadata if present | ||
cell.model.metadata.delete(metadataSection); | ||
|
||
// Store results as metadata | ||
const expressions: IUserExpressionMetadata[] = []; | ||
for (const key in content.user_expressions) { | ||
const expr = namedExpressions.get(key); | ||
|
||
if (expr === undefined) { | ||
console.error( | ||
"namedExpressions doesn't have key. This should never happen" | ||
); | ||
continue; | ||
} | ||
const result = content.user_expressions[key] as IExpressionResult; | ||
|
||
const expressionMetadata: IUserExpressionMetadata = { | ||
expression: expr, | ||
result: result | ||
}; | ||
Comment on lines
+89
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We then pick up the results and get them into the metadata. |
||
expressions.push(expressionMetadata); | ||
|
||
console.debug(`Saving ${expr} to cell attachments`, expressionMetadata); | ||
} | ||
|
||
// Update cell metadata | ||
cell.model.metadata.set(metadataSection, expressions); | ||
// Rerender the cell with React | ||
console.debug('Render cell after the metadata is added'); | ||
cell.mystRender(); | ||
}; | ||
|
||
await future.done; | ||
} | ||
|
||
export function notebookExecuted( | ||
notebook: Notebook, | ||
cell: Cell, | ||
tracker: INotebookTracker | ||
): void { | ||
Comment on lines
+108
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function adds execution functionality to the kernel. |
||
// Find the Notebook panel | ||
const panel = tracker.find((w: NotebookPanel) => { | ||
return w.content === notebook; | ||
}); | ||
// Retrieve the kernel context | ||
const ctx = panel?.sessionContext; | ||
if (ctx === undefined) { | ||
return; | ||
} | ||
// Load the user expressions for the given cell. | ||
if (!isMarkdownCell(cell)) { | ||
return; | ||
} | ||
console.debug( | ||
`Markdown cell ${cell.model.id} was executed, waiting for render to complete ...` | ||
); | ||
|
||
cell.doneRendering.then(() => executeUserExpressions(cell, ctx)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,11 @@ import { | |
INotebookTracker, | ||
INotebookWidgetFactory, | ||
NotebookPanel, | ||
NotebookWidgetFactory | ||
NotebookWidgetFactory, | ||
NotebookActions, | ||
Notebook | ||
} from '@jupyterlab/notebook'; | ||
import { Cell } from '@jupyterlab/cells'; | ||
import { MySTContentFactory } from './MySTContentFactory'; | ||
|
||
import { ISessionContextDialogs } from '@jupyterlab/apputils'; | ||
|
@@ -20,6 +23,7 @@ import { ITranslator } from '@jupyterlab/translation'; | |
import { LabIcon } from '@jupyterlab/ui-components'; | ||
|
||
import mystIconSvg from '../style/mystlogo.svg'; | ||
import { notebookExecuted } from './actions'; | ||
|
||
const mystIcon = new LabIcon({ | ||
name: 'myst-notebook-extension:mystIcon', | ||
|
@@ -117,4 +121,25 @@ const legacyPlugin: JupyterFrontEndPlugin<void> = { | |
} | ||
}; | ||
|
||
export default [plugin, legacyPlugin]; | ||
/** | ||
* The notebook cell executor. | ||
*/ | ||
const executor: JupyterFrontEndPlugin<void> = { | ||
id: 'jupyterlab-myst:executor', | ||
Comment on lines
+127
to
+128
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We add another plugin to allow markdown cells to be executed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so 2 plugins are installed now? and the both work independently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A single JupyterLab extension can install multiple plugins. Here we provide the "execute inline expressions and store results" using this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does mean that you can turn parts of it off at a time, which is quite nice! |
||
requires: [INotebookTracker], | ||
autoStart: true, | ||
activate: (app: JupyterFrontEnd, tracker: INotebookTracker) => { | ||
console.log('Using jupyterlab-myst:executor'); | ||
|
||
NotebookActions.executed.connect( | ||
(sender: any, value: { notebook: Notebook; cell: Cell }) => { | ||
const { notebook, cell } = value; | ||
notebookExecuted(notebook, cell, tracker); | ||
} | ||
); | ||
|
||
return; | ||
} | ||
}; | ||
|
||
export default [plugin, legacyPlugin, executor]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now a promise for when the initial mdast parse is complete for the cell, then this can trigger the inline execution.