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

Add debugger variable renderer based on mime type #10299

Merged
merged 1 commit into from Sep 1, 2021

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented May 26, 2021

Status :

Waiting on #10406 to ensure no visual regression
i won't have time to address #10406. This can be merged if approved.

References

Xeus-python is providing a new debug message to get a rich variable representation:
jupyter-xeus/xeus-python#446

Fixes #9867

This adds a command and a panel to display such rich variable.

Code changes

  • Add a new command to trigger the variable rich display
  • Add context menu on variable explorer
    • That required to add an interface on the explorer to get the selected variable
  • In the tree view an action button is displayed on hover to render the variable
  • Add some styling to make the error message in MainAreaWidget appear on error background.
  • the kernel capability is returned in the debugInfo request to add this feature or not.

User-facing changes

New context menu on the variable explorer. It has one entry on the tree view to render the variable based on mime type information. And on the grid view it has a second entry to inspect the object

The rich renderer can also be triggered from an action button displayed on hover in the tree view.

Backwards-incompatible changes

N/A

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jtpio jtpio added this to the 3.1 milestone May 26, 2021
@github-actions github-actions bot added Design System CSS pkg:apputils tag:CSS For general CSS related issues and pecadilloes labels May 27, 2021
@fcollonval fcollonval marked this pull request as ready for review June 1, 2021 08:20
@jtpio
Copy link
Member

jtpio commented Jun 1, 2021

Thanks!

Mind adding a quick screencast or screenshot to the description to have a quick idea of what it looks like??

@fcollonval
Copy link
Member Author

Here comes the demo:

demo_renderer

@jtpio
Copy link
Member

jtpio commented Jun 2, 2021

Nice, thanks for sharing!

@krassowski
Copy link
Member

krassowski commented Jun 2, 2021

This looks wonderful! One design point: the context menus are not greatly discoverable. Would it be a good idea to add a button on the listing to expose this feature more? For example, RStudio has small icons on the right-hand side of the variable explorer:

Screenshot from 2021-06-02 13-31-27

Edit: to clarify I did run the binder when this PR was originally opened and could not find a way to display the preview so gave up (I did not have time to look at the code first); if seasoned users cannot find it, newbies won't either.

@jtpio
Copy link
Member

jtpio commented Jun 2, 2021

@fcollonval
Copy link
Member Author

Code is linted and I added a button to display the rich view:

demo_render_variable

@krassowski
Copy link
Member

How can we test this? Does it need a new version of xeus or some other component? Binder gives me:

Error: Unable to create message from dict: {'seq': 11, 'type': 'request', 'command': 'richInspectVariables', 'arguments': {'variableName': 'test', 'variablesReference': 0}}. richInspectVariables not in ['attach', 'breakpointLocations', 'cancel', 'completions', 'configurationDone', 'continue', 'dataBreakpointInfo', 'disassemble', 'disconnect', 'evaluate', 'exceptionInfo', 'goto', 'gotoTargets', 'initialize', 'launch', 'loadedSources', 'modules', 'next', 'pause', 'pydevdAuthorize', 'pydevdSystemInfo', 'readMemory', 'restart', 'restartFrame', 'reverseContinue', 'runInTerminal', 'scopes', 'setBreakpoints', 'setDataBreakpoints', 'setDebuggerProperty', 'setExceptionBreakpoints', 'setExpression', 'setFunctionBreakpoints', 'setInstructionBreakpoints', 'setPydevdSourceMap', 'setVariable', 'source', 'stackTrace', 'stepBack', 'stepIn', 'stepInTargets', 'stepOut', 'terminate', 'terminateThreads', 'threads', 'variables']

@SylvainCorlay
Copy link
Member

How can we test this? Does it need a new version of xeus or some other component? Binder gives me:

It requires xeus-python master, but it is basically a chicken and egg situation. I presume it is safe to release xeus-python with this when this gets merged.

@jtpio
Copy link
Member

jtpio commented Jun 4, 2021

[x] How to know the kernel as that feature? => proposal to return the kernel capability in the debugInfo request.

Is there a link to that proposal? Or maybe a work-in progress PR adding it to xeus-python?

@jtpio
Copy link
Member

jtpio commented Jun 4, 2021

For now this is what it looks like on Binder if the kernel does not support the feature:

rich-inspect-variable-binder.mp4

* @param variablesReference The variable reference to request
* @returns The mime renderer data model
*/
inspectRichVariable(
Copy link
Member

Choose a reason for hiding this comment

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

The debug message is called richInspectVariables, should the public method on the service named the same (in singular form)?

Not strong opinion, just raising the question before this gets in the pubic IDebugger API.

@fcollonval fcollonval marked this pull request as draft June 4, 2021 12:02
@fcollonval
Copy link
Member Author

fcollonval commented Jun 4, 2021

the kernel does not support the feature:

Thanks for testing @jtpio

I swiched the PR to draft as indeed it does not make much sense without having a feedback from the kernel to know if the feature is supported or not => waiting on @JohanMabille for that part.

The debug message is called richInspectVariables, should the public method on the service named the same (in singular form)?

Actually this is not the case for some other messages

method request
inspectVariables variables
displayDefinedVariables inspectVariables

I switches the word because I like the convention of starting a method with a verb. But as it is not listed in the project style guidelines, I'm fine switch to your proposal.

Side note: the example highlights an unfortunate choice of words as a method and a request have the same name without being related.

@jtpio
Copy link
Member

jtpio commented Jun 7, 2021

Side note: the example highlights an unfortunate choice of words as a method and a request have the same name without being related.

Right, the inspectVariables request was added after (for 3.1). Maybe there will be an opportunity to clean this up for 4.0 if this changes the API of the IDebugger service.

@JohanMabille
Copy link
Member

JohanMabille commented Jun 9, 2021

xeus-python 0.12.5 has been released. It has full support for the richInspectVariables request.
Notice the additional field richRendering in the debug_info_response message. This field is optional (for backward compatibility) and should be tested since the support for richInspectVariable request has not been implemented in ipykernel yet.

@jtpio
Copy link
Member

jtpio commented Sep 1, 2021

Thanks @fcollonval.

Just tried on Binder and the following code snippet:

from ipywidgets import IntSlider

slider = IntSlider()
slider

Gives the following behavior:

rich-variable-not-supported.mp4

This is with xeus-python 0.12.5:

image

@fcollonval
Copy link
Member Author

Thanks for testing. I'm confused because this works with the same version on my machine 😕

slider_mimetype

@fcollonval
Copy link
Member Author

Would it be that you are using the stable ipywidgets library? It works only with the upcoming version v8 for widgets.

@jtpio
Copy link
Member

jtpio commented Sep 1, 2021

Would it be that you are using the stable ipywidgets library? It works only with the upcoming version v8 for widgets.

Yes it was with the latest stable ipywidgets 7.6.4. Works well with the latest prerelease 👍

I guess it would be fine to ship it before ipywidgets 8.0 is out. It will not be optimal to ipywidgets 7 users, but will work for other mime types.

@@ -672,12 +770,12 @@ export namespace IDebugger {
/**
* Signal emitted when the current frame has changed.
*/
readonly currentFrameChanged: ISignal<this, IDebugger.IStackFrame>;
readonly currentFrameChanged: ISignal<this, IDebugger.IStackFrame | null>;
Copy link
Member

Choose a reason for hiding this comment

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

Let's tag as api-change since there are a couple of signature changes in the public interfaces.

@jtpio jtpio added the api-change A change that should be accompanied by a major version increase label Sep 1, 2021
@jtpio
Copy link
Member

jtpio commented Sep 1, 2021

@goanpeca do we want to get this in the 3.2 release? Looks like the PR is listed in #10726

If so the comment above about the API change might be relevant?

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@jtpio
Copy link
Member

jtpio commented Sep 1, 2021

If so the comment above about the API change might be relevant?

In that case we might have to bump to the 4.0 milestone.

@fcollonval
Copy link
Member Author

I think we can bump to 4.0 as the lifecycle of the panels need improvements for it to be really useful - the current behavior close the panel each time the notebook changes.

@jtpio jtpio modified the milestones: 3.2.x, 4.0 Sep 1, 2021
@jtpio
Copy link
Member

jtpio commented Sep 1, 2021

OK switching to 4.0 then.

@blink1073 blink1073 merged commit b98301c into jupyterlab:master Sep 1, 2021
@blink1073
Copy link
Member

Thank you!

@meeseeksmachine
Copy link
Contributor

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/how-to-use-debugger-in-jupyterlab/11275/14

@krassowski krassowski mentioned this pull request Dec 15, 2021
20 tasks
@fcollonval
Copy link
Member Author

@meeseeksdev please backport to 3.3.x

@lumberbot-app
Copy link

lumberbot-app bot commented Jan 14, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 3.3.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 b98301c28da54acdc1360df37c9f8945dd0a6d5a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #10299: Add debugger variable renderer based on mime type'
  1. Push to a named branch:
git push YOURFORK 3.3.x:auto-backport-of-pr-10299-on-3.3.x
  1. Create a PR against branch 3.3.x, I would have named this PR:

"Backport PR #10299 on branch 3.3.x (Add debugger variable renderer based on mime type)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-change A change that should be accompanied by a major version increase Design System CSS enhancement pkg:apputils pkg:debugger tag:CSS For general CSS related issues and pecadilloes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Debugger] Rich mimetype rendering in the variable inspector
9 participants