Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/client/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { isTestExecution } from './common/constants';
import { DebugAdapterNewPtvsd } from './common/experiments/groups';
import { traceError } from './common/logger';
import { IConfigurationService, IExperimentsManager, Resource } from './common/types';
import { IDataViewerDataProvider } from './datascience/data-viewing/types';
import { IDataViewer, IDataViewerFactory } from './datascience/types';
import {
getDebugpyLauncherArgs,
getDebugpyPackagePath,
Expand Down Expand Up @@ -64,6 +66,15 @@ export interface IExtensionApi {
*/
getExecutionCommand(resource?: Resource): string[] | undefined;
};
datascience: {
/**
* Launches Data Viewer component.
* @param {IDataViewerDataProvider} dataProvider Instance that will be used by the Data Viewer component to fetch data.
* @param {string} title Data Viewer title
* @returns {Promise<IDataViewer>}
*/
showDataViewer(dataProvider: IDataViewerDataProvider, title: string): Promise<IDataViewer>;
};
}

export function buildApi(
Expand Down Expand Up @@ -118,6 +129,12 @@ export function buildApi(
// If pythonPath equals an empty string, no interpreter is set.
return pythonPath === '' ? undefined : [pythonPath];
}
},
datascience: {
async showDataViewer(dataProvider: IDataViewerDataProvider, title: string) {
const dataViewerProviderService = serviceContainer.get<IDataViewerFactory>(IDataViewerFactory);
return dataViewerProviderService.create(dataProvider, title);
}
}
};

Expand Down
81 changes: 44 additions & 37 deletions src/client/datascience/data-viewing/dataViewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
'use strict';
import '../../common/extensions';

import { inject, injectable, named } from 'inversify';
import { inject, injectable } from 'inversify';
import * as path from 'path';
import { ViewColumn } from 'vscode';

Expand All @@ -16,18 +16,23 @@ import * as localize from '../../common/utils/localize';
import { noop } from '../../common/utils/misc';
import { StopWatch } from '../../common/utils/stopWatch';
import { sendTelemetryEvent } from '../../telemetry';
import { HelpLinks, Identifiers, Telemetry } from '../constants';
import { HelpLinks, Telemetry } from '../constants';
import { JupyterDataRateLimitError } from '../jupyter/jupyterDataRateLimitError';
import { ICodeCssGenerator, IDataViewer, IJupyterVariable, IJupyterVariables, INotebook, IThemeFinder } from '../types';
import { ICodeCssGenerator, IDataViewer, IThemeFinder } from '../types';
import { WebViewHost } from '../webViewHost';
import { DataViewerMessageListener } from './dataViewerMessageListener';
import { DataViewerMessages, IDataViewerMapping, IGetRowsRequest } from './types';
import {
DataViewerMessages,
IDataFrameInfo,
IDataViewerDataProvider,
IDataViewerMapping,
IGetRowsRequest
} from './types';

const dataExplorereDir = path.join(EXTENSION_ROOT_DIR, 'out', 'datascience-ui', 'viewers');
@injectable()
export class DataViewer extends WebViewHost<IDataViewerMapping> implements IDataViewer, IDisposable {
private notebook: INotebook | undefined;
private variable: IJupyterVariable | undefined;
private dataProvider: IDataViewerDataProvider | undefined;
private rowsTimer: StopWatch | undefined;
private pendingRowsCount: number = 0;

Expand All @@ -37,7 +42,6 @@ export class DataViewer extends WebViewHost<IDataViewerMapping> implements IData
@inject(ICodeCssGenerator) cssGenerator: ICodeCssGenerator,
@inject(IThemeFinder) themeFinder: IThemeFinder,
@inject(IWorkspaceService) workspaceService: IWorkspaceService,
@inject(IJupyterVariables) @named(Identifiers.ALL_VARIABLES) private variableManager: IJupyterVariables,
@inject(IApplicationShell) private applicationShell: IApplicationShell,
@inject(IExperimentsManager) experimentsManager: IExperimentsManager,
@inject(UseCustomEditorApi) useCustomEditorApi: boolean
Expand All @@ -57,33 +61,43 @@ export class DataViewer extends WebViewHost<IDataViewerMapping> implements IData
useCustomEditorApi,
false
);
}

// Load the web panel using our current directory as we don't expect to load any other files
super.loadWebPanel(process.cwd()).catch(traceError);
private static trimTitle(title: string): string {

Choose a reason for hiding this comment

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

I'd let the UI (VSCode or webview) handle long titles (I.e. I'd remove this).

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, it would be better for webview or VSCode to handle this.

Do you know how likely is to have long titles? I mention it because originally Data Viewer is trimming the titles, looking at the code and doing some testing removing this will show long names in the tabs (tested up to 200 chars), not sure if this could represent a regression for some users?

Choose a reason for hiding this comment

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

@rchiodo @IanMatthewHuff @DavidKutu What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The dataviewer name is based off the variable, right? Pretty sure we put this in as longer variable names could eat up like half of your tab real estate as VSCode didn't do anything to shorten them. Speaking for myself, I'd go for just letting the long names through. Let VSCode deal with it and don't have to own any custom trimming. The argument against is that customer are not picking variable names the same way they pick file names and might not be expecting to have so big of a tab.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this would be the right spot though in any case. Some consumers might want to trim, others might not. They should be in charge of passing in the right length title that they want. I would think that the viewer should just show it.

Copy link
Author

Choose a reason for hiding this comment

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

@IanMatthewHuff agree, I will remove the trimming from DataViewer and if the decision is to still trim titles we can do it from the client.

Choose a reason for hiding this comment

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

Speaking for myself, I'd go for just letting the long names through. Let VSCode deal with it and don't have to own any custom trimming.

Agreed.

const TRIM_LENGTH = 40;
if (title && title.length > TRIM_LENGTH) {
title = `${title.substr(0, TRIM_LENGTH)}...`;
}
return title;
}

public async showVariable(variable: IJupyterVariable, notebook: INotebook): Promise<void> {
public async showData(dataProvider: IDataViewerDataProvider, title: string): Promise<void> {
if (!this.isDisposed) {
// Save notebook this is tied to
this.notebook = notebook;
// Save the data provider
this.dataProvider = dataProvider;

// Fill in our variable's beginning data
this.variable = await this.prepVariable(variable, notebook);
// Load the web panel using our current directory as we don't expect to load any other files
await super.loadWebPanel(process.cwd()).catch(traceError);

Choose a reason for hiding this comment

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

Why are we swallowing errors here. Not sure this is the right thing to do.
How about changing from process.cwd to dataExplorereDir, makes it future proof as well, if we wanted to load soemthing.

Copy link
Author

Choose a reason for hiding this comment

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

@DonJayamanne great comments :) thanks

  1. Error swallowing: hmm I think it is because this call was done from the constructor, I moved here to make sure the webpanel is loaded before calling setTitle and show. I never realized the error swallowing though, will remove it.

  2. Directory: Good point, just wondering if there is another reason why it was done like that. @rchiodo @IanMatthewHuff any thoughts?


// Create our new title with the variable name
let newTitle = `${localize.DataScience.dataExplorerTitle()} - ${variable.name}`;
const TRIM_LENGTH = 40;
if (newTitle.length > TRIM_LENGTH) {
newTitle = `${newTitle.substr(0, TRIM_LENGTH)}...`;
}

super.setTitle(newTitle);
super.setTitle(DataViewer.trimTitle(title));

// Then show our web panel. Eventually we need to consume the data
await super.show(true);

const dataFrameInfo = await this.prepDataFrameInfo(dataProvider);

// Send a message with our data
this.postMessage(DataViewerMessages.InitializeData, this.variable).ignoreErrors();
this.postMessage(DataViewerMessages.InitializeData, dataFrameInfo).ignoreErrors();
}
}

public dispose(): void {
super.dispose();

if (this.dataProvider) {
// Call dispose on the data provider
this.dataProvider.dispose();
this.dataProvider = undefined;
}
}

Expand All @@ -109,9 +123,9 @@ export class DataViewer extends WebViewHost<IDataViewerMapping> implements IData
super.onMessage(message, payload);
}

private async prepVariable(variable: IJupyterVariable, notebook: INotebook): Promise<IJupyterVariable> {
private async prepDataFrameInfo(dataProvider: IDataViewerDataProvider): Promise<IDataFrameInfo> {
this.rowsTimer = new StopWatch();
const output = await this.variableManager.getDataFrameInfo(variable, notebook);
const output = await dataProvider.getDataFrameInfo();

// Log telemetry about number of rows
try {
Expand All @@ -131,13 +145,8 @@ export class DataViewer extends WebViewHost<IDataViewerMapping> implements IData

private async getAllRows() {
return this.wrapRequest(async () => {
if (this.variable && this.variable.rowCount && this.notebook) {
const allRows = await this.variableManager.getDataFrameRows(
this.variable,
this.notebook,
0,
this.variable.rowCount
);
if (this.dataProvider) {
const allRows = await this.dataProvider.getAllRows();
this.pendingRowsCount = 0;
return this.postMessage(DataViewerMessages.GetAllRowsResponse, allRows);
}
Expand All @@ -146,12 +155,10 @@ export class DataViewer extends WebViewHost<IDataViewerMapping> implements IData

private getRowChunk(request: IGetRowsRequest) {
return this.wrapRequest(async () => {
if (this.variable && this.variable.rowCount && this.notebook) {
const rows = await this.variableManager.getDataFrameRows(
this.variable,
this.notebook,
if (this.dataProvider) {
const rows = await this.dataProvider.getRows(
request.start,
Math.min(request.end, this.variable.rowCount)
Math.min(request.end, (await this.dataProvider.getDataFrameInfo()).rowCount)
);
return this.postMessage(DataViewerMessages.GetRowsResponse, {
rows,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,15 @@ import { inject, injectable } from 'inversify';

import { IAsyncDisposable, IAsyncDisposableRegistry } from '../../common/types';
import { IServiceContainer } from '../../ioc/types';
import { IDataViewer, IDataViewerProvider, IJupyterVariable, INotebook } from '../types';
import { DataViewerDependencyService } from './dataViewerDependencyService';
import { IDataViewer, IDataViewerFactory } from '../types';
import { IDataViewerDataProvider } from './types';

@injectable()
export class DataViewerProvider implements IDataViewerProvider, IAsyncDisposable {
export class DataViewerFactory implements IDataViewerFactory, IAsyncDisposable {
private activeExplorers: IDataViewer[] = [];
constructor(
@inject(IServiceContainer) private serviceContainer: IServiceContainer,
@inject(IAsyncDisposableRegistry) asyncRegistry: IAsyncDisposableRegistry,
@inject(DataViewerDependencyService) private dependencyService: DataViewerDependencyService
@inject(IAsyncDisposableRegistry) asyncRegistry: IAsyncDisposableRegistry
) {
asyncRegistry.push(this);
}
Expand All @@ -25,18 +24,17 @@ export class DataViewerProvider implements IDataViewerProvider, IAsyncDisposable
await Promise.all(this.activeExplorers.map((d) => d.dispose()));
}

public async create(variable: IJupyterVariable, notebook: INotebook): Promise<IDataViewer> {
public async create(dataProvider: IDataViewerDataProvider, title: string): Promise<IDataViewer> {
let result: IDataViewer | undefined;

// Create the data explorer (this should show the window)
// Create the data explorer
const dataExplorer = this.serviceContainer.get<IDataViewer>(IDataViewer);
try {
// Verify this is allowed.
await this.dependencyService.checkAndInstallMissingDependencies(notebook.getMatchingInterpreter());

// Then load the data.
this.activeExplorers.push(dataExplorer);
await dataExplorer.showVariable(variable, notebook);

// Show the window and the data
await dataExplorer.showData(dataProvider, title);
result = dataExplorer;
} finally {
if (!result) {
Expand Down
104 changes: 104 additions & 0 deletions src/client/datascience/data-viewing/jupyterVariableDataProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';
import '../../common/extensions';

import { inject, injectable, named } from 'inversify';

import { JSONObject } from '@phosphor/coreutils';

import { Identifiers } from '../constants';
import { IJupyterVariable, IJupyterVariableDataProvider, IJupyterVariables, INotebook } from '../types';
import { DataViewerDependencyService } from './dataViewerDependencyService';
import { ColumnType, IDataFrameInfo } from './types';

@injectable()
export class JupyterVariableDataProvider implements IJupyterVariableDataProvider {
private initialized: boolean = false;

constructor(
@inject(IJupyterVariables) @named(Identifiers.ALL_VARIABLES) private variableManager: IJupyterVariables,
@inject(DataViewerDependencyService) private dependencyService: DataViewerDependencyService,
private variable: IJupyterVariable,
private notebook: INotebook
) {}

/**
* Normalizes column types to the types the UI component understands.
* Defaults to 'string'.
* @param columns
* @returns Array of columns with normalized type
*/
private static getNormalizedColumns(columns: { key: string; type: string }[]): { key: string; type: ColumnType }[] {
return columns.map((column: { key: string; type: string }) => {
let normalizedType: ColumnType;
switch (column.type) {
case 'bool':
normalizedType = ColumnType.Bool;
break;
case 'integer':
case 'int32':
case 'int64':
case 'float':
case 'float32':
case 'float64':
case 'number':
normalizedType = ColumnType.Number;
break;
default:
normalizedType = ColumnType.String;
}
return {
key: column.key,
type: normalizedType
};
});
}

public dispose(): void {
return;
}

public async getDataFrameInfo(): Promise<IDataFrameInfo> {
await this.ensureInitialized();
return {
columns: this.variable.columns
? JupyterVariableDataProvider.getNormalizedColumns(this.variable.columns)
: this.variable.columns,
indexColumn: this.variable.indexColumn,
rowCount: this.variable.rowCount || 0
};
}

public async getAllRows() {
let allRows: JSONObject = {};
await this.ensureInitialized();
if (this.variable.rowCount) {
allRows = await this.variableManager.getDataFrameRows(
this.variable,
this.notebook,
0,
this.variable.rowCount
);
}
return allRows;
}

public async getRows(start: number, end: number) {
let rows: JSONObject = {};
await this.ensureInitialized();
if (this.variable.rowCount) {
rows = await this.variableManager.getDataFrameRows(this.variable, this.notebook, start, end);
}
return rows;
}

private async ensureInitialized(): Promise<void> {
// Postpone pre-req and variable initialization until data is requested.
if (!this.initialized) {
await this.dependencyService.checkAndInstallMissingDependencies(this.notebook.getMatchingInterpreter());
this.variable = await this.variableManager.getDataFrameInfo(this.variable, this.notebook);
this.initialized = true;
}
}
}
24 changes: 21 additions & 3 deletions src/client/datascience/data-viewing/types.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';
import type { JSONObject } from '@phosphor/coreutils';

import { JSONObject } from '@phosphor/coreutils';
import { SharedMessages } from '../messages';
import { IJupyterVariable } from '../types';

export const CellFetchAllLimit = 100000;
export const CellFetchSizeFirst = 100000;
Expand Down Expand Up @@ -43,10 +42,29 @@ export interface IGetRowsResponse {
export type IDataViewerMapping = {
[DataViewerMessages.Started]: never | undefined;
[DataViewerMessages.UpdateSettings]: string;
[DataViewerMessages.InitializeData]: IJupyterVariable;
[DataViewerMessages.InitializeData]: IDataFrameInfo;
[DataViewerMessages.GetAllRowsRequest]: never | undefined;
[DataViewerMessages.GetAllRowsResponse]: JSONObject;
[DataViewerMessages.GetRowsRequest]: IGetRowsRequest;
[DataViewerMessages.GetRowsResponse]: IGetRowsResponse;
[DataViewerMessages.CompletedData]: never | undefined;
};

export interface IDataFrameInfo {
columns?: { key: string; type: ColumnType }[];
indexColumn?: string;
rowCount: number;
}

export interface IDataViewerDataProvider {
dispose(): void;
getDataFrameInfo(): Promise<IDataFrameInfo>;
getAllRows(): Promise<JSONObject>;
getRows(start: number, end: number): Promise<JSONObject>;
}

export enum ColumnType {
String = 'string',
Number = 'number',
Bool = 'bool'
}
Loading