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

outputs_hidden vs collapsed metadata #137

Open
jasongrout opened this issue Mar 26, 2019 · 17 comments
Open

outputs_hidden vs collapsed metadata #137

jasongrout opened this issue Mar 26, 2019 · 17 comments

Comments

@jasongrout
Copy link
Member

jasongrout commented Mar 26, 2019

I'm trying to implement the appropriate semantics for collapsed/jupyter.outputs_hidden metadata in JupyterLab.

The format docs seem ambiguous to me about the difference between the cell collapsed vs jupyter.outputs_hidden metadata.

In the nbformat docs, we have the following descriptions:

collapsed: Whether the cell’s output container should be collapsed
jupyter.outputs_hidden: Whether the cell’s outputs should be shown

In the discussion introducing outputs_hidden (#94), @ellisonbg mentions the confusion, and @rgbkrk and @damianavila mention in the next two comments that they view collapsed as essentially setting a small fixed height for the output, while outputs_hidden is completely hiding the outputs. (To me, the separate scrolled metadata is about setting a height for the outputs.)

How has this played out in practice over the last several years since the outputs_hidden field was defined? Classic notebook still only sets collapsed, and setting it hides the outputs, not just shortening them (scrolled metadata shortens the output region). As far as I can tell, the classic notebook doesn't recognize the jupyter.outputs_hidden metadata.

Does anyone else respect the jupyter.outputs_hidden metadata, or treat it differently from the collapsed metadata?

CC @rgbkrk, @mpacer, @ellisonbg

@saulshanabrook
Copy link
Contributor

Not sure if you have seen this already, but there was a discussion on this topic when I was implementing storing this state in Jupyterlab: jupyterlab/jupyterlab#3981 (comment). What I implemented only supports collapsed, not outputs_hidden, although from the discussion it seems like the plan was to possibly support both.

@jasongrout
Copy link
Member Author

Tangentially related - the source_hidden field specified in the same nbformat PR was implemented in papermill in nteract/papermill#135, but not the outputs_hidden field.

@jasongrout
Copy link
Member Author

CC also @MSeal, who was involved in the source_hidden implementation in papermill.

@jasongrout
Copy link
Member Author

@rgbkrk mentioned in nteract/papermill#135 (comment) that the plan was for nteract to support outputs_hidden.

jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Mar 26, 2019
We *set* the `jupyter.outputs_hidden` metadata for completeness, but we only pay attention to the top-level `collapsed` metadata field.

We might remove even setting the jupyter.outputs_hidden metadata depending on the resolution of jupyter/nbformat#137
@MSeal
Copy link
Contributor

MSeal commented Mar 26, 2019

If it helps, jupyter/notebook#534 is a thread that needs to be reignited to add support for the source_hidden and outputs_hidden in classic (I get this requests a lot through papermill to add support to classic UI). I don't have a strong opinion on the fields, beyond that we have a consistent contract set in nbformat and conform the various interfaces to it. I've found *_hidden in the cell and notebook level to be the obvious for users to try and use, and discover doesn't work in all places. I'd be ok with conforming to that naming convention moving forward, though I have little stake or requirements to maintain collapsed so I'll keep to supporting what we finalize on in the schema.

@jasongrout
Copy link
Member Author

jasongrout commented Mar 27, 2019

FYI, because of the apparent conflict between collapsed and jupyter.outputs_hidden, what I implemented in my PR to JupyterLab is that it only reads the collapsed field and ignores the jupyter.outputs_hidden field, but it will write to both collapsed and jupyter.outputs_hidden. I did this to maintain backwards compatibility with the classic notebook.

I'm tempted to make it just ignore jupyter.outputs_hidden altogether, unless other tools are using that field, or unless there actually is a semantic difference between the two fields.

@jasongrout
Copy link
Member Author

I decided to go ahead and not implement jupyter.outputs_hidden in my current PR to JupyterLab, given the redundancy in the format spec. I think it's better to ignore the field than to possibly overwrite a conflict between it and collapsed without recognizing the conflict. See jupyterlab/jupyterlab#5968 (comment).

jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Mar 27, 2019
Do not set jupyter.outputs_hidden until we can fully support it both in reading and writing. See jupyterlab#5968 (comment) and jupyter/nbformat#137. Basically, it is unclear what to do in case of a conflict between the `collapsed` and `jupyter.outputs_hidden` metadata fields. For backwards compatibility, we clearly need to respect and write to `collapsed`. Since `jupyter.outputs_hidden` is relatively new and unimplemented, let’s wait for clarification on it before supporting it, so we don’t have to change how we support it in the future.
@MSeal
Copy link
Contributor

MSeal commented Mar 27, 2019

Maybe we should get weigh in from others before finalizing this in a release? I'd prefer we're all consistent and have the opinion hit nbformat close there-after. If there's not consistent feedback today we should probably support one as a fallback name for the other until it's unified in nbformat. I would note that jupyter.outputs_hidden is a much clearer name than collapsed if we're future looking for format adoption.

@captainsafia
Copy link
Member

Chiming in with my two cents: the description for collapsed defines it as a way to show and expand a cell, where as outputs_hidden and source_hidden are for the individual outputs/inputs within a cell. For example, have collapsed set to true produces the same effect as having outputs_hidden and source_hidden set to true.

Similar to @MSeal's perspective, I lean towards standardizing formally on output_hidden and input_hidden. The terms are precise, clear, and granular enough to be spec-worthy.

@jasongrout
Copy link
Member Author

@captainsafia, thanks. In the nbformat docs, it explicitly says collapsed is "Whether the cell’s output container should be collapsed" - to me, that's not saying anything about the input/source.

I don't think we can ignore collapsed in nbformat 4.x, since the classic notebook already has a behavior for collapsed, and possibly millions of notebooks out there use that interpretation. I don't think we can change that behavior in a minor version rev of nbformat. (I don't think anyone is suggesting that we ignore collapsed, but just wanted to be clear...)

@MSeal, you suggested one to be a fallback of the other. Which one do you suggest be the primary source of truth? For pragmatic reasons, it makes sense to me for applications to write to both jupyter.outputs_hidden and collapsed (so the output collapsing happens in current/older versions of classic notebook). But if you're doing that, then you might as well just write to collapsed and ignore jupyter.outputs_hidden.

Another thing I realized: though not explicitly defined in the standard, each of these keys pragmatically has a default which is used when there is not a value for the field. These defaults probably ought to be explicitly defined in the standard. (For example, collapsed I think is generally understood to default to false).

For nbformat 5, I totally agree that we can resolve the confusion and have the elegance of the namespace by dropping collapsed.

@MSeal
Copy link
Contributor

MSeal commented Mar 28, 2019

Yes, my comment was more suggesting a naming opinion for v5 if we want to unify more there for the future.

I see what you're saying with conforming to v4 naming, however along those line of thinking code cells already have jupyter.source_hidden, jupyter.outputs_hidden and collapsed defined in the spec: https://github.com/jupyter/nbformat/blob/master/nbformat/v4/nbformat.v4.schema.json#L199-L211. I don't think we could remove one in v4 but if you're conforming to the spec, jlab should respect all 3 fields for v4 notebooks. If the implementation for output removal is the same for the two overlapping fields in jlab, then perhaps make cells outputs visible by basic boolean logic of the the union of the signals visible = (jupyter.outputs_hidden == null || jupyter.outputs_hidden) && (collapsed == null || collapsed). By the spec argument classic is also not fully supporting the v4 format today either.

I like the default value in the spec idea. We should absolutely add that. @rgbkrk is unfortunately on paternity leave, but I believe his input for v5 changes would be best to get before we commit to anything there.

@captainsafia
Copy link
Member

@jasongrout Oh boy! Looks like the docs and the schema disagree. In the schema, it refers to the entire cell. Assuming the docs are the source of truth, I've submitted #141 to correct this.

Agreed on dropping collapsed in nbformat 5.

@jasongrout
Copy link
Member Author

@captainsafia - oh, great catch! In cases where there is ambiguity between docs and spec, I think the classic notebook behavior should be the source of truth, since I think that is still how the vast majority of users have interacted with the notebook format (if not currently, then certainly in the past...)

@jasongrout
Copy link
Member Author

jasongrout commented Mar 29, 2019

however along those line of thinking code cells already have jupyter.source_hidden, jupyter.outputs_hidden and collapsed defined in the spec: https://github.com/jupyter/nbformat/blob/master/nbformat/v4/nbformat.v4.schema.json#L199-L211

Yes, technically, since 2017. Though I think this conversation was started partly because we realized that most frontends we know about were actually only implementing nbformat 4.2, so I feel like we have a bit more leeway in how we use the new 4.4 fields than the collapsed field from forever ago.

By the spec argument classic is also not fully supporting the v4 format today either.

That's why I think I want to still write to collapsed - to support users that want to use the classic notebook.

perhaps make cells outputs visible by basic boolean logic of the the union of the signals

Maybe what I'll do is essentially:

if (metadata.jupyter.outputs_hidden !== undefined) {
  if (metadata.collapsed === undefined) {
    metadata.collapsed = metadata.jupyter.outputs_hidden;
  }
  delete metadata.jupyter.outputs_hidden;
}

That way collapsed has precedence, but if jupyter.outputs_hidden is defined and collapsed isn't, I can still render appropriately, and as a bonus the new metadata is recognized in the classic notebook.

@MSeal
Copy link
Contributor

MSeal commented Mar 29, 2019

That way collapsed has precedence, but if jupyter.outputs_hidden is defined and collapsed isn't, I can still render appropriately, and as a bonus the new metadata is recognized in the classic notebook.

Sure. That'll work well enough. We can talk about v5 field support separately.

jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Mar 30, 2019
@jasongrout
Copy link
Member Author

jasongrout commented Apr 1, 2019

Actually, I changed my mind again. jasongrout/jupyterlab@36dfc3e in JupyterLab ensures that collapsed and jupyter.outputs_hidden are always the same thing, with preference to collapsed if it is defined and there is a conflict.

jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Apr 1, 2019
This makes sure that these redundant pieces of metadata always agree.

See jupyter/nbformat#137
jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Apr 1, 2019
This makes sure that these redundant pieces of metadata always agree.

See jupyter/nbformat#137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants