🤩 Inline expressions #92

rowanc1 · 2023-02-20T16:07:30Z

This PR add the ability to have inline expressions, and is largely based off work by @agoose77 in jupyterlab-imarkdown, see #72.

The bulk of this is working, but we should add the textRendererFactory to improve inline styles. The mime_bundle is stored in options and can be re-rendered without the kernel, which is important for other applications like MyST Websites.

The inline expressions work, can be both with simple evaluations, inline widgets, or spark-lines using matplotlib.

github-actions · 2023-02-20T16:07:41Z

👈 Launch a Binder on branch executablebooks/jupyterlab-myst/feat/inline-expressions

rowanc1

Annotated review of what is going on!

rowanc1 · 2023-02-20T16:13:28Z

src/MySTMarkdownCell.tsx

+    super(options);
+
+    this.__rendermime = options.rendermime.clone();
+    // this.__rendermime.addFactory(textRendererFactory);


We should add or improve the inline rendering.

@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!

yes, so before defering to the standard rendermime renderers, take a different rendering route based on mimetype and skip the defaults altogether, for text etc...

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 style attributes on widgets themselves. https://ipywidgets.readthedocs.io/en/stable/examples/Widget%20Styling.html#The-style-attribute

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 text/markdown outputs get rendered (as MyST?)

The downside of this is now other JupyterLab extensions can't override e.g. text/plain rendering, but it's not necessarily a problem; we just need to define what should be the behaviour here. My take is that we special-case text/plain, errors, LaTeX, and Markdown (maybe only if it's MyST markdown), and the rest goes through the rendermime interface.

Nice, agree with all of that! Will pick it up in a future PR!

rowanc1 · 2023-02-20T16:13:57Z

src/MySTMarkdownCell.tsx


 export class MySTMarkdownCell
  extends MarkdownCell
  implements IMySTMarkdownCell
 {
+  private _doneRendering = new PromiseDelegate<void>();


There is now a promise for when the initial mdast parse is complete for the cell, then this can trigger the inline execution.

rowanc1 · 2023-02-20T16:14:26Z

src/MySTMarkdownCell.tsx

+    const expressions = selectAll('inlineExpression', mdast);
+    return expressions.map(node => (node as any).value);


Gather all of the expressions to be evaluated. This is just the body of the role.

So much nicer.

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.

Assuming that we're referring to inlineExpression, I'd probably prefer e.g. expr or expression?

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!

rowanc1 · 2023-02-20T16:15:02Z

src/MySTMarkdownCell.tsx

+        <CellMetadataProvider
+          metadata={metadata}
+          trusted={this.model.trusted}
+          rendermime={this.__rendermime}
+        >


We add a cell metadata provider to give access to these values in the inline renderers.

rowanc1 · 2023-02-20T16:15:32Z

src/actions.ts

+  const content: KernelMessage.IExecuteRequestMsg['content'] = {
+    code: '',
+    user_expressions: userExpressions
+  };


user_expressions are a Jupyter thing, that map is provided and executed.

rowanc1 · 2023-02-20T16:17:10Z

src/index.ts

+const executor: JupyterFrontEndPlugin<void> = {
+  id: 'jupyterlab-myst:executor',


We add another plugin to allow markdown cells to be executed.

so 2 plugins are installed now? and the both work independently?
i.e. the other plugin takes care of Markdown cell rendering whilst this one takes care of execution post render? or are we also rendering here when there is an inline expression?

A single JupyterLab extension can install multiple plugins. Here we provide the "execute inline expressions and store results" using this :executor plugin. The renderering is still exclusively performed by the MySTMarkdownCell work.

It does mean that you can turn parts of it off at a time, which is quite nice!

rowanc1 · 2023-02-20T16:18:49Z

src/inlineExpression.tsx

+  }
+}
+
+export function InlineRenderer({ value }: { value?: string }): JSX.Element {


This react component gets attached in the inlineExpression for mdast. We have to be careful here to create the rendermime one time, and then update it when the expression result changes. There is also ways to dispose of the renderer when this component gets removed.

@stevejpurves you might have comments here from your experience working with react and thebe?!

this is ok especially if it works. I have had issues with render passes previously before a ref was resolved and solved that by using "callback refs" to only attach to the dom once the ref was available. I think that is more of an issue in components with conditional rendering -- which this one has i.e. when there is no expressionResult but if this pattern works and we reliably get the output attached to the dom, then no need to change it.

rowanc1 · 2023-02-20T16:19:04Z

src/myst.ts

+const evalRole: RoleSpec = {
+  name: 'eval',
+  body: {
+    type: ParseTypesEnum.string,
+    required: true
+  },
+  run(data: RoleData): GenericNode[] {
+    const value = data.body as string;
+    return [{ type: 'inlineExpression', value }];
+  }
+};


This is the role spec!

rowanc1 · 2023-02-20T16:19:56Z

src/myst.ts

@@ -125,4 +147,6 @@ export function parseContent(notebook: StaticNotebook): void {
    cell.myst.post = mdast.children[index];
    cell.mystRender();
  });
+
+  return Promise.all(promises).then(() => undefined);


This content now waits on the render, and then returns so that execution can happen. We must parse the cells BEFORE we execute.

rowanc1 · 2023-02-20T16:20:11Z

src/renderers.tsx

+  inlineExpression: (node, children) => {
+    return <InlineRenderer value={node.value} />;
+  }


This is the new myst renderer for the node.

agoose77 · 2023-02-20T19:36:07Z

src/MySTMarkdownCell.tsx

-
-    this.__rendermime = options.rendermime.clone();
+    // Note we cannot clone this, and it must be the parents (the notebooks)
+    this.__rendermime = parent.rendermime;


Ah! I didn't get this far in my reading, nice spot.

I am not totally sure why this happens, I feel like the clone should work!?

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 jupyterlab-imarkdown in the very early work and never noticed it. We want to use the same rendermime registry as passed in:

Suggested change

this.__rendermime = parent.rendermime;

this.__rendermime = options.rendermime;

src/MySTMarkdownCell.tsx

agoose77 · 2023-02-20T21:04:54Z

src/MySTMarkdownCell.tsx

+    const expressions = selectAll('inlineExpression', mdast);
+    return expressions.map(node => (node as any).value);


So much nicer.

README.md

Co-authored-by: Angus Hollands <goosey15@gmail.com>

rowanc1 · 2023-02-20T22:17:02Z

Was going to share the binder link -- but we don't have matplotlib or numpy on it!

Errors are pretty great though!

rowanc1 · 2023-02-20T23:45:30Z

Binder is now working well!!

stevejpurves · 2023-02-21T11:25:28Z

src/inlineExpression.tsx

+
+export function InlineRenderer({ value }: { value?: string }): JSX.Element {
+  const ref = useRef<HTMLDivElement>(null);
+  const { metadata, trusted, rendermime } = useCellMetadata();


I find the hook name confusing, as it makes me think e are reading the rendermime from metadata when its an runtime object coming from the notebook/tracker

I have changed this to a cellProvider, which ... provides the cell! :)

stevejpurves · 2023-02-21T11:31:19Z

src/renderers.tsx

+import { NodeRenderer } from '@myst-theme/providers';
+import { InlineRenderer } from './inlineExpression';
+
+export const renderers: Record<string, NodeRenderer> = {


there are a lot of renderers flying around! these are mystRenderers specifically here though it seems

Yeah, I think there is a wider clean up for the whole repo as we start to get a bit more complicated roles and parsing structure in this repo.

I am inclined to kick this down the road though!

rowanc1 · 2023-02-21T21:05:04Z

Ok, @agoose77 and @stevejpurves. I think this PR is ready to come in!

We will have another wuick PR on styles later today, and then aim to release as 1.1.0? or maybe 1.0.2? Feels more minor than patch?

agoose77 · 2023-02-21T21:13:04Z

Minor, agreed!

rowanc1 · 2023-02-21T21:14:08Z

In it goes!!

rowanc1 requested review from agoose77 and stevejpurves February 20, 2023 16:07

Base automatically changed from feat/myst-styles to main February 20, 2023 16:10

🤩 Working prototype of inline expressions

6215bbd

rowanc1 force-pushed the feat/inline-expressions branch from cbfc114 to 6215bbd Compare February 20, 2023 16:10

rowanc1 added the enhancement New feature or request label Feb 20, 2023

rowanc1 commented Feb 20, 2023

View reviewed changes

🐛 Typo

b83e308

This comment was marked as outdated.

Sign in to view

🐛 Use the notebook rendermime

5e33162

agoose77 reviewed Feb 20, 2023

View reviewed changes

rowanc1 added 2 commits February 20, 2023 14:39

👩🏻‍🎨 Update a few styles for inline execution

ffa600e

📖 Update documentation, add interactive example

fa73593

rowanc1 marked this pull request as ready for review February 20, 2023 21:55

🙅🏼‍♀️ Reject promise if rendering fails

5037a5d

agoose77 reviewed Feb 20, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

📖 Update README.md

36fda0b

Co-authored-by: Angus Hollands <goosey15@gmail.com>

📘 Update binder dependencies for demos

fbfc0b6

stevejpurves reviewed Feb 21, 2023

View reviewed changes

🌀 Refactor and simplify providers

13c86d1

rowanc1 merged commit 270dc61 into main Feb 21, 2023

rowanc1 deleted the feat/inline-expressions branch February 21, 2023 21:14

		const expressions = selectAll('inlineExpression', mdast);
		return expressions.map(node => (node as any).value);

		const executor: JupyterFrontEndPlugin<void> = {
		id: 'jupyterlab-myst:executor',

	this.__rendermime = parent.rendermime;
	this.__rendermime = options.rendermime;

🤩 Inline expressions #92

🤩 Inline expressions #92

Conversation

rowanc1 commented Feb 20, 2023 • edited Loading

github-actions bot commented Feb 20, 2023

rowanc1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoose77 Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevejpurves Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanc1 commented Feb 20, 2023

rowanc1 commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanc1 commented Feb 21, 2023

agoose77 commented Feb 21, 2023

rowanc1 commented Feb 21, 2023

rowanc1 commented Feb 20, 2023 •

edited

Loading

agoose77 Feb 21, 2023 •

edited

Loading

stevejpurves Feb 21, 2023 •

edited

Loading