Skip to content

Fallback to kernelspec to check if it's a Python notebook#12875

Merged
dhruvmanila merged 1 commit intomainfrom
dhruv/kernel-spec
Aug 14, 2024
Merged

Fallback to kernelspec to check if it's a Python notebook#12875
dhruvmanila merged 1 commit intomainfrom
dhruv/kernel-spec

Conversation

@dhruvmanila
Copy link
Copy Markdown
Member

Summary

This PR adds a fallback logic for is_python_notebook to check the kernelspec.language field.

Reference implementation in VS Code: https://github.com/microsoft/vscode/blob/1c31e758985efe11bc0453a45ea0bb6887e670a4/extensions/ipynb/src/deserializers.ts#L20-L22

It's also required for the kernel to provide the language they're implementing based on https://jupyter-client.readthedocs.io/en/stable/kernels.html#kernel-specs reference although that's for the kernel.json file but is also included in the notebook metadata.

Closes: #12281

Test Plan

Add a test case for is_python_notebook and include the test notebook for round trip validation.

The test notebook contains two cells, one is JavaScript (denoted via the vscode.languageId metadata) and the other is Python (no metadata). The notebook metadata only contains kernelspec and the language_info is absent.

I also verified that this is a valid notebook by opening it in Jupyter Lab, VS Code and using nbformat validator.

@dhruvmanila dhruvmanila added the notebook Related to (Jupyter) notebooks label Aug 14, 2024
Comment thread crates/ruff_notebook/src/notebook.rs Outdated
Comment on lines +413 to +419
self.raw
.metadata
.language_info
.as_ref()
.map_or(true, |language| language.name == "python")
if let Some(language_info) = self.raw.metadata.language_info.as_ref() {
return language_info.name == "python";
}
if let Some(kernel_spec) = self.raw.metadata.kernelspec.as_ref() {
return kernel_spec.language.as_deref() == Some("python");
}
false
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not exactly sure why was the default value true in case language_info is absent but that doesn't seem correct to me. If we can't infer the language from either language_info or kernelspec, we'll now return false instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, so the notebooks in the ecosystem diff doesn't have both language_info and kernelspec in which case we're not marking it as Python notebook. I think I'd need to revert this change to return true in this case.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 14, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dhruvmanila
Copy link
Copy Markdown
Member Author

Need to look at what's going on with the notebooks in ecosystem output.

Copy link
Copy Markdown
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks @dhruvmanila for pushing this changes before the release!

Comment thread crates/ruff_notebook/src/notebook.rs
@MichaReiser MichaReiser added this to the v0.6 milestone Aug 14, 2024
@dhruvmanila dhruvmanila merged commit 2520ebb into main Aug 14, 2024
@dhruvmanila dhruvmanila deleted the dhruv/kernel-spec branch August 14, 2024 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notebook Related to (Jupyter) notebooks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Only Python cells should be validated in Jupyter notebooks

2 participants