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

JEP for copy-to-globals support in debugger_info #93

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Feb 20, 2023

Introducing a new field to the kernel debugger_info response.
This new field will inform the UI that the debugger supports the copyToGlobals request.

Related to jupyterlab/jupyterlab#13476 (comment), which introduces a new feature in the debugger. It would be useful to know if this feature is supported by the kernel from UI side.

cc. @JohanMabille @krassowski

Voting from @jupyter/software-steering-council

@JohanMabille
Copy link
Member

+1 on this, it has a very limited impact on the protocol and is backward compatible.


# Proposed Enhancement

We propose to add a new `copyToGlobals` boolean field to the `debugger_info`
Copy link
Member

Choose a reason for hiding this comment

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

Would not it be more in line with DAP to advertise it via capabilities of initialize request response? For example, "copy to clipboard" can be implemented relying on supportsClipboardContext?: boolean;.

I see that this would require modifying the message in flight, but it seems cleaner on the API side.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the same goes for richRendering (which we have not codified so far).

Copy link
Member

Choose a reason for hiding this comment

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

I guess the same goes for richRendering (which we have not codified so far).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @krassowski

These "capabilities" are really kernel capabilities, and not debugger capabilities. I'm more in favor of separating the 2 types of capabilities, for clarity. An other solution could be to add a capabilities object in the debugInfo response, but that would not be backward compatible.

If I am not mistaken, all requests/responses defined in DAP are only forwarded by the kernel. It is just an interface between UI and debugger. Changes to the DAP message format will only affect the UI and not the kernels, making maintenance easier.

Copy link
Member

@JohanMabille JohanMabille Feb 23, 2023

Choose a reason for hiding this comment

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

Would not it be more in line with DAP to advertise it via capabilities of initialize request response? For example, "copy to clipboard" can be implemented relying on supportsClipboardContext?: boolean;.

I'm not sure. The idea until now was to extend the existing DAP by adding new messages, not by modifying the existing messages or types used to compose the messages. Adding new capabilities to the existing ones in the DAP would mean introducing a new interface (inheriting form the Capabilities one) and replacing the InitializeResponse spec with our own. From a protocol side, it is easier to maintain a clear separation between the DAP original specification, and our adds to it.

@JohanMabille
Copy link
Member

Additional context on this: the copyToGlobals request was added to the Jupyter Debugger Protocol and implemented in xeus-python and ipykernel. However, this request:

Therefore, a solution is to make it optional, like the richInspectVariables request, and avertise for its availabitily in the debugInfo request.

@fcollonval
Copy link
Contributor

Thanks for this JEP @brichet

I would suggest describing in the JEP the request and response format of this new message; adding the section Reference-level explanation seems the right place for that.

@brichet
Copy link
Contributor Author

brichet commented Nov 21, 2023

Thanks for this JEP @brichet

I would suggest describing in the JEP the request and response format of this new message; adding the section Reference-level explanation seems the right place for that.

Thanks @fcollonval, I updated the description.

@ivanov
Copy link
Member

ivanov commented Nov 27, 2023

I am abstaining from this vote as I have no familiarity with the debugger. I have never had occasion to use it as a user, let alone dive into its implementation details as a developer.

@gabalafou
Copy link
Contributor

I wanted to familiarize myself more with this before voting, but realistically, I'm not going to find the time, so I'm abstaining.

@brichet
Copy link
Contributor Author

brichet commented Dec 4, 2023

Some additional information to summarize this feature:

When the debugger stop on a breakpoint, some variables are in the global scope and some other can be in a local scope (in a function). Using the debugger tools only does not help for some introspection in complexe variables.

When the execution ends, the variable(s) in the local scope will not be reachable anymore.

We added a feature in some python kernels (ipykernel and xeus-python, see #93 (comment)) to be able to copy a variable from a local scope to a global scope during a breakpoint. This way, the variable(s) can be introspected after the code execution, since it still exists in the global scope.

The aim of this JEP is only to send a flag to Jupyterlab (or other UI), whether this feature is supported or not by the kernel.

brichet and others added 2 commits December 7, 2023 10:26
Co-authored-by: gabalafou <gabriel@fouasnon.com>
Co-authored-by: gabalafou <gabriel@fouasnon.com>
@JohanMabille
Copy link
Member

6 yes, 3 abstains, 2 no votes, let's merge!

@JohanMabille JohanMabille merged commit 0e3d954 into jupyter:master Dec 11, 2023
1 check passed
@brichet brichet deleted the debugger_info_copy_to_globals branch December 11, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants