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

🤖 Make name & display_name required properties for KernelSpec #1490

Merged
merged 9 commits into from
Aug 28, 2024

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Aug 23, 2024

The nbformat spec requires that name and display_name are set in a kernelspec object. Presently, we don't require either of these fields.

This is a problem when e.g. invoking jupytext on a notebook that only has name:

jupytext input.md --output output.md --from md:myst --to md:myst

which fails with:

[jupytext] Reading input.md in format md:myst
Traceback (most recent call last):
  File "/tmp/tmp.jZemAlJPwg/.venv/bin/jupytext", line 8, in <module>
    sys.exit(jupytext())
             ^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/jupytext/cli.py", line 497, in jupytext
    exit_code += jupytext_single_file(nb_file, args, log)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/jupytext/cli.py", line 561, in jupytext_single_file
    notebook = read(nb_file, fmt=fmt, config=config)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/jupytext/jupytext.py", line 432, in read
    return read(stream, as_version=as_version, fmt=fmt, config=config, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/jupytext/jupytext.py", line 441, in read
    return reads(fp.read(), fmt, config=config, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/jupytext/jupytext.py", line 390, in reads
    notebook = reader.reads(text, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/jupytext/jupytext.py", line 103, in reads
    return myst_to_notebook(s)
           ^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/jupytext/myst.py", line 295, in myst_to_notebook
    notebook = nbf_version.new_notebook(**kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/nbformat/v4/nbbase.py", line 170, in new_notebook
    validate(nb)
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/nbformat/v4/nbbase.py", line 41, in validate
    return validate_orig(node, ref=ref, version=nbformat)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmp.jZemAlJPwg/.venv/lib/python3.11/site-packages/nbformat/validator.py", line 509, in validate
    raise error
nbformat.validator.NotebookValidationError: 'display_name' is a required property

Failed validating 'required' in notebook['properties']['metadata']['properties']['kernelspec']:

On instance['metadata']['kernelspec']:
{'name': 'python3'}

This PR makes name and display_name "soft" required; default values will be chosen if they're missing, but warnings will also be issued.

In future, we can eventually just make them required.

This PR also changes our execution hash key strategy. Now, it includes the kernelspec name! It's highly unlikely to be a common bug, but users could e.g. switch from one Python kernel to another, and the cache would remain unchanged. This would definitely be a problem if different kernels had different environments.

As a result of this change, existing caches will be invalidated. But, I think that's OK given how young execution is (I don't think we advertise it as stable yet).

Copy link

changeset-bot bot commented Aug 23, 2024

🦋 Changeset detected

Latest commit: 9688ccd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
myst-frontmatter Minor
myst-common Minor
myst-config Minor
myst-spec-ext Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@agoose77 agoose77 requested a review from fwkoch August 23, 2024 12:18
@agoose77 agoose77 requested review from rowanc1 and removed request for fwkoch August 23, 2024 12:55
Copy link
Collaborator

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, haven't tested. Let me know if you want me to do that, but the changes look straight forward.


// Build a hash from notebook state
return createHash('md5')
.update(kernelSpec.name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose all hashes will change. That is fine though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I made a note of that in the description. It's unfortunate, and we could add some backwards-compat logic, but I think it's early enough in the project's history to just break it.

@@ -281,6 +284,15 @@ export type Options = {
*/
export async function kernelExecutionTransform(tree: GenericParent, vfile: VFile, opts: Options) {
const log = opts.log ?? console;

// We need the kernelspec to proceed
if (opts.frontmatter.kernelspec === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should never be triggered in the current CLI workflow, right? Because we always have a valid object coming out of frontmatter validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't define frontmatter.kernelspec, you don't get a kernelspec object:

kernelspec?: KernelSpec;

@agoose77
Copy link
Contributor Author

@rowanc1 are you happy to merge this in now?

@rowanc1 rowanc1 merged commit 9c1b8c7 into main Aug 28, 2024
7 checks passed
@rowanc1 rowanc1 deleted the agoose77/fix-kernelspec-validation branch August 28, 2024 18:24
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.

2 participants