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

Support debugging IW using Jupyter protocol #10105

Merged
merged 15 commits into from
May 24, 2022
Merged

Support debugging IW using Jupyter protocol #10105

merged 15 commits into from
May 24, 2022

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented May 24, 2022

Fixes #10037

Todo:
[ ] Add tests

const firstExecutableLineIndex = lineList.length
? metadata.interactive.line + lineList[0]
: metadata.interactive.line;
return { stripped, trueStartLine, firstExecutableLineIndex };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need the first executable line as we need to add a breakpoint to that line.

*/
@injectable()
export class InteractiveWindowDebuggingManager
extends DebuggingManagerBase
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved a lot of stuff into a base class

import { getInteractiveCellMetadata } from '../../../interactive-window/helpers';
import { KernelDebugAdapterBase } from '../../../kernels/debugger/kernelDebugAdapterBase';

export class KernelDebugAdapter extends KernelDebugAdapterBase {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved a lot of stuff into a base class

public generateCode(
metadata: Pick<InteractiveCellMetadata, 'interactive' | 'id'>,
debug: boolean,
usingJupyterDebugProtocol?: boolean
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added argument to decide what kind of debugging code is to be added.
Didn't want to pass in IKernel or other stuff.
the desire is that we'll switch over to the new Jupyter protocol debugger for everything and deprecate the old IW debugger code

@@ -519,7 +522,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
const promises = cells.map((c) => {
// Add the cell first. We don't need to wait for this part as we want to add them
// as quickly as possible
const notebookCellPromise = this.addNotebookCell(c, fileUri, line, isDebug);
const notebookCellPromise = this.addNotebookCell(c, fileUri, line);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot generate the code for IW cell at this stage, it can only be done once we have resolved the kernel.
E.g. if using remote kernel, then we don't inject debugger() statement.


const metadata: InteractiveCellMetadata = {
interactiveWindowCellMarker,
interactive,
generatedCode,
id: uuid()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found a bug, the id passed into the generated code was wrong, as that's not whats stored in the metadata.
I.e. line 754 and 763 use two different ids.

};

const edit = new WorkspaceEdit();
const cellEdit = NotebookEdit.updateCellMetadata(cell.index, newMetadata);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The IW code is generated only after we resolve the kernel.
Once we have a single debugger implementation we should be able to move this back into the place where the cell is created.

break;
case 'breakpoint':
sourceHook((event as DebugProtocol.BreakpointEvent).body.breakpoint.source);
sourceHook(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ability to translate the line numbers as well.

@DonJayamanne DonJayamanne marked this pull request as ready for review May 24, 2022 01:44
@DonJayamanne DonJayamanne requested a review from a team as a code owner May 24, 2022 01:44
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

Merging #10105 (bd7265d) into main (760d61e) will increase coverage by 0%.
The diff coverage is 100%.

❗ Current head bd7265d differs from pull request most recent head 6ccc99d. Consider uploading reports for the commit 6ccc99d to get more accurate results

@@          Coverage Diff          @@
##           main   #10105   +/-   ##
=====================================
  Coverage    64%      64%           
=====================================
  Files       203      203           
  Lines      9239     9241    +2     
  Branches   1488     1488           
=====================================
+ Hits       5927     5929    +2     
  Misses     2852     2852           
  Partials    460      460           
Impacted Files Coverage Δ
src/platform/serviceRegistry.node.ts 98% <100%> (+<1%) ⬆️

@DonJayamanne DonJayamanne merged commit 191122c into main May 24, 2022
@DonJayamanne DonJayamanne deleted the issue10037Part2 branch May 24, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support debugging of Interactive Window using the Jupyter debugger protocol
4 participants