Sort variables by name in variable explorer WITH test added. #5886

vandyliu · 2021-05-14T04:27:05Z

For #4585

10,000 variables test

Attempt 3

Will try to add 'type' column to be sortable afterwards

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 enhancements.
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).

codecov-commenter · 2021-05-14T05:31:55Z

Codecov Report

Merging #5886 (f2e1c5e) into main (b3f22c6) will decrease coverage by 0%.
The diff coverage is 83%.

@@          Coverage Diff          @@
##            main   #5886   +/-   ##
=====================================
- Coverage     72%     72%   -1%     
=====================================
  Files        405     405           
  Lines      27198   27204    +6     
  Branches    3972    3975    +3     
=====================================
- Hits       19620   19608   -12     
+ Misses      5947    5940    -7     
- Partials    1631    1656   +25

Impacted Files	Coverage Δ
.../datascience/interactive-common/synchronization.ts	`84% <ø> (ø)`
src/client/datascience/jupyter/kernelVariables.ts	`70% <83%> (-1%)`	⬇️
src/client/common/process/pythonDaemonPool.ts	`75% <0%> (-5%)`	⬇️
src/client/datascience/raw-kernel/rawSocket.ts	`84% <0%> (-2%)`	⬇️
...tascience/jupyter/liveshare/hostJupyterNotebook.ts	`44% <0%> (-2%)`	⬇️
src/client/datascience/jupyter/jupyterNotebook.ts	`75% <0%> (-1%)`	⬇️
...active-common/intellisense/intellisenseProvider.ts	`74% <0%> (-1%)`	⬇️
src/client/common/utils/decorators.ts	`77% <0%> (ø)`
src/client/datascience/webviews/webviewHost.ts	`87% <0%> (ø)`
... and 7 more

rchiodo · 2021-05-14T17:15:46Z

src/client/datascience/jupyter/kernelVariables.ts

+                    return (a[sortColumn] > b[sortColumn]) ? 1 : -1;
+                }
+            };
+            result.pageResponse.sort(comparer);


This isn't going to work. It's only sorting a single page at a time. The page retrieved might even be the wrong one.

Imagine you had 10,000 variables, let's say with names like:

A1
A2
A3
.
.
B1
B2
B3
.

And you fetched the first page. It will return the A1 page. You then sort descending. It still fetch the first page, you'll just have that page sorted descending, but what the user wanted to see was the Z900 to Z800 set of variables.

You have to push the sorting all the way into the python code.

And that's rather complicated because it requires reworking how variables are fetched (we only fetch names and then with a separate request we fetch types).

Ahh, I see where I went wrong. Thanks for all your input :)
I shall try again

rchiodo

⏲️

rchiodo · 2021-05-14T17:27:31Z

I think here

vscode-jupyter/src/client/datascience/jupyter/kernelVariables.ts

Line 371 in 82b2b5b

variables: (await this.getVariableNamesFromKernel(notebook)).map((n) => {

I think the request there is going to change. You won't be getting the list of names, but instead getting all of the data for a page at a time from the server.

Some piece of python code that will:

Retrieve the list of variables and their types
Sort based on name or type
Get the values for the page requested (as they're visible)
Returning just that page

You might be able to do this instead (I'm not 100% sure this will work)

Send request to server to retrieve sorted list of variables with name and type information (not values)
Replace the code in the kernelVariableProvider to retrieve this list based on if the sort order changes
Keep the same code that already gets the value for each variable in the page requested

…ft#5901)

Co-authored-by: Ian Huff <ianhuff@lcs2d-Dell-04.northamerica.corp.microsoft.com>

rchiodo · 2021-05-19T19:34:56Z

Looks great to me. Will you do type in a subsequent PR? Seems like a good idea.

vandyliu · 2021-05-19T20:01:17Z

Looks great to me. Will you do type in a subsequent PR? Seems like a good idea.

Yeah, I'm gonna try to add type, but wanted do the name one first to keep PRs smaller

IanMatthewHuff

Looks great! Maybe next step on this we should add a test for this in our CI.

vandyliu · 2021-05-20T20:54:39Z

@IanMatthewHuff I added a test for it on this PR.
If it's good, can someone merge 😁

rchiodo · 2021-05-20T20:59:55Z

@IanMatthewHuff I added a test for it on this PR.
If it's good, can someone merge 😁

I believe you have linting errors. If you look at the linting task it should tell you what the problem is.

Alternatively if you install ESLint in VS Code and give it permission to run, it should catch the errors when you save a file locally.

rchiodo · 2021-05-20T21:12:04Z

It looks like the linting error is this:

Error: Files are being ignored that should be linted: src/datascience-ui/native-editor/redux/actions.ts
src/datascience-ui/history-react/redux/actions.ts

That means in our .eslintrc file you have to remove src/datascience-ui/history-react/redux/actions.ts from the ignore list.

We didn't start using eslint until somewhat recently and some of the files were skipped as we didn't want to fix all eslint errors at once. As we edit them the build task makes sure the linter will run on newly edited files. After you fix the ignore problem, you might have to fix linter errors in that file too.

rchiodo · 2021-05-20T22:07:24Z

@DavidKutu @IanMatthewHuff @joyceerhl Are those errors expected now? I believe they were some sort of kernel finding problem which shouldn't have been caused by @vandyliu's changes.

joyceerhl · 2021-05-20T22:11:27Z

The local notebook failures seem new. I'm not seeing those fail on the latest main run or on the latest PR run from 1hr ago. Try rerunning?

IanMatthewHuff · 2021-05-20T22:12:00Z

@DavidKutu @IanMatthewHuff @joyceerhl Are those errors expected now? I believe they were some sort of kernel finding problem which shouldn't have been caused by @vandyliu's changes.

@rchiodo The one failure in nonConda remote notebook was expected. The failure in nonConda 'local' was not expected from what I had last seen.

DavidKutu · 2021-05-20T22:42:42Z

The nonConda local errors seem unrelated. They were already fixed but I guess something might've broken them again.
I just merged something and they passed.

Please try merging main here to see if they still fail.
If they do, go ahead and merge and file a bug and link to this run (https://github.com/microsoft/vscode-jupyter/pull/5886/checks?check_run_id=2634150447)

vandyliu marked this pull request as ready for review May 14, 2021 17:06

vandyliu requested a review from a team as a code owner May 14, 2021 17:06

rchiodo reviewed May 14, 2021

View reviewed changes

vandyliu force-pushed the sortingvariables2 branch from 959b7ac to 50c3a9d Compare May 14, 2021 17:16

rchiodo requested changes May 14, 2021

View reviewed changes

Sort variables by name and type in variable explorer.

4ef94f9

vandyliu force-pushed the sortingvariables2 branch from 50c3a9d to 4ef94f9 Compare May 15, 2021 02:09

vandyliu changed the title ~~Sort variables by name and type in variable explorer.~~ WIP Sort variables by name and type in variable explorer. May 18, 2021

vandy liu and others added 3 commits May 18, 2021 17:38

Sorting variables deeper

b0d62cc

More safety when reading notebook.cellToolbarLocation keys (microso…

b0c92cc

…ft#5901)

force on middleware (microsoft#5919)

0f9aea2

Co-authored-by: Ian Huff <ianhuff@lcs2d-Dell-04.northamerica.corp.microsoft.com>

vandyliu force-pushed the sortingvariables2 branch from 7d3d0cb to 0f9aea2 Compare May 19, 2021 00:40

vandyliu and others added 9 commits May 18, 2021 17:40

Merge branch 'microsoft:main' into sortingvariables2

e978a38

undo commits

4706ecd

redo commits from main

fb4bc75

cleanup

dd84f2a

cleanup again

48fb0c4

sort only name for now

e5dd7a9

remove unnecessary code

33e3207

small fix

6fe7e8e

remove type sorting for now

da37a89

vandyliu requested a review from rchiodo May 19, 2021 03:05

vandyliu changed the title ~~WIP Sort variables by name and type in variable explorer.~~ Sort variables by name and type in variable explorer. May 19, 2021

vandyliu changed the title ~~Sort variables by name and type in variable explorer.~~ Sort variables by name in variable explorer. May 19, 2021

rchiodo approved these changes May 19, 2021

View reviewed changes

joyceerhl approved these changes May 19, 2021

View reviewed changes

IanMatthewHuff approved these changes May 19, 2021

View reviewed changes

DavidKutu approved these changes May 19, 2021

View reviewed changes

vandy liu added 2 commits May 19, 2021 19:51

add a test

538543d

added test for sorting variables by name column

4944a5d

vandyliu requested a review from IanMatthewHuff May 20, 2021 20:01

vandyliu changed the title ~~Sort variables by name in variable explorer.~~ Sort variables by name in variable explorer WITH test added. May 20, 2021

fixed spacing

1607bcc

undo accidental commit

238db68

vandy liu added 2 commits May 20, 2021 14:18

ran prettier

329108d

edit eslintrc files

2da9d33

vandy liu added 2 commits May 20, 2021 16:29

Merge branch 'main' into sortingvariables2

58df38b

small change to test

f2e1c5e

rchiodo merged commit f65b6af into microsoft:main May 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort variables by name in variable explorer WITH test added. #5886

Sort variables by name in variable explorer WITH test added. #5886

vandyliu commented May 14, 2021 •

edited

Loading

codecov-commenter commented May 14, 2021 •

edited

Loading

rchiodo May 14, 2021

rchiodo May 14, 2021

rchiodo May 14, 2021

vandyliu May 14, 2021

rchiodo left a comment

rchiodo commented May 14, 2021

rchiodo commented May 19, 2021

vandyliu commented May 19, 2021

IanMatthewHuff left a comment

vandyliu commented May 20, 2021

rchiodo commented May 20, 2021

rchiodo commented May 20, 2021

rchiodo commented May 20, 2021

joyceerhl commented May 20, 2021

IanMatthewHuff commented May 20, 2021

DavidKutu commented May 20, 2021

Sort variables by name in variable explorer WITH test added. #5886

Sort variables by name in variable explorer WITH test added. #5886

Conversation

vandyliu commented May 14, 2021 • edited Loading

codecov-commenter commented May 14, 2021 • edited Loading

Codecov Report

rchiodo May 14, 2021

Choose a reason for hiding this comment

rchiodo May 14, 2021

Choose a reason for hiding this comment

rchiodo May 14, 2021

Choose a reason for hiding this comment

vandyliu May 14, 2021

Choose a reason for hiding this comment

rchiodo left a comment

Choose a reason for hiding this comment

rchiodo commented May 14, 2021

rchiodo commented May 19, 2021

vandyliu commented May 19, 2021

IanMatthewHuff left a comment

Choose a reason for hiding this comment

vandyliu commented May 20, 2021

rchiodo commented May 20, 2021

rchiodo commented May 20, 2021

rchiodo commented May 20, 2021

joyceerhl commented May 20, 2021

IanMatthewHuff commented May 20, 2021

DavidKutu commented May 20, 2021

vandyliu commented May 14, 2021 •

edited

Loading

codecov-commenter commented May 14, 2021 •

edited

Loading