Enable an experimental setting for the new kernel picker UI #10807

rchiodo · 2022-07-14T17:48:37Z

For the most part I left a copy of the code for the UI parts. This was simpler than trying to make the new code handle the old UI and the new.

…_experiment

rchiodo · 2022-07-14T18:09:30Z

package.json

+                    "description": "%jupyter.configuration.jupyter.showOnlyOneTypeOfKernel.markdownDescription%",
+                    "scope": "machine",
+                    "tags": [
+                        "experimental"


This tag enables the changing of this setting through an experiment service.

rchiodo · 2022-07-14T18:10:02Z

src/notebooks/controllers/commands/serverConnectionControllerCommands.ts

+            this.showingRemoteNotWebContext.set(!isLocal && !this.isWeb).ignoreErrors();
+            this.showingRemoteContext.set(!isLocal && this.controllerRegistration.values.length > 0).ignoreErrors();
+        } else {
+            this.showingLocalOrWebEmptyContext.set(false).ignoreErrors();


Commands are always hidden if the setting is turned off

rchiodo · 2022-07-14T18:10:48Z

src/notebooks/controllers/controllerRegistration.ts

@@ -228,6 +239,13 @@ export class ControllerRegistration implements IControllerRegistration {
        this.onDidChangeFilter();
    }

+    private onDidChangeConfiguration(e: ConfigurationChangeEvent) {
+        if (e.affectsConfiguration('jupyter.showOnlyOneTypeOfKernel')) {
+            this.inKernelExperiment = this.workspace.getConfiguration('jupyter')?.get('showOnlyOneTypeOfKernel', false);


When onDidChangeConfiguration fires, the config service doesn't necessarily have the update yet (it's listening to the same event), so had to use the workspace.getConfiguration call here instead.

rchiodo · 2022-07-14T18:11:13Z

src/notebooks/controllers/kernelFilter/kernelFilterService.ts

@@ -139,3 +144,10 @@ type InterpreterFiter = {
     */
    path: string;
 };
+type RemoteSpecFilter = {


Added support for remote filtering. Made testing this easier.

rchiodo · 2022-07-14T18:12:00Z

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

-        await selector.selectJupyterURI();
-        let value = await storage.getUri();
-        assert.equal(value, Settings.JupyterServerLocalLaunch, 'Default should pick local launch');
+    suite('Experimental', () => {


Two copies of the tests.

One for new way the server selector works
One for the old way.

rchiodo · 2022-07-14T18:12:50Z

src/notebooks/controllers/vscodeNotebookController.ts

@@ -183,6 +183,13 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
        this.controller.supportedLanguages = this.languageService.getSupportedLanguages(kernelConnection);
        // Hook up to see when this NotebookController is selected by the UI
        this.controller.onDidChangeSelectedNotebooks(this.onDidChangeSelectedNotebooks, this, this.disposables);
+        this.notebookApi.onDidCloseNotebookDocument(


Associated documents were surviving tests closing notebooks.

Which could also potentially happen in the real product.

User opens notebook
Connects to remote
User closes notebook
User opens another notebook
Connects to local
Kernel list still includes remote connection from notebook that isn't open anymore

New test I added is verifying this isn't the case

This might improve performance and potentially fix other unnoticed bugs! I think? This looks really promising to me

src/platform/common/application/notebook.ts

sadasant · 2022-07-14T18:50:28Z

src/kernels/jupyter/serverSelector.ts

+
+        return items;
+    }
+}


Cleanly done! I like it

sadasant · 2022-07-14T18:55:43Z

src/notebooks/controllers/kernelFilter/kernelFilterUI.ts

@@ -65,7 +71,7 @@ export class KernelFilterUI implements IExtensionSyncActivationService, IDisposa
                    })
                    .map((item) => {
                        return <QuickPickType>{
-                            label: getDisplayNameOrNameOfKernelConnection(item),
+                            label: getKernelLabel(item),
                            picked: !this.kernelFilter.isKernelHidden(item),
                            description: getKernelConnectionPath(item, this.workspace),
                            detail:


src/notebooks/controllers/types.ts

DonJayamanne · 2022-07-14T18:58:12Z

src/notebooks/controllers/controllerRegistration.ts

@@ -183,7 +189,12 @@ export class ControllerRegistration implements IControllerRegistration {
        const userFiltered = this.kernelFilter.isKernelHidden(metadata);
        const connectionTypeFiltered = isLocalConnection(metadata) !== this.isLocalLaunch;
        const urlFiltered = isRemoteConnection(metadata) && this.serverUriStorage.currentServerId !== metadata.serverId;


I missed the additoin of this new function,
Can we change the code in isRemoteConnection to just call isLocalConnection and negate the value.
This way we don't have the logic in two functions.
I.e. isRemoteConnection would be the opposite of the return value of isLocalConnection this would allow us to keep the logic in one place.

DonJayamanne

looks ok, except for a problem with the all & registeredMetadatas properties

sadasant

LGTM

rchiodo · 2022-07-14T19:02:14Z

Please see my reply to the 'values' and 'all' conversation. Maybe I should rename 'values' to 'filtered'. It is the filtered list, which when you're local it only has local values, but not remote and vice versa. The 'all' property is every kernel connection every registered.

codecov-commenter · 2022-07-14T19:07:24Z

Codecov Report

Merging #10807 (3320ec0) into main (4d1a5c7) will increase coverage by 0%.
The diff coverage is 68%.

@@           Coverage Diff           @@
##            main   #10807    +/-   ##
=======================================
  Coverage     63%      63%            
=======================================
  Files        481      481            
  Lines      33427    33628   +201     
  Branches    5447     5488    +41     
=======================================
+ Hits       21119    21276   +157     
- Misses     10267    10311    +44     
  Partials    2041     2041

Impacted Files	Coverage Δ
src/notebooks/controllers/liveKernelSwitcher.ts	`60% <0%> (ø)`
src/notebooks/controllers/types.ts	`100% <ø> (ø)`
src/platform/common/types.ts	`100% <ø> (ø)`
...ks/controllers/kernelFilter/kernelFilterService.ts	`45% <16%> (-3%)`	⬇️
...tebooks/controllers/kernelFilter/kernelFilterUI.ts	`32% <16%> (-1%)`	⬇️
...ers/commands/serverConnectionControllerCommands.ts	`32% <66%> (+1%)`	⬆️
src/kernels/jupyter/serverSelector.ts	`63% <68%> (+3%)`	⬆️
...rc/notebooks/controllers/controllerRegistration.ts	`86% <80%> (-3%)`	⬇️
src/kernels/types.ts	`100% <100%> (ø)`
...ollers/commands/installPythonControllerCommands.ts	`57% <100%> (-1%)`	⬇️
... and 39 more

DonJayamanne · 2022-07-14T19:07:25Z

he 'values' and 'all' conversation. Maybe I should rename 'values' to 'filtered'.

Or just reigsteredControllers, I'd avoid using the word filter if possible, as thats just a way of deciding what's registered and what's not. At the end of the day, we're only interested in whats been registered with vs code.
Just a thought.

rchiodo added 4 commits July 13, 2022 15:12

Enable experiment for kernel picking

be3514f

Update tests

5b7a695

Fix tests and closing notebooks

00f9382

Add news entry

2d0b1aa

rchiodo requested a review from a team as a code owner July 14, 2022 17:48

Merge remote-tracking branch 'origin/main' into rchiodo/kernel_remote…

dfcbded

…_experiment

rchiodo commented Jul 14, 2022

View reviewed changes

DonJayamanne reviewed Jul 14, 2022

View reviewed changes

src/platform/common/application/notebook.ts Show resolved Hide resolved

sadasant reviewed Jul 14, 2022

View reviewed changes

src/kernels/jupyter/serverSelector.ts

return items;

}

}

Copy link

Contributor

sadasant Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanly done! I like it

sadasant reviewed Jul 14, 2022

View reviewed changes

DonJayamanne reviewed Jul 14, 2022

View reviewed changes

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

DonJayamanne reviewed Jul 14, 2022

View reviewed changes

DonJayamanne requested changes Jul 14, 2022

View reviewed changes

DonJayamanne approved these changes Jul 14, 2022

View reviewed changes

sadasant approved these changes Jul 14, 2022

View reviewed changes

rchiodo added 2 commits July 14, 2022 12:04

Update isRemoteConnection

7c372aa

Review feedback

83ede86

Update name of 'values' to 'registered'

3320ec0

rchiodo merged commit a5f8fe4 into main Jul 14, 2022

rchiodo deleted the rchiodo/kernel_remote_experiment branch July 14, 2022 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable an experimental setting for the new kernel picker UI #10807

Enable an experimental setting for the new kernel picker UI #10807

rchiodo commented Jul 14, 2022

rchiodo Jul 14, 2022

rchiodo Jul 14, 2022

rchiodo Jul 14, 2022

rchiodo Jul 14, 2022

rchiodo Jul 14, 2022

rchiodo Jul 14, 2022

rchiodo Jul 14, 2022

rchiodo Jul 14, 2022

sadasant Jul 14, 2022

sadasant Jul 14, 2022

sadasant Jul 14, 2022

DonJayamanne Jul 14, 2022

DonJayamanne left a comment

sadasant left a comment

rchiodo commented Jul 14, 2022

codecov-commenter commented Jul 14, 2022 •

edited

Loading

DonJayamanne commented Jul 14, 2022

Enable an experimental setting for the new kernel picker UI #10807

Enable an experimental setting for the new kernel picker UI #10807

Conversation

rchiodo commented Jul 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

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

DonJayamanne left a comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

rchiodo commented Jul 14, 2022

codecov-commenter commented Jul 14, 2022 • edited Loading

Codecov Report

DonJayamanne commented Jul 14, 2022

codecov-commenter commented Jul 14, 2022 •

edited

Loading