Support remote commands in the kernel picker #10602

rchiodo · 2022-06-28T00:20:39Z

Refactor remote support such that:

There's a top level command in the kernel picker for switching to remote
You can only be in 'remote' or 'local' mode
The remote picker lets you type the URI right into the first stage
If you're in remote mode already, you can
- Enter another URI
- Go back to local mode
If you're in local mode, you can enter remote mode by entering a URI
The 'Jupyter specify server connection' command also is the same URI picker too. No more 'None' and no more 'Existing' step.

Telemetry for these new modes is already handled. Commands send telemetry on their own and remote/local mode is already sent in telemetry.

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
Has a news entry file (remember to thank yourself!).
Appropriate comments and documentation strings in the code.
Has sufficient logging.
Has telemetry for feature-requests.
Unit tests & system/integration tests are added/updated.
Test plan is updated as appropriate.
package-lock.json has been regenerated by running npm install (if dependencies have changed).

rchiodo · 2022-06-28T00:21:04Z

This can be merged after endgame.

rchiodo · 2022-06-28T00:25:27Z

src/kernels/jupyter/launcher/serverConnectionType.ts

-export const mementoKeyToIndicateIfConnectingToLocalKernelsOnly = 'connectToLocalKernelsOnly';
-
-@injectable()
-export class ServerConnectionType {


I deleted this class and moved its functionality to the JupyterServerUriStorage class.

The reason for this is that this class was essentially exposing the internal state of the JupyterServerUriStorage class and they had to be in sync. And they weren't always.

To make this impossible, I just removed this class altogether and added a new interface to the JupyterServerUriStorage class. Most of the changes for this is simply using an interface instead of a class.

Good use case for why we should expose interfaces to the DI container and not classes.

rchiodo · 2022-06-28T00:25:51Z

src/kernels/jupyter/launcher/serverUriStorage.ts

    ) {
+        // Remember if local only


THis is the code that was originally in the serverConnectionType.ts file.

rchiodo · 2022-06-28T00:26:53Z

src/kernels/jupyter/serverSelector.ts

-            title: allowLocal
-                ? DataScience.jupyterSelectURIQuickPickTitle()
-                : DataScience.jupyterSelectURIQuickPickTitleRemoteOnly(),
+            acceptFilterBoxTextAsSelection: true,


Since URI entering happens on the quick pick, we can't use input box validation. So I added this to allow a modal dialog to appear.

rchiodo · 2022-06-28T00:28:19Z

src/notebooks/controllers/controllerLoader.ts

@@ -183,10 +180,6 @@ export class ControllerLoader implements IControllerLoader, IExtensionSyncActiva

    private async loadControllersImpl(cancelToken: vscode.CancellationToken) {
        let cachedConnections = await this.listKernels(cancelToken, 'useCache');
-        // Remove all remove kernels if we're no longer interested in them.


All filtering is done in the controllerRegistration.

rchiodo · 2022-06-28T00:29:47Z

src/platform/common/utils/multiStepInput.ts

+                            if (message) {
+                                resolvable = false;
+                                // No validation allowed on a quick pick. Have to put up a dialog instead
+                                await this.shell.showErrorMessage(message, { modal: true });


I think I need to change this to support markdown. For remote there's a URL the user can click on and it doesn't seem to be clickable at the moment. Maybe dialogs don't support this.

I think they don't sadly. Our "Install Python" popup tries to link to Python.org, but it's not clickable. Pretty sure when we talked about it in v-team that's just how it is.

rchiodo · 2022-06-28T00:30:42Z

src/standalone/activation/activationManager.ts

+        // Activate all activation services together. Don't fail them all if one fails.
+        await Promise.all([
+            this.singleActivationServices.map(async (item) => {
+                const promise = item.activate();


I had a bug in my activate and it made the extension not start. Figured this was better.

rchiodo · 2022-06-28T00:31:38Z

src/test/datascience/jupyter/serverSelector.unit.test.ts


            assert.equal(showInputBox.firstCall.args[0].value, expectedDefaultUri);
        }

        test('Display default uri', async () => {
            await testDefaultUri(defaultUri);
        });
-        test('Display default uri if clipboard is empty', async () => {


Not supporting the clipboard anymore. Fixes the problem with web where the user may not have clipboard permissions.

New workflow doesn't really need clipboard as the user can also filter in the same entry so defaulting to the url in the clipboard doesn't really make sense.

DonJayamanne · 2022-06-28T21:45:23Z

package.nls.json

@@ -955,5 +955,10 @@
    "Installer.dataScienceInstallPrompt": "Data Science library {0} is not installed. Install?",
    "Products.installingModule": "Installing {0}",
    "DataScience.webNotSupported": "Operation not supported in web version of Jupyter Extension.",
-    "DataScience.outputSizeExceedLimit": "Output exceeds the <a href={0}>size limit</a>. Open the full output data <a href={1}>in a text editor</a>"
+    "DataScience.outputSizeExceedLimit": "Output exceeds the <a href={0}>size limit</a>. Open the full output data <a href={1}>in a text editor</a>",
+    "DataScience.installPythonTitle": "Install Python",


@IanMatthewHuff looks like this will need to be reviewed closly as you're also working in the similar area (installing python when its missing). I.e. you might want to review the workflow & the changes.
Feels like this stuff related to no python might require its own folder/set of classes & its been worked on for months if not years (i.e. workflow keeps changing, hence better to keep it isolated & easier to manage)

Agai, I haven't reviewed anything yet, but just a thought about this (prompting to install python - getting started) part of the exetnsion

@DonJayamanne I think Rich was just adding on the Command localization that I missed in my addition. The only real conflict to consider here is just the fact that we won't have only one command anymore in the new user scenario. Means that we don't get that nice "just hit the run button" launch, but we can tweak that as we move forward. Maybe prioritization for commands in the kernel picker somehow.

src/kernels/jupyter/launcher/serverUriStorage.ts

codecov-commenter · 2022-06-28T22:12:16Z

Codecov Report

Merging #10602 (12db879) into main (b1e9cbe) will decrease coverage by 0%.
The diff coverage is 60%.

@@           Coverage Diff           @@
##            main   #10602    +/-   ##
=======================================
- Coverage     71%      70%    -1%     
=======================================
  Files        475      475            
  Lines      28056    28205   +149     
  Branches    4706     4736    +30     
=======================================
- Hits       19990    19977    -13     
- Misses      6187     6348   +161     
- Partials    1879     1880     +1

Impacted Files	Coverage Δ
src/notebooks/controllers/remoteSwitcher.ts	`94% <0%> (ø)`
src/notebooks/controllers/types.ts	`100% <ø> (ø)`
...ers/commands/serverConnectionControllerCommands.ts	`31% <31%> (ø)`
...rc/kernels/jupyter/launcher/commandLineSelector.ts	`43% <50%> (ø)`
.../notebooks/controllers/vscodeNotebookController.ts	`80% <50%> (-1%)`	⬇️
src/platform/common/utils/multiStepInput.ts	`68% <51%> (-6%)`	⬇️
src/kernels/jupyter/serverSelector.ts	`61% <68%> (+<1%)`	⬆️
src/platform/common/utils/localize.ts	`75% <71%> (-1%)`	⬇️
src/kernels/jupyter/launcher/serverUriStorage.ts	`69% <84%> (-3%)`	⬇️
...rc/notebooks/controllers/controllerRegistration.ts	`95% <97%> (+13%)`	⬆️
... and 41 more

src/notebooks/controllers/controllerRegistration.ts

IanMatthewHuff · 2022-06-30T18:59:56Z

src/kernels/jupyter/launcher/serverUriStorage.ts

        await this.addToUriList(uri, Date.now(), displayName);
+        await this.setUri(uri);


Looks like addToUriList is happening in two spots? Both here and in setUri, do we need both?

IanMatthewHuff · 2022-06-30T19:06:39Z

src/kernels/jupyter/serverSelector.ts

@@ -188,17 +157,17 @@ export class JupyterServerSelector {
                    );
                    items.splice(items.indexOf(e.item), 1);
                    onDidChangeItems.fire(items.concat([]));
+                } else if (e.button) {


Sorry a bit lost on this bit. Maybe there are just more buttons added and I'm not there in the review yet? I thought there was only the remove button. So having this case seems odd since it's not scoped to a specific button. Just any button that doesn't match the remove button goes back?

Yeah there's only two buttons for the server selector. Back and the 'delete' this URI.

IanMatthewHuff · 2022-06-30T19:23:14Z

src/notebooks/controllers/commands/serverConnectionControllerCommands.ts

+
+        // Then group them by kind
+        const kernelsPerCategory = groupBy(picks, (a, b) =>
+            compareIgnoreCase(a.controller.controller.kind || 'z', b.controller.controller.kind || 'z')


Is the 'z' for alphabetizing? Doesn't seem like that would work for non-en.

I copied this from the core, but it does seem wrong. 'z' only works in ascii.

I'll enter an issue to relook at this in both the core and here.

IanMatthewHuff · 2022-06-30T19:29:00Z

src/notebooks/controllers/controllerRegistration.ts


    public get onCreated(): Event<IVSCodeNotebookController> {
        return this.creationEmitter.event;
    }
    public get values(): IVSCodeNotebookController[] {
        return [...this.registeredControllers.values()];
    }
-    private get connections(): KernelConnectionMetadata[] {
-        return [...this.registeredConnections.values()];
+    private get metadatas(): KernelConnectionMetadata[] {


Just a naming opinion here. But to me this takes it the wrong direction. Metadata is super generic. Can mean anything. Before a 'connection' kinda represented a possible connection. It's wordier, but 'connectionMetadata' if we wanted to be clearer.

IanMatthewHuff · 2022-06-30T19:32:10Z

src/platform/common/utils/multiStepInput.ts

+                            if (message) {
+                                resolvable = false;
+                                // No validation allowed on a quick pick. Have to put up a dialog instead
+                                await this.shell.showErrorMessage(message, { modal: true });


I think they don't sadly. Our "Install Python" popup tries to link to Python.org, but it's not clickable. Pretty sure when we talked about it in v-team that's just how it is.

IanMatthewHuff · 2022-06-30T19:34:04Z

Seems good to me. This is going to be a big improvement. Honestly the hardest part of this PR was that I find the MultiStep input control flow really wacky and hard to read. But that's not anything on you or this PR, I think I just need to play with that myself a bit versus looking at the PR.

rchiodo · 2022-06-30T19:57:01Z

Seems good to me. This is going to be a big improvement. Honestly the hardest part of this PR was that I find the MultiStep input control flow really wacky and hard to read. But that's not anything on you or this PR, I think I just need to play with that myself a bit versus looking at the PR.

I agree about the multistep input. It's really a state machine. I think it would be better with an iterator type pattern instead of a promise type pattern. Took me a long time to get the promises to work out correctly.

sadasant · 2022-07-01T21:01:05Z

src/kernels/jupyter/serverSelector.ts

@@ -54,11 +51,7 @@ export type SelectJupyterUriCommandSource =
    | 'prompt';
 @injectable()
 export class JupyterServerSelector {
-    private readonly localLabel = `$(zap) ${DataScience.jupyterSelectURINoneLabel()}`;
-    private readonly newLabel = `$(server) ${DataScience.jupyterSelectURINewLabel()}`;
-    private readonly remoteLabel = `$(server) ${DataScience.jupyterSelectURIRemoteLabel()}`;


Are these not needed anymore?

No they shouldn't be. Maybe. I think we might need the 'None' label.

sadasant · 2022-07-01T21:08:42Z

src/notebooks/controllers/commands/serverConnectionControllerCommands.ts

+        }
+    }
+    return result;
+}


Ohh fancy! Would it make sense to have a common place for functional utilities like this?

Maybe. Not sure where that would be. It's only used in this class at the moment.

sadasant · 2022-07-01T21:13:23Z

src/test/datascience/debugger.vscode.test.ts

@@ -118,7 +118,7 @@ suite('VSCode Notebook - Run By Line', function () {
            );

            // Give the files a quick chance to clean up
-            await sleep(1000);
+            await sleep(3000);


Is this necessary?

Test was failing attempting to see if files were cleaned up. Stopped failing after I did this, so yeah probably.

There's likely another way to accomplish this but I wasn't working on this code so I just wanted the test to pass

Ok that makes sense! Thanks for the context

src/test/datascience/interactiveDebugging.vscode.common.ts

rchiodo added 20 commits June 22, 2022 16:59

Add commands and fixup the server selector

c1bb432

Merge remote-tracking branch 'origin/main' into rchiodo/toggle_remote

9690f55

Filter list of controllers by connection type

e667411

Wire up commands

22f7599

Update telemetry

c610482

Add another command for when already remote

4e05ded

Make sure activation failures don't stop other activations

4a3e97b

Merge remote-tracking branch 'origin/main' into rchiodo/toggle_remote

a4e478c

Some more parts working

ca03019

Using multistep to setup back button and filtering

5c5a7db

Update telemetry

0af1629

Fix showing duplicates in custom quick pick

abd9596

Fix localization and refreshing between local and remote

da88bc5

First try at a test

4361612

Remove ability to set local launch independent of URI

a8073e8

Fix back button

5ab57a7

Get all tests passing

dc68e1e

Merge remote-tracking branch 'origin/main' into rchiodo/toggle_remote

d8a013a

Update telemetry

40ea168

Add news entries

ed9d88e

rchiodo requested a review from a team as a code owner June 28, 2022 00:20

rchiodo marked this pull request as draft June 28, 2022 00:20

rchiodo commented Jun 28, 2022

View reviewed changes

rchiodo changed the title ~~Rchiodo/toggle remote~~ Support remote commands in the kernel picker Jun 28, 2022

DonJayamanne reviewed Jun 28, 2022

View reviewed changes

src/kernels/jupyter/launcher/serverUriStorage.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Jun 28, 2022

View reviewed changes

src/kernels/jupyter/launcher/serverUriStorage.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Jun 28, 2022

View reviewed changes

src/kernels/jupyter/launcher/serverUriStorage.ts Outdated Show resolved Hide resolved

Fix isLocalLaunch to actually use the internal flag

efcafc8

DonJayamanne reviewed Jun 28, 2022

View reviewed changes

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

rchiodo added 4 commits June 29, 2022 16:53

Fix unit tests and use serverId

c188e9f

Merge remote-tracking branch 'origin/main' into rchiodo/toggle_remote

14f9dcc

Update telemetry

13d7912

Fix bug in load order

9094eb8

IanMatthewHuff reviewed Jun 30, 2022

View reviewed changes

rchiodo mentioned this pull request Jun 30, 2022

Kind ordering for kernels is 'english' specific #10656

Closed

DonJayamanne approved these changes Jun 30, 2022

View reviewed changes

IanMatthewHuff approved these changes Jun 30, 2022

View reviewed changes

rchiodo added 2 commits June 30, 2022 14:44

Merge remote-tracking branch 'origin/main' into rchiodo/toggle_remote

f98ba1f

Update telemetry

12db879

rchiodo marked this pull request as ready for review June 30, 2022 21:53

rchiodo merged commit e92cac6 into main Jun 30, 2022

rchiodo deleted the rchiodo/toggle_remote branch June 30, 2022 22:27

sadasant reviewed Jul 1, 2022

View reviewed changes

src/test/datascience/interactiveDebugging.vscode.common.ts Show resolved Hide resolved

		await this.addToUriList(uri, Date.now(), displayName);
		await this.setUri(uri);

Support remote commands in the kernel picker #10602

Support remote commands in the kernel picker #10602

Uh oh!

Conversation

rchiodo commented Jun 28, 2022

Uh oh!

rchiodo commented Jun 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff commented Jun 30, 2022

Uh oh!

rchiodo commented Jun 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 28, 2022 •

edited

Loading