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

Handle missing notebook metadata file_extension #127

Merged
merged 4 commits into from
Jun 28, 2020

Conversation

chrisjsewell
Copy link
Contributor

@chrisjsewell chrisjsewell commented May 15, 2020

The file_extension metadata is added to the notebook by the kernel, during execution.
This means that, in cases where the notebook has failed to execute, this line can raise an exception and kill the whole build, for example executablebooks/MyST-NB#175.
If this happens, we would like this line to fail gracefully; either by just not dumping the script file or (as proposed here) defaulting to a specific extension.

jupyter_sphinx/execute.py Outdated Show resolved Hide resolved
@choldgraf choldgraf closed this May 28, 2020
@choldgraf choldgraf reopened this May 28, 2020
@choldgraf
Copy link
Contributor

oops, sorry, accidental close! I meant to ask where we were on this one...are we just waiting on inserting the location for the warning? If so, I'd be +1 on just merging this since it fixes some downstream bugs (I'm not sure how to test this, but that would be good to)

@chrisjsewell chrisjsewell marked this pull request as ready for review May 28, 2020 16:27
@akhmerov
Copy link
Member

Having read a bit more about the upstream bug, it seems this addresses a use case where:

  • jupyter-sphinx code tries to save notebooks
  • code isn't executed

Under which conditions does this happen? Or are you using some specific components of jupyter-sphinx as a library?

@chrisjsewell
Copy link
Contributor Author

Or are you using some specific components of jupyter-sphinx as a library?

Well yes in MyST-NB we are using jupyter-sphinx as a library, but I guess it may still happen using jupyter-sphinx directly. Basically, the only metadata you actually need to have in a notebook, such that it may run, is the kernel name to load. The kernel itself will populate the rest of the metadata specified in the jsonschema. In fact more specifically, language_info.file_extension is not a required property of the notebook: https://github.com/jupyter/nbformat/blob/a523c7189310698dd49fed94e7d84d2946ee4064/nbformat/v4/nbformat.v4.4.schema.json#L31.
If, for whatever reason, the kernel fails to run/populate the notebook, then language_info.file_extension may not be present (unless it has specifically been added).
In this case jupyter_spinx should fail gracefully (just emitting a warning) rather than raising an obscure exception that halts the entire build.

@chrisjsewell
Copy link
Contributor Author

can this be merged @akhmerov?

@akhmerov
Copy link
Member

@chrisjsewell I thought there is an unresolved conversation and a TODO. What's the status of that?

@chrisjsewell
Copy link
Contributor Author

@chrisjsewell I thought there is an unresolved conversation and a TODO. What's the status of that?

oh yeh done 👍

@akhmerov
Copy link
Member

Thanks!

One last point: you add notebook validation and patching to the code that saves it, which seems to be outside of that function's concern. Wouldn't validation of the metadata be better used outside of that function and before it is called? Or are you using the function directly in myst-nb?

@chrisjsewell
Copy link
Contributor Author

Wouldn't validation of the metadata be better used outside of that function and before it is called?

well it is validated, by nbformat when it is read. But as mentioned, language_info.file_extension is not specifically required by the validating jsonschema in nbformat. So it is then this functions responsibility to account for if it is missing.

@akhmerov
Copy link
Member

OK, sounds reasonable.

@akhmerov akhmerov merged commit 3cfddda into jupyter:master Jun 28, 2020
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.

3 participants