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

Enabling persisting notebook kernel information in notebooks #130602

Closed
mjbvz opened this issue Aug 11, 2021 · 7 comments · Fixed by #131219
Closed

Enabling persisting notebook kernel information in notebooks #130602

mjbvz opened this issue Aug 11, 2021 · 7 comments · Fixed by #131219
Assignees
Labels
api candidate Issue identified as probable candidate for fixing in the next release feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 11, 2021

No description provided.

@mjbvz mjbvz added the api label Aug 11, 2021
@mjbvz mjbvz self-assigned this Aug 11, 2021
@mjbvz mjbvz added this to the August 2021 milestone Aug 11, 2021
mjbvz added a commit that referenced this issue Aug 20, 2021
Fixes #130602

This adds a new API to the built-in ipynb extension that lets other extension set the kernelspec metadata on a notebook file
mjbvz added a commit that referenced this issue Aug 24, 2021
Fixes #130602

This adds a new API to the built-in ipynb extension that lets other extension set the kernelspec metadata on a notebook file
mjbvz added a commit that referenced this issue Aug 25, 2021
* Add API for setting kernelspec in ipynb files

Fixes #130602

This adds a new API to the built-in ipynb extension that lets other extension set the kernelspec metadata on a notebook file

* Temporarily skip the notebook editor tests

We need the new webview content to be published before these can run

* Use `custom`  instead of top level property
@mjbvz mjbvz added feature-request Request for new features or functionality verification-needed Verification of issue is requested labels Aug 25, 2021
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 25, 2021

To test:

  1. Using an extension with proposed api enabled
  2. Try setting the kernelspec of the active notebook using the new api:
		const ext = vscode.extensions.getExtension('vscode.ipynb');
		const api = await ext?.activate();

		if (vscode.window.activeNotebookEditor) {
			api.setKernelSpec(vscode.window.activeNotebookEditor.document.uri, { test: 'bla' });
		}
  1. Confirm document becomes dirty
  2. Save
  3. Now reopen the editor with the text editor and confirm that kernelspec is set

I've been testing this using the test.ipynb file in our repo

@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 25, 2021

Hm, not sure this api is doing the correct thing after following those steps again.

Running against

, results in:

{
 "cells": [ ...
]
 "metadata": {
  "interpreter": {
   "hash": "815c6b7592bf74925ca002a1774bcf064bae9d6a27e7933fd9109275fb484258"
  },
  "kernelspec": {
   "name": "python3",
   "display_name": "Python 3.9.5 64-bit ('myvenv': venv)"
  },
  "language_info": {
   "codemirror_mode": {
    "name": "ipython",
    "version": 3
   },
   "file_extension": ".py",
   "mimetype": "text/x-python",
   "name": "python",
   "nbconvert_exporter": "python",
   "pygments_lexer": "ipython3",
   "version": "3.9.5"
  }
 },
 "nbformat": 4,
 "nbformat_minor": 4,
 "kernelspec": {
  "test": "bla"
 }
}

The notebook should also be marked dirty after its metadata is changed. It currently is not

@roblourens @jrieken Are you more familiar with the notebook editing api? Is it expected that replaceNotebookMetadata would not mark the targeted notebook as dirty?

@mjbvz mjbvz reopened this Aug 25, 2021
@jrieken
Copy link
Member

jrieken commented Aug 25, 2021

@roblourens @jrieken Are you more familiar with the notebook editing api? Is it expected that replaceNotebookMetadata would not mark the targeted notebook as dirty?

It depends on the notebook serializer - they can mark what metadata is transient and what is persisted. This is how it is defined for our ipynb extension:

context.subscriptions.push(vscode.workspace.registerNotebookSerializer('jupyter-notebook', serializer, {

mjbvz added a commit that referenced this issue Aug 25, 2021
For #130602

I think we need to write it into metadata section instead of the top level
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 25, 2021

@roblourens @DonJayamanne Pushed b2bbd76 that writes the kernelspec into the metadata.kernelspec in the ipynb file. I have to admit I don't understand what is going on with the custom property or where the kernelspec needs to be, but this seems to align with how the kernelspec is written in existing notebooks

Also, I believe the ipynb serializer does not set transientDocumentMetadata so all metadata should be non-transient, however writing the new metadata still doesn't mark the notebook as dirty. Still looking into this

@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 25, 2021

I believe this line is why the notebook document is never marked dirty:

I don't really understand what it is doing but it always returns true when I tru updating the custom metadata

@DonJayamanne
Copy link
Contributor

n't understand what is going on with the custom property or where the kernelspec needs t

In the past all custom notebook metadata had to be set in a property named custom (within the notebook metadata object). this is the API that was exposed to extension authors. Later the API was changed to allow extension authors to store anything within the metadata object. I.e. metadata was completely for extension authors.

Unfortunately by that time .NET team and Jupyter team was already relying on custom to be the place where jupyter notebook metadata would be stored/accessed from.

The plan is to move away from that custom property, however we'd have to let others know about this change (else it could just break .NET, jupyter extensions).
I think we should do this in debt week & submit PRs to the relevant extension so that they can migrate to a path without using custom.

@rzhao271 rzhao271 added the candidate Issue identified as probable candidate for fixing in the next release label Aug 30, 2021
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 30, 2021

Fixed by #131681 and ported to the release branch

See original verification steps: #130602 (comment)

@mjbvz mjbvz closed this as completed Aug 30, 2021
@rzhao271 rzhao271 added the verified Verification succeeded label Aug 31, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api candidate Issue identified as probable candidate for fixing in the next release feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@jrieken @DonJayamanne @rzhao271 @mjbvz and others