Improve handling of stderr formatting

[akhmerov]

I am unsure whether this qualifies as an enhancement or a bug. There are several relevant scenarios in which the current implementation either matches the documentation in a way that may surprise the user, or limits the range of possibilities. Below I will list the different situation, and follow with the proposed change.

Problem 1: avoiding the error

Imagine I have a cell with pesky stderr output that I can't suppress, and want to remove. So I add remove-stderr tag to the cell. I would also like to make sure that no other cell emits stderr, so I set nb_output_stderr = "severe". This is a reasonable usage scenario, which will however not work, because the error emitting code is unconditional. I would argue that this is somewhat close to a bug, although the documentation isn't clear.

Problem 2: showing stderr conditionally

Imagine the single most common usage scenario of showing stderr (let me know if you don't think it is most common). Imagine the user really doesn't want stderr to appear anywhere, but has a couple of exclusions where they want to show it (e.g. explaining which warnings occur). In that case the user would need to have the global behavior equivalent to the current nb_output_stderr = "warn", but they have no option to override this within the cell.

Analysis of the current implementation

Docs reference, implementation]. Right now there's a global option nb_output_stderr, which may be one of show, remove, remove-warn, warn, error, severe. The flow control (warning/error) is the sole authority of the global config, while removal of the stderr happens if either the global is set or the cell tag is present.

Observe that cell tags become inconvenient to accommodate both of the above usage scenarios at once, since this would require introducing two mutually exclusive tags: show-stderr, remove-stderr.

Proposal for an improved behavior

1. Make cells that have an explicit instruction to remove stderr never trigger flow messages.
2. Introduce cell metadata for allowing to unconditionally show stderr without triggering errors. I propose to use a key-value metadata pair show-stderr: T/F.
3. Make remove-warn the default behavior of nb_output_stderr because it's by far the most relevant setting.

Remarks:

• As a related option I have considered separating nb_output_stderr into two options: one for flow control, another for rendering, but ultimately I didn't find a reason to do so.

• Introducing 2 cell tags (remove-stderr show-stderr) is an alternative for using a key-value metadata pair, however I think it is inferior for the following reasons:

  • following this track would double the amount of cell tags since a lot of other configuration options should follow the same hierarchical behavior (cf executablebooks/jupyter-book#666, #249)
  • key-value pairs are also easier to implement as a dictionary merge
  • key-value pairs they don't allow illegal inputs where both tags are present

  I do remember the previous discussions about cell tags being easier to edit, and leave it to maintainers to decide what they prefer.

Since the implementation is straightforward and isolated in the code, and since I'm blocked by executablebooks/jupyter-book#666, I would be available to implement this if a decision on the preferred implementation is taken.

[chrisjsewell]

Yep, at a first glance, the issues here sound reasonable 👍 I'll try to give a proper response soon. I would also like to consider a consistent approach to handling of project/notebook/cell(/output) level metadata/configuration. Ideally, options like output_stderr could be set any of these levels, with the "lowest" taking priority.

[akhmerov]

I agree about priorities, however I propose starting with the minimal viable amount of levels. Global and cell level seems to cover most of the use cases that I can think of.

[akhmerov]

@chrisjsewell and others: I'd like to give this a shot, do you have any objections or feedback regarding the proposed design?

[choldgraf]

Sounds good to me although I don't have super strong opinions in general as long as we are consistent and don't make the configuration process too complex (in general the more things we let people configure the more confusing things become / hard to maintain or change in the future, so let's just keep that in mind) - the one thing I'd advise against is allowing for cell-level metadata, at least right now. IMO we shouldn't be encouraging users to write raw JSON on their own because that's a big PITA. I actually opened up an issue to discuss this in JupyterLab: https://github.com/jupyterlab/jupyterlab/issues/9064

[akhmerov]

So your preferred option would be pairs of tags (remove-stderr/show-stderr), and then same for hide/show input/output/cell?

Also note that myst-nb already introduces own metadata in the render namespace.

[choldgraf]

well I'd rather we suggest to JupyterLab that they improve metadata editing capabilities. I don't have a problem with using key: val pairs in principle, but I just don't think it's feasible to ask most users to edit raw JSON. The main reason we use tags now is simply because that UI is much easier to use (and because for simple flags it's probably simpler in general to use tags)

I think right now we should assume that any functionality that requires cell-level metadata to be hand-added will only be usable by people using MyST Notebooks

[chrisjsewell]

in general the more things we let people configure the more confusing things become / hard to maintain or change in the future, so let's just keep that in mind

Not if you do it well. There should be a "central" configuration instance, similar to https://github.com/executablebooks/MyST-Parser/blob/26f469e85efb41e6fe4bf0366743ba24f3ae4938/myst_parser/main.py#L24 Then there should be a single interface to it, when retrieving configuration values, e.g.

value = get_nb_config(app, cell, key)

This function would look for the value at the project, notebook and cell level, and return the highest priority one.

For "boolean flags", it should be a bit similar to https://click.palletsprojects.com/en/7.x/options/#boolean-flags, with well-defined prefixes.

So for example with stderr, it would be something like:

@attr.s()
class NbConfig:
    stderr: str = attr.ib(
        default="remove-warn", validator=in_(["show", "remove", "remove-warn", "warn", "error", "severe"])
    )

Which would automate the setup and validation of the conf.py variable nb_stderr (as is done in https://github.com/executablebooks/MyST-Parser/blob/26f469e85efb41e6fe4bf0366743ba24f3ae4938/myst_parser/init.py#L16)

Then when you call: value = get_nb_config(app, cell, "stderr"), it would retrieve the config value from app, and also look for any tags in the cell metadata ending -stderr (warning if there were multiple tags, or if the prefix was not in the validation list), which would take priority over the config value. It would then also be easy to extend this to looking in the notebook level metadata (stored as docinfo), for a notebook level value, with priority: cell > notebook > app

[akhmerov]

I like that this approach unifies treatment of multiple configuration fields. How would you approach the case when global configuration differs from the cell-level configuration? To make it concrete, in the example of treatment of stderr it doesn't seem to make sense to allow or document "remove-warn", "warn", "error", "severe" on cell level.

Also while I agree that streamlining configuration makes this part of the code easier to maintain, introducing more options than necessary still makes it harder for the users and still makes it harder to change or remove something.

[akhmerov]

On the side note, why attrs and not traitlets? The latter seems standard in the jupyter stack, while attrs is only used in markdown-it-py (out of the ebp stack dependencies).

[chrisjsewell]

Oh not a chance I'm using traitlets 😄, attrs is far superior (simpler, more lightweight, etc) and more ubiquitous in the general python world. There has already been conversations about removing traitlets from nbconvert, because of its complexity: https://github.com/jupyter/nbconvert/issues/1045#issuecomment-549066660 There's a reason why basically no one else uses traitlets, whilst attrs has essentially been adopted into python core via dataclasses (see https://www.python.org/dev/peps/pep-0557/)

[chrisjsewell]

it doesn't seem to make sense to allow or document "remove-warn", "warn", "error", "severe" on cell level

why not? I'm sure there are use-cases when these could be helpful

introducing more options than necessary still makes it harder for the users

I don't think so. My approach to configuration, is that the defaults should be good enough for most "basic" users, and so they shouldn't need to even look at configuration, but for power users everything possible should be configurable. Streamlining configuration makes it easy to document consistently, and so less confusing to users, and also easier to add deprecation warnings when things need to be changed/removed.

[akhmerov]

attrs is far superior

Thanks for the explanation, good to know.

I'm sure there are use-cases when these could be helpful

Can you describe such a use case? I lack your certainty. To the best of my knowledge of sphinx and its ecosystem, exception control in sphinx stack is only available at a global level. Allowing these options to the cell level breaks separation of concerns by making documents control exceptions in addition to other things they do.