Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Use the kernel spec metadata['debugger'] to check if a kernel supports debugging #457

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Jun 3, 2020

Fixes #220.
Alternative to #456 (see this comment: #456 (comment))

The issue is that at the moment the kernel_info_request message is sent on the shell channel. If the execution has stopped (breakpoint hit), the debugger frontend forever waits for the kernel_info_reply to decide whether the kernel supports debugging or not, because the debugger key is in the kernel info reply.

Instead of waiting for the kernel_info_reply, the debugger frontend now checks the kernel spec to know if a kernel supports debugging or not.

This change requires the following change to the kernel spec, and the kernels (for now only xeus-python) would have to implement that change too:

EDIT: this has been added to xeus-python=0.8.0 (jupyter-xeus/xeus-python#282):

cat ${CONDA_PREFIX}/share/jupyter/kernels/xpython/kernel.json
{
  "display_name": "xpython",
  "argv": [
      "/home/jtp/miniconda/envs/debugger/bin/xpython",
      "-f",
      "{connection_file}"
  ],
  "language": "python",
  "metadata": { "debugger": true }
}

Visually the result looks similar as in #456, except that there is no need for a timeout anymore:

refresh-on-stopped-kernel-spec

refresh-on-stopped-5

This would also require an update to the sequence diagram in #64.

/**
* The kernel specs manager.
*/
specsManager: KernelSpec.IManager;
Copy link
Member Author

@jtpio jtpio Jun 3, 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 make this optional, so instantiating a DebuggerService without a kernel specs manager would always assume debugging is enabled (for example in an custom JupyterFrontEnd where only one kernel is available).

@jtpio
Copy link
Member Author

jtpio commented Jun 3, 2020

cc @JohanMabille @afshin

Leaving it as a draft for now as it would require changes to xeus-python.

This is something we already discussed before. It looks like having the debugger key in the kernel spec metadata is cleaner than what is implemented in #456.

Also if we go for this or #456, the debugger key wouldn't be strictly needed in the kernel_info_reply anymore (jupyter/jupyter_client#486).

@afshin
Copy link
Member

afshin commented Jun 5, 2020

I think it's reasonable for a kernel that supports DAP to have metadata that says it supports debugging. This seems like the fastest route to get that information and it also seems the least kernel-traffic-dependent way of doing it. I support this approach over the other PR.

@JohanMabille
Copy link
Member

JohanMabille commented Jun 5, 2020

I totally agree with @afshin and @jtpio, this is cleaner than sending a request to the kernel. I will implement the change in xeus-python quickly.

@jtpio jtpio force-pushed the kernelspec-debugger branch from b6c1004 to 7465dc9 Compare June 8, 2020 09:15
@jtpio
Copy link
Member Author

jtpio commented Jun 8, 2020

It looks like we need to mock the KernelSpecManager instance used in the tests, similar to what is done in core lab: https://github.com/jupyterlab/jupyterlab/blob/b1e2b83047421bf7196bec5f2a94d0616dcb2329/packages/services/test/kernelspec/manager.spec.ts#L17-L25

Retrieving the specs from the local jupyter server (child process) would otherwise return a 403, as the token is generated when the JupyterServer is started here.

@jtpio jtpio marked this pull request as ready for review June 8, 2020 11:12
@jtpio jtpio added this to the 0.3.0 milestone Jun 9, 2020
Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

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

Thanks! This is a much nicer way to detect debugger functionality.

👍

@kycutler
Copy link

Is this documented anywhere? I see the debugger flag documented in kernel info here, but nothing about the flag in kernelspecs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refreshing when stopped on a breakpoint
4 participants