Pass resource to track Kernel Metadata against a resource #4760

DonJayamanne · 2021-02-11T18:32:02Z

Addresses the comments from here #4742 (comment)

DonJayamanne · 2021-02-11T18:38:15Z

src/client/datascience/jupyter/kernels/kernelSelector.ts

@@ -147,8 +147,7 @@ export class KernelSelector implements IKernelSelectionUsage {
        notebookMetadata?: nbformat.INotebookMetadata,
        disableUI?: boolean,
        cancelToken?: CancellationToken,
-        ignoreDependencyCheck?: boolean,
-        ignoreTrackingKernelInformation?: boolean


Removed the hacks & assumptions.
Just because some of these methods were called doesn't mean the kernel was changed.
We now track the kernel against the URI where these methods are called & kernel information is actually changed/set. No assumptions.

DonJayamanne · 2021-02-11T19:49:02Z

src/client/datascience/telemetry/interpreterPackages.ts

@@ -86,9 +86,7 @@ export class InterpreterPackages {
        const packageAndVersions = new Map<string, string>();
        // Add defaults.
        interestedPackages.forEach((item) => {
-            if (!packageAndVersions.has(item)) {
-                packageAndVersions.set(item, 'NOT INSTALLED');


Should have been hashed (no harm, it was a noop)

DonJayamanne · 2021-02-11T19:49:45Z

src/client/datascience/commands/notebookCommands.ts


            // Select a new kernel using the connection information
            const kernel = await this.kernelSelector.selectJupyterKernel(
-                options.identity,
+                options.resource,


codecov-io · 2021-02-11T19:53:45Z

Codecov Report

Merging #4760 (afc933f) into main (93d6475) will increase coverage by 0%.
The diff coverage is 81%.

@@          Coverage Diff          @@
##            main   #4760   +/-   ##
=====================================
  Coverage     76%     77%           
=====================================
  Files        411     411           
  Lines      26963   26958    -5     
  Branches    3909    3906    -3     
=====================================
+ Hits       20741   20782   +41     
+ Misses      4611    4576   -35     
+ Partials    1611    1600   -11

Impacted Files	Coverage Δ
...client/datascience/commands/activeEditorContext.ts	`91% <ø> (ø)`
...active-common/intellisense/intellisenseProvider.ts	`75% <ø> (+<1%)`	⬆️
...ent/datascience/jupyter/jupyterNotebookProvider.ts	`75% <ø> (ø)`
...science/jupyter/liveshare/guestJupyterExecution.ts	`73% <ø> (ø)`
...datascience/jupyter/liveshare/hostJupyterServer.ts	`86% <ø> (ø)`
...lient/datascience/jupyter/liveshare/serverCache.ts	`83% <ø> (ø)`
src/client/datascience/notebook/kernelProvider.ts	`80% <ø> (ø)`
src/client/datascience/notebook/notebookEditor.ts	`45% <0%> (-2%)`	⬇️
...atascience/notebookStorage/nativeEditorProvider.ts	`80% <ø> (ø)`
src/client/datascience/types.ts	`100% <ø> (ø)`
... and 38 more

joyceerhl · 2021-02-11T20:01:00Z

src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts

@@ -183,6 +183,7 @@ export class IPyWidgetScriptSource implements ILocalResourceUriConverter {
        if (!this.notebook) {
            this.notebook = await this.notebookProvider.getOrCreateNotebook({
                identity: this.identity,
+                resource: this.identity,


Why is resource the same as identity? Or does it not matter in this context?

Doesn't matter in this context (basically use the best we have, majority of the time identify = resoruce, except in interactive window when you open it without a correspoding py file or when you have multiples.

IanMatthewHuff · 2021-02-11T21:49:34Z

src/client/datascience/jupyter/serverPreload.ts


            // If it didn't start, attempt for local and if allowed.
            if (!providerConnection && !this.configService.getSettings(undefined).disableJupyterAutoStart) {
                // Local case, try creating one
                providerConnection = await this.notebookProvider.connect({
                    getOnly: false,
+                    resource: undefined,


Does this then get updated later? Just making sure that if a server is preloaded that the undefined here was going to stay even when we start up the actual notebook later when opening the file.

ChangeKernel should trigger the update it looks like?

When we start the actual notebook, then we'd be connecting to some kernel, hence at that point with Jupyter we'd change the kernel.
And with Raw, we'd end up starting a different kernel.

I.e. passing undefined is ok for auto start.

DonJayamanne added 4 commits February 11, 2021 10:28

Pass resource information around

9732da9

Fixes

c800fa1

Fixes

7f5801d

Fixes

fda8ec7

DonJayamanne commented Feb 11, 2021

View reviewed changes

DonJayamanne added 3 commits February 11, 2021 10:39

Bug fix

d3a4e7c

Fixes

76f33b5

Fixes

cfce6b9

DonJayamanne commented Feb 11, 2021

View reviewed changes

DonJayamanne marked this pull request as ready for review February 11, 2021 19:50

DonJayamanne requested a review from a team as a code owner February 11, 2021 19:50

Fix linter

75cfd83

DonJayamanne changed the title ~~Pass resource information around~~ Pass resource information around to track selected Kernel Metadata against a resoruce Feb 11, 2021

DonJayamanne changed the title ~~Pass resource information around to track selected Kernel Metadata against a resoruce~~ Pass resource information around to track selected Kernel Metadata against a resource Feb 11, 2021

DonJayamanne requested review from DavidKutu, IanMatthewHuff, joyceerhl and rchiodo February 11, 2021 19:53

joyceerhl reviewed Feb 11, 2021

View reviewed changes

DonJayamanne added 2 commits February 11, 2021 12:02

Fix linter

afc933f

Fix linter

e6f6d15

DonJayamanne requested a review from joyceerhl February 11, 2021 20:08

Fix test

e1e9e69

IanMatthewHuff reviewed Feb 11, 2021

View reviewed changes

DonJayamanne requested a review from IanMatthewHuff February 11, 2021 21:56

IanMatthewHuff approved these changes Feb 11, 2021

View reviewed changes

DonJayamanne changed the title ~~Pass resource information around to track selected Kernel Metadata against a resource~~ Pass resource to track Kernel Metadata against a resource Feb 11, 2021

DonJayamanne merged commit 4a0eefa into main Feb 11, 2021

DonJayamanne deleted the fixResources branch February 11, 2021 22:05

DonJayamanne added a commit that referenced this pull request Feb 11, 2021

Pass resource to track Kernel Metadata against a resource (#4760)

7f9fd91

DonJayamanne added a commit that referenced this pull request Feb 11, 2021

Cherry pick commits #4742, #4760, #4766

ed32da6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass resource to track Kernel Metadata against a resource #4760

Pass resource to track Kernel Metadata against a resource #4760

DonJayamanne commented Feb 11, 2021

DonJayamanne Feb 11, 2021

DonJayamanne Feb 11, 2021

DonJayamanne Feb 11, 2021

codecov-io commented Feb 11, 2021 •

edited

Loading

joyceerhl Feb 11, 2021

DonJayamanne Feb 11, 2021

IanMatthewHuff Feb 11, 2021

IanMatthewHuff Feb 11, 2021

DonJayamanne Feb 11, 2021

DonJayamanne Feb 11, 2021

Pass resource to track Kernel Metadata against a resource #4760

Pass resource to track Kernel Metadata against a resource #4760

Conversation

DonJayamanne commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 11, 2021 • 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

codecov-io commented Feb 11, 2021 •

edited

Loading