Fixes to blowing away of kernel info & not using right startup info #14295

DonJayamanne · 2020-10-06T23:55:36Z

When starting a kernel we're re-generating the kernel connection information
We're blowing away language_info
With non-python kernels we're storing python information in the notebook

Here's what the metadata in ipynb looks like:

// Julia
 "metadata": {
  "kernelspec": {
   "display_name": "Julia 1.5.2",
   "name": "julia-1.5"
  },
  "language_info": {
   "file_extension": ".jl",
   "mimetype": "application/julia",
   "name": "julia",
   "version": "1.5.2"
  },
  "orig_nbformat": 2
 },

// C#
 "metadata": {
  "kernelspec": {
   "display_name": ".NET (C#)",
   "name": ".net-csharp"
  },
  "language_info": {
   "file_extension": ".cs",
   "mimetype": "text/x-csharp",
   "name": "C#",
   "pygments_lexer": "csharp",
   "version": "8.0"
  },

// Python
 "metadata": {
  "kernelspec": {
   "display_name": "Python 3.8.2 64-bit ('venvDS': venv)",
   "name": "python38264bitvenvdsvenv6326db73e23e42c7a6a8df252feeb441"
  },
  "language_info": {
   "codemirror_mode": {
    "name": "ipython",
    "version": 3
   },
   "file_extension": ".py",
   "mimetype": "text/x-python",
   "name": "python",
   "nbconvert_exporter": "python",
   "pygments_lexer": "ipython3",
   "version": "3.8.2"
  },

DonJayamanne · 2020-10-06T23:56:17Z

src/client/datascience/baseJupyterSession.ts

@@ -122,7 +122,12 @@ export abstract class BaseJupyterSession implements IJupyterSession {
            );
        }
    }
-
+    public async requestKernelInfo(): Promise<KernelMessage.IInfoReplyMsg> {


This is the information stored in the metatadata of the notebook.
Today we're handcrafting this, we can instead get this from the kernel.

DonJayamanne · 2020-10-06T23:59:25Z

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

@@ -124,8 +127,9 @@ export class Kernel implements IKernel {
        } else {
            await this.validate(this.uri);
            const metadata = ((getDefaultNotebookContent().metadata || {}) as unknown) as nbformat.INotebookMetadata;
+            // Create a dummy notebook metadata & update the metadata before starting the notebook (required to ensure we fetch & start the right kernel).
+            // Lower layers of code below getOrCreateNotebook searches for kernels again using the metadata.


This is the code that's not right.
I've created an issue for this (possibly closed).
Basically we're doing all of the searching for kernels & finding the kernel information, etc..
However, when starting a notebook, we have to pass the notebook metadata as that is what's used by NotebookProvider.getOrCreateNotebook to start the real notebook/kernel.
However this is based on the notebook metadata, & we're not passing that in here, instead we pass the kernel connection metatadata. & the work around here is to generate a dummy object that looks like a notebook metadata & hydrate that with information from the kernel connection.

I still don't like this,
Basically we do all of the search, translate to kernelConnection & then we build another object from that & lower down we search for the right kernel using the information in what's supposed to be the metadata in the ipynb file.

DonJayamanne · 2020-10-07T00:00:10Z

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

@@ -142,15 +141,7 @@ export class KernelExecution implements IDisposable {
        await this.validateKernel(document);
        let kernel = this.kernelProvider.get(document.uri);
        if (!kernel) {
-            const activeInterpreter = await this.interpreterService.getActiveInterpreter(document.uri);
-            kernel = this.kernelProvider.getOrCreate(document.uri, {


This was wrong, we were hand crafting the kernel connection metadata instead of using the one provided.

Basically starting a kernel from a kernel spec would never have worked with the old code (for native notebooks).

DonJayamanne · 2020-10-07T00:02:33Z

src/client/datascience/notebook/helpers/helpers.ts

+    data.metadata = kernelConnection;
+    kernelInformationForNotebooks.set(document, data);
+}
+export function updateKernelInfoInNotebookMetadata(


We keep track of the kernel information, just as we keep track of the notebook metadata & update it only when writing to disc.
Using same (existing) technique.

I.e. the contents (metadata) get computed only at the time data is written to disc

DonJayamanne · 2020-10-07T00:03:17Z

src/client/datascience/notebook/kernelProvider.ts

    }
    public cancelCellExecution(_: NotebookDocument, cell: NotebookCell) {
        this.kernelProvider.get(cell.notebook.uri)?.interrupt(); // NOSONAR
    }
    public cancelAllCellsExecution(document: NotebookDocument) {
        this.kernelProvider.get(document.uri)?.interrupt(); // NOSONAR
    }
+    private updateKernelInfoInNotebookWhenAvailable(kernel: IKernel, doc: NotebookDocument) {
+        const disposable = kernel.onStatusChanged(() => {


Kernel information is only available after the kernel has been started.
Unfortunately the kernel does not have a link back to the notebook, hence we need to monitor the status here & update the metadata accordingly.

DonJayamanne · 2020-10-07T00:04:25Z

src/client/datascience/notebookStorage/baseModel.ts

-        metadata.language_info = undefined;
-        changed = true;
+    if (kernelInfo && kernelInfo.status === 'ok') {
+        if (!fastDeepEqual(metadata.language_info, kernelInfo.language_info)) {


If we have kernel info, use that.
Else do not blow away existing data, blow it away only for python (backwards compatible).
Personally I think we should never hand craft it, as we can get all of this from the kernel (do what Jupyter does always).

DonJayamanne · 2020-10-07T00:04:34Z

src/client/datascience/notebookStorage/notebookStorageProvider.ts

+            file: uri,
+            possibleContents,
+            skipLoadingDirtyContents: true,
+            isNative: forVSCodeNotebooks


Fixed a bug.

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

codecov-io · 2020-10-07T01:15:41Z

Codecov Report

Merging #14295 into main will increase coverage by 0.10%.
The diff coverage is 62.26%.

@@            Coverage Diff             @@
##             main   #14295      +/-   ##
==========================================
+ Coverage   59.75%   59.85%   +0.10%     
==========================================
  Files         697      709      +12     
  Lines       38649    39362     +713     
  Branches     5577     5705     +128     
==========================================
+ Hits        23094    23562     +468     
- Misses      14364    14558     +194     
- Partials     1191     1242      +51

Impacted Files	Coverage Δ
src/client/activation/common/activatorBase.ts	`14.41% <0.00%> (+1.19%)`	⬆️
...rc/client/activation/jedi/multiplexingActivator.ts	`17.74% <0.00%> (ø)`
src/client/activation/node/languageServerProxy.ts	`26.58% <0.00%> (ø)`
src/client/activation/refCountedLanguageServer.ts	`41.30% <0.00%> (ø)`
src/client/activation/types.ts	`100.00% <ø> (ø)`
src/client/common/process/pythonDaemon.ts	`14.28% <0.00%> (ø)`
src/client/common/utils/localize.ts	`96.24% <ø> (ø)`
src/client/datascience/baseJupyterSession.ts	`56.30% <0.00%> (-0.52%)`	⬇️
.../datascience/interactive-common/interactiveBase.ts	`5.78% <0.00%> (+<0.01%)`	⬆️
...ient/datascience/interactive-ipynb/nativeEditor.ts	`7.78% <0.00%> (ø)`
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c248197...d3a05ca. Read the comment docs.

sonarqubecloud · 2020-10-07T21:26:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

No Coverage information
0.0% Duplication

…icrosoft#14295)

Store the right kerneo info in notebook

2d67d2b

DonJayamanne commented Oct 6, 2020

View reviewed changes

Oops

40e4345

DonJayamanne commented Oct 6, 2020

View reviewed changes

DonJayamanne commented Oct 7, 2020

View reviewed changes

DonJayamanne requested review from rchiodo, IanMatthewHuff, DavidKutu, greazer and joyceerhl October 7, 2020 00:04

DonJayamanne added the no-changelog No news entry required label Oct 7, 2020

IanMatthewHuff reviewed Oct 7, 2020

View reviewed changes

src/client/datascience/jupyter/kernels/kernel.ts Outdated Show resolved Hide resolved

rchiodo approved these changes Oct 7, 2020

View reviewed changes

IanMatthewHuff approved these changes Oct 7, 2020

View reviewed changes

DonJayamanne added 3 commits October 7, 2020 10:21

misc

91c9cff

Update titles

a8a6914

Code review changes

d3a05ca

DonJayamanne merged commit 8b8fbbf into microsoft:main Oct 7, 2020

DonJayamanne deleted the kernelInfo branch October 7, 2020 22:32

luabud pushed a commit to luabud/vscode-python that referenced this pull request Oct 26, 2020

Fixes to blowing away of kernel info & not using right startup info (m…

6e9faed

…icrosoft#14295)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes to blowing away of kernel info & not using right startup info #14295

Fixes to blowing away of kernel info & not using right startup info #14295

Uh oh!

DonJayamanne commented Oct 6, 2020 •

edited

Loading

Uh oh!

DonJayamanne Oct 6, 2020

Uh oh!

DonJayamanne Oct 6, 2020

Uh oh!

DonJayamanne Oct 7, 2020

Uh oh!

DonJayamanne Oct 7, 2020 •

edited

Loading

Uh oh!

DonJayamanne Oct 7, 2020

Uh oh!

DonJayamanne Oct 7, 2020

Uh oh!

DonJayamanne Oct 7, 2020

Uh oh!

DonJayamanne Oct 7, 2020

Uh oh!

Uh oh!

codecov-io commented Oct 7, 2020 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Oct 7, 2020

Uh oh!

Uh oh!

Fixes to blowing away of kernel info & not using right startup info #14295

Fixes to blowing away of kernel info & not using right startup info #14295

Uh oh!

Conversation

DonJayamanne commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DonJayamanne Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Oct 7, 2020

Uh oh!

Uh oh!

DonJayamanne commented Oct 6, 2020 •

edited

Loading

DonJayamanne Oct 7, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading