Move NotebookControllerManager into the DI container for web #9690

rchiodo · 2022-04-14T22:30:38Z

rchiodo · 2022-04-14T22:48:35Z

build/webpack/webpack.extension.web.config.js

@@ -32,23 +32,6 @@ const config = {
    },
    module: {
        rules: [
-            {
-                // JupyterServices imports node-fetch.


As soon as I started importing these into the web bundle, this code failed to work.

I believe we don't need in the webbundle anymore.

Yeah, I'm not seeing the node-fetch usage anymore in jupyterlab services.

rchiodo · 2022-04-14T22:49:25Z

src/intellisense/serviceRegistry.node.ts

@@ -16,4 +21,15 @@ export function registerTypes(serviceManager: IServiceManager, _isDevMode: boole
        IExtensionSingleActivationService,
        PythonKernelCompletionProviderRegistration
    );
+    serviceManager.addSingleton<IExtensionSyncActivationService>(


Moved a bunch of stuff here from the 'notebooks' serviceRegistry. Seemed weird that notebooks would register stuff from the intellisense folder.

rchiodo · 2022-04-14T22:53:16Z

src/interactive-window/interactiveWindow.node.ts

@@ -186,7 +173,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
            };
            // When connecting, we need to update the sys info message
            this.updateSysInfoMessage(this.getSysInfoMessage(metadata, SysInfoReason.Start), false, sysInfoCell);
-            const kernel = await connectToKernel(
+            const kernel = await KernelConnector.connectToKernel(


I moved all the loose functions for connecting to a kernel into a new static class - KernelConnector.

rchiodo · 2022-04-14T22:54:25Z

src/kernels/installer/productInstaller.ts

+ * Keep track of the fact that we attempted to install a package into an interpreter.
+ * (don't care whether it was successful or not).
+ */
+export async function trackPackageInstalledIntoInterpreter(


These loose functions didn't have any .node dependencies but were used elsewhere so moved them into a separate file.

Sometimes those are helpers.ts? Or does that note fit here?

rchiodo · 2022-04-14T22:55:41Z

src/kernels/ipywidgets-message-coordination/ipyWidgetScriptSourceProvider.ts

-        }
-
-        this.scriptProviders = scriptProviders;
+        // Use the platform specific factory to get our providers


In order to make this class platform agnostic, the creation of the script providers was moved to a platform specific factory.

The web version only support remote right now, but we might be able to do something about loading it from the CDN later.

rchiodo · 2022-04-14T22:56:15Z

src/kernels/ipywidgets-message-coordination/scriptSourceProviderFactory.node.ts

+        @inject(IPythonExecutionFactory) private readonly factory: IPythonExecutionFactory
+    ) {}
+
+    public getProviders(kernel: IKernel, uriConverter: ILocalResourceUriConverter, httpClient: IHttpClient) {


This function used to be in ipyWidgetScriptSourceProvider.ts. This is the node version.

codecov-commenter · 2022-04-14T22:56:18Z

Codecov Report

Merging #9690 (49adb87) into main (ecc0aaf) will increase coverage by 0%.
The diff coverage is 77%.

❗ Current head 49adb87 differs from pull request most recent head bd0382a. Consider uploading reports for the commit bd0382a to get more accurate results

@@         Coverage Diff          @@
##           main   #9690   +/-   ##
====================================
  Coverage    63%     63%           
====================================
  Files       201     204    +3     
  Lines      9826    9837   +11     
  Branches   1557    1556    -1     
====================================
+ Hits       6230    6245   +15     
+ Misses     3097    3091    -6     
- Partials    499     501    +2

Impacted Files	Coverage Δ
src/platform/api/apiAccessService.ts	`44% <ø> (ø)`
src/platform/common/application/clipboard.ts	`42% <ø> (ø)`
src/platform/common/application/documentManager.ts	`82% <ø> (ø)`
...rc/platform/common/application/encryptedStorage.ts	`44% <ø> (ø)`
src/platform/common/application/notebook.ts	`87% <ø> (ø)`
src/platform/common/asyncDisposableRegistry.ts	`75% <ø> (ø)`
src/platform/common/platform/types.ts	`100% <ø> (ø)`
src/platform/common/utils/multiStepInput.ts	`75% <ø> (ø)`
src/platform/common/utils/regexp.ts	`66% <ø> (ø)`
.../platform/common/variables/systemVariables.node.ts	`39% <0%> (ø)`
... and 52 more

rchiodo · 2022-04-14T22:56:36Z

src/kernels/ipywidgets-message-coordination/scriptSourceProviderFactory.web.ts

+@injectable()
+export class ScriptSourceProviderFactory implements IWidgetScriptSourceProviderFactory {
+    public getProviders(kernel: IKernel, _uriConverter: ILocalResourceUriConverter, httpClient: IHttpClient) {
+        const scriptProviders: IWidgetScriptSourceProvider[] = [];


THis is the web version of the same function.

rchiodo · 2022-04-14T23:00:58Z

src/kernels/ipywidgets-message-coordination/scriptUriConverter.node.ts

+const sanitize = require('sanitize-filename');
+
+@injectable()
+export class ScriptUriConverter implements ILocalResourceUriConverter {


I also had to separate out the URI converter part from the ipyWidgetScriptSource because it has node specific stuff in it too (reading/writing to local disk)

rchiodo · 2022-04-14T23:03:32Z

src/kernels/jupyter/remoteKernelFinder.web.ts

+        _connInfo: INotebookProviderConnection | undefined
+    ): Promise<KernelConnectionMetadata[]> {
+        sendKernelListTelemetry(resource, []);
+        return [];


This will likely be the same as the node version once I get all of the session classes below this to work. For now there are two so I can stop this PR at this point.

rchiodo · 2022-04-14T23:04:38Z

src/kernels/raw/finder/localKernelFinder.web.ts

+// Instead there will be a IKernelFinder for web and an IKernelFinder for node. It will coalesce the local/remote bits.
+// But for now, it was simpler to just make this kernel finder that returns nothing.
+@injectable()
+export class LocalKernelFinder implements ILocalKernelFinder {


This class should be temporary. The reason this exists is because the NotebookControllerManager is doing special logic for local.

I hope to remove this entirely in a IKernelFinder implementation later.

rchiodo · 2022-04-14T23:07:44Z

src/kernels/helpers.node.ts

 import { getDisplayPath } from '../platform/common/platform/fs-paths';
 import { IPythonExecutionFactory } from '../platform/common/process/types.node';
-import {


Most of this file moved to helpers.ts. Only the 'node' specific stuff remained here.

rchiodo · 2022-04-14T23:08:42Z

src/kernels/kernel.base.ts

+import { SilentExecutionErrorOptions } from './helpers';
+import { originalFSPath } from '../platform/vscode-path/resources';
+
+export abstract class BaseKernel implements IKernel {


Created this as the common parts of the IKernel implementation.

rchiodo · 2022-04-14T23:09:27Z

src/kernels/kernel.node.ts

+            configService,
+            workspaceService,
+            cellHashProviderFactory
+        );
        this.kernelExecution = new KernelExecution(


KernelExecution is node specific at the moment, so chopped off this work at this point.

When I get to implementing the kernel for web, KernelExecution will likely be the starting point.

rchiodo · 2022-04-14T23:10:20Z

src/kernels/kernelConnector.ts

+/**
+ * Class used for connecting a controller to an instance of an IKernel
+ */
+export class KernelConnector {


This is all the functions that used to be in helper.ts for connecting to a kernel. There was even a little bit of state. Figured it made sense to put the functions into a class to hold the state.

rchiodo · 2022-04-14T23:11:00Z

src/kernels/kernelProvider.base.ts

+import { noop } from '../platform/common/utils/misc';
+import { IKernel, IKernelProvider, KernelOptions } from './types';
+
+export abstract class BaseKernelProvider implements IKernelProvider {


KernelProvider creates the Kernel class directly so I had to split it into two parts as well. This is the base class for them both.

rchiodo · 2022-04-14T23:11:29Z

src/kernels/kernelProvider.node.ts

-        this._onDidDisposeKernel.dispose();
-        this._onDidRestartKernel.dispose();
-        this._onKernelStatusChanged.dispose();
-    }
    public getOrCreate(uri: Uri, options: KernelOptions): IKernel {


This is the only function that's specific to each version.

rchiodo · 2022-04-14T23:14:52Z

src/notebooks/controllers/condaControllerRefresher.node.ts

+import { INotebookControllerManager } from '../types';
+
+@injectable()
+export class CondaControllerRefresher implements IExtensionSingleActivationService {


This file exists because the NotebookControllerManager used to listen to the conda service in order to refresh its list of controllers.

Conda Service is node only, so I created this one off to accomplish the same thing.

rchiodo · 2022-04-14T23:17:11Z

src/notebooks/controllers/vscodeNotebookController.ts

 import { ConsoleForegroundColors, TraceOptions } from '../../platform/logging/types';
+import { KernelConnector } from '../../kernels/kernelConnector';

 export class VSCodeNotebookController implements Disposable {


This file was the reason for this PR. And just getting this to build caused all of these other changes :)

IanMatthewHuff · 2022-04-14T23:18:36Z

src/interactive-window/helpers.ts

 import { IInteractiveWindowProvider, IInteractiveWindow } from './types';

+export type InteractiveCellMetadata = {


We have a src/interactive-window/types.ts maybe put the type into that?

rchiodo · 2022-04-14T23:18:48Z

src/platform/common/utils/platform.node.ts

 import { fsPathToUri } from '../../vscode-path/utils';
 import { EnvironmentVariables } from '../variables/types';
 import { getOSType, OSType } from './platform';
 export * from './platform';

-// eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
-const untildify = require('untildify');
-const homePath = untildify('~');


This was kinda redundant. Untilidify uses the homedir to replace ~.

rchiodo · 2022-04-14T23:19:11Z

src/platform/common/utils/platform.ts

@@ -22,3 +22,7 @@ export function getOSType(platform: string = process.platform): OSType {
        return OSType.Unknown;
    }
 }
+
+export function untildify(path: string, home: string) {


This is the platform neutral version.

rchiodo · 2022-04-14T23:19:45Z

src/platform/common/crypto.ts

 import { injectable } from 'inversify';
 import { traceError } from '../logging';
 import { ICryptoUtils, IHashFormat } from './types';
+import * as hashjs from 'hash.js';


We don't need a secure hash for this stuff, we just use it to create names of files.

Plus crypto is deprecated I think.

rchiodo · 2022-04-14T23:20:39Z

src/platform/pythonEnvironments/info/interpreter.ts

+import { PythonEnvironment } from '.';
+import { getOSType, OSType } from '../../common/utils/platform';
+
+export function getInterpreterHash(interpreter: PythonEnvironment | {uri: Uri}){


All of these functions could be used in both .node and .web.

IanMatthewHuff · 2022-04-14T23:48:17Z

src/platform/vscode-path/resources.ts

 import { compare as strCompare, equalsIgnoreCase } from './strings';
 import { Schemas } from './utils';

-function originalFSPath(uri: URI): string {
-    return uri.fsPath;
+export function originalFSPath(uri: URI | undefined): string {


I might not have payed enough attention to where this was used, but the name seems a bit off now. A .path property for a web URI isn't an fspath.

Actually looking at the usage I think this works. NVMD

rchiodo added 5 commits April 13, 2022 17:34

Preliminary work

eac3cb4

Fix build errors and unit tests

54c4224

Turn on noImplicitOverrides and create the kernelProvider for web

9bf7798

Add cellhashprovider to web container

5bdc8f6

Fixup DI problem in node version

ecb66b5

rchiodo requested a review from a team as a code owner April 14, 2022 22:30

Linter problem

ace93c1

rchiodo commented Apr 14, 2022

View reviewed changes

IanMatthewHuff reviewed Apr 14, 2022

View reviewed changes

rchiodo commented Apr 14, 2022

View reviewed changes

IanMatthewHuff reviewed Apr 14, 2022

View reviewed changes

IanMatthewHuff approved these changes Apr 14, 2022

View reviewed changes

rchiodo added 3 commits April 14, 2022 16:53

Fix vscode test failures

d38a8e0

Fix launching jupyter server and smoke for web

49adb87

Fix unit tests on windows and add some missing bindings for web

bd0382a

rchiodo merged commit 261afd9 into main Apr 15, 2022

rchiodo deleted the rchiodo/notebook_controller_web branch April 15, 2022 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move NotebookControllerManager into the DI container for web #9690

Move NotebookControllerManager into the DI container for web #9690

rchiodo commented Apr 14, 2022

rchiodo Apr 14, 2022

IanMatthewHuff Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

IanMatthewHuff Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

codecov-commenter commented Apr 14, 2022 •

edited

Loading

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

IanMatthewHuff Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

rchiodo Apr 14, 2022

IanMatthewHuff Apr 14, 2022

rchiodo Apr 14, 2022

IanMatthewHuff Apr 14, 2022

IanMatthewHuff Apr 14, 2022

		import { IInteractiveWindowProvider, IInteractiveWindow } from './types';

		export type InteractiveCellMetadata = {

Move NotebookControllerManager into the DI container for web #9690

Move NotebookControllerManager into the DI container for web #9690

Conversation

rchiodo commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 14, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 14, 2022 •

edited

Loading