Web version of the DataFrame #10545

sadasant · 2022-06-23T03:51:34Z

The only thing missing on the web to enable the DataFrame features was to have a web-ready version of the DataViewerDependencyService.

After discussing ideas with @DonJayamanne and @rchiodo , Rich's approach seemed the most straightforward way to add web support to DataViewerDependencyService. Rich's approach, as far as I understood, was to:

DataViewerDependencyService to take a dependency on the IKernel and have it execute a %pip install pandas instead of using IInstaller.
- It would use either %conda or %pip based on the IKernel's KernelConnectionMetadata. %conda if it had an interpreter (local case) that was conda based. Otherwise %pip.
For figuring out the version of pandas, use the kernel. Just import pandas and print the version.

This PR follows that approach. Besides that:

Moves the parseSemVer function from platform/common/utils.node to platform/common/utils.
Adds IDataViewerDependencyService to the extension-side web service registry.
removes the isWebconditional from thevariableExplorerButtonCellFormatter.tsx`.

Thus:
Fixes #9665
Fixes #10310

Feedback always appreciated!

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
Has a news entry file (remember to thank yourself!).
Appropriate comments and documentation strings in the code.
Has sufficient logging.
Has telemetry for feature-requests.
Unit tests & system/integration tests are added/updated.
Test plan is updated as appropriate.
package-lock.json has been regenerated by running npm install (if dependencies have changed).

src/webviews/extension-side/serviceRegistry.node.ts

IanMatthewHuff · 2022-06-23T13:08:03Z

Hmmm, the new test that I added for opening DataViewer from the debug menu looks to be failing in this PR. I'd expect my test was having issue, but I don't see it failing in the most recent schedule run: https://github.com/microsoft/vscode-jupyter/runs/7020267384?check_suite_focus=true. Might be something up here, I can help look into.

That code path also uses the DataViewerDependencyService to check Pandas version. I can help show that scenario.

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts

rchiodo

Looks good, although I think you could have made the unit tests much simpler with a sinon.stub of 'executeSilently' instead of having to mock the kernel behavior.

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

IanMatthewHuff

We chatted in teams. Looks like this change will be a forcing factor for checking pandas version in the python debug scenario the right way. Instead of executing the code snippit on the kernel session we'll execute it on the activeDebugSession. And hey, we know the end to end test works now :).

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

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts

DonJayamanne

API doesn't look right, we're picking random kernels, instead of a the kernel associated with the current execution.

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

… another for Kernels (Web)

codecov-commenter · 2022-06-24T12:02:39Z

Codecov Report

Merging #10545 (8ac4217) into main (440397a) will decrease coverage by 0%.
The diff coverage is 85%.

❗ Current head 8ac4217 differs from pull request most recent head 88d099b. Consider uploading reports for the commit 88d099b to get more accurate results

@@          Coverage Diff           @@
##            main   #10545   +/-   ##
======================================
- Coverage     71%      71%   -1%     
======================================
  Files        472      473    +1     
  Lines      27993    28068   +75     
  Branches    4691     4702   +11     
======================================
- Hits       19946    19937    -9     
- Misses      6179     6262   +83     
- Partials    1868     1869    +1

Impacted Files	Coverage Δ
src/platform/common/utils.node.ts	`53% <ø> (-5%)`	⬇️
src/telemetry.ts	`100% <ø> (ø)`
src/webviews/extension-side/dataviewer/types.ts	`100% <ø> (ø)`
src/platform/common/utils.ts	`80% <75%> (-1%)`	⬇️
...ion-side/dataviewer/dataViewerDependencyService.ts	`84% <84%> (ø)`
src/platform/common/utils/localize.ts	`76% <85%> (+<1%)`	⬆️
...eter/nbconvertInterpreterDependencyChecker.node.ts	`95% <100%> (ø)`
...nsion-side/dataviewer/dataViewerCommandRegistry.ts	`72% <100%> (ø)`
...ide/dataviewer/dataViewerDependencyService.node.ts	`86% <100%> (+<1%)`	⬆️
...ion-side/dataviewer/jupyterVariableDataProvider.ts	`59% <100%> (-10%)`	⬇️
... and 21 more

sadasant · 2022-06-24T12:25:41Z

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

+        throw new Error('Not implemented');
+    }
+
+    public async checkAndInstallMissingDependenciesOnEnvironment(interpreter: PythonEnvironment): Promise<void> {


The alternative I thought of was to keep the same method as before but to accept options or a variadic set of parameters. I thought that had unnecessary implications -- unnecessary since I did not intend to implement the installer to support the PythonEnvironment in the web.

id still prefer to have the same method name and the same argument interpreter.
you can get the interpreter from the kernel. connectionMetadata. interpreter.

ie it is doable, and i feel that's much better than having two methods, one for local and one for remote.

I do like that idea! Thank you! I'll figure it out 👍

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

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts

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

sadasant · 2022-06-24T17:30:35Z

@DonJayamanne thank you for your feedback! I agree with all of them! 🙂 I'll do the updates!

…ing feedback from Don and Rich at the same time

IanMatthewHuff · 2022-06-24T19:27:07Z

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

+ * Responsible for managing dependencies of a Data Viewer.
+ */
+@injectable()
+export class DataViewerDependencyService implements IDataViewerDependencyService {


Does this need .web file naming since it's the class we inject in the web scenario? I'm guessing you might not have since it doesn't actually have web only dependencies, but in function is seems to be.

This file should work in Node without troubles as long as IKernel is sent. I wouldn't think .web.ts applies in that case.

IanMatthewHuff · 2022-06-24T19:35:04Z

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

+        @inject(IsCodeSpace) private isCodeSpace: boolean
+    ) {}
+
+    private packaging(kernel: IKernel): 'pip' | 'conda' {


There are a lot of different env types here. Might not be safe to just call everything not conda as pip. As a first pass we could just ignore the other types. Poetry and what not.

I think that's what we're doing 🤔 isConda ? 'conda' : 'pip'

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

… new lines

DonJayamanne · 2022-06-24T23:05:37Z

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

+    }
+
+    public async checkAndInstallMissingDependenciesOnKernel(kernel: IKernel): Promise<void> {
+        const pandasVersion = await this.getVersionOfPandas(kernel);


tomorrow, if someone comes along and modifies the code to call this method instead of the ones with the argument, it's not wrong.
but will break things.
i think the problem is there's no way for someone to know which methods to call and when.
hence the suggestion to have a single method that takes the kernel argument.
as mentioned earlier from the kernel we can get the interpreter in one line.

we can get the kernel for any given resource as well, all you need is the nottebook or notebookuri. i

I do like that idea! Thank you! I'll figure it out 👍

DonJayamanne · 2022-06-24T23:10:37Z

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts


-        const promise = dependencyService.checkAndInstallMissingDependencies(interpreter);
+        const stub = sinon.stub(helpers, 'executeSilently');


not sure if this works, however ifeel it might be easier to stub the kernel. session methods.

If it isn't working I'm hallucinating 😅 I wouldn't discard that possibility though

DonJayamanne · 2022-06-24T23:14:55Z

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

@@ -130,5 +130,6 @@ export interface IJupyterVariableDataProviderFactory {

 export const IDataViewerDependencyService = Symbol('IDataViewerDependencyService');
 export interface IDataViewerDependencyService {
-    checkAndInstallMissingDependencies(interpreter: PythonEnvironment): Promise<void>;
+    checkAndInstallMissingDependenciesOnEnvironment(environment: PythonEnvironment): Promise<void>;
+    checkAndInstallMissingDependenciesOnKernel(kernel: IKernel): Promise<void>;
 }


id prefer to haber a single method, two feels like a work around which introduces debt.
there's nothing preventing one from using the wrong method.

DonJayamanne · 2022-06-24T23:21:05Z

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

+                    pythonEnv &&
+                        (await this.dataViewerDependencyService.checkAndInstallMissingDependenciesOnEnvironment(
+                            pythonEnv
+                        ));
                }


wait, will this work with remote debugging.
i don't think so.
ie if im debugging a remote kernel, then will this fixed path work?
i think we'll need to get that working as well for the web.
in there loader it didn't work, but now that we're enabling df viewer for web, 100% of web users will use remote kernels, hence this won't work when they're debugging.

again, i think this is what @IanMatthewHuff mentioned.
we'd need to change the code as we should be sending the request using the debugger instead of interpreter or kernel (whether it's local or remote)

I don't think the command to debug a python file with the Python extension is working on the web. I can't get to make it work.

Besides that, this approach you are pointing out would be broken on remote today, is that the case?

I would like to separate the problem of the debugger into a separate PR, because the debugger in itself is fairly complex and it does not seem to be working working on the web anyway.

Also, this file is using the pre-existing dataViewerDependencyService, not the new one

rchiodo · 2022-06-24T23:50:17Z

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

@@ -39,7 +39,11 @@ export class DataViewerDependencyService implements IDataViewerDependencyService
        @inject(IsCodeSpace) private isCodeSpace: boolean
    ) {}

-    public async checkAndInstallMissingDependencies(interpreter: PythonEnvironment): Promise<void> {
+    public async checkAndInstallMissingDependenciesOnKernel(): Promise<void> {


Why do we have two versions? Can't they both use the kernel?

sadasant · 2022-06-25T00:44:55Z

After speaking with @rchiodo about the debugger, I will include the debugger changes in this PR. I will close it for now, to make sure we have something cleaner to review with the debugger changes included.

sadasant added 6 commits June 21, 2022 16:31

wip

de6d0bd

wip

6ce33b5

this version actually works

c3ff8b7

Works in the web! Fixes microsoft#10310

609dc02

first unit test passing

c04a1c2

finished dataViewerDependencyService.unit.test.ts, as far as I can tell

96b7f84

sadasant requested a review from a team as a code owner June 23, 2022 03:51

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

83aee7a

sadasant commented Jun 23, 2022

View reviewed changes

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

rchiodo reviewed Jun 23, 2022

View reviewed changes

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts Outdated Show resolved Hide resolved

rchiodo approved these changes Jun 23, 2022

View reviewed changes

IanMatthewHuff reviewed Jun 23, 2022

View reviewed changes

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

IanMatthewHuff requested changes Jun 23, 2022

View reviewed changes

DonJayamanne reviewed Jun 23, 2022

View reviewed changes

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

DonJayamanne reviewed Jun 23, 2022

View reviewed changes

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts Outdated Show resolved Hide resolved

DonJayamanne requested changes Jun 23, 2022

View reviewed changes

rchiodo reviewed Jun 23, 2022

View reviewed changes

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

sadasant added 2 commits June 24, 2022 08:02

Separating dependency service, one for Python environments (Node) and…

63b59d1

… another for Kernels (Web)

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

c0d7976

sadasant commented Jun 24, 2022

View reviewed changes

sadasant added 3 commits June 24, 2022 08:39

fixing the tests

94709af

lint fix

f713b7c

Using Sinon stubs

ae2d667

DonJayamanne reviewed Jun 24, 2022

View reviewed changes

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

DonJayamanne reviewed Jun 24, 2022

View reviewed changes

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts Show resolved Hide resolved

DonJayamanne reviewed Jun 24, 2022

View reviewed changes

src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts Show resolved Hide resolved

DonJayamanne reviewed Jun 24, 2022

View reviewed changes

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

microsoft deleted a comment from Kingpancho Jun 24, 2022

sadasant added 2 commits June 24, 2022 15:09

Ensuring we test the right Python code is being executed, and address…

2a44df0

…ing feedback from Don and Rich at the same time

fixing ends of lines after clearing my related Git configs

8ac4217

microsoft deleted a comment from Kingpancho Jun 24, 2022

IanMatthewHuff reviewed Jun 24, 2022

View reviewed changes

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

sadasant added 2 commits June 24, 2022 16:03

this change was needed for web, and a test to verify output ending in…

89d1691

… new lines

using Jupyter magic commands %

88d099b

DonJayamanne reviewed Jun 24, 2022

View reviewed changes

rchiodo reviewed Jun 24, 2022

View reviewed changes

sadasant closed this Jun 25, 2022

sadasant mentioned this pull request Jun 27, 2022

DataFrame working in web extension #9665

Closed

sadasant deleted the web/9665-5 branch July 20, 2022 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web version of the DataFrame #10545

Web version of the DataFrame #10545

sadasant commented Jun 23, 2022 •

edited

Loading

IanMatthewHuff commented Jun 23, 2022 •

edited

Loading

rchiodo left a comment

IanMatthewHuff left a comment •

edited

Loading

DonJayamanne left a comment

codecov-commenter commented Jun 24, 2022 •

edited

Loading

sadasant Jun 24, 2022

DonJayamanne Jun 24, 2022

sadasant Jun 24, 2022

sadasant commented Jun 24, 2022

IanMatthewHuff Jun 24, 2022

sadasant Jun 24, 2022

IanMatthewHuff Jun 24, 2022

sadasant Jun 24, 2022

DonJayamanne Jun 24, 2022

sadasant Jun 24, 2022

DonJayamanne Jun 24, 2022

sadasant Jun 24, 2022

DonJayamanne Jun 24, 2022

DonJayamanne Jun 24, 2022

sadasant Jun 24, 2022

sadasant Jun 24, 2022

rchiodo Jun 24, 2022

sadasant commented Jun 25, 2022


		const promise = dependencyService.checkAndInstallMissingDependencies(interpreter);
		const stub = sinon.stub(helpers, 'executeSilently');

Web version of the DataFrame #10545

Web version of the DataFrame #10545

Conversation

sadasant commented Jun 23, 2022 • edited Loading

IanMatthewHuff commented Jun 23, 2022 • edited Loading

rchiodo left a comment

Choose a reason for hiding this comment

IanMatthewHuff left a comment • edited Loading

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 24, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant commented Jun 24, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant commented Jun 25, 2022

sadasant commented Jun 23, 2022 •

edited

Loading

IanMatthewHuff commented Jun 23, 2022 •

edited

Loading

IanMatthewHuff left a comment •

edited

Loading

codecov-commenter commented Jun 24, 2022 •

edited

Loading