FEAT: allow any value for expr

[agoose77]

Currently, the eval role and directives only allow for C-like identifiers.

Proposal in this PR

Remove the restriction on expression values in eval contexts.

Advantages of this restriction (to be removed by this PR)

• Users are discouraged from writing unreadably complex code that breaks the narrative flow.
• Users are discouraged from writing code that can have side-effects (as there are far fewer rich-display hook implementations than there are user functions that modify globals).
  • Non-"pure" functions would lead to different results when executing the notebook in environments that do not support expression execution, e.g. jupyter-cache or Jupyter Lab¹.

Disadvantages of this restriction (to be resolved by this PR)

• Every language has a different notion of "identifier". We're exclusively permitting C-like identifiers only. The current REGEX actually doesn't support certain valid Python identifiers that include unicode (e.g. ℘ or gänseblümchen. Whilst some of these might be a little far-fetched, I find myself regularly using τ for Mathematical expressions.
• Expressions have explanatory value. An identifier requires the author to jump to the definition of the identifier, which breaks the flow of reading the document source.
• Libraries without rich-display hooks that use display methods (e.g. plot.display() for k3d) need to use a capturing context object + re-reraise the MIME bundle (ugly)
• Expressions are more concise than assignments to name
  e.g.

  This is the sum {eval}`x.sum()`

  vs

  ```{code-cell}
  x_sum = x.sum()
  ```
  This is the sum {eval}`x_sum`

• Other implementations of in-line expressions support this e.g. RMarkdown

Given that notebooks can already have side effects, I fairly strongly disagree with trying to fix this at the MyST level. I feel it's better for users to burn their own fingers than for us to impose restrictions upon them.

Maybe, if someone can find use for this kind of restriction, we can make it an opt-in configuration where the user can turn on REGEX matching, and specify the REGEX?

EDIT: Expounded pros and cons

Footnotes

1. jupyterlab-myst does support inline expressions. ↩

[codecov]

Codecov Report

Patch coverage: 66.66% and no project coverage change.

Comparison is base (1aee72d) 81.47% compared to head (56adeb9) 81.47%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #429   +/-   ##
=======================================
  Coverage   81.47%   81.47%           
=======================================
  Files          29       29           
  Lines        2618     2618           
=======================================
  Hits         2133     2133           
  Misses        485      485           

Flag	Coverage Δ
pytests	81.47% <66.66%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
myst_nb/core/execute/base.py	97.50% <ø> (-0.07%)	⬇️
myst_nb/ext/eval/__init__.py	74.16% <0.00%> (ø)
myst_nb/core/config.py	78.22% <100.00%> (+0.17%)	⬆️
myst_nb/core/execute/inline.py	79.81% <100.00%> (+0.18%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[choldgraf]

Sorry for the slow response, I didn't catch this one. FWIW, if this becomes a complex discussion we might want to have it in an issue so that there is more visibility to discussion that isn't attached to the implementation.

I think this is an important option to consider. As you note, it's important to consider both the "pros", the "cons", and the "assumptions" we're bringing. Some quick thoughts on each from me:

I assume most users are going to think of this feature very similarly to RMarkdown and knitr, which I believe do allow for arbitrary expressions (e.g. here's a tutorial that shows this). Most users also do not have a mental model of "returning a variable vs. defining a variable". They just think of it all as "code that gets executed".

To that extent, if we can make the inline code behave the same as code blocks, that would match the assumptions users are bringing, and would also reduce the amount of writing needed for certain patterns (like the pattern @agoose77 shared above).

The main downside that I can think of is that this might have "unintended consequences" for users if they're modifying state in those expressions. However I do think it's a reasonable argument that Jupyter is already rife with this anyway, and it doesn't seem too hard to teach users that "the code you run in-line might change other things in your page". (one example where state is confusing an RMarkdown user interactively).

Are there any other major downsides? (e.g., in implementation, complexity, edge-cases and errors, etc?)

I think @stefanv and @matthew-brett mentioned the value of in-line expressions as well, and suggested that x.sum() was the kind of thing they'd naturally want to do (not be force to assign that to a variable first).

[agoose77]

I've updated my original post to condense the pros/cons as I see them, including those that you've mentioned @choldgraf

[matthew-brett]

Right - consider text like this:

x has a mean of `np.mean(x)` and standard deviation of `np.std(x)`.

It makes for more verbose code, having to make an otherwise unnecessary set of variables to insert here. And yes, I agree, it's perfectly reasonable to expect the user to take the same care about side effects that they do in code cells.

If it's important, I bet you could get a good idea of how often people use variables vs expressions in RMarkdown, by analyzing R notebooks. And I bet that a large proportion of RMarkdown inline expressions are expressions rather than variables.

[choldgraf]

@agoose77 just a note that I updated your top comment to try and make it clear what this PR was proposing and what it wasn't, since your terminology made me think that "this restriction" was being applied to this PR, not removed by it :-)

[chrisjsewell]

Well I do want to be clear here, that what is being proposed is to ditch the whole concept of a myst markdown notebook having a 1:1 correspondence with a Jupyterbook, e.g. if you execute the notebook top to bottom in JupyterLab you may well now get a different outcome than executing it via myst-nb.
That's the point/goal of the constraint, that eval does not "alter the state" of the kernel, only retrieve state from it.
It would be ideal if there was a more concrete API for this in Jupyter, but unfortunately I don't believe this to be the case.

Having a 1:1 correspondence was a clear design constraint when creating myst-nb.
I would note this constraint was not made by me, it was imposed by others in EBP, including @choldgraf.
Not that I'm against, but just pointing out the background.

If we go this direction, then this is no different than allowing code-cell to be nested.
This has pros, but the cons include that (a) you can no longer convert between myst notebooks and jupyter notebooks in any facile/meaningful way, (b) you can no longer use jupyter-cache

[agoose77]

Right, this is a valid concern (and I've updated the table).

As I see it, what we're actually discussing here is not about side-effects per-se, but rather that we right now do not support expressions in jupyter-cache (and native Lab). Rather than implementing that support, we have this restriction.

I can understand this restriction, after all - you've done most (all?) of the implementation work on expressions and it's not a small task. So, keeping things constrained makes for a deliverable result. But, longer term, I think we would benefit from adding that support.

Arguably, even just limiting users to Python expressions (and not Walrus assignments) would be nearly as safe, but with far fewer restrictions. Of course, this means we have a language-specific solution, which I'm not wholly in favour of, even if I think we could do it with a per-lang table.

Practically speaking, I think there are two separate issues here;

• Adding eval support to jupyter-cache.
• Aligning authoring experiences with rendered results.

jupyterlab-myst already handles eval contexts, which means that if the users install the package, they will be unlikely to run into this authorship problem. There's a lot more we can do here in terms of standards and QoL (the current implementation is barebones), but it's a good starting point.

The problem of caching is already a significant one. Any notebook using inline expressions can no longer use the caching system, and I found this to be very restrictive whilst writing my thesis. Even if we don't address this PR, I think we will want to solve the caching problem. As we discussed on Slack, I think we can do this in a generalised manner through the use of a MyST plugin to jupyter-cache.

EDIT: Finally, I think this also falls into the category of "trust your users, but maybe warn them about footguns".

[chrisjsewell]

Right, this is a valid concern (and I've updated the table).
So, keeping things constrained makes for a deliverable result.

Exactly, I'm not dismissing this, but I want to be very clearly why the constraint is in there in the first place, and what the philosophical implications of removing it are

I think we can do this in a generalised manner through the use of a MyST plugin to jupyter-cache.

Personally I think you would just need to create a different package to jupyter-cache, that acts different,
since the key design philosophy of that package, is that you can cache a notebook by its "code" content.
I feel this change is out of scope for it.

[agoose77]

Preface the below with the disclaimer that this is just design discussion rather than a concrete plan of action.

To my mind, jupyter-cache comprises a few orthogonal components:

1. Hashing of the execution-context of the notebook (i.e. the ordered code source). Naively in Python this is just something like

   notebook_hash = hash(tuple([c.source for c in nb.cells if c.cell_type == "code"]))

2. Execution of the notebook and serialisation of the execution results
3. Cache-aware lookup interface to satisfy get_execution_results(notebook)

I feel that it is in scope to extend the implementation of components (1) and (2) to incorporate Markdown expressions. To be pedantic, it's already trivially possible to fool the cache into thinking the cache state is valid. Any Python cell that inspects the current saved notebook will do this. Similarly, anything that references random or time-sensitive data. Right now I suspect we'd all argue that this is simply "wrong usage" as it violates the assumption of the cache-system (that only the ordering and contents of the code cells affects the final cache result)

Nevertheless, I think we need to consider this for jupyter-cache for several reasons:

• I can't find any other execution caching libraries for notebooks (so, we'd likely be forking jupyter-cache)
• There are other ways in which a developer might wish to extend the capabilities of the caching mechanism.

Being able to standardise the execution interface of a cached notebook whilst supporting plugins would be immensely useful. I can think of other use cases:

• Parameterised notebooks that depend upon external files can have a parameter-aware cache invalidator.
• Time-sensitive notebooks can have an age-aware cache invalidator.

It might be that we need to restructure jupyter-cache significantly to do this. I'm mostly concerned with the very-high-level jupyter cache API rather than the lower level "how we do this right now" problem. I don't think it would be too tricky to do this for Markdown expressions. I'll come up with a rough API design and maybe we (EBP + @agoose77 + whomever) can bikeshed it?

[choldgraf]

I agree with @chrisjsewell's concerns above - those are definitely extra cons to consider. A few quick thoughts:

if you execute the notebook top to bottom in JupyterLab you may well now get a different outcome than executing it via myst-nb.

It looks like RStudio has some of the same confusion (e.g., this SO post is from a person who saw one thing in RStudio and would get another when things are knit with knitr).

I'd hope that we can document well-enough to let users know how the two are different, and what behavior to expect. We could include documentation like the following here or in Jupyter Book (feel free to take as much of this language as is useful and add it to the PR if you like):

Note
If you evaluate inline code in your content, be careful not to alter the state of your kernel unless you really mean to do so.

For example, the following expressions do not modify state, and are probably fine to execute:

• x (returning a variable that has already been declared)
• x.mean() (calling a method on a variable that has already been declared)
• np.sum(x) (calling a function on a variable that returns a result, and does not modify it)

The following expressions do modify state, and we recommend against using them in in-line code execution:

• x = 2 (assigning a valuable to a variable)
• x.append("foo") (calling a method that modifies the variable in-place)
• sort(x) (calling a function on a variable that modifies the variable in-place)

It would be ideal if there was a more concrete API for this in Jupyter, but unfortunately I don't believe this to be the case.
...
This has pros, but the cons include that (a) you can no longer convert between myst notebooks and jupyter notebooks in any > facile/meaningful way, (b) you can no longer use jupyter-cache

I agree - though getting core APIs added / changed is a lot of work to align the stakeholders needed to make it happen. I think projects like Jupyter Book / JupyterLab are opportunities to prototype and experiment new patterns. We can see how users like them and work out the kinks, and this will hopefully make it easier to gain broader adoption within Jupyter at a later time (e.g., it'll be easier to value in-line execution to the nbclient module if there are users clamoring for this feature in a specific sub-project). In that way we can help push the standard/core functionality evolve.

So I think to implement this, we should add a big "Prototype / Beta" caution at the top of the documentation, and link to an issue or a discussion to gather feedback from people about it. We'd need to tell users that this behavior is not "core Jupyter" and so they should expect some more speed bumps if they want in-line execution. We should also say that the goal is to collect feedback and use-cases so that we can then make more informed decisions about how / whether inline execution should exist within Jupyter's standard behavior.

Something like:

Warning
In-line execution is not supported in the core Jupyter protocols or execution libraries. We are prototyping this feature within MyST-NB and Jupyter Book to collect user feedback and understand the right User Experience. We hope that this will ultimately help bring this functionality into core Jupyter tools, but for now this is not the case.
Here are a few considerations to take:

• MyST Markdown notebooks that use in-line execution will behave differently in Jupyter Book and in Jupyter Lab. This is because Jupyter Lab has no concept of in-line execution. However, you may enable this with the JupyterLab extension jupyterlab-myst`.
• In-line execution cannot be cached by Jupyter Cache at present, so you must re-execute your code on each page build.

Also just a final note that some of these are things that we might be able to get funding to make happen, so it is good to keep this kind of stuff in mind for future grant opportunities etc.

[agoose77]

It's been a while since we looked at this.

The regex-based approach we currently use here is fairly non-general, and limits legitimate useful expressions. Ultimately, the notion of what has side effects is not something that can be resolved statically, at least not without full static analysis tools.

I do see merit in the argument that we don't want users to shoot themselves in the foot. I'm not sure how likely that is; this is an advanced feature, and to discover it you need to read the docs. That said, I'd be willing to move on throwing out the regex entirely; could we implement a switch to opt-out of this check? That way, users can turn off this sanity check if they need the feature. Better would be to also have a per-kernel identifier table so that languages with more dynamic identifier patterns are supported.

Thoughts?

[agoose77]

I think we need to consider this again, now that jupyterlab-myst implements inline expressions. With jupyterlab-myst being the canonical way to write a MyST notebook in JupyterLab, we're in a better place to avoid implementation discrepancies between JB and JupyterLab.

I suspect VSCode is an outlier here, as I have not been following the development. @chrisjsewell do we implement inline-expression support in VSCode? I took a look at their extension API and it looks like it might be a challenge.

In any case, the problem I'm trying to solve is myst-nb inline execution being unusable with certain kernels. I can revise this PR if we want to make it a config setting, rather than a default off!

[chrisjsewell]

With jupyterlab-myst being the canonical way to write a MyST notebook in JupyterLab
I took a look at their extension API and it looks like it might be a challenge.

Until there are clear standards for this, there is no "canonical" way to write MyST in notebooks,
and also myst-nb is not just intended to support jupyterlab, at least that's not what it has been before.

I say this, perhaps prematurely, to put a marker down for issues amounting to: "jupyterlab-myst does XXX, so myst-nb MUST do XXX"
It's not to say that new features from jupyterlab-myst can't be added in myst-nb, just that its not sufficient to just use that as your argument, particularly when then are reasons it may conflict with the design of myst-nb

do we implement inline-expression support in VSCode? I took a look at their extension API and it looks like it might be a challenge.

Even more so for inline variables, I feel it needs to be resolved in some kind of actual standard, i.e. https://discourse.jupyter.org/t/inline-variable-insertion-in-markdown/10525,
if there is ever to be a general solution to this rather than the very bespoke implementation here and in jupyterlab-myst

I can revise this PR if we want to make it a config setting, rather than a default off!

Yep I think that is the compromise, a config setting that is also very clearly documented as to why it is not the default (as I have already explained here)

[agoose77]

Until there are clear standards for this, there is no "canonical" way to write MyST in notebooks,
and also myst-nb is not just intended to support jupyterlab, at least that's not what it has been before.

There's a nuance in what I wrote above; jupyterlab-myst is going to be the canonical way to write MyST notebooks in JupyterLab; it's our own endorsed extension for JupyterLab, and it succeeds jupyterlab-myst < 1.0.

I mentioned this in reply to one of the arguments in favour of being aggressive in defining valid eval syntax:

Well I do want to be clear here, that what is being proposed is to ditch the whole concept of a myst markdown notebook having a 1:1 correspondence with a Jupyterbook, e.g. if you execute the notebook top to bottom in JupyterLab you may well now get a different outcome than executing it via myst-nb.

Although we've had inline expression support for a while, the new iteration of the extension makes it simpler to install and prettier too!

I say this, perhaps prematurely, to put a marker down for issues amounting to: "jupyterlab-myst does XXX, so myst-nb MUST do XXX"

It's worth noting here. My understanding is that we want to collectively standardise the various parts of MyST / JB so that we can collectively avoid divergences.

It's not to say that new features from jupyterlab-myst can't be added in myst-nb, just that its not sufficient to just use that as your argument, particularly when then are reasons it may conflict with the design of myst-nb

Agreed, and note that I'm not making this argument (see above).

Even more so for inline variables, I feel it needs to be resolved in some kind of actual standard, i.e. discourse.jupyter.org/t/inline-variable-insertion-in-markdown/10525,
if there is ever to be a general solution to this rather than the very bespoke implementation here and in jupyterlab-myst

As I understand it, the aspect to standardise here is the output metadata. AFAICR, myst-nb doesn't look at this as it's executing the notebook from top-down anyway. Perhaps that might be useful, though, for the non-executing use-case.
The actual syntax of what's allowed in {eval} could also be "standardised", but I think that's already fairly clear; whatever Jupyter kernels implement (i.e. per-kernel). It's my view that we already have enough problems to solve that dealing with any language spec is a lot of time we can't afford to spend. So, letting the kernels handle this like we do for {code-cell} seems like the simplest path forward.

Yep I think that is the compromise, a config setting that is also very clearly documented as to why it is not the default (as I have already explained here)

We can agree on introducing a config variable. Though I'd prefer it to be non-default (i.e. I think the onus is on the user to behave instead of breaking some workflows by default), I don't have the motivation to push that case any further. I'll revise this PR in due course to introduce a new config setting, with suitable documentation.

[chrisjsewell]

It's worth noting here. My understanding is that we want to collectively standardise the various parts of MyST / JB so that we can collectively avoid divergences.

yep exactly, the point is to converge on a central standard, that's not tied to one implementation, as opposed to any one implementation being the standard itself. If that makes sense.

[chrisjsewell]

As I understand it, the aspect to standardise here is the output metadata.

No I'd say, if eval can change the state of the kernel, then its more involved than that;
then effectively every "markdown" cell also become a "code" cell, and should be treated accordingly, e.g. they should affect the execution counter and get their own execution_count cell key, etc

[agoose77]

they should affect the execution counter and get their own execution_count cell key, etc

Er, that too!

[chrisjsewell]

Er, that too!

Exactly!
It is no-doubt is a great feature to have but, and really quite simple to hack it into a jupyter client (you are just calling the server/kernel some more times)

But, if you want to do it in a really "thorough" way, that all notebook clients (which is a subclass of jupyter clients) can standardise around, then thats gonna take some thinking.

[choldgraf]

FWIW from the perspective of guiding principles to make decisions about introducing new functionality and potentially surprising people, giving us flexibility to re-implement, etc, my thoughts are still pretty much:

#429 (comment)

So my suggestion is to figure out the minimum viable improvement that will satisfy "people want to be able to execute code inline", make it a feature flag, and then ask for feedback from others and iterate from there. We don't want "perfect" to be a gatekeeper of iterative progress if it's a feature that people clearly want, and that will be complex/tricky to get right.

[chrisjsewell]

make it a feature flag, and then ask for feedback from others and iterate from there.

Yep then main is that feature flags/config give you a clear hook for deprecations/changes.

If you introduce a feature, people will use and will be annoyed if you remove/change it, however many warnings you give them lol

[agoose77]

If you introduce a feature, people will use and will be annoyed if you remove/change it, however many warnings you give them lol

This is true, though I also think there's room for "we gave you sufficient warning, you shouldn't do that". There's a balance to be struck with these things, and it's all social convention.

[agoose77]

@choldgraf is this OK to merge?

[choldgraf]

If you think it's fine to merge then I'd say go for it @agoose77 . I don't have time to give it a deep dive though!

[agoose77]

@choldgraf can you approve, so that I can merge? :)

[choldgraf]

Of course!