Support returning a preferred remote controller #9978

DonJayamanne · 2022-05-11T04:06:55Z

Part of #9683

Part of this PR:

Introduce the method selectJuptyerUri (but renamed to getSuggestedController)
Ensure extensions won't need to add an entry named pythonRemoteServerProvider into their package.json
Rename computeUri to computeServerId, that seems more meaningful as that's exactly what it is.
Throw an error to handle situations where server providers (3rd party extensions) cannot return the IJupyterServerUri information (this way we can display meaning full errors)
Changes to code to return preferred remote notebook controller (look for kernelspec name, else just fallback to the language)

Note:
The api method selectJuptyerUri described here microsoft/vscode#146942 has been renamed to getSuggestedController, as the Jupyter API doesn't select any controller, it merely returns the suggested controller for a given remote Jupyter server.
(discussed with Peng), and will update the upstream issue once this work has been completed.

DonJayamanne · 2022-05-11T04:11:09Z

.vscode/settings.json

@@ -1,7 +1,7 @@
 // Place your settings in this file to overwrite default and user settings.
 {
    "files.exclude": {
-        "out": true, // set this to true to hide the "out" folder with the compiled JS files
+        "out": false, // set this to true to hide the "out" folder with the compiled JS files


I think this is ok to have, i keep setting this.

DonJayamanne · 2022-05-11T04:11:46Z

package.nls.json

-    "DataScience.remoteJupyterConnectionFailedWithServer": "Failed to connect to the remote Jupyter Server {0}. Verify the server is running and reachable.",
-    "DataScience.remoteJupyterConnectionFailedWithServerWithError":"Failed to connect to the remote Jupyter Server {0}. Verify the server is running and reachable. ({1}).",
+    "DataScience.remoteJupyterConnectionFailedWithServer": "Failed to connect to the remote Jupyter Server '{0}'. Verify the server is running and reachable.",
+    "DataScience.remoteJupyterConnectionFailedWithServerWithError":"Failed to connect to the remote Jupyter Server '{0}'. Verify the server is running and reachable. ({1}).",


Was previously displaying the baseUrl, but in the case of somethign like Azure ML, we can display the displayName (which could contain spaces), hence quoting this string.

DonJayamanne · 2022-05-11T04:12:51Z

src/kernels/jupyter/commands/serverSelector.ts

@@ -48,7 +48,7 @@ export class JupyterServerSelectorCommand implements IDisposable {
            // Find one that is the default for this remote
            if (notebook) {
                // Recompute the preferred controller
-                await this.controllerManager.computePreferredNotebookController(notebook);
+                await this.controllerManager.initializePreferredNotebookController(notebook);


This was not only computing the preferred controller, it was also setting it using the VS Code api.
I've renamed this to be more specific.
The method computePreferredNotebookController is now used to return the preferred controller without changing it.

DonJayamanne · 2022-05-11T04:13:33Z

src/kernels/jupyter/jupyterConnection.ts

@@ -65,7 +66,7 @@ export class JupyterConnection implements IExtensionSyncActivationService {
    private async getUriFromServerId(serverId: string) {
        // Since there's one server per session, don't use a resource to figure out these settings
        const savedList = await this.serverUriStorage.getSavedUriList();
-        return savedList.find((item) => computeUriHash(item.uri) === serverId)?.uri;
+        return savedList.find((item) => computeServerId(item.uri) === serverId)?.uri;


Renamed computeUriHash to be computeServerId, the implementation should not define the method name, its a server id, hence the name should reflect that.

DonJayamanne · 2022-05-11T04:14:12Z

src/kernels/jupyter/jupyterConnection.ts

                }
+            } catch (ex) {
+                throw new RemoteJupyterServerUriProviderError(idAndHandle.id, idAndHandle.handle, ex);


Throw a custom error so we can handle this, e.g. possible the jupyter server is down and user can now choose to ignore this connection completely.

DonJayamanne · 2022-05-11T04:15:51Z

src/kernels/jupyter/jupyterUriProviderRegistration.ts

@@ -53,14 +61,35 @@ export class JupyterUriProviderRegistration implements IJupyterUriProviderRegist
    }

    private async loadOtherExtensions(): Promise<void> {
+        const extensionIds = new Set<string>();
+        this.globalMemento


In the case of github extensions, they can register the lazy proxy controller when a notebook opens and they register the provider with us.
When we reload vscode, we try to get the provider information, however its possible github or other extensions haven't activated yet, for such extensions we don'e need an entry in package.json for pythonRemoteServerProvider, instead we can just keep track of the extensions that own a specific provider and load and activate those extensions.

DonJayamanne · 2022-05-11T04:16:22Z

src/kernels/jupyter/jupyterUriProviderWrapper.ts

@@ -14,6 +14,9 @@ export class JupyterUriProviderWrapper implements IJupyterUriProvider {
        return this.provider.id;
    }
    public getQuickPickEntryItems(): vscode.QuickPickItem[] {
+        if (!this.provider.getQuickPickEntryItems) {


Extensions such as github or others won't need to implement getQuickPickEntryItems (see type definition)

DonJayamanne · 2022-05-11T04:17:36Z

src/kernels/jupyter/serverSelector.ts

+
+    @captureTelemetry(Telemetry.EnterJupyterURI)
+    @traceDecoratorError('Failed to enter Jupyter Uri')
+    public async enterJupyterURI(): Promise<string | undefined> {


Wrote this code for server picker for our own lazy controller, don't see any harm keeping this here.
Not used for now, as i didn't want to checkin the layzy proxy controller as that API could change.
Requries a bit more work to remove this hence left it, hope its ok (the plan is to make use of this in the future)

src/kernels/jupyter/session/backingFileCreator.base.ts

DonJayamanne · 2022-05-11T04:19:22Z

src/kernels/jupyter/session/jupyterSession.ts

@@ -234,7 +235,7 @@ export class JupyterSession extends BaseJupyterSession {

        // Create our session options using this temporary notebook and our connection info
        const sessionOptions: Session.ISessionOptions = {
-            path: backingFile?.filePath || `${uuid()}.ipynb`, // Name has to be unique
+            path: backingFile?.filePath || generateBackingIPyNbFileName(this.resource), // Name has to be unique


Now all paths generate have a predictable name & we can strip out the unnecessary guff and have a meaningful and prettier display name in the kernel picker, previously we'd just display a guid.

DonJayamanne · 2022-05-11T04:20:09Z

src/kernels/jupyter/types.ts

+    readonly id: string;
+    onDidChangeHandlers?: Event<void>;
+    getQuickPickEntryItems?(): QuickPickItem[];
+    handleQuickPick?(item: QuickPickItem, backEnabled: boolean): Promise<JupyterServerUriHandle | 'back' | undefined>;


Extensions such as github or others won't need to implement getQuickPickEntryItems
Haven't hooked up the code to make use of this API just yet, but added this nevertheless, basically as defined here microsoft/vscode#146942

The code that uses this has a few more changes, figured i'd submit this PR without those changes as its pretty large as is.

DonJayamanne · 2022-05-11T04:21:34Z

src/kernels/kernelFinder.base.ts

        cancelToken?: CancellationToken,
-        useCache?: 'useCache' | 'ignoreCache'
+        useCache?: 'useCache' | 'ignoreCache',
+        serverId?: string


Added a new argument serverId so that we can filter out the kernels that apply to a specific remote jupyter server.
Applies to the lazy controller that spins up jupyter and then asks us to get the prefered controller (surely at that point we need to return the preferred controller specific to that jupyter server)

DonJayamanne · 2022-05-11T04:23:26Z

src/kernels/kernelFinder.base.ts

-            const isPythonNbOrInteractiveWindow = isPythonNotebook(notebookMetadata) || resourceType === 'interactive';
-
-            // Always include the interpreter in the search if we can
-            const preferredInterpreter =


Removed this code from the base method, the exact same code preferredInterpreter is calculated in the calling code, hence might as well pass that into this method instead of computing this here, also remove a few dependencies.

codecov-commenter · 2022-05-11T04:53:42Z

Codecov Report

Merging #9978 (0e4e290) into main (8f7f38e) will decrease coverage by 0%.
The diff coverage is 52%.

❗ Current head 0e4e290 differs from pull request most recent head ad7abf6. Consider uploading reports for the commit ad7abf6 to get more accurate results

@@         Coverage Diff          @@
##           main   #9978   +/-   ##
====================================
- Coverage    63%     63%   -1%     
====================================
  Files       208     209    +1     
  Lines      9912    9948   +36     
  Branches   1574    1591   +17     
====================================
+ Hits       6314    6323    +9     
- Misses     3090    3113   +23     
- Partials    508     512    +4

Impacted Files	Coverage Δ
src/platform/common/utils/localize.ts	`98% <ø> (ø)`
src/platform/errors/types.ts	`77% <ø> (ø)`
src/platform/api.ts	`48% <38%> (-9%)`	⬇️
src/platform/errors/errorHandler.ts	`64% <45%> (-4%)`	⬇️
...form/errors/remoteJupyterServerUriProviderError.ts	`88% <88%> (ø)`
...rc/platform/common/dataScienceSurveyBanner.node.ts	`65% <0%> (-6%)`	⬇️

src/notebooks/controllers/notebookControllerManager.ts

src/platform/api.ts

src/telemetry/index.ts

src/notebooks/controllers/notebookControllerManager.ts

IanMatthewHuff · 2022-05-11T18:05:09Z

src/notebooks/controllers/notebookControllerManager.ts

+                }
+                if (item.connection.kernelSpec.name === kernelName) {
+                    matchingKernelNameController = item;
+                } else if (item.connection.kernelSpec.language === language) {


When I moved over the exact match code I ran into one existing error case where the kernelspec language didn't match up with the notebook language_info languages due to capitalization, so maybe need a toLower check here? Pretty sure it was for a C# file.

Oh shoot, I thought I had a translation code, will use that.

The function that you can use to translate from kenel to monaco language is translateKernelLanguageToMonaco
Here it doesn't apply as we're only dealing with languages of kernelspecs & no language components from VS Code (monaco).

IanMatthewHuff

Two small comments, but looks good to me.

Support returning a preferred remote controller

dfccf92

DonJayamanne force-pushed the issue9683 branch from 75e748f to dfccf92 Compare May 11, 2022 04:10

Misc

c92e452

DonJayamanne commented May 11, 2022

View reviewed changes

src/kernels/jupyter/session/backingFileCreator.base.ts Show resolved Hide resolved

DonJayamanne commented May 11, 2022

View reviewed changes

DonJayamanne added 4 commits May 11, 2022 14:24

Misc

2311175

Misc

aeebb92

Misc

113c075

Misc

c093652

DonJayamanne added 5 commits May 11, 2022 16:30

Misc

499b82e

Misc

99c3734

Misc

13cba72

Misc

ab1ac8b

Misc

0e4e290

DonJayamanne marked this pull request as ready for review May 11, 2022 17:03

DonJayamanne requested a review from a team as a code owner May 11, 2022 17:04

rchiodo reviewed May 11, 2022

View reviewed changes

src/notebooks/controllers/notebookControllerManager.ts Outdated Show resolved Hide resolved

rchiodo reviewed May 11, 2022

View reviewed changes

src/platform/api.ts Outdated Show resolved Hide resolved

rchiodo reviewed May 11, 2022

View reviewed changes

src/telemetry/index.ts Show resolved Hide resolved

DonJayamanne added 2 commits May 12, 2022 03:37

Misc

6df8fda

Misc

ad7abf6

IanMatthewHuff reviewed May 11, 2022

View reviewed changes

src/notebooks/controllers/notebookControllerManager.ts Show resolved Hide resolved

IanMatthewHuff reviewed May 11, 2022

View reviewed changes

IanMatthewHuff approved these changes May 11, 2022

View reviewed changes

rchiodo approved these changes May 11, 2022

View reviewed changes

DonJayamanne merged commit 38caef1 into main May 11, 2022

DonJayamanne deleted the issue9683 branch May 11, 2022 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support returning a preferred remote controller #9978

Support returning a preferred remote controller #9978

DonJayamanne commented May 11, 2022 •

edited

Loading

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

codecov-commenter commented May 11, 2022 •

edited

Loading

IanMatthewHuff May 11, 2022

DonJayamanne May 11, 2022

DonJayamanne May 11, 2022

IanMatthewHuff left a comment

Support returning a preferred remote controller #9978

Support returning a preferred remote controller #9978

Conversation

DonJayamanne commented May 11, 2022 • edited Loading

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 May 11, 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

IanMatthewHuff left a comment

Choose a reason for hiding this comment

DonJayamanne commented May 11, 2022 •

edited

Loading

codecov-commenter commented May 11, 2022 •

edited

Loading