Make gather an optional component (#9755)

* Update .npmrc to get latest python

* Update to latest analysis and fix gather icon

* Use sliceLastExecution and fix
cell executionId for gather.

* Fix import of python analysis

* Update to python-analysis 0.4.12

* get gathered text from cell.textSlice

* Updates to fix gather logging

* All updates to using new nb owned gather.

* Working cell execution count codelenses.

* Rename gather* to match existing classes
and interfaces

* Update to latest python analysis

* lock python analysis ver

* Fix MockJupyterNotebook

* Fix casing of getgatherProvider

* Private Gather support #1
Dynamically look for python-program-analysis. If not there
gather is unsupported.

* Ensure cellHashProvider can be found

* Revert back to uri for NBExecutionActivated

* Update new jupyterServerWrapper

* Workaround fix for screwed up cellhashes
Being fixed by Ian in a separate branch.

* Gracefully fail if python-pgm-analysis is absent

* Resolve obvious unit test problems.

* Fixes to make sure that gather.ts is loadable.

* Optional gather build, editing, running works.

* Remove unecessary getAll of IGatherProviders.

* Code cleanup

* Fix gather notebook header
- Also remove reference to python-program-analysis in package.json

* Gather icon not only on hover

* Enable gather only in insiders

* Init gather only if enabled.

* Add link to survey

* Fix spacing in initialization.yml

* Make linter and prettier happy

* Make webpack happy with regard to gather

* Don't define ENABLE_GATHER env var

* Couple minor issues found in test

* Fix a few tests

* Everything but dataScienceIocContainer.ts

* More fixes

* Whoops, fix IW functional test back to original

* Temporarily include datascienceioccontainer in 120
character line length.

* Make linter finally happy.

* Fix Gather functional tests

* A bit more cleanup

* React to PR feedback

* Couple more PR review changes.

* Tweak

* More minor cleanup

* Fix provider finders.

* Actually fix the provider get problem.

* Fix smoke test

* Fix unit test

Author: greazer

.npmrc

@@ -1 +1 @@
-@types:registry=https://registry.npmjs.org
+@types:registry=https://registry.npmjs.org

.prettierrc.js

@@ -8,6 +8,12 @@ module.exports = {
             options: {
                 tabWidth: 2
             }
+        },
+        {
+            files: ['**/datascience/serviceRegistry.ts'],
+            options: {
+                printWidth: 240
+            }
         }
     ]
 };

build/ci/templates/steps/initialization.yml

@@ -18,6 +18,7 @@ parameters:
   compile: 'true'
   sqlite: 'false'
   installVSCEorNPX: 'true'
+  enableGather: 'false'
 
 steps:
     - bash: |
@@ -92,6 +93,15 @@ steps:
           verbose: true
           customCommand: ci
 
+    - task: Npm@1
+      displayName: "Install optional python-program-analysis"
+      condition: and(succeeded(), eq('${{ parameters.enableGather }}', 'true'))
+      inputs:
+          workingDir: ${{ parameters.workingDirectory }}
+          command: custom
+          verbose: true
+          customCommand: "install @msrvida/python-program-analysis"
+
     # On Mac, the command `node` doesn't always point to the current node version.
     # Debugger tests use the variable process.env.NODE_PATH
     - script: |
@@ -101,7 +111,7 @@ steps:
       displayName: "Setup NODE_PATH for extension (Debugger Tests)"
       condition: and(succeeded(), eq(variables['agent.os'], 'Darwin'))
 
-    # Install vsce 
+    # Install vsce
     - bash: |
           npm install -g vsce
       displayName: "Install vsce"

build/webpack/webpack.extension.config.js

@@ -13,6 +13,9 @@ const configFileName = path.join(constants.ExtensionRootDir, 'tsconfig.extension
 const existingModulesInOutDir = common.getListOfExistingModulesInOutDir();
 // tslint:disable-next-line:no-var-requires no-require-imports
 const FileManagerPlugin = require('filemanager-webpack-plugin');
+// If ENABLE_GATHER variable is defined, don't exclude the python-program-analysis pacakge.
+// See externals, below.
+const ppaPackageList = process.env.ENABLE_GATHER ? [] : ['@msrvida/python-program-analysis'];
 const config = {
     mode: 'production',
     target: 'node',
@@ -56,7 +59,10 @@ const config = {
             { enforce: 'post', test: /linebreak[\/\\]src[\/\\]linebreaker.js/, loader: 'transform-loader?brfs' }
         ]
     },
-    externals: ['vscode', 'commonjs', ...existingModulesInOutDir],
+    // Packages listed in externals keeps webpack from trying to package them.
+    // The ppaPackageList variable is set to non-empty if the build pipeline has been
+    //authenticated to install @msrvida/python-program-analysis.
+    externals: ['vscode', 'commonjs', ...ppaPackageList, ...existingModulesInOutDir],
     plugins: [
         ...common.getDefaultPlugins('extension'),
         // Copy pdfkit bits after extension builds. webpack can't handle pdfkit.

package-lock.json

@@ -1833,13 +1833,13 @@
                 "python.dataScience.enableGather": {
                     "type": "boolean",
                     "default": false,
-                    "description": "Enable code gather for executed cells. For a gathered cell, that cell and only the code it depends on will be exported to a new notebook.",
+                    "description": "Enable experimental code gathering for executed cells. For a gathered cell, that cell and only the code it depends on will be exported to a new notebook.",
                     "scope": "resource"
                 },
                 "python.dataScience.gatherToScript": {
                     "type": "boolean",
                     "default": true,
-                    "description": "Gather code to a python script rather than a notebook.",
+                    "description": "If experimental code gather is enabled, gather code to a python script rather than a notebook.",
                     "scope": "resource"
                 },
                 "python.dataScience.codeLenses": {
@@ -2868,7 +2868,6 @@
         "@jupyterlab/services": "^4.2.0",
         "@koa/cors": "^3.0.0",
         "@loadable/component": "^5.12.0",
-        "@msrvida/python-program-analysis": "^0.4.1",
         "ansi-regex": "^4.1.0",
         "arch": "^2.1.0",
         "azure-storage": "^2.10.3",

package.json

@@ -418,7 +418,7 @@
     "DataScience.findJupyterCommandProgressCheckInterpreter": "Checking {0}.",
     "DataScience.findJupyterCommandProgressSearchCurrentPath": "Searching current path.",
     "DataScience.gatheredScriptDescription": "# This file contains only the code required to produce the results of the gathered cell.\n",
-    "DataScience.gatheredNotebookDescriptionInMarkdown": "# Gathered Notebook\nGenerated from ```{0}```\n\nThis notebook contains only the code and cells required to produce the same results as the gathered cell.\n\nPlease note that the python analysis is quite conservative, so if it is unsure whether a line of code is necessary for execution, it will err on the side of including it.",
+    "DataScience.gatheredNotebookDescriptionInMarkdown": "## Gathered Notebook\nGenerated from ```{0}```\n\nThis notebook contains only the code and cells required to produce the same results as the gathered cell.\n\nPlease note that the python analysis is quite conservative, so if it is unsure whether a line of code is necessary for execution, it will err on the side of including it.\n\nAs this is an experimental feature, please let us know how well Gather works for you at [https://aka.ms/gathersurvey](https://aka.ms/gathersurvey)",
     "DataScience.savePngTitle": "Save Image",
     "DataScience.jupyterSelectURIQuickPickTitle": "Pick how to connect to Jupyter",
     "DataScience.jupyterSelectURIQuickPickPlaceholder": "Choose an option",

package.nls.json

@@ -726,7 +726,7 @@ export namespace DataScience {
     );
     export const gatheredNotebookDescriptionInMarkdown = localize(
         'DataScience.gatheredNotebookDescriptionInMarkdown',
-        '# Gathered Notebook\nGenerated from ```{0}```\n\nThis notebook contains only the code and cells required to produce the same results as the gathered cell.\n\nPlease note that the python analysis is quite conservative, so if it is unsure whether a line of code is necessary for execution, it will err on the side of including it.'
+        '## Gathered Notebook\nGenerated from ```{0}```\n\nThis notebook contains only the code and cells required to produce the same results as the gathered cell.\n\nPlease note that the python analysis is quite conservative, so if it is unsure whether a line of code is necessary for execution, it will err on the side of including it.\n\nAs this is an experimental feature, please let us know how well Gather works for you at [https://aka.ms/gathersurvey](https://aka.ms/gathersurvey)'
     );
     export const savePngTitle = localize('DataScience.savePngTitle', 'Save Image');
     export const fallbackToUseActiveInterpeterAsKernel = localize(

src/client/common/utils/localize.ts

@@ -0,0 +1,48 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+'use strict';
+import { inject, injectable } from 'inversify';
+
+import { traceError } from '../../common/logger';
+import { noop } from '../../common/utils/misc';
+import { Identifiers } from '../constants';
+import { ICell, ICellHashLogger, ICellHashProvider } from '../types';
+import { CellHashProvider } from './cellhashprovider';
+
+// This class provides hashes for debugging jupyter cells. Call getHashes just before starting debugging to compute all of the
+// hashes for cells.
+@injectable()
+export class CellHashLogger implements ICellHashLogger {
+    constructor(@inject(ICellHashProvider) private provider: ICellHashProvider) {}
+
+    public async preExecute(cell: ICell, silent: boolean): Promise<void> {
+        const providerObj: CellHashProvider = this.provider as CellHashProvider;
+
+        try {
+            if (!silent) {
+                // Don't log empty cells
+                const stripped = providerObj.extractExecutableLines(cell);
+                if (stripped.length > 0 && stripped.find(s => s.trim().length > 0)) {
+                    // When the user adds new code, we know the execution count is increasing
+                    providerObj.incExecutionCount();
+
+                    // Skip hash on unknown file though
+                    if (cell.file !== Identifiers.EmptyFileName) {
+                        await providerObj.addCellHash(cell, providerObj.getExecutionCount());
+                    }
+                }
+            }
+        } catch (exc) {
+            // Don't let exceptions in a preExecute mess up normal operation
+            traceError(exc);
+        }
+    }
+
+    public async postExecute(_cell: ICell, _silent: boolean): Promise<void> {
+        noop();
+    }
+
+    public getCellHashProvider(): ICellHashProvider {
+        return this.provider;
+    }
+}

src/client/datascience/editor-integration/cellhashLogger.ts

@@ -21,8 +21,7 @@ import {
     ICellHashListener,
     ICellHashProvider,
     IFileHashes,
-    IInteractiveWindowListener,
-    INotebookExecutionLogger
+    IInteractiveWindowListener
 } from '../types';
 
 interface IRangedCellHash extends ICellHash {
@@ -36,16 +35,16 @@ interface IRangedCellHash extends ICellHash {
 // This class provides hashes for debugging jupyter cells. Call getHashes just before starting debugging to compute all of the
 // hashes for cells.
 @injectable()
-export class CellHashProvider implements ICellHashProvider, IInteractiveWindowListener, INotebookExecutionLogger {
+export class CellHashProvider implements ICellHashProvider, IInteractiveWindowListener {
     // tslint:disable-next-line: no-any
     private postEmitter: EventEmitter<{ message: string; payload: any }> = new EventEmitter<{
         message: string;
         // tslint:disable-next-line: no-any
         payload: any;
     }>();
     // Map of file to Map of start line to actual hash
-    private hashes: Map<string, IRangedCellHash[]> = new Map<string, IRangedCellHash[]>();
     private executionCount: number = 0;
+    private hashes: Map<string, IRangedCellHash[]> = new Map<string, IRangedCellHash[]>();
     private updateEventEmitter: EventEmitter<void> = new EventEmitter<void>();
 
     constructor(
@@ -127,19 +126,7 @@ export class CellHashProvider implements ICellHashProvider, IInteractiveWindowLi
         noop();
     }
 
-    private onChangedDocument(e: TextDocumentChangeEvent) {
-        // See if the document is in our list of docs to watch
-        const perFile = this.hashes.get(e.document.fileName);
-        if (perFile) {
-            // Apply the content changes to the file's cells.
-            const docText = e.document.getText();
-            e.contentChanges.forEach(c => {
-                this.handleContentChange(docText, c, perFile);
-            });
-        }
-    }
-
-    private extractExecutableLines(cell: ICell): string[] {
+    public extractExecutableLines(cell: ICell): string[] {
         const cellMatcher = new CellMatcher(this.configService.getSettings(getCellResource(cell)).datascience);
         const lines = splitMultilineString(cell.data.source);
         // Only strip this off the first line. Otherwise we want the markers in the code.
@@ -149,44 +136,7 @@ export class CellHashProvider implements ICellHashProvider, IInteractiveWindowLi
         return lines;
     }
 
-    private handleContentChange(docText: string, c: TextDocumentContentChangeEvent, hashes: IRangedCellHash[]) {
-        // First compute the number of lines that changed
-        const lineDiff = c.range.start.line - c.range.end.line + c.text.split('\n').length - 1;
-        const offsetDiff = c.text.length - c.rangeLength;
-
-        // Compute the inclusive offset that is changed by the cell.
-        const endChangedOffset = c.rangeLength <= 0 ? c.rangeOffset : c.rangeOffset + c.rangeLength - 1;
-
-        hashes.forEach(h => {
-            // See how this existing cell compares to the change
-            if (h.endOffset < c.rangeOffset) {
-                // No change. This cell is entirely before the change
-            } else if (h.startOffset > endChangedOffset) {
-                // This cell is after the text that got replaced. Adjust its start/end lines
-                h.line += lineDiff;
-                h.endLine += lineDiff;
-                h.startOffset += offsetDiff;
-                h.endOffset += offsetDiff;
-            } else if (h.startOffset === endChangedOffset) {
-                // Cell intersects but exactly, might be a replacement or an insertion
-                if (h.deleted || c.rangeLength > 0 || lineDiff === 0) {
-                    // Replacement
-                    h.deleted = docText.substr(h.startOffset, h.endOffset - h.startOffset) !== h.realCode;
-                } else {
-                    // Insertion
-                    h.line += lineDiff;
-                    h.endLine += lineDiff;
-                    h.startOffset += offsetDiff;
-                    h.endOffset += offsetDiff;
-                }
-            } else {
-                // Intersection, delete if necessary
-                h.deleted = docText.substr(h.startOffset, h.endOffset - h.startOffset) !== h.realCode;
-            }
-        });
-    }
-
-    private async addCellHash(cell: ICell, expectedCount: number): Promise<void> {
+    public async addCellHash(cell: ICell, expectedCount: number): Promise<void> {
         // Find the text document that matches. We need more information than
         // the add code gives us
         const doc = this.documentManager.textDocuments.find(d => this.fileSystem.arePathsSame(d.fileName, cell.file));
@@ -284,6 +234,63 @@ export class CellHashProvider implements ICellHashProvider, IInteractiveWindowLi
         }
     }
 
+    public getExecutionCount(): number {
+        return this.executionCount;
+    }
+
+    public incExecutionCount(): void {
+        this.executionCount += 1;
+    }
+
+    private onChangedDocument(e: TextDocumentChangeEvent) {
+        // See if the document is in our list of docs to watch
+        const perFile = this.hashes.get(e.document.fileName);
+        if (perFile) {
+            // Apply the content changes to the file's cells.
+            const docText = e.document.getText();
+            e.contentChanges.forEach(c => {
+                this.handleContentChange(docText, c, perFile);
+            });
+        }
+    }
+
+    private handleContentChange(docText: string, c: TextDocumentContentChangeEvent, hashes: IRangedCellHash[]) {
+        // First compute the number of lines that changed
+        const lineDiff = c.range.start.line - c.range.end.line + c.text.split('\n').length - 1;
+        const offsetDiff = c.text.length - c.rangeLength;
+
+        // Compute the inclusive offset that is changed by the cell.
+        const endChangedOffset = c.rangeLength <= 0 ? c.rangeOffset : c.rangeOffset + c.rangeLength - 1;
+
+        hashes.forEach(h => {
+            // See how this existing cell compares to the change
+            if (h.endOffset < c.rangeOffset) {
+                // No change. This cell is entirely before the change
+            } else if (h.startOffset > endChangedOffset) {
+                // This cell is after the text that got replaced. Adjust its start/end lines
+                h.line += lineDiff;
+                h.endLine += lineDiff;
+                h.startOffset += offsetDiff;
+                h.endOffset += offsetDiff;
+            } else if (h.startOffset === endChangedOffset) {
+                // Cell intersects but exactly, might be a replacement or an insertion
+                if (h.deleted || c.rangeLength > 0 || lineDiff === 0) {
+                    // Replacement
+                    h.deleted = docText.substr(h.startOffset, h.endOffset - h.startOffset) !== h.realCode;
+                } else {
+                    // Insertion
+                    h.line += lineDiff;
+                    h.endLine += lineDiff;
+                    h.startOffset += offsetDiff;
+                    h.endOffset += offsetDiff;
+                }
+            } else {
+                // Intersection, delete if necessary
+                h.deleted = docText.substr(h.startOffset, h.endOffset - h.startOffset) !== h.realCode;
+            }
+        });
+    }
+
     private adjustRuntimeForDebugging(
         cell: ICell,
         source: string[],