Web version of the DataFrame viewer #10825

sadasant · 2022-07-15T01:01:28Z

(The prior take on solving #9665 was PR: #10604)

Last month we tried to get the DataFrame viewer working on the web. In summary, we arrived at a set of bigger goals that are available here: #10638 . The first step actually does solve #9665 , but with an imperfect approach that is nonetheless good enough to get this feature out and get us started towards our longer term goals.

The first step, solved in this PR, goes as follows:

Enable the Data Frame on the web (Fixing #9665)

By changing the IDataViewerDependencyService to receive an IKernel besides the PythonEnvironment that it receives today.

By adding a universal version of the dataViewerDependencyService that would only work if IKernel is provided and throw otherwise an error saying Installation of "pandas" not supported while debugging. Please ensure you have "pandas" installed.

By keeping the current node-specific version of the dataViewerDependencyService that would extract the interpreter from the IKernel if the IKernel is provided.

Besides ensuring the dataViewerDependencyService is registered on the web service registry, and removing the conditional that disables the Data Frame on the web.

This PR contains those changes, and includes unit tests.

Fixes #9665

Feedback always appreciated!

src/webviews/extension-side/dataviewer/dataViewerDependencyService.ts

codecov-commenter · 2022-07-15T02:03:02Z

Codecov Report

Merging #10825 (3536b30) into main (d37e4a9) will increase coverage by 0%.
The diff coverage is 85%.

❗ Current head 3536b30 differs from pull request most recent head c3341d7. Consider uploading reports for the commit c3341d7 to get more accurate results

@@           Coverage Diff           @@
##            main   #10825    +/-   ##
=======================================
  Coverage     63%      63%            
=======================================
  Files        482      486     +4     
  Lines      33652    33736    +84     
  Branches    5488     5499    +11     
=======================================
+ Hits       21295    21413   +118     
+ Misses     10304    10274    -30     
+ Partials    2053     2049     -4

Impacted Files	Coverage Δ
src/platform/common/utils.node.ts	`58% <ø> (-4%)`	⬇️
src/telemetry.ts	`100% <ø> (ø)`
src/webviews/extension-side/dataviewer/types.ts	`100% <ø> (ø)`
src/platform/common/utils/localize.ts	`76% <66%> (+<1%)`	⬆️
src/platform/common/utils.ts	`80% <75%> (-1%)`	⬇️
...viewer/kernelDataViewerDependencyImplementation.ts	`83% <83%> (ø)`
...erpreterDataViewerDependencyImplementation.node.ts	`84% <84%> (ø)`
...eter/nbconvertInterpreterDependencyChecker.node.ts	`95% <100%> (ø)`
...rc/webviews/extension-side/dataviewer/constants.ts	`100% <100%> (ø)`
...ide/dataviewer/dataViewerDependencyService.node.ts	`100% <100%> (+13%)`	⬆️
... and 21 more

src/webviews/extension-side/dataviewer/dataViewerDependencyService.node.ts

src/webviews/extension-side/dataviewer/dataViewerDependencyService.ts

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts

…iledToInstallPandas message

IanMatthewHuff · 2022-07-18T12:28:54Z

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts

+    constructor(private readonly applicationShell: IApplicationShell, private isCodeSpace: boolean) {}
+
+    protected async execute(command: string, kernel: IKernel): Promise<(string | undefined)[]> {
+        const outputs = await executeSilently(kernel.session!, command);


The assumption that the kernel has a session (with the !) seems like it might be sneaking around the type system a bit. I don't see, at least at this point, an explicit reason the session would be undefined. Per the usage of this function it look like it should maybe just take a Session as a non optional parameter? I don't believe anything else on the Kernel is used, and the caller should guarantee that there is a session.

Yeah, I see the check at the start of the dependency check later in the code. Seems like after that you could just use the session.

This function is called only by a function that checks for the session, but I like your idea! Passing the session directly, rather than the kernel. Thank you 🙏

How about this? c3341d7

IanMatthewHuff · 2022-07-18T12:36:19Z

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts

+        const pandasVersion = await this.getVersion(kernel);
+
+        if (pandasVersion) {
+            if (pandasVersion.compare(pandasMinimumVersionSupported) > 0) {


Might be nice to have another telemetry event here? It's the type of thing that we can probably calculate using other events, but an explict Pandas === Ok event might help calculating how many Pandas failures we see.

How about this? c0f10c7

IanMatthewHuff · 2022-07-18T12:52:02Z

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts

+export const kernelGetPandasVersion =
+    'import pandas as _VSCODE_pandas;print(_VSCODE_pandas.__version__);del _VSCODE_pandas';
+
+function kernelPackaging(kernel: IKernel): '%conda' | '%pip' {


@DonJayamanne might know best here. But this will use %pip for envs like poetry. In those cases should we just not do the install and ask them to install manually?

Yes agreed,

what does that mean? how do I catch environments like poetry?

I think it's unlikely this will ever pick 'conda' either. When we have an IKernel, we're in a remote situation. Otherwise we'd be using the interpreter.

I think we can leave this as is for now, and then figure out some other way to detect the presence of the package manager later.

src/platform/common/utils/localize.ts

DonJayamanne · 2022-07-18T19:16:48Z

src/webviews/extension-side/dataviewer/interpreterDataViewerDependencyImplementation.node.ts

+                sendTelemetryEvent(Telemetry.PandasTooOld);
+                // Warn user that we cannot start because pandas is too old.
+                const versionStr = `${pandasVersion.major}.${pandasVersion.minor}.${pandasVersion.build}`;
+                throw new Error(DataScience.pandasTooOldForViewingFormat().format(versionStr));


Shouldn't the prompt be displayed at this point in time?
I.e. if the version is too old, shouldn't we display the prmonpt and ask to install.

DonJayamanne · 2022-07-18T19:20:43Z

src/webviews/extension-side/dataviewer/interpreterDataViewerDependencyImplementation.node.ts

+                sendTelemetryEvent(Telemetry.UserInstalledPandas);
+            }
+        } else {
+            sendTelemetryEvent(Telemetry.UserDidNotInstallPandas);


These lines are still duplicated, and will be duplicated in 3 places again.

Here's my suggestion

protected async installMissingDependencies(token) { const pandasVersions = await this.getPandasVersion(token); if (pandasVersion && ) { if (pandasVersion.compare(pandasMinimumVersionSupportedByVariableViewer) > 0) { return; } sendTelemetryEvent(Telemetry.PandasTooOld); } else { sendTelemetryEvent(Telemetry.PandasNotInstalled); await this.installMissingDependencies(tokenSource); } protected abstract getPandasVersion(token); protected installMissingDependencies(token) { sendTelemetryEvent(Telemetry.PythonModuleInstall, undefined, { action: 'displayed', moduleName: ProductNames.get(Product.pandas)!, pythonEnvType: interpreter?.envType }); const promptResult = .... sendTelemetry(...) if (doNotInstall) { sendTelemetryEvent(....) } else { sendTelemetryEvent(....) } }

this way most of the common code is in the base class, and the child classes will inherit the base class and only implement the code to provide the versions and fill inn the blanks for installing.
& all of the code for sending telemetry if prompt is cancelled or clicked ok, or the like is in one place.
Right now its still copied around and still duplicated & I believe it can still be improved to avoid the duplication.

These kind of abstractions seem like early optimizations to me, given that they give the impression that the installer can become fully generic, when it isn't ready for doing so. For example:

Why tie this to Pandas specifically?

How do we pass the kernel or interpreter around? Would we use an abstract type on the abstract class to determine the type of "evaluator" that will be passed along?

If not passed around, should the evaluator (kernel / interpreter) be part of the state of the class? If so, should we have a form of setup function that ensures that we always have that evaluator? because we can't use the constructor for this at the moment.

Alternatively, should we move the instantiaton of the *DataViewerDependencyImplementation to the checkAndInstallMissingDependencies rather than keeping it in the constructor of the DataViewerDependencyService? That would come at the compromise of having to instantiate a class on every checkAndInstallMissingDependencies call.

I believe Don is just suggesting that shared code should be in a base class. Simply to eliminate duplicate code. The design should be relatively close to the same I'd think.

Besides that, the behavior is not exactly the same with the kernel as with the interpreter, because the interpreter version does have some of the installing abstracted out into the IInstaller.

I'd like to reach to a level of compromise for this PR, then get the Plot Viewer working on the web, then come back to the debugger installer, then move all to the IInstaller as the last step.

So, what do you think of this?

export abstract class BaseDataViewerDependencyImplementation<TExecuter> implements IDataViewerDependencyService { protected abstract _getVersion(executer: TExecuter): Promise<string | undefined>; protected async getVersion(executer: TExecuter): Promise<SemVer | undefined> { try { const version = await this._getVersion(executer); return typeof version === 'string' ? parseSemVer(version) : version; } catch (e) { traceWarning(DataScience.failedToGetVersionOfPandas(), e.message); return; } }

Then

export class KernelDataViewerDependencyImplementation extends BaseDataViewerDependencyImplementation<IKernel> { protected async _getVersion(kernel: IKernelWithSession): Promise<string | undefined> { const outputs = await this.execute(kernelGetPandasVersion, kernel); return outputs.map((text) => (text ? text.toString() : undefined)).find((item) => item); }

I'm thinking I'd rather focus on telemetry wrappers than on splitting the steps into common functions.

Feedback appreciated!

The above is just an idea. Once I understand what kind of approach/pattern to take, I can extrapolate

Perhaps more like this?

protected abstract _getVersion(executer: TExecuter): Promise<string | undefined>; @traceDecoratorWarn(DataScience.failedToGetVersionOfPandas()) protected async getVersion(executer: TExecuter): Promise<SemVer | undefined> { const version = await this._getVersion(executer); return typeof version === 'string' ? parseSemVer(version) : version; }

How does this look? sadasant#1

cc: @DonJayamanne , @IanMatthewHuff , @rchiodo

I merged that PR!

DonJayamanne · 2022-07-18T19:21:38Z

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts

+                throw new Error(DataScience.failedToInstallPandas());
+            }
+        } else {
+            sendTelemetryEvent(Telemetry.UserDidNotInstallPandas);


Sending the telemetry after installing and when failing, or when not required to install is still duplicated and feel this can be moved into the base class.

DonJayamanne · 2022-07-18T19:22:20Z

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts

+            }
+            sendTelemetryEvent(Telemetry.PandasTooOld);
+            // Warn user that we cannot start because pandas is too old.
+            const versionStr = `${pandasVersion.major}.${pandasVersion.minor}.${pandasVersion.build}`;


This code can also be moved into the base class, all the implementations need to do is get the version and return it, and the base class can check if a version was available or not.

DonJayamanne

I still think a lof of the code can be moved into a base class, even if debugger implementation comes in later.

Refactoring

rchiodo · 2022-07-19T17:41:27Z

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts

+import { BaseDataViewerDependencyImplementation } from './baseDataViewerDependencyImplementation';
+
+export const kernelGetPandasVersion =
+    'import pandas as _VSCODE_pandas;print(_VSCODE_pandas.__version__);del _VSCODE_pandas';


Seems weird to have this one one line? Why not have line feeds?

For the debugger I'll definitely need to work in line feeds, at least the last time I tried it. So, we'll get there 👍

sadasant · 2022-07-19T19:11:08Z

Reported a (new?) flaky test: #10860

sadasant · 2022-07-19T19:53:20Z

Now I'm getting two different failures, one regarding DataScience Exports, and another one where the Log test results job Failed to print test summary. I'm assuming this PR is good to go. I'll merge it.

sadasant added 6 commits July 14, 2022 15:43

indirect changes

5b9350a

wip

c267967

all the changes

241c272

test fix

59023b4

cleanups

8bd4032

unnecessary code

6b4f59e

sadasant self-assigned this Jul 15, 2022

sadasant added 2 commits July 14, 2022 21:10

throwing if an interpreter is provided on the web

ca30491

renamed file

9350045

sadasant commented Jul 15, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/dataViewerDependencyService.ts Outdated Show resolved Hide resolved

sadasant commented Jul 15, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/dataViewerDependencyService.ts Outdated Show resolved Hide resolved

sadasant commented Jul 15, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/dataViewerDependencyService.ts Outdated Show resolved Hide resolved

sadasant commented Jul 15, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/dataViewerDependencyService.ts Outdated Show resolved Hide resolved

rchiodo reviewed Jul 15, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/dataViewerDependencyService.node.ts Outdated Show resolved Hide resolved

rchiodo reviewed Jul 15, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/dataViewerDependencyService.ts Outdated Show resolved Hide resolved

sadasant added 2 commits July 15, 2022 20:58

Refactor: Two implementations and one service

ab08bd5

Telemetry update

01ad320

sadasant marked this pull request as ready for review July 16, 2022 01:09

sadasant requested a review from a team as a code owner July 16, 2022 01:09

removing unnecessary injectable

fe8863a

DonJayamanne reviewed Jul 17, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Jul 17, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Jul 17, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Jul 17, 2022

View reviewed changes

src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts Outdated Show resolved Hide resolved

sadasant added 5 commits July 17, 2022 11:31

removing unecessary code paths and adding the error message to the fa…

d38850e

…iledToInstallPandas message

telemetry update

81aadad

rework of the pandas version

4cd0251

Merge remote-tracking branch 'VSCode/main' into web/9665-8

44cca5e

missed this one

3536b30

IanMatthewHuff reviewed Jul 18, 2022

View reviewed changes

sadasant added 6 commits July 18, 2022 12:06

sending the error through sendTelemetryEvent

9bdd963

localization fixes

4ddaa72

pandasMinimumVersionSupportedByVariableViewer

d6870dc

isPythonEnvironment

f24f3cd

telemetry cleanups

4dc3be3

BaseDataViewerDependencyImplementation

60dcbf9

DonJayamanne reviewed Jul 18, 2022

View reviewed changes

src/platform/common/utils/localize.ts Outdated Show resolved Hide resolved

coerced kernel with session

c3341d7

DonJayamanne reviewed Jul 18, 2022

View reviewed changes

sadasant added 2 commits July 18, 2022 15:25

telemetry: PandasOK and PandasInstallCanceled

c0f10c7

unlocalizing DataScience.noActiveKernelSession

52b30e3

DonJayamanne approved these changes Jul 18, 2022

View reviewed changes

Refactoring based on microsoft#10825 (comment)

af9d2be

sadasant mentioned this pull request Jul 18, 2022

Refactoring sadasant/vscode-jupyter#1

Merged

sadasant and others added 2 commits July 19, 2022 12:00

removed console.logs

d59afc3

Merge pull request #1 from sadasant/web/9665-8-refactoring

9ef5978

Refactoring

rchiodo reviewed Jul 19, 2022

View reviewed changes

rchiodo approved these changes Jul 19, 2022

View reviewed changes

sadasant mentioned this pull request Jul 19, 2022

Flaky test: Kernel.IKernelConnection Events #10860

Closed

sadasant merged commit 22d43ce into microsoft:main Jul 19, 2022

sadasant deleted the web/9665-8 branch July 19, 2022 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web version of the DataFrame viewer #10825

Web version of the DataFrame viewer #10825

sadasant commented Jul 15, 2022 •

edited

Loading

codecov-commenter commented Jul 15, 2022 •

edited

Loading

IanMatthewHuff Jul 18, 2022

IanMatthewHuff Jul 18, 2022

sadasant Jul 18, 2022 •

edited

Loading

sadasant Jul 18, 2022

IanMatthewHuff Jul 18, 2022

sadasant Jul 18, 2022

IanMatthewHuff Jul 18, 2022

DonJayamanne Jul 18, 2022

sadasant Jul 18, 2022

rchiodo Jul 18, 2022

DonJayamanne Jul 18, 2022

DonJayamanne Jul 18, 2022

sadasant Jul 18, 2022 •

edited

Loading

rchiodo Jul 18, 2022

sadasant Jul 18, 2022

sadasant Jul 18, 2022

sadasant Jul 18, 2022

sadasant Jul 18, 2022

sadasant Jul 18, 2022

sadasant Jul 19, 2022

DonJayamanne Jul 18, 2022

DonJayamanne Jul 18, 2022

DonJayamanne left a comment

rchiodo Jul 19, 2022

sadasant Jul 19, 2022

sadasant commented Jul 19, 2022

sadasant commented Jul 19, 2022

Web version of the DataFrame viewer #10825

Web version of the DataFrame viewer #10825

Conversation

sadasant commented Jul 15, 2022 • edited Loading

codecov-commenter commented Jul 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant commented Jul 19, 2022

sadasant commented Jul 19, 2022

sadasant commented Jul 15, 2022 •

edited

Loading

codecov-commenter commented Jul 15, 2022 •

edited

Loading

sadasant Jul 18, 2022 •

edited

Loading

sadasant Jul 18, 2022 •

edited

Loading