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

Kernel architecture review and polish #10832

Closed
8 tasks done
rebornix opened this issue Jul 15, 2022 · 5 comments
Closed
8 tasks done

Kernel architecture review and polish #10832

rebornix opened this issue Jul 15, 2022 · 5 comments
Assignees
Labels
debt Code quality issues
Milestone

Comments

@rebornix
Copy link
Member

rebornix commented Jul 15, 2022

This issue covers the architecture review and polish @DonJayamanne and I are working on. The main goals of this task are

  • Finalize how the APIs from src/kernels are being used by the core system (notebooks, interactive-window etc) and 3rd party extensions. There will be restriction and clear separation between internal and external use of the kernel interfaces.
  • Internal architecture review and polish of src/kernels component. The kernel component will stay minimal and limited responsibilities without worrying about business logics of how kernels are being used (i.e., how to read variables, how to install dependencies into the kernels, etc)

Here is a list of tasks we will tackle:

Tasks

No tasks being tracked yet.
@rebornix rebornix added bug Issue identified by VS Code Team member as probable bug debt Code quality issues and removed bug Issue identified by VS Code Team member as probable bug labels Jul 15, 2022
@github-actions github-actions bot added the triage-needed Issue needs to be triaged label Jul 15, 2022
@greazer greazer removed the triage-needed Issue needs to be triaged label Jul 18, 2022
@greazer greazer added this to the July 2022 milestone Jul 18, 2022
@rebornix
Copy link
Member Author

rebornix commented Jul 20, 2022

A gist of our discussion of how the kernel detection and launching works currently

image

@rebornix
Copy link
Member Author

rebornix commented Jul 26, 2022

We have polished and narrowed down the API we expose to the core and 3rd party extensions. The core currently only needs to use IKernelFinder, IKernelProvider, and IKernel. The core can always get a IKernel which is attached to a NotebookDocument and NotebookController while 3rd party extensions can request for headless kernels but those kernels won't be visible to the core system.

In August, we would look into what we can do to improve IKernelFinder and see if we can make it transparent to other components or make it easier to use. Along with that we would have telemetry ready to give better insights into how each kernel type (raw, pyenv, remote) works in overall right now.

Draft interface we discussed offline:

interface KernelManager {
    registerKernelProvider(provider: KernelProvider)
}

interface KernelProvider {
    kernelInfos: KernelConnectionMetadata[];
    onDidChangeKernelInfo: Event<void>;

    getKernelOrKernelFactory(metadata: KernelConnectionMetadata)
}

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jul 26, 2022

Here's my proposal (from my private project)

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { Kernel, ServerConnection } from '@jupyterlab/services';
import { Disposable, Uri } from 'vscode';

export interface KernelSpec {
    readonly language?: string;
    readonly env?: NodeJS.ProcessEnv;
    readonly metadata?: Record<string, unknown>;
    readonly argv: string[];
    readonly interrupt_mode?: 'message' | 'signal';
}
type BaseKernelConnectionInfo = {
    id: string;
    /**
     * Languages supported by the kernel.
     * First language in the list is considered to be the default & primary language of the kernel.
     * Cannot be empty.
     */
    languages: string[];
    label: string;
    description?: string;
    detail?: string;
    /**
     * Connections with this same value will be grouped together.
     */
    group: string;
    /**
     * Sort order for connections.
     */
    sort: string;
};
export type KernelSpecConnectionInfo = BaseKernelConnectionInfo & {
    resolve: (resource: Uri | undefined) => Promise<KernelSpec>;
};
export type KernelFactoryConnectionInfo = BaseKernelConnectionInfo & {
    create(resource: Uri | undefined, settingsFactory: SettingsFactory): Promise<Kernel.IKernelConnection>;
};
export interface SettingsFactory {
    makeSettings(options?: Partial<ServerConnection.ISettings>): ServerConnection.ISettings;
}

export const KernelRegistry = Symbol('KernelRegistry');
export interface KernelRegistry {
    registerKernel(connection: KernelSpecConnectionInfo | KernelFactoryConnectionInfo): Disposable;
}

I don't think we need onDidChangeKernelnfo
Updated and i think we should discuss the changes and where we want this API/changes to go..

Note: The above API is structured with the intent of exposing them for 3rd party.
We could treat the local kernel finder and remote kernel finders and the like as standalone components in the extension that contribute kernels using the above API...
Lets discuss

@rebornix
Copy link
Member Author

The only task left is simplying kernel finder, which is now tracked separately in #11153.

@DonJayamanne
Copy link
Contributor

Closing this issue for now, as we have other issues to track this work

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

3 participants