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

Allow notebook format version 4.1 mimebundle keys ending in +json to have arbitrary JSON #167

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

jasongrout
Copy link
Member

There are a number of people posting issues with nbformat 5 being stricter about validating notebook format 4.1, including:

#160
#161
jupyter-widgets/ipywidgets#2553
jupyter-widgets/ipywidgets#2788

Essentially, nbformat package version 4.x allowed noncompliant format 4.1 notebooks to be verified as valid, leading to many notebooks in the wild having major/minor format version 4.1, but with widgets and other json outputs that were technically invalid.

Upgrading to nbformat package 5.x correctly flagged these notebook as noncompliant. This is correct technically. However, practically it means that all these notebooks files tagged as format 4.1 that were working fine suddenly won't even open after upgrading to nbformat version 5. This is a pain.

This retroactively upgrades the format 4.1 schema to allow json in these cases, since in practice there are lots of notebooks labeled as format 4.1, I think by official Jupyter software, that have json in the mimebundle output. Essentially, this acknowledges that in the official implementations from Jupyter, notebook format 4.1 has indeed had arbitrary JSON values in mimebundles, and we cannot in good conscience decree it invalid.

…o have arbitrary JSON

There are a number of people posting issues with nbformat 5 being stricter about validating notebook format 4.1, including:

jupyter#160
jupyter#161
jupyter-widgets/ipywidgets#2553
jupyter-widgets/ipywidgets#2788


Essentially, nbformat package version 4.x allowed noncompliant format 4.1 notebooks to be verified as valid, leading to many notebooks in the wild having major/minor format version 4.1, but with widgets and other json outputs that were technically invalid.

Upgrading to nbformat package 5.x correctly flagged these notebook as noncompliant. This is correct technically. However, practically it means that all these notebooks files tagged as format 4.1 that were working fine suddenly won't even open after upgrading to nbformat version 5. This is a pain.

This retroactively upgrades the format 4.1 schema to allow json in these cases, since in practice there are lots of notebooks labeled as format 4.1, I think by official Jupyter software, that have json in the mimebundle output. Essentially, this acknowledges that in the official implementations from Jupyter, notebook format 4.1 has indeed had arbitrary JSON values in mimebundles, and we cannot in good conscience decree it invalid.
@jasongrout
Copy link
Member Author

CC @minrk, @MSeal

@minrk minrk merged commit 1b30177 into jupyter:master Mar 12, 2020
@minrk
Copy link
Member

minrk commented Mar 12, 2020

Thanks, Jason!

@jasongrout
Copy link
Member Author

For completeness - I only moved this back to 4.1, and not 4.0, because all the cases I saw were dealing with notebooks with version 4.1.

@minrk
Copy link
Member

minrk commented Mar 12, 2020

I think that makes sense

@jasongrout
Copy link
Member Author

For completeness - I only moved this back to 4.1, and not 4.0, because all the cases I saw were dealing with notebooks with version 4.1.

Looks like cocalc was generating format version 4.0 with widgets outputs, so at least one client has been releasing non-compatible 4.0 notebooks that we haven't been complaining about.

Thoughts? Should we retroactively modify format version 4.0 as well, or ask cocalc to upgrade their format version and have people deal with the pain of existing notebooks not being compatible? Given that we didn't have strict compatibility tests before, I think it's probably more on us to retroactively modify format version 4.0 as well. On the other hand, if someone wrote a library that depends on a strict interpretation of our docs, as they were before, they could have reason to complain that we broke their library.

@MSeal
Copy link
Contributor

MSeal commented Mar 26, 2020

Given how old 4.0 format is, and the number of other things that break in < 4.2 versions already anyway I think it's reasonably ok to update to match the reality of schemas that were produced at this point.

jasongrout added a commit to jasongrout/nbformat that referenced this pull request Mar 26, 2020
…nding with +json.

See jupyter#167 for more discussion. Essentially, there are a lot of notebooks in the wild that have arbitrary JSON in the mimebundles (for example, from ipywidgets), and we never flagged them before, but with nbformat package 5, they are now suddenly they are flagged as noncompliant.

This is a follow-up to jupyter#167, and extends the compatibility changes from 4.1 back to 4.0.
@jasongrout
Copy link
Member Author

For completeness - I only moved this back to 4.1, and not 4.0, because all the cases I saw were dealing with notebooks with version 4.1.

#169 extends this to 4.0, since we are seeing 4.0 notebooks in the wild also having this issue.

@blois
Copy link

blois commented Apr 4, 2020

we are seeing 4.0 notebooks in the wild also having this issue.

Just noticed this and wanted to note that for example if you open a 4.0 notebook in Jupyter Notebook, display a widget, then save that you get a dialog that the file was saved, but is invalid.

image

I agree with the resolution here, but want to highlight some difficulties in validating the data. Should a notebook editor that only knows about the existence of 4.0 strictly validate all display data? I believe that most editors consider display data from the kernel to essentially be user-specified.

@MSeal
Copy link
Contributor

MSeal commented Apr 4, 2020

Isn't that notebook a version 2 notebook, not a version 4 based on the error message?

I don't think we're reaching back to pre-v4 notebooks for backporting.

@jasongrout
Copy link
Member Author

No, that is a notebook 4 error message. It is just displaying data from a widget protocol v2.

@blois, with the latest release of nbformat, this is now a moot point. But technically yes, there is a standard that should be validated against.

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

Successfully merging this pull request may close these issues.

4 participants