Refactor how variables and DF info is fetched #10557

DonJayamanne · 2022-06-23T23:44:42Z

DonJayamanne · 2022-06-23T23:45:54Z

src/kernels/variables/debuggerVariables.ts

-    private async importDataFrameScripts(): Promise<void> {
-        try {
-            // Run our dataframe scripts only once per session because they're slow
-            const key = this.debugService.activeDebugSession?.id;


This same code was repeated in two places, now one place

DonJayamanne · 2022-06-23T23:46:52Z

src/kernels/variables/debuggerVariables.ts

        const results = await this.evaluate(
-            `${GetVariableInfo.VariableInfoFunc}(${variable.name})`,


The problem here is when debugging we have an await and its possible after we first import the scripts the stack frame has changed, hence the variables will be lost (not in scope).

This fixes that by ensureing that the function call always imports the necessary scripts (previously it was done seprately)

THe original code worked because of it loading a module but also had the added benefit of it not needing to be loaded more than once.

Could we change the new code to create a module to accomplish the same thing? So we don't have to load it more than once?

Meaning we wouldn't have to recreate the functions on every stack frame.

The side benefit is we would have less extra cruft in the debugger variable list too.

I do'nt see any harm in re-creating (re-loading this everytime).
Also we cannot go with a module as that won't work in web/remote.

I'll update the code to delete the imported namespaces & the functions.

Baically change to as follows:

import xyz def __vsc_getDataFrameInfo(): .... print(results) delete xyz del __vsc_getDataFrameInfo

This way we clean up all of the imports and also the function definitions we create.

I don't see that as being an issue, its not a large chunk of code that could slow things down, only runs when the user has stopped at a stack frame & running that code is super fast

I think we could leave the module there. We did before.

wouldn't need to reload on EVERY variable request

Agree, however running such code is very simple & fast.
Also not leaving the module as users have already complained about *.vscode variables, next they'll complain about the modules as well, hence figured its better to clean that up.

I guess it depends upon how long this extra code takes to run. Could have up to 30 requests for this (assuming variable view was showing 30 variables at a time)

Do you have any idea how long it takes? 50ms each eval? 100ms?

Maybe we could only load and delete it once per frame? Does python have destructors? (Was thinking of how to delete on scope leaving)

Could have up to 30 requests for this (assuming variable view was showing 30 variables at a time)

Will check.
FYI - we only have this when debugging, e.g. in regular execution, we send everything as a single request.
Will look into the execution times now.

src/kernels/variables/debuggerVariables.ts

codecov-commenter · 2022-06-24T00:23:29Z

Codecov Report

Merging #10557 (9d61910) into main (f548f40) will decrease coverage by 0%.
The diff coverage is 77%.

❗ Current head 9d61910 differs from pull request most recent head d573ac4. Consider uploading reports for the commit d573ac4 to get more accurate results

@@          Coverage Diff           @@
##            main   #10557   +/-   ##
======================================
- Coverage     71%      71%   -1%     
======================================
  Files        473      475    +2     
  Lines      28025    28050   +25     
  Branches    4698     4706    +8     
======================================
+ Hits       19968    19977    +9     
- Misses      6189     6191    +2     
- Partials    1868     1882   +14

Impacted Files	Coverage Δ
src/platform/common/scriptConstants.ts	`100% <ø> (ø)`
src/kernels/variables/pythonVariableRequester.ts	`55% <61%> (-15%)`	⬇️
src/kernels/variables/debuggerVariables.ts	`68% <65%> (-2%)`	⬇️
src/platform/common/variableScriptGenerator.ts	`75% <75%> (ø)`
src/platform/common/dataFrameScriptGenerator.ts	`90% <90%> (ø)`
src/platform/common/serviceRegistry.node.ts	`100% <100%> (ø)`
src/platform/common/types.ts	`100% <100%> (ø)`
src/kernels/kernelDependencyService.node.ts	`81% <0%> (-5%)`	⬇️
src/kernels/variables/kernelVariables.ts	`56% <0%> (-2%)`	⬇️
... and 8 more

DonJayamanne · 2022-06-24T01:21:53Z

pythonFiles/vscode_datascience_helpers/getVariableInfo/vscodeGetVariableInfo.py

-# Query Jupyter server for the info about a dataframe
-import json as _VSCODE_json
-import builtins as _VSCODE_builtins
+def _VSCODE_getVariable(what_to_get, *args):


@rchiodo I've modified the code so that everything is under a function now, all we need to do is delete the imported modules & delete this function that we declare (when cleaning up).
If we were to import a module, we'd still need to delete the imported module,

Now we should not end up with tonnes of function definitinos as everything is local to this one function, hence no need to clean those individual functions (as its all local & goes out of scope)

DonJayamanne · 2022-06-24T01:57:24Z

src/test/datascience/variableView/variableView.vscode.test.ts

+        // Sample output is `["test", "test2", "os", "sys"]`
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        const vars = ((outputs[0].data as any)['text/plain'] as string)
+            .trim()


Added a new test to ensure we don't leave any messy variables behind

Filed a bug to remove os & sys (stuff that we add when starting kernels & the like)

src/kernels/variables/pythonVariableRequester.ts

DonJayamanne requested a review from a team as a code owner June 23, 2022 23:44

DonJayamanne commented Jun 23, 2022

View reviewed changes

src/kernels/variables/debuggerVariables.ts Show resolved Hide resolved

DonJayamanne marked this pull request as draft June 24, 2022 00:24

DonJayamanne removed the request for review from a team June 24, 2022 00:24

DonJayamanne commented Jun 24, 2022

View reviewed changes

DonJayamanne requested a review from rchiodo June 24, 2022 01:29

DonJayamanne marked this pull request as ready for review June 24, 2022 01:33

DonJayamanne commented Jun 24, 2022

View reviewed changes

rchiodo reviewed Jun 24, 2022

View reviewed changes

src/kernels/variables/pythonVariableRequester.ts Show resolved Hide resolved

DonJayamanne force-pushed the issue10516 branch 2 times, most recently from cb437af to 08251bc Compare June 26, 2022 22:36

DonJayamanne requested a review from rchiodo June 26, 2022 22:36

rchiodo approved these changes Jun 27, 2022

View reviewed changes

DonJayamanne added 11 commits June 28, 2022 08:29

Refactor how variables and DF info is fetched

a47071f

Nerws

bf4ead2

oops

9d21755

oops

5a270ac

Fixes

83d407f

Even better

e6634e7

Misc

305aa9d

Fix formatting

5b4043d

Misc

d5fa12c

Fix test

b55f2f9

Fixes

4050478

DonJayamanne force-pushed the issue10516 branch from 08251bc to 1394960 Compare June 27, 2022 22:29

Fixes

579f1f1

DonJayamanne force-pushed the issue10516 branch from d573ac4 to 579f1f1 Compare June 28, 2022 11:48

DonJayamanne merged commit 975fb96 into main Jun 28, 2022

DonJayamanne deleted the issue10516 branch June 28, 2022 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor how variables and DF info is fetched #10557

Refactor how variables and DF info is fetched #10557

DonJayamanne commented Jun 23, 2022

DonJayamanne Jun 23, 2022

DonJayamanne Jun 23, 2022

rchiodo Jun 24, 2022

rchiodo Jun 24, 2022

DonJayamanne Jun 24, 2022 •

edited

Loading

DonJayamanne Jun 24, 2022

rchiodo Jun 27, 2022

DonJayamanne Jun 27, 2022

rchiodo Jun 27, 2022

rchiodo Jun 27, 2022

DonJayamanne Jun 27, 2022

codecov-commenter commented Jun 24, 2022 •

edited

Loading

DonJayamanne Jun 24, 2022

DonJayamanne Jun 24, 2022

DonJayamanne Jun 24, 2022

		const results = await this.evaluate(
		`${GetVariableInfo.VariableInfoFunc}(${variable.name})`,

Refactor how variables and DF info is fetched #10557

Refactor how variables and DF info is fetched #10557

Conversation

DonJayamanne commented Jun 23, 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

DonJayamanne Jun 24, 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

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

DonJayamanne Jun 24, 2022 •

edited

Loading

codecov-commenter commented Jun 24, 2022 •

edited

Loading