Skip to content

Commit 5cf8775

Browse files
rchiodorchiodo
authored
Add extra code to compute shape for python objects. (microsoft#11200)
* Add extra code to compute shape for python objects. * Fix autocomplete tests. Not sure how these are passing in master * May as well skip list if not necessary * Fix native editor intellisense unit tests
1 parent 4bed560 commit 5cf8775

9 files changed

Lines changed: 117 additions & 25 deletions

File tree

‎news/2 Fixes/10825.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix problem with shape not being computed for some types in the variable explorer.

‎pythonFiles/vscode_datascience_helpers/getJupyterVariableShape.py‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# Query Jupyter server for defined variables list
2+
# Tested on 2.7 and 3.6
3+
import json as _VSCODE_json
4+
5+
# In IJupyterVariables.getValue this '_VSCode_JupyterTestValue' will be replaced with the json stringified value of the target variable
6+
# Indexes off of _VSCODE_targetVariable need to index types that are part of IJupyterVariable
7+
_VSCODE_targetVariable = _VSCODE_json.loads("""_VSCode_JupyterTestValue""")
8+
9+
_VSCODE_evalResult = eval(_VSCODE_targetVariable["name"])
10+
11+
# Find shape and count if available
12+
if hasattr(_VSCODE_evalResult, "shape"):
13+
try:
14+
# Get a bit more restrictive with exactly what we want to count as a shape, since anything can define it
15+
if isinstance(_VSCODE_evalResult.shape, tuple):
16+
_VSCODE_shapeStr = str(_VSCODE_evalResult.shape)
17+
if (
18+
len(_VSCODE_shapeStr) >= 3
19+
and _VSCODE_shapeStr[0] == "("
20+
and _VSCODE_shapeStr[-1] == ")"
21+
and "," in _VSCODE_shapeStr
22+
):
23+
_VSCODE_targetVariable["shape"] = _VSCODE_shapeStr
24+
del _VSCODE_shapeStr
25+
except TypeError:
26+
pass
27+
28+
if hasattr(_VSCODE_evalResult, "__len__"):
29+
try:
30+
_VSCODE_targetVariable["count"] = len(_VSCODE_evalResult)
31+
except TypeError:
32+
pass
33+
34+
print(_VSCODE_json.dumps(_VSCODE_targetVariable))
35+
del _VSCODE_json

‎src/client/common/process/pythonDaemon.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ export class PythonDaemonExecutionService implements IPythonDaemonExecutionServi
4949
public get isAlive(): boolean {
5050
return this.connectionClosedMessage === '';
5151
}
52+
private disposed = false;
5253
constructor(
5354
protected readonly pythonExecutionService: IPythonExecutionService,
5455
protected readonly pythonPath: string,
@@ -64,6 +65,7 @@ export class PythonDaemonExecutionService implements IPythonDaemonExecutionServi
6465
}
6566
public dispose() {
6667
try {
68+
this.disposed = true;
6769
// The daemon should die as a result of this.
6870
this.connection.sendNotification(new NotificationType('exit'));
6971
this.proc.kill();
@@ -382,11 +384,13 @@ export class PythonDaemonExecutionService implements IPythonDaemonExecutionServi
382384
private monitorConnection() {
383385
// tslint:disable-next-line: no-any
384386
const logConnectionStatus = (msg: string, ex?: any) => {
385-
this.connectionClosedMessage += msg + (ex ? `, With Error: ${util.format(ex)}` : '');
386-
this.connectionClosedDeferred.reject(new ConnectionClosedError(this.connectionClosedMessage));
387-
traceWarning(msg);
388-
if (ex) {
389-
traceError('Connection errored', ex);
387+
if (!this.disposed) {
388+
this.connectionClosedMessage += msg + (ex ? `, With Error: ${util.format(ex)}` : '');
389+
this.connectionClosedDeferred.reject(new ConnectionClosedError(this.connectionClosedMessage));
390+
traceWarning(msg);
391+
if (ex) {
392+
traceError('Connection errored', ex);
393+
}
390394
}
391395
};
392396
this.disposables.push(this.connection.onClose(() => logConnectionStatus('Daemon Connection Closed')));

‎src/client/datascience/interactive-common/intellisense/intellisenseDocument.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -207,8 +207,11 @@ export class IntellisenseDocument implements TextDocument {
207207
};
208208
}
209209

210-
public loadAllCells(cells: { code: string; id: string }[]): TextDocumentContentChangeEvent[] {
211-
if (!this.inEditMode) {
210+
public loadAllCells(
211+
cells: { code: string; id: string }[],
212+
notebookType: 'interactive' | 'native'
213+
): TextDocumentContentChangeEvent[] {
214+
if (!this.inEditMode && notebookType === 'native') {
212215
this.inEditMode = true;
213216
return this.reloadCells(cells);
214217
}
@@ -593,7 +596,9 @@ export class IntellisenseDocument implements TextDocument {
593596
}
594597

595598
// in interactive window
596-
return this._cellRanges[this._cellRanges.length - 1].start;
599+
return this._cellRanges && this._cellRanges.length > 0
600+
? this._cellRanges[this._cellRanges.length - 1].start
601+
: 0;
597602
}
598603

599604
private getLineFromOffset(offset: number) {

‎src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener {
6868
}>();
6969
private cancellationSources: Map<string, CancellationTokenSource> = new Map<string, CancellationTokenSource>();
7070
private notebookIdentity: Uri | undefined;
71+
private notebookType: 'interactive' | 'native' = 'interactive';
7172
private potentialResource: Uri | undefined;
7273
private sentOpenDocument: boolean = false;
7374
private languageServer: ILanguageServer | undefined;
@@ -688,7 +689,8 @@ export class IntellisenseProvider implements IInteractiveWindowListener {
688689
code: concatMultilineStringInput(cell.data.source),
689690
id: cell.id
690691
};
691-
})
692+
}),
693+
this.notebookType
692694
);
693695

694696
await this.handleChanges(document, changes);
@@ -708,6 +710,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener {
708710
private setIdentity(identity: INotebookIdentity) {
709711
this.notebookIdentity = identity.resource;
710712
this.potentialResource = identity.resource;
713+
this.notebookType = identity.type;
711714
}
712715

713716
private async getNotebook(): Promise<INotebook | undefined> {

‎src/client/datascience/jupyter/jupyterVariables.ts‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ import { IFileSystem } from '../../common/platform/types';
1515
import { IConfigurationService } from '../../common/types';
1616
import * as localize from '../../common/utils/localize';
1717
import { EXTENSION_ROOT_DIR } from '../../constants';
18-
import { Identifiers, Settings } from '../constants';
18+
import { captureTelemetry } from '../../telemetry';
19+
import { Identifiers, Settings, Telemetry } from '../constants';
1920
import {
2021
ICell,
2122
IJupyterVariable,
@@ -48,6 +49,7 @@ interface INotebookState {
4849
export class JupyterVariables implements IJupyterVariables {
4950
private fetchDataFrameInfoScript?: string;
5051
private fetchDataFrameRowsScript?: string;
52+
private fetchVariableShapeScript?: string;
5153
private filesLoaded: boolean = false;
5254
private languageToQueryMap = new Map<string, { query: string; parser: RegExp }>();
5355
private notebookState = new Map<Uri, INotebookState>();
@@ -58,6 +60,7 @@ export class JupyterVariables implements IJupyterVariables {
5860
) {}
5961

6062
// IJupyterVariables implementation
63+
@captureTelemetry(Telemetry.VariableExplorerFetchTime, undefined, true)
6164
public async getVariables(
6265
notebook: INotebook,
6366
request: IJupyterVariablesRequest
@@ -102,6 +105,9 @@ export class JupyterVariables implements IJupyterVariables {
102105
);
103106
this.fetchDataFrameInfoScript = await this.fileSystem.readFile(file);
104107

108+
file = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'vscode_datascience_helpers', 'getJupyterVariableShape.py');
109+
this.fetchVariableShapeScript = await this.fileSystem.readFile(file);
110+
105111
file = path.join(
106112
EXTENSION_ROOT_DIR,
107113
'pythonFiles',
@@ -361,7 +367,7 @@ export class JupyterVariables implements IJupyterVariables {
361367
targetVariable: IJupyterVariable,
362368
notebook: INotebook
363369
): Promise<IJupyterVariable> {
364-
const result = { ...targetVariable };
370+
let result = { ...targetVariable };
365371
if (notebook) {
366372
const output = await notebook.inspect(targetVariable.name);
367373

@@ -411,6 +417,26 @@ export class JupyterVariables implements IJupyterVariables {
411417
}
412418
}
413419

420+
// For a python kernel, we might be able to get a better shape. It seems the 'inspect' request doesn't always return it.
421+
// Do this only when necessary as this is a LOT slower than an inspect request. Like 4 or 5 times as slow
422+
if (
423+
result.type &&
424+
result.count &&
425+
!result.shape &&
426+
notebook.getKernelSpec()?.language === 'python' &&
427+
result.supportsDataExplorer &&
428+
result.type !== 'list' // List count is good enough
429+
) {
430+
const computedShape = await this.runScript<IJupyterVariable>(
431+
notebook,
432+
result,
433+
result,
434+
() => this.fetchVariableShapeScript
435+
);
436+
// Only want shape and count from the request. Other things might have been destroyed
437+
result = { ...result, shape: computedShape.shape, count: computedShape.count };
438+
}
439+
414440
return result;
415441
}
416442
}

‎src/test/datascience/intellisense.unit.test.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { expect } from 'chai';
55
import * as TypeMoq from 'typemoq';
66
import * as uuid from 'uuid/v4';
77

8+
import { Uri } from 'vscode';
89
import { IWorkspaceService } from '../../client/common/application/types';
910
import { PythonSettings } from '../../client/common/configSettings';
1011
import { IFileSystem } from '../../client/common/platform/types';
@@ -40,7 +41,7 @@ df
4041
`;
4142

4243
// tslint:disable-next-line: max-func-body-length
43-
suite('Data Science Intellisense Unit Tests', () => {
44+
suite('DataScience Intellisense Unit Tests', () => {
4445
let intellisenseProvider: IntellisenseProvider;
4546
let intellisenseDocument: IntellisenseDocument;
4647
let interpreterService: TypeMoq.IMock<IInterpreterService>;
@@ -291,6 +292,12 @@ suite('Data Science Intellisense Unit Tests', () => {
291292

292293
function loadAllCells(allCells: ICell[]): Promise<void> {
293294
cells = allCells;
295+
intellisenseProvider.onMessage(InteractiveWindowMessages.NotebookIdentity, {
296+
resource: Uri.parse('file:///foo.ipynb'),
297+
type: 'native'
298+
});
299+
300+
// Load all cells will actually respond with a notification, NotebookIdentity won't so don't wait for it.
294301
return sendMessage(InteractiveWindowMessages.LoadAllCellsComplete, { cells });
295302
}
296303

‎src/test/datascience/testHelpers.tsx‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,16 @@ export function runDoubleTest(
156156
test(`${name} (native)`, async () => testInnerLoop(name, (ioc) => mountWebView(ioc, 'native'), testFunc, getIOC));
157157
}
158158

159+
export function runInteractiveTest(
160+
name: string,
161+
testFunc: (wrapper: ReactWrapper<any, Readonly<{}>, React.Component>) => Promise<void>,
162+
getIOC: () => DataScienceIocContainer
163+
) {
164+
// Run the test with just the interactive window
165+
test(`${name} (interactive)`, async () =>
166+
testInnerLoop(name, (ioc) => mountWebView(ioc, 'interactive'), testFunc, getIOC));
167+
}
168+
159169
export function mountWebView(
160170
ioc: DataScienceIocContainer,
161171
type: 'native' | 'interactive'

‎src/test/datascience/variableexplorer.functional.test.tsx‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import { CommonActionType } from '../../datascience-ui/interactive-common/redux/
1515
import { DataScienceIocContainer } from './dataScienceIocContainer';
1616
import { addCode } from './interactiveWindowTestHelpers';
1717
import { addCell, createNewEditor } from './nativeEditorTestHelpers';
18-
import { runDoubleTest, waitForMessage } from './testHelpers';
18+
import { runDoubleTest, runInteractiveTest, waitForMessage } from './testHelpers';
1919

2020
// tslint:disable: no-var-requires no-require-imports
2121
const rangeInclusive = require('range-inclusive');
@@ -92,7 +92,7 @@ suite('DataScience Interactive Window variable explorer tests', () => {
9292
}
9393
}
9494

95-
runDoubleTest(
95+
runInteractiveTest(
9696
'Variable explorer - Exclude',
9797
async (wrapper) => {
9898
const basicCode: string = `import numpy as np
@@ -158,7 +158,7 @@ value = 'hello world'`;
158158
}
159159
);
160160

161-
runDoubleTest(
161+
runInteractiveTest(
162162
'Variable explorer - Update',
163163
async (wrapper) => {
164164
const basicCode: string = `value = 'hello world'`;
@@ -254,7 +254,7 @@ value = 'hello world'`;
254254
);
255255

256256
// Test our display of basic types. We render 8 rows by default so only 8 values per test
257-
runDoubleTest(
257+
runInteractiveTest(
258258
'Variable explorer - Types A',
259259
async (wrapper) => {
260260
const basicCode: string = `myList = [1, 2, 3]
@@ -285,7 +285,7 @@ myDict = {'a': 1}`;
285285
type: 'dict',
286286
size: 54,
287287
shape: '',
288-
count: 0,
288+
count: 1,
289289
truncated: false
290290
},
291291
{
@@ -295,7 +295,7 @@ myDict = {'a': 1}`;
295295
type: 'list',
296296
size: 54,
297297
shape: '',
298-
count: 0,
298+
count: 3,
299299
truncated: false
300300
},
301301
// Set can vary between python versions, so just don't both to check the value, just see that we got it
@@ -306,7 +306,7 @@ myDict = {'a': 1}`;
306306
type: 'set',
307307
size: 54,
308308
shape: '',
309-
count: 0,
309+
count: 1,
310310
truncated: false
311311
}
312312
];
@@ -317,7 +317,7 @@ myDict = {'a': 1}`;
317317
}
318318
);
319319

320-
runDoubleTest(
320+
runInteractiveTest(
321321
'Variable explorer - Basic B',
322322
async (wrapper) => {
323323
const basicCode: string = `import numpy as np
@@ -366,7 +366,7 @@ myTuple = 1,2,3,4,5,6,7,8,9
366366
supportsDataExplorer: true,
367367
type: 'DataFrame',
368368
size: 54,
369-
shape: '',
369+
shape: '(3, 1)',
370370
count: 0,
371371
truncated: false
372372
},
@@ -400,7 +400,7 @@ Name: 0, dtype: float64`,
400400
supportsDataExplorer: true,
401401
type: 'Series',
402402
size: 54,
403-
shape: '',
403+
shape: '(3,)',
404404
count: 0,
405405
truncated: false
406406
},
@@ -410,7 +410,7 @@ Name: 0, dtype: float64`,
410410
supportsDataExplorer: false,
411411
type: 'tuple',
412412
size: 54,
413-
shape: '',
413+
shape: '9',
414414
count: 0,
415415
truncated: false
416416
},
@@ -420,7 +420,7 @@ Name: 0, dtype: float64`,
420420
supportsDataExplorer: true,
421421
type: 'ndarray',
422422
size: 54,
423-
shape: '',
423+
shape: '(3,)',
424424
count: 0,
425425
truncated: false
426426
}
@@ -450,7 +450,8 @@ Name: 0, dtype: float64`,
450450
};
451451
}
452452

453-
// Test our limits. Create 1050 items.
453+
// Test our limits. Create 1050 items. Do this with both to make
454+
// sure no perf problems with one or the other and to smoke test the native editor
454455
runDoubleTest(
455456
'Variable explorer - A lot of items',
456457
async (wrapper) => {

0 commit comments

Comments
 (0)