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

Investigate an extension contribution point for a notebook type debugger #132014

Closed
roblourens opened this issue Aug 31, 2021 · 7 comments
Closed
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality notebook under-discussion Issue is under discussion for relevance, priority, approach

Comments

@roblourens
Copy link
Member

roblourens commented Aug 31, 2021

Breakpoints in notebook cells show up when there is a debugger registered for that language, same as editors. This isn't really right for notebooks where the Python DA doesn't work for Python cells, the Jupyter DA is separate. So we should consider a change to the package.json contribution. Have discussed this a little the past couple months but haven't done any work for it.

  • There is a breakpoints contribution point which is a list of { language: string }. That object could also include notebookType
  • When deciding to show the breakpoint margin in a notebook, we would check this property and not check the language property

cc @weinand @isidorn does this sound ok?

Something that I don't remember is why the debug adapter has a "languages" property which isn't used for this, and the breakpoints config is separate from the debug adapter. Would there be debug adapters that don't support breakpoints?

@roblourens roblourens added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues notebook labels Aug 31, 2021
@roblourens roblourens added this to the Backlog milestone Aug 31, 2021
@roblourens roblourens self-assigned this Aug 31, 2021
@roblourens roblourens added the under-discussion Issue is under discussion for relevance, priority, approach label Aug 31, 2021
@weinand
Copy link
Contributor

weinand commented Sep 1, 2021

@roblourens you said

Breakpoints in notebook cells show up when there is a debugger registered for that language, same as editors

The "same as editors" part is not true. Here is why:

Breakpoints are independent from debuggers and they are not "owned" by a specific debugger; breakpoints don't have a "type" that ties them to a debugger.

Editors will have the breakpoint gutter enabled and allow adding breakpoints if there is a breakpoints contribution for the language shown in the editor (or if the debug.allowBreakpointsEverywhere setting is true). So users will be able to set breakpoints even if they don't have a debugger for them (Sounds strange but some users are using breakpoints as bookmarks).

Something that I don't remember is why the debug adapter has a "languages" property which isn't used for this, and the breakpoints config is separate from the debug adapter. Would there be debug adapters that don't support breakpoints?

The breakpoints contribution is independent from debuggers for two reasons:

  • breakpoints exist independent from debuggers,
  • debuggers don't know how languages are transpiled into other languages. So the breakpoints contribution for TS comes from the typescript language extension (same for coffeescript), and not from js-debug.

The debuggers.languages contribution point lists languages for which the debugger is the "default" debugger.
This property is used by VS Code if no launch.json exists and a users presses "F5" in an open editor. It is not used to enable breakpoints.

@isidorn is my explanation of the current implementation correct?


Introducing a breakpoint.notebookType is a bit against the model explained above.
Do we really need this? What is the problem if we don't have the notebookType property?

Another problem is the semantic of the extended breakpoint type (assuming that notebookType is a bool):

    "breakpoints": [
      {
        "language": "python",
        "notebookType": true
      }
    ],

What does this mean? "breakpoints are enabled in python notebook cells" and in "python editors"? or just "breakpoints are enabled in python notebook cells"?

This is complicated by the fact that "breakpoint" contributions for the same language can come from different extensions and they have to be merged with predictable semantics.

A better approach would be something like this:

    "breakpoints": [
      {
        "language": "python",
        "target": [ "editor", "notebook" ]
      }
    ],

with the additional rule: if target is missing, "editor" is assumed.

@isidorn
Copy link
Contributor

isidorn commented Sep 1, 2021

@weinand your description of the implementation above is correct. We have a bit more use cases, best is to look for references of isDebuggerInterestedIn
@roblourens in short think of the language contribution as a hint from the debug extension in what languages it is interested in (independent of breakpoints).

I like @weinand proposal from above with the target.

@roblourens
Copy link
Member Author

Thanks @weinand for reminding me how this works. It turns out that for Jupyter at least, we require the Python extension to be installed to run cells, at least in some cases. It's not clear whether debugging will be supported without the Python extension installed. If it is required, then this isn't needed after all because the Python extension will provide the breakpoints registration. It might still be relevant for another notebook type but I wouldn't implement this without a consumer.

@weinand
Copy link
Contributor

weinand commented Sep 9, 2021

@roblourens does this mean that we do not have to introduce a "notebook debugger" contribution point (at least not in September)?
If this is the case, please close the issue and update the plan.

@roblourens
Copy link
Member Author

Possibly - will do, if that's where we land

@roblourens
Copy link
Member Author

For now we are assuming that the Python extension will be installed when debugging Python cells, so we don't need this for now.

@roblourens roblourens removed this from the September 2021 milestone Sep 27, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality notebook under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

5 participants
@roblourens @rebornix @weinand @isidorn and others