Skip to content

Check vars on execution count #5214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 11, 2019
Merged

Check vars on execution count #5214

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
664af82
initial prereview
IanMatthewHuff Apr 11, 2019
0512b58
pre-review changes
IanMatthewHuff Apr 11, 2019
39f8d6f
review feedback
IanMatthewHuff Apr 11, 2019
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/1 Enhancements/4948.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Variable explorer handles new cell submissions
3 changes: 1 addition & 2 deletions pythonFiles/datascience/getJupyterVariableList.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@
print(_VSCODE_json.dumps([{'name': var,
'type': type(eval(var)).__name__,
'size': _VSCODE_getsizeof(var),
'supportsDataExplorer': type(eval(var)).__name__ in _VSCode_supportsDataExplorer,
'expensive': True
'supportsDataExplorer': type(eval(var)).__name__ in _VSCode_supportsDataExplorer
} for var in _VSCode_JupyterVars]))

del _VSCode_supportsDataExplorer
Expand Down
9 changes: 7 additions & 2 deletions src/client/datascience/history/history.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ export class History extends WebViewHost<IHistoryMapping> implements IHistory {
break;

case HistoryMessages.GetVariablesRequest:
this.requestVariables().ignoreErrors();
this.requestVariables(payload).ignoreErrors();
break;

case HistoryMessages.GetVariableValueRequest:
Expand Down Expand Up @@ -918,10 +918,15 @@ export class History extends WebViewHost<IHistoryMapping> implements IHistory {
}
}

private requestVariables = async (): Promise<void> => {
private requestVariables = async (executionCount: number): Promise<void> => {
// Request our new list of variables
let vars: IJupyterVariable[] = await this.jupyterVariables.getVariables();

// Tag all of our jupyter variables with the execution count of the request
vars.forEach((value: IJupyterVariable) => {
value.executionCount = executionCount;
});

const settings = this.configuration.getSettings();
const excludeString = settings.datascience.variableExplorerExclude;

Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/history/historyTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export class IHistoryMapping {
public [HistoryMessages.RemoteAddCode]: IRemoteAddCode;
public [HistoryMessages.Activate] : never | undefined;
public [HistoryMessages.ShowDataViewer]: string;
public [HistoryMessages.GetVariablesRequest]: never | undefined;
public [HistoryMessages.GetVariablesRequest]: number;
public [HistoryMessages.GetVariablesResponse]: IJupyterVariable[];
public [HistoryMessages.GetVariableValueRequest]: IJupyterVariable;
public [HistoryMessages.GetVariableValueResponse]: IJupyterVariable;
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,13 +262,13 @@ export interface IDataScienceExtraSettings extends IDataScienceSettings {
export interface IJupyterVariable {
name: string;
value: string | undefined;
executionCount?: number;
supportsDataExplorer: boolean;
type: string;
size: number;
shape: string;
count: number;
truncated: boolean;
expensive: boolean;
columns?: { key: string; type: string }[];
rowCount?: number;
}
Expand Down
35 changes: 24 additions & 11 deletions src/datascience-ui/history-react/MainPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export class MainPanel extends React.Component<IMainPanelProps, IMainPanelState>
private mainPanel: HTMLDivElement | null = null;
private variableExplorerRef: React.RefObject<VariableExplorer>;
private styleInjectorRef: React.RefObject<StyleInjector>;
private currentExecutionCount: number = 0;

// tslint:disable-next-line:max-func-body-length
constructor(props: IMainPanelProps, _state: IMainPanelState) {
Expand Down Expand Up @@ -668,7 +669,8 @@ export class MainPanel extends React.Component<IMainPanelProps, IMainPanelState>
}

// When a cell is finished refresh our variables
if (getSettings && getSettings().showJupyterVariableExplorer) {
// Use the ref here to maintain var explorer independence
if (this.variableExplorerRef.current && this.variableExplorerRef.current.state.open) {
this.refreshVariables();
}
}
Expand All @@ -693,9 +695,14 @@ export class MainPanel extends React.Component<IMainPanelProps, IMainPanelState>
}
}

private getInputExecutionCount(cellVMs: ICellViewModel[]) : number {
private updateExecutionCount(cellVMs: ICellViewModel[]) {
const realCells = cellVMs.filter(c => c.cell.data.cell_type === 'code' && !c.editable && c.cell.data.execution_count);
return realCells && realCells.length > 0 ? parseInt(realCells[realCells.length - 1].cell.data.execution_count!.toString(), 10) + 1 : 1;
this.currentExecutionCount = realCells && realCells.length > 0 ? parseInt(realCells[realCells.length - 1].cell.data.execution_count!.toString(), 10) : 0;
}

private getInputExecutionCount = (cellVMs: ICellViewModel[]) : number => {
this.updateExecutionCount(cellVMs);
Copy link

Choose a reason for hiding this comment

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

updateExecutionCount [](start = 13, length = 20)

Seems weird that this get function is updating state. Maybe do the steps separately? getInputExecutionCount didn't update any state before.
Copy link

Choose a reason for hiding this comment

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

I think then I'd call it updateCurrentExecutionCount and have it call the getInputExecutionCount and subtract 1?

In reply to: 274679371 [](ancestors = 274679371)
Copy link

Choose a reason for hiding this comment

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

Or maybe just stick this on the state instead of in a member variable?

In reply to: 274680128 [](ancestors = 274680128,274679371)
Copy link

Choose a reason for hiding this comment

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

Cause what happens if there's no input box?

In reply to: 274680321 [](ancestors = 274680321,274680128,274679371)
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, not great. How about this? The basic atomic action (get execution count) get its own function. Get input doesn't set any state and does the +1. Execution count is set just where we are going to request a new set of variables which is where we need it set. I don't believe that this should be in state as it's only used in communications it's not actually part of the react rendering code or passed down as a prop.

In reply to: 274680448 [](ancestors = 274680448,274680321,274680128,274679371)
return this.currentExecutionCount + 1;
}

private submitInput = (code: string) => {
Expand Down Expand Up @@ -747,7 +754,7 @@ export class MainPanel extends React.Component<IMainPanelProps, IMainPanelState>

// When the variable explorer wants to refresh state (say if it was expanded)
private refreshVariables = () => {
this.sendMessage(HistoryMessages.GetVariablesRequest);
this.sendMessage(HistoryMessages.GetVariablesRequest, this.currentExecutionCount);
}

// Find the display value for one specific variable
Expand All @@ -761,8 +768,11 @@ export class MainPanel extends React.Component<IMainPanelProps, IMainPanelState>
if (payload) {
const variable = payload as IJupyterVariable;

if (this.variableExplorerRef.current) {
this.variableExplorerRef.current.newVariableData(variable);
// Only send the updated variable data if we are on the same execution count as when we requsted it
if (variable && variable.executionCount !== undefined && variable.executionCount === this.currentExecutionCount) {
if (this.variableExplorerRef.current) {
this.variableExplorerRef.current.newVariableData(variable);
}
}
}
}
Expand All @@ -773,12 +783,15 @@ export class MainPanel extends React.Component<IMainPanelProps, IMainPanelState>
if (payload) {
const variables = payload as IJupyterVariable[];

if (this.variableExplorerRef.current) {
this.variableExplorerRef.current.newVariablesData(variables);
}
// Check to see if we have moved to a new execution count only send our update if we are on the same count as the request
if (variables.length > 0 && variables[0].executionCount !== undefined && variables[0].executionCount === this.currentExecutionCount) {
if (this.variableExplorerRef.current) {
this.variableExplorerRef.current.newVariablesData(variables);
}

// Now put out a request for all of the sub values for the variables
variables.forEach(this.refreshVariable);
// Now put out a request for all of the sub values for the variables
variables.forEach(this.refreshVariable);
}
}
}
}
2 changes: 1 addition & 1 deletion src/datascience-ui/history-react/variableExplorer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export class VariableExplorer extends React.Component<IVariableExplorerProps, IV
{key: 'type', name: getLocString('DataScience.variableExplorerTypeColumn', 'Type'), type: 'string', width: 120},
{key: 'size', name: getLocString('DataScience.variableExplorerSizeColumn', 'Size'), type: 'number', width: 120, formatter: <VariableExplorerCellFormatter cellStyle={CellStyle.numeric} />},
{key: 'value', name: getLocString('DataScience.variableExplorerValueColumn', 'Value'), type: 'string', width: 300},
{key: 'buttons', name: '', type: 'boolean', width: 32, formatter: <VariableExplorerButtonCellFormatter showDataExplorer={this.props.showDataExplorer} baseTheme={this.props.baseTheme} /> }
{key: 'buttons', name: '', type: 'boolean', width: 34, formatter: <VariableExplorerButtonCellFormatter showDataExplorer={this.props.showDataExplorer} baseTheme={this.props.baseTheme} /> }
];
this.state = { open: false,
gridColumns: columns,
Expand Down
6 changes: 3 additions & 3 deletions src/datascience-ui/history-react/variableExplorerGrid.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@
#variable-explorer-data-grid .react-grid-HeaderCell {
background-color: var(--vscode-editor-lineHighlightBorder);
color: var(--vscode-editor-foreground);
border-style: none none solid none;
border-bottom-color: var(--vscode-badge-background);
padding: 4px;
border-style: solid;
border-color: var(--vscode-badge-background);
padding: 2px;
}


Expand Down
22 changes: 11 additions & 11 deletions src/test/datascience/jupyterVariables.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ suite('JupyterVariables', () => {

fakeServer.setup(fs => fs.execute(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny(), undefined, typemoq.It.isAny()))
.returns(() => Promise.resolve(generateCells(
'[{"name": "big_dataframe", "type": "DataFrame", "size": 62, "expensive": true}, {"name": "big_dict", "type": "dict", "size": 57, "expensive": true}, {"name": "big_float", "type": "float", "size": 58, "expensive": true}, {"name": "big_int", "type": "int", "size": 56, "expensive": true}, {"name": "big_list", "type": "list", "size": 57, "expensive": true}, {"name": "big_nparray", "type": "ndarray", "size": 60, "expensive": true}, {"name": "big_series", "type": "Series", "size": 59, "expensive": true}, {"name": "big_string", "type": "str", "size": 59, "expensive": true}, {"name": "big_tuple", "type": "tuple", "size": 58, "expensive": true}]',
'[{"name": "big_dataframe", "type": "DataFrame", "size": 62}, {"name": "big_dict", "type": "dict", "size": 57}, {"name": "big_float", "type": "float", "size": 58}, {"name": "big_int", "type": "int", "size": 56}, {"name": "big_list", "type": "list", "size": 57}, {"name": "big_nparray", "type": "ndarray", "size": 60}, {"name": "big_series", "type": "Series", "size": 59}, {"name": "big_string", "type": "str", "size": 59}, {"name": "big_tuple", "type": "tuple", "size": 58]',
'stream'
)))
.verifiable(typemoq.Times.never());
Expand Down Expand Up @@ -157,7 +157,7 @@ suite('JupyterVariables', () => {

fakeServer.setup(fs => fs.execute(typemoq.It.isValue('test'), typemoq.It.isValue(Identifiers.EmptyFileName), typemoq.It.isValue(0), typemoq.It.isAnyString(), undefined, typemoq.It.isValue(true)))
.returns(() => Promise.resolve(generateCells(
'[{"name": "big_dataframe", "type": "DataFrame", "size": 62, "expensive": true}, {"name": "big_dict", "type": "dict", "size": 57, "expensive": true}, {"name": "big_int", "type": "int", "size": 56, "expensive": true}, {"name": "big_list", "type": "list", "size": 57, "expensive": true}, {"name": "big_nparray", "type": "ndarray", "size": 60, "expensive": true}, {"name": "big_string", "type": "str", "size": 59, "expensive": true}]',
'[{"name": "big_dataframe", "type": "DataFrame", "size": 62}, {"name": "big_dict", "type": "dict", "size": 57}, {"name": "big_int", "type": "int", "size": 56}, {"name": "big_list", "type": "list", "size": 57}, {"name": "big_nparray", "type": "ndarray", "size": 60}, {"name": "big_string", "type": "str", "size": 59}]',
'stream'
)))
.verifiable(typemoq.Times.once());
Expand All @@ -168,12 +168,12 @@ suite('JupyterVariables', () => {
assert.equal(results.length, 6);

// Check our items (just the first few real items, no need to check all 19)
assert.deepEqual(results[0], {name: 'big_dataframe', size: 62, type: 'DataFrame', expensive: true});
assert.deepEqual(results[1], {name: 'big_dict', size: 57, type: 'dict', expensive: true});
assert.deepEqual(results[2], {name: 'big_int', size: 56, type: 'int', expensive: true});
assert.deepEqual(results[3], {name: 'big_list', size: 57, type: 'list', expensive: true});
assert.deepEqual(results[4], {name: 'big_nparray', size: 60, type: 'ndarray', expensive: true});
assert.deepEqual(results[5], {name: 'big_string', size: 59, type: 'str', expensive: true});
assert.deepEqual(results[0], {name: 'big_dataframe', size: 62, type: 'DataFrame'});
assert.deepEqual(results[1], {name: 'big_dict', size: 57, type: 'dict'});
assert.deepEqual(results[2], {name: 'big_int', size: 56, type: 'int'});
assert.deepEqual(results[3], {name: 'big_list', size: 57, type: 'list'});
assert.deepEqual(results[4], {name: 'big_nparray', size: 60, type: 'ndarray'});
assert.deepEqual(results[5], {name: 'big_string', size: 59, type: 'str'});

fakeServer.verifyAll();
});
Expand All @@ -186,17 +186,17 @@ suite('JupyterVariables', () => {

fakeServer.setup(fs => fs.execute(typemoq.It.isValue('test'), typemoq.It.isValue(Identifiers.EmptyFileName), typemoq.It.isValue(0), typemoq.It.isAnyString(), undefined, typemoq.It.isValue(true)))
.returns(() => Promise.resolve(generateCells(
'{"name": "big_complex", "type": "complex", "size": 60, "expensive": true, "value": "(1+1j)"}',
'{"name": "big_complex", "type": "complex", "size": 60, "value": "(1+1j)"}',
'stream'
)))
.verifiable(typemoq.Times.once());

const testVariable: IJupyterVariable = { name: 'big_complex', type: 'complex', size: 60, expensive: true, truncated: false, count: 0, shape: '', value: '', supportsDataExplorer: false };
const testVariable: IJupyterVariable = { name: 'big_complex', type: 'complex', size: 60, truncated: false, count: 0, shape: '', value: '', supportsDataExplorer: false };

const resultVariable = await jupyterVariables.getValue(testVariable);

// Verify the result value should be filled out from fake server result
assert.deepEqual(resultVariable, {name: 'big_complex', size: 60, type: 'complex', expensive: true, value: '(1+1j)'});
assert.deepEqual(resultVariable, {name: 'big_complex', size: 60, type: 'complex', value: '(1+1j)'});
fakeServer.verifyAll();
});
});