Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract out python-specific code out of KernelVariables and into PythonVariablesRequester (and Sort variable explorer by types) #6042

Merged
merged 14 commits into from
Jun 4, 2021

Conversation

vandyliu
Copy link
Contributor

@vandyliu vandyliu commented May 31, 2021

For #4585

This changes some things in KernelVariables so not entirely how badly this would affect other kernels. This addition may not be worth it depending on that. Any/all comments appreciated

Summary:

  • Created interface: IKernelVariableRequester
  • Created class PythonVariableRequester (which extends the interface above)
  • Code that used to be in KernelVariables (and was Python-specific) is now in PythonVariableRequester, and KernelVariables calls IKernelVariableRequester functions
  • 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).

@vandyliu vandyliu requested a review from a team as a code owner May 31, 2021 21:44
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2021

Codecov Report

Merging #6042 (f46ec8f) into main (d458944) will decrease coverage by 0%.
The diff coverage is 69%.

@@          Coverage Diff           @@
##            main   #6042    +/-   ##
======================================
- Coverage     72%     71%    -1%     
======================================
  Files        398     399     +1     
  Lines      26846   26866    +20     
  Branches    3950    3952     +2     
======================================
- Hits       19428   19300   -128     
+ Misses      6537    5938   -599     
- Partials     881    1628   +747     
Impacted Files Coverage Δ
src/client/datascience/jupyter/kernelVariables.ts 68% <53%> (-3%) ⬇️
...ent/datascience/jupyter/pythonVariableRequester.ts 74% <74%> (ø)
src/client/datascience/constants.ts 99% <100%> (+<1%) ⬆️
src/client/datascience/serviceRegistry.ts 97% <100%> (+<1%) ⬆️
src/client/datascience/types.ts 100% <100%> (ø)
...ience/variablesView/variableViewMessageListener.ts 22% <0%> (-78%) ⬇️
...ent/common/application/webviewViews/webviewView.ts 14% <0%> (-72%) ⬇️
...c/client/datascience/variablesView/variableView.ts 18% <0%> (-58%) ⬇️
src/client/datascience/webviews/webviewViewHost.ts 19% <0%> (-53%) ⬇️
...t/datascience/jupyter/jupyterInvalidKernelError.ts 66% <0%> (-34%) ⬇️
... and 137 more


// VariableTypesFunc takes in list of vars and the corresponding var names
const results = await notebook.execute(
`print(${GetVariableInfo.VariableTypesFunc}([${matches}], [${matchesAsStr}]))`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obviously python specific. The query.query was intended to support other languages. I think we could at least extract out the python part into a separate class? Maybe make an interface for getting names and types, like IKernelVariableRequester or something.

So instead of having getVariableNamesandTypesFromKernel be a function, there would be an interface with this single method on it.

Right now we'd just support python, but this interface could be expanded (or suppled by other extensions) for other languages later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other functions that do some python specific things. I'm thinking about putting those into the PythonVariableRequester as well.

@@ -58,3 +58,16 @@ def _VSCODE_getVariableProperties(var, listOfAttributes):
if hasattr(var, attr)
}
return _VSCODE_json.dumps(result)


def _VSCODE_getVariableTypes(vars, varnames):
Copy link
Contributor

Choose a reason for hiding this comment

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

How long does this take to execute for say 10000 variables?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if we're making another request for types, could we do the list and the types at the same time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Internally this would essentially call the same function we do now to list the names (you'd have to look up what the magic %_who_ls does in ipykernel)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

think I got something that works well

Copy link
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

⏲️

@vandyliu vandyliu changed the title Sort variable explorer by types WIP: Sort variable explorer by types Jun 1, 2021
@vandyliu vandyliu changed the title WIP: Sort variable explorer by types Extract out python-specific code out of KernelVariables and into PythonVariablesRequester (and Sort variable explorer by types) Jun 2, 2021
Copy link
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

Looks good. Like the python variable separation. Should make supporting other languages a lot easier.

@vandyliu vandyliu merged commit 9719f6f into microsoft:main Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants