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

Provide a mechanism for an extension to determine or set a preferred kernel. #4423

Closed
brettfo opened this issue Jan 19, 2021 · 16 comments
Closed
Assignees

Comments

@brettfo
Copy link
Member

brettfo commented Jan 19, 2021

As a corollary to #4363, there should be a way for a VS Code concept of a kernel to map to an existing .ipynb file without requiring an installed kernelspec file. E.g., consider a situation where a user installs this extension as well as the .NET Interactive extension. From there they open an existing .ipynb file. At that point, there is no mechanism for the VS Code version of the .NET Interactive kernel to be auto-selected.

This issue is hopefully more of a discussion about how to add this functionality, and the end result may require changes from VS Code itself.

As a single data point, if a new .ipynb notebook is created, the kernel set to .NET Interactive and it's saved, the notebook's metadata looks like this:

...
"metadata": {
    "language_info": {
        "name": "csharp",
        "nbconvert_exporter": "python"
    },
    "orig_nbformat": 2
}
...
@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jan 20, 2021

Thanks @brettfo
We have discussed this internally & our plan was to not pre-select the non-python kernels unless we had an exact match for the kernel.
For instance in this example, we might not have a matching kernel & we shoudlnt' ideally pre-select any kernel.
Then the .net extension can nominate its kernel as a preferred kernel using the NotebookKernel.isPreferred property.

I dont think we'd need any additional VSC API for this.

@DonJayamanne
Copy link
Contributor

At our end the fix woudl be in getKernelForLocalRawConnection.

	const kernelSpec = await this.kernelFinder.findKernelSpec(resource, notebookMetadata, cancelToken);
....
....
        if (!kernelSpec && activeInterpreter) {
            await this.installDependenciesIntoInterpreter(activeInterpreter, ignoreDependencyCheck, cancelToken);

            // Return current interpreter.
            return {
                kind: 'startUsingPythonInterpreter',
                interpreter: activeInterpreter
            };
        } else if (kernelSpec) {

To discuss

  • Should we ever preselect a kernel & just add an exception for .NET interactive?
    • I think yes, majority of our (Python/Jupyter) users will have our extension.
    • We can add exceptions for .NET kernels & look for a more generic solution later (if other extensions do not like this pre-selection of kernels by jupyter extension).
    • If the preselected kernel is a .NET kernel & .NET interactive is installed, then never preselect a .NET kernelspec

Note:
@brettfo VS Code has added the ability for users to define the default kernel provider.
I.e. user can go into the kernel picker and chose .NET kernels as their preferred kernels. This way when .NET extension exposes kernels & has isPreferred set to true, then VS Code will always use those instead of those contributed by Jupyter extension or any other. I.e. the user has the ability to configure their preference.

@brettfo
Copy link
Member Author

brettfo commented Jan 20, 2021

On a related note, is there a way that our extension can set the .ipynb kernelspec metadata? I'm thinking of this scenario where I don't have any Jupyter kernels installed locally, but I create a new notebook and want to share it with somebody that's using Jupyter Lab.

I tried setting the notebook document's metadata to add the kernelspec part, but it wasn't serialized to disk; I'm guessing because your extension is explicitly setting that iff there's a Jupyter kernel specified. Can we have an override for that metadata?

This paired with some hard-coded logic to select our kernel if notebook.metadata.kernelspec.name === '.net-csharp' (or .net-fsharp, .net-powershell, or .startsWith('.net-')) should get us everything we need.

@greazer greazer changed the title Allow mapping between ipynb kernel_info and VS Code kernels without Jupyter kernelspec files. Provide a mechanism for an extension to determine or set a preferred kernel. Jan 21, 2021
@DonJayamanne
Copy link
Contributor

@brettfo
The metadata is stored in the document.metadata.custom property. This stores data thats of type Partial<nbformat.INotebookContent> where nbformat is a namespace from the Jupyter package. See here import { nbformat } from '@jupyterlab/coreutils';

You could update the metadata using the VSCode API.
If the object is null, then initialize it with a simple dict thats matches teh type of Partial<nbformat.INotebookContent>.

Once done, that should work.

However I can also see there's a bug at our end, we need to clear the kernel selection when user selects another kenerl (we update the metadata with the last selected Jupyter kernel).
Simple enough fix at our end.

@brettfo
Copy link
Member Author

brettfo commented Jan 26, 2021

@DonJayamanne thanks for the metadata pointer, I'm now able to set kernelspec metadata (and any other metadata) and get it serialized on save.

So now we just need a way for this document metadata to somehow be mappable to our VS Code kernel when there are no Jupyter kernels installed on disk. E.g., if jupyter kernelspec list returns nothing or if I don't have anything Jupyter-related installed, then how can the document/kernelspec metadata that looks like this:

...
"metadata": {
    "kernelspec": {
        "display_name": ".NET (C#)",
        "name": ".net-csharp"
    }
}
...

know to map to the VS Code concept of a kernel with id === 'dotnet-interactive'? Should we add "vscode": { "extension_id": "ms-dotnettools.dotnet-interactive-vscode", "kernel_id": "dotnet-interactive" } to the metadata as well? My only issue with that is that Jupyter Lab will only write out the kernelspec part without us developing a custom Jupyter extension purely to write this additional metadata.

@DonJayamanne
Copy link
Contributor

only issue with that is that Jupyter Lab will only write out the kernelspec part without us developing a custom Jupyter

In this instance, doesn't this mean the user already has Jupyter kernels installed.

@rchiodo
Copy link
Contributor

rchiodo commented Feb 18, 2021

Changing this to point release so this works for the .NET team's release date.

@rchiodo rchiodo self-assigned this Feb 22, 2021
@IanMatthewHuff
Copy link
Member

@rchiodo Do we have a specific validation here? Did this work for .NET interactive release? If so I would consider that validation.

@rchiodo
Copy link
Contributor

rchiodo commented Mar 2, 2021

For validation: Install both us and .NET interactive. Make sure python and .NET notebooks work.

@IanMatthewHuff
Copy link
Member

@rchiodo Not sure if this is on our side our their side. But currently when I do the command to create a new blank .NET Interactive notebook it pops up and briefly shows C# (.NET Interactive) as the cell language. Then immediately, without changing it manually, it changes to Python. When launching it tries to launch with my current interpreter.

@rchiodo
Copy link
Contributor

rchiodo commented Mar 2, 2021

For an existing notebook that was saved to use the .NET kernel, does that still open and use the .NET kernel? I believe that's what this bug was about.

Creating blank notebooks is a separate issue.

@IanMatthewHuff
Copy link
Member

That scenario doesn't work right for me either. It actually does open the saved .NET notebook correctly. But I then opened up a previous saved python notebook of mine. It started up a .net kernel when I opened it, and changed all the cell languages to C#. You can see both kernels starting up in the .Net interactive log. VariableTest.ipynb is a python notebook that I use frequently that has a saved kernelspec.

image

@IanMatthewHuff
Copy link
Member

@rchiodo Preference on me pulling this back to backlog versus resolving it (since the saved .NET file does open and use the .NET kernel) and opening a new issue for the issue above?

@rchiodo
Copy link
Contributor

rchiodo commented Mar 2, 2021

That last part is the bug then. That's what is supposed to work. We should open a new bug on .NET interactive to fix that.

@rchiodo
Copy link
Contributor

rchiodo commented Mar 2, 2021

I think we would then mark this bug as upstream.

@IanMatthewHuff
Copy link
Member

IanMatthewHuff commented Mar 2, 2021

Sorry, didn't get around to looking at this until after lunch. I see the same behavior on stable VS Code with our release branch bits. Still sounds like from what you said it's upstream either way, so I'll file an issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants