Skip to content

Make data viewer openable from the variables window while debugging #14269

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.

Sign up for GitHub

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

Closed
joyceerhl wants to merge 9 commits into from
Closed

Make data viewer openable from the variables window while debugging #14269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d21c822
Show 'View Tensor in Data Viewer' for tensors
joyceerhl Oct 5, 2020
ca3c6c3
Need to add property directly to object
joyceerhl Oct 6, 2020
5c00335
Hack
joyceerhl Oct 6, 2020
e6b5ccf
Wire up context menu item to open data viewer
joyceerhl Oct 6, 2020
15cfbe6
Reuse interface
joyceerhl Oct 6, 2020
ce5191b
Got opening dataviewer from variables window working
joyceerhl Oct 8, 2020
1e84606
Cleanup
joyceerhl Oct 8, 2020
815bbba
Check column size before opening in dataviewer
joyceerhl Oct 8, 2020
1679b3e
Make data provider not require a notebook
joyceerhl Oct 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,11 @@
"title": "DataScience.latestExtension",
"category": "Python"
},
{
"command": "python.datascience.showDataViewer",
"title": "View Tensor in Data Viewer",
Comment thread
joyceerhl marked this conversation as resolved.
Outdated
"category": "Python"
},
{
"command": "python.analysis.restartLanguageServer",
"title": "%python.command.python.analysis.restartLanguageServer.title%",
Expand Down Expand Up @@ -1655,6 +1660,14 @@
"when": "view == python_tests && viewItem == suite && !busyTests",
"group": "inline@0"
}
],
"debug/variables/context": [
{
"command": "python.datascience.showDataViewer",
"group": "1_view",
"title": "View Tensor in Data Viewer",
"when": "debugProtocolVariableMenuContext == 'Tensor'"
Comment thread
joyceerhl marked this conversation as resolved.
Outdated
}
]
},
"breakpoints": [
Expand Down
43 changes: 25 additions & 18 deletions src/client/datascience/jupyter/debuggerVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,24 +274,31 @@ export class DebuggerVariables implements IConditionalJupyterVariables, DebugAda
? this.configService.getSettings().datascience.variableExplorerExclude?.split(';')
: [];

const allowedVariables = variablesResponse.body.variables.filter((v) => {
if (!v.name || !v.type || !v.value) {
return false;
}
if (exclusionList && exclusionList.includes(v.type)) {
return false;
}
if (v.name.startsWith('_')) {
return false;
}
if (KnownExcludedVariables.has(v.name)) {
return false;
}
if (v.type === 'NoneType') {
return false;
}
return true;
});
const allowedVariables = variablesResponse.body.variables
Copy link
Copy Markdown

@DonJayamanne DonJayamanne Oct 6, 2020

Choose a reason for hiding this comment

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

We might want to get in touch with Debugger team about this.
Its something they might find useful or be aware of (the fact that we're adding this).
Personally I'd prefer if all of this was done in debugpy and we just hooked into some message or the like (e.g. we could use capabilities or other flags & use custom messages)
@int19h @karthiknadig /cc

Copy link
Copy Markdown

@int19h int19h Oct 6, 2020

Choose a reason for hiding this comment

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

It would be hard to make it quite as flexible as client-side filtering. If we hardcode the filters, debugpy would have to be touched every time this logic needs revising. And if we don't hardcode them, then the protocol would need to be expressive enough to express all those conditions, which would also make the implementation expensive.

I think client-side filtering is the better option here, unless there are important perf reasons to exclude some variables.

Copy link
Copy Markdown
Author

@joyceerhl joyceerhl Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just in the interest of clarity (even though I think the conclusion is the same), we're not asking for filtering logic in debugpy. What we're after is debugpy setting the __vscodeVariableMenuContext property on DebugProtocol.Variables before they are sent over to VS Code, several lines down from this spot: https://github.com/microsoft/vscode-python/pull/14269/files#diff-d8fc76e8c5b19f7c5df2b4282409e727R297-R300

As background for anyone unfamiliar with why we need this, the reason we need this property is so that we can make use of the new contribution point for the variable context menu introduced in the August release. VS Code uses the hidden __vscodeVariableMenuContext property to set a per-variable debugProtocolVariableMenuContext key with the same contents (details). Extensions can then use the debugProtocolVariableMenuContext key in when clauses that specify whether a given context menu item with an associated command should be displayed for a given variable.

.filter((v) => {
if (!v.name || !v.type || !v.value) {
return false;
}
if (exclusionList && exclusionList.includes(v.type)) {
return false;
}
if (v.name.startsWith('_')) {
return false;
}
if (KnownExcludedVariables.has(v.name)) {
return false;
}
if (v.type === 'NoneType') {
return false;
}
return true;
})
.map((v) => {
return v.type === 'Tensor'
? // tslint:disable-next-line: no-object-literal-type-assertion
({ ...v, __vscodeVariableMenuContext: 'Tensor' } as DebugProtocol.Variable)
: v;
});

this.lastKnownVariables = allowedVariables.map((v) => {
return {
Expand Down