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.

Sign up for GitHub

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

Jump to bottom

declare source_hidden and outputs_hidden official code cell metadata #94

Merged
merged 4 commits into from
Jul 21, 2017
Merged

declare source_hidden and outputs_hidden official code cell metadata #94

merged 4 commits into from
Jul 21, 2017

Conversation

rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Jun 5, 2017 •
edited
Loading

Following discussion from Jupyter Team Meeting, here's a follow on PR for the metadata I'd like to see us standandardize on.

This PR uses source_hidden within .cells[].metadata.input_hidden, only for code cells (since markdown and raw cells imply hidden by default in most views (I'd be happy to add it to markdown if someone wanted it to be explicit though...)

xrefs:

/cc @ellisonbg @mpacer @jcb91

@ellisonbg
Copy link
Contributor

ellisonbg commented Jun 6, 2017 •
edited by rgbkrk
Loading

@rgbkrk thanks for kicking this off!

Some questions/thoughts:

There were other attributes being discussed along with the input hiding:

  • hiding output (currently collapsed, but that should probably change to be parallel with input)
  • hide the full cell (but keep in the document)
  • Remove/kill cell (upon conversion).

I have been doing a bunch of work with user defined tags this weekend, and after that experience, I feel pretty strongly we should not use tags for things like this - it would pollute the users tag namespace in a way that would be very awkward.

Previous names (the tag ones) were in the active tense such as hide_input and hide_output, but we have collapsed and scrolled in the existing format.

Do I remember that we have decided that we will put any metadata into a jupyter namespace as well?

My own preference would be the following:

  • Deprecate collapsed and autoscroll.
  • Add cell level metadata input_hidden and output_hidden (current metadata is passive tense).
  • More specifically, I think that the output_hidden should not be output area metadata as the implementation of that in frontends will likely be handled by the cell itself.
  • Don't add metadata for destructively killing/removing cells in nbconvert. That can be implemented by a --remove-cells=[tags] feature to nbconvert in a more flexible manner and would lead to more semantic and maintainable notebooks.
  • Don't add metadata for hiding an entire cell. I think frontends and nbconvert should implement full cell hiding using tags ("hide cells with these tags").

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

hiding output (currently collapsed, but that should probably change to be parallel with input)

I'd like to do the smallest amount of migrations as possible, so I'm happy to keep the old name for a minor change in the format. By the way, isn't collapsed not fully hidden?

Add cell level metadata input_hidden and output_hidden (current metadata is passive tense).

👍

More specifically, I think that the output_hidden should not be output area metadata as the implementation of that in frontends will likely be handled by the cell itself.

What do you mean by not output area metadata? As in metadata for a single output (rather than cell)?

Don't add metadata for destructively killing/removing cells in nbconvert. That can be implemented by a --remove-cells=[tags] feature to nbconvert in a more flexible manner and would lead to more semantic and maintainable notebooks.

👍

Don't add metadata for hiding an entire cell. I think frontends and nbconvert should implement full cell hiding using tags ("hide cells with these tags").

Definitely in agreement with that one. A frontend can expose this as a toggle that enables both of input_hidden and output_hidden, though it's not a strict requirement.

As for the jupyter namespaced metadata, I'm definitely in agreement there. It also makes it far simpler for typed languages to deal with, since we'll have it specced for us while allowing extensions to use their practices separately.

@ellisonbg
Copy link
Contributor

By the way, isn't collapsed not fully hidden?

Ohhh, yeah, that is separate I guess.

What do you mean by not output area metadata? As in metadata for a single output (rather than cell)?

There is separate output level metadata with one field currently:

http://nbformat.readthedocs.io/en/latest/format_description.html#output-metadata

As for the jupyter namespaced metadata, I'm definitely in agreement there. It also makes it far simpler for typed languages to deal with, since we'll have it specced for us while allowing extensions to use their practices separately.

What isn't clear to me is do we move other, existing metadata into the new jupyter sub-dict?

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

What isn't clear to me is do we move other, existing metadata into the new jupyter sub-dict?

I think in the short term we should make input_hidden and output_hidden be in the jupyter namespace while we deprecate the other fields as part of a minor change in the format. Later we can move them all into the jupyter namespace and call it a major change in the format.

@ellisonbg
Copy link
Contributor

ellisonbg commented Jun 6, 2017 via email

@mpacer
Copy link
Member

mpacer commented Jun 6, 2017

This sounds like a good plan to me.

NB: There are other output level metadata keys that are used by different kernels (including the IPython kernel). This is particularly relevant for mimetype-keyed metadata.

I'd suggest that we have remove_cells remove_input and remove_output as separate nbconvert traitlets. But we don't need to deal with that here.

I also want to implement a similar hide_cells, hide_input, hide_output for the HTMLExporter specifically that has the toggleable ability, but that will be less of a priority unless someone specifically requests it since it has more in the way of implementation details that need to be worked out (see @parente's work on this in jupyter/nbviewer#681 & mine at https://github.com/mpacer/hiding_tags_nbconvert/).

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

Now that I'm moving this, I'm thinking it should be source_hidden to match the field name for what inputs are. The input name is more an artifact of the className in the notebook implementation.

It would then be source_hidden for hidden source for a cell and outputs_hidden for hiding the overall outputs.

@rgbkrk rgbkrk changed the title declare input_hidden official code cell metadata declare source_hidden and outputs_hidden official code cell metadata Jun 6, 2017
@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

Let me know what you both think of matching the names of the other fields here, since I diverted from the original name proposed. The last commit shows it in the jupyter namespace as well.

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

Adding a test now while I'm at it.

@mpacer
Copy link
Member

mpacer commented Jun 6, 2017 •
edited
Loading

I do think there should be a way to hide markdown cells. It may be that people want to use that in a pedagogical context (e.g., show/hide solutions or derivations) like @willingc mentioned at the dev meeting last week.

@ellisonbg
Copy link
Contributor

ellisonbg commented Jun 6, 2017 via email

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

source_hidden on markdown has a bit different of a semantic since we tend to think of it as "rendered" vs. "editing". I've added source_hidden for markdown cells just now, though the meaning may be a bit different.

@ellisonbg
Copy link
Contributor

ellisonbg commented Jun 6, 2017 via email

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

Should we stick the markdown part of this discussion in a separate PR or just go with all the options presented here?

@ellisonbg
Copy link
Contributor

I think putting it all in one PR makes sense. Should we also add the source_hidden to the raw cell? I can imagine wanting to hide that in the frontend at times.

The last bit would be to add verbiage about this to the sphinx docs.

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

Updated, rebased. I'll handle the docs next.

@rgbkrk rgbkrk modified the milestones: 4.2.1, 4.3 Jun 6, 2017
@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

Documentation updated. I have to admit after updating the docs that the jupyter namespace feels... weird...

@ellisonbg
Copy link
Contributor

Forget to push the docs? I can read through and see how the jupyter namespace feels.

@ellisonbg
Copy link
Contributor

Just saw it - yeah I see what you mean. It sort of seems like all of the formally specified metadata is the jupyter stuff. An different approach would be to tell folks that jupyter owns the top level keys and that they need to put all of their stuff in a namespace that doesn't collide with our keys. I am fine with that.

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 6, 2017

Yeah sorry the GitHub outage was right when I pushed and was writing this comment. I did both in the opposite order when GitHub came back.

@damianavila
Copy link
Member

So +1 for us namespacing our metadata. That also allows us to strictly verify the official Jupyter metadata.

I agree with this vision...
But I also see the jupyter namespace feeling weird in the current proposal... maybe because I would expect collapsed and scrollable under the same jupyter namespace, I guess...

I think in the short term we should make input_hidden and output_hidden be in the jupyter namespace while we deprecate the other fields as part of a minor change in the format. Later we can move them all into the jupyter namespace and call it a major change in the format.

Can't we try to do this in one step, maybe? Or do you think is too much disrupting?

@sccolbert
Copy link

I'm with Damian. It feels weird to have these new metadata values in a namespace, but collapsed is not in that same namespace. So my preference would be either a) keep our stuff top level and say something like "start all your namespaces with $ to avoid any potential conflict" (and we never use a leading $ for our stuff), or b) we move all our stuff to a namespace.

I'm basically only 👎 for anything which is not consistent.

@jasongrout
Copy link
Member

Definitely +1 for consistency.

a) keep our stuff top level and say something like "start all your namespaces with $ to avoid any potential conflict"

Which is essentially doing the same thing as namespacing official things off from other metadata namespaces. However, then we make every other third party pay this extra price of awkwardness of having a leading $ on top of their namespaces, instead of us paying the same price everyone else of having a top-level namespace. I think I'd still rather move to having a jupyter namespace over having some convention for jupyter vs other metadata keys.

We could also move these special fields out of the metadata key, and make the metadata key solely reserved for third-party namespaces.

@mgeier
Copy link
Contributor

mgeier commented Jun 9, 2017 •
edited
Loading

Coming from the mailing list (thanks @ellisonbg for mentioning it there!) ...

👍 for putting official Jupyter stuff into the top-level namespace, 👎 for forcing something awkward like leading $ for third-party metadata.
Instead I would suggest that third-party metadata should be prefixed by the respective project name.

Except for silly project names like remove or source or outputs, this should avoid clashes and has the added benefit of knowing which project added which metadata.

As a third-party author, I would naturally add my project name as a prefix or sub-namespace anyway, and as a matter of fact, I did exactly that (I'm not sure, but this was probably even suggested somewhere in the docs?).

@ellisonbg
Copy link
Contributor

Here is a jupyterlab implementation of input/output hiding:

jupyterlab/jupyterlab#2474

Right now this is view only state, but once we decide on the metadata we can persist it to the metadata.

@ellisonbg ellisonbg mentioned this pull request Jun 20, 2017
Open
@ellisonbg
Copy link
Contributor

Thinking a bit more...doesn't the new outputs_hidden essentially replace the old collapsed metadata? The old scrolled metadata is different, but maybe we should also say that collapsed is deprecated or clarify its relationship with the new one?

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 21, 2017 •
edited
Loading

To me collapsed means truncated (in view only):

render() {
  return (
    <Outputs
      height="100px"
      scrollable="totes"
    />
  );
}

whereas hidden means:

render() {
  return null;
}

@damianavila
Copy link
Member

I agree with @rgbkrk comment above, I think collapsed and hidden are different concepts. Maybe we should be more clear at the time to define it/explain it.

@mpacer
Copy link
Member

mpacer commented Jun 21, 2017 via email

@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 21, 2017

The namespace requirement is for fields where we are going to have a strict adherence to the specification we outline (and that others can contribute to, becoming part of "we").

@ellisonbg
Copy link
Contributor

I talked with @rgbkrk yesterday and we both feel that the current state of this PR is worth merging. There are larger discussions about the notebook format as it relates to the purpose of metadata and where to put it, but these particular attributes are a huge blocker for us right now in JupyterLab and nteract. Because this PR is a strict improvement and won't prevent or hinder future changes to the format, I am going to merge (@rgbkrk concurs with this decision).

@ellisonbg ellisonbg merged commit d79ee76 into jupyter:master Jul 21, 2017
@rgbkrk rgbkrk deleted the input_hidden branch July 21, 2017 21:07
@rgbkrk
Copy link
Member Author

rgbkrk commented Jul 21, 2017

Thanks all!

@mpacer
Copy link
Member

mpacer commented Jul 21, 2017 via email

@rgbkrk
Copy link
Member Author

rgbkrk commented Jul 21, 2017

The only way we can have a guarantee is if we bump the nbformat by a major version. If and when we do that, we should probably move it out of metadata and into the top level portion of a cell where it really belongs as core functionality.

@westurner
Copy link
Contributor

westurner commented Jul 23, 2017 via email

@rgbkrk
Copy link
Member Author

rgbkrk commented Jul 23, 2017

Cell ids are not part of this issue, please stay on topic or find the relevant issue to comment on.

@westurner
Copy link
Contributor

westurner commented Jul 23, 2017 via email

@rgbkrk rgbkrk mentioned this pull request Jul 27, 2017
Open
@mpacer mpacer modified the milestones: 4.3, 4.4 Aug 11, 2017
@saulshanabrook saulshanabrook mentioned this pull request May 22, 2018
Merged
4 tasks
@jasongrout jasongrout mentioned this pull request Mar 26, 2019
Open
@ociule ociule mentioned this pull request Nov 21, 2019
Open
@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/how-can-i-get-my-images-and-videos-to-render-without-the-user-having-to-run-all-cells/5021/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellisonbg ellisonbg ellisonbg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
9 participants
@rgbkrk @ellisonbg @mpacer @jasongrout @damianavila @sccolbert @mgeier @westurner @meeseeksmachine