Use breakpiont() in the new IW Jupyter Debugger #10114

DonJayamanne · 2022-05-24T17:41:10Z

Fixes #10037
Fixes issues identified in #10106 (just for IW)
Adds tests for the new Debugger (sharing the same tests as the existing IW debugger)

DonJayamanne · 2022-05-24T17:41:59Z

src/interactive-window/debugger/jupyter/debugCellControllers.ts

 await cellDebugSetup(this.kernel, this.debugAdapter);
-
- const realPath = this.debugAdapter.getSourcePath(metadata.interactive.uristring);


No more adding of breakpoints, we use the technique of injecting breakpoint(), that works much better.

DonJayamanne · 2022-05-24T17:42:15Z

src/interactive-window/debugger/jupyter/kernelDebugAdapter.ts

@@ -64,15 +64,6 @@ export class KernelDebugAdapter extends KernelDebugAdapterBase {
 return super.handleMessage(message);
 }

- public override async dumpAllCells() {


Addressing previous PR comments

DonJayamanne · 2022-05-24T17:43:10Z

src/interactive-window/debugger/jupyter/kernelDebugAdapter.ts

- lineOffset: metadata.interactive.line + 1 // Add an extra 1 for the cell marker.
+ lineOffset:
+ metadata.interactive.lineIndex +
+ (metadata.generatedCode?.lineOffsetRelativeToIndexOfFirstLineInCell || 0)


I think this is more desecriptive way.
Also I changed the line property to be lineIndex as its the index, where as the line property in the generated code is actually the line number, was very confusing and found this was because i assumed both were line numbers.

DonJayamanne · 2022-05-24T17:43:49Z

src/interactive-window/editor-integration/codeGenerator.ts

- ? metadata.interactive.line + lineList[0]
- : metadata.interactive.line;
- return { stripped, trueStartLine, firstExecutableLineIndex };
+ return { stripped, trueStartLine };


Simpler now

DonJayamanne · 2022-05-24T17:45:02Z

src/interactive-window/editor-integration/codeGenerator.ts

- // Compute the code that will really be sent to jupyter
- const stripped = this.extractExecutableLines(metadata.interactive.originalSource);
+ // Compute the code that will really be sent to jupyter (including the cell marker)
+ const { lines: stripped } = this.extractExecutableLines(metadata.interactive.originalSource);


Note: I've simplified the code by sending the cell marker as well to the Jupyter kernel.
This way we don't have to strip white space and don't have to take into account any leading white space having been stripped off by Jupyter (as we'll have a cell marker - except in the case where user uses shift enter when there's no cell marker - and tha'ts handled as well and we have tests for that too)

Sounds good, I was about to ask about the shift-enter case. Sounds like it's covered.

DonJayamanne · 2022-05-24T17:46:11Z

src/interactive-window/editor-integration/types.ts

@@ -101,7 +102,7 @@ export type InteractiveCellMetadata = {
 interactiveWindowCellMarker?: string;
 interactive: {
 uristring: string;
- line: number;
+ lineIndex: number;


See above, in IGeneratedCode the line property is ` based, and this is 0 based, hence renamed to avoid confusions.

DonJayamanne · 2022-05-24T17:46:40Z

src/interactive-window/outputs/tracebackFormatter.ts

 return traceback.map((traceFrame) => {
 // Check IPython8. We handle that one special
- if (/^[Input|File].*?\n.*/.test(traceFrame)) {
+ if (useIPython8Format) {


This was incorrect, we need to test the regex on the entire output, not just one line.

DonJayamanne · 2022-05-24T17:47:06Z

src/interactive-window/outputs/tracebackFormatter.ts

- const newLine = match!.firstNonBlankLineIndex + n - 1;
- return `${prefix}<a href='${matchUri?.toString()}?line=${newLine}'>${newLine + 1}</a>${suffix}`;
+ const lineNumberOfFirstLineInCell = match!.hasCellMarker ? match!.line - 1 : match!.line;
+ const lineIndexOfFirstLineInCell = lineNumberOfFirstLineInCell - 1;


Split this up into few more lines to make it more obvious what we were doing.

DonJayamanne · 2022-05-24T17:47:59Z

src/kernels/debugger/kernelDebugAdapterBase.ts

@@ -222,9 +222,17 @@ export abstract class KernelDebugAdapterBase implements DebugAdapter, IKernelDeb
 return this.session.customRequest('setBreakpoints', args);
 }

- public abstract dumpAllCells(): Promise<void>;
+ public async dumpAllCells() {


Addressing previous code review comment

DonJayamanne · 2022-05-24T17:50:19Z

src/kernels/jupyter/launcher/notebookServerProvider.ts

@@ -140,7 +140,7 @@ export class NotebookServerProvider implements IJupyterServerProvider {
 } catch (e) {
 disposeAllDisposables(disposables);
 // If user cancelled, then do nothing.
- if (options.token?.isCancellationRequested && e instanceof CancellationError) {
+ if (options.token?.isCancellationRequested && isCancellationError(e)) {


Found in the PRs that when you run tests locally, we have a lot of unhandled exceptions, that's because some of the cancellation errors were not handled.
We have our own cancellation error as well, removed ours and used the VS Code cancellation error and added a utility function.

Now,

When we run tests locally we don't have the same number of unhandled errors

The output log is cleaner (previously we'd see 1000s of entries after the test output and its impossible to see the test results as the console output is too large, too large even to scroll - as we've exceeded vscodes buffer limit)

DonJayamanne · 2022-05-24T17:50:42Z

src/kernels/kernel.base.ts

+ info.push(`EnvType: ${this.kernelConnectionMetadata.interpreter.envType}`);
+ info.push(`EnvName: '${this.kernelConnectionMetadata.interpreter.envName}'`);
+ info.push(`Version: ${this.kernelConnectionMetadata.interpreter.version?.raw}`);
+ pythonInfo = ` (${info.join(', ')})`;


I've always wanted to see the version and the type of the python environment used, figured i'd add this now

DonJayamanne · 2022-05-24T17:50:52Z

src/kernels/kernel.base.ts

@@ -604,7 +612,7 @@ export abstract class BaseKernel implements IKernel {
 // Have our debug cell script run first for safety
 if (
 isLocalConnection(this.kernelConnectionMetadata) &&
- !this.configService.getSettings(undefined).useJupyterDebugger
+ !this.configService.getSettings(undefined).forceIPyKernelDebugger


Address comment from previous PR

DonJayamanne · 2022-05-24T17:51:58Z

src/kernels/kernelConnector.ts

- error
- );
+ if (!isCancellationError(error)) {
+ traceWarning(


No point adding logs when its a cancellation error, we just end up with lots of Errors logged in the output along with stack trace when these are not real errors (basically filling the error logs when running simple tests) - thus leading to false positives - i.e. it looks as though there are errors, when infact there aren't any.

DonJayamanne · 2022-05-24T17:52:24Z

src/kernels/raw/launcher/kernelLauncher.node.ts

 );
+ if (token.isCancellationRequested) {


Found that this code was running even after tests completed, adding cancellation to fix that.

DonJayamanne · 2022-05-24T17:54:06Z

src/platform/common/cancellation.ts

-/**
- * Error type thrown when canceling.
- */
-export class CancellationError extends BaseError {


Removed our CancellationError, we weren't testing for this everywhere, hence we had a few weird behaviours when it came to handling of cancellation errors (basically we weren't handling it correctly as we weren't account for VSC and our cancellation error)
Now there's only one.

DonJayamanne · 2022-05-24T17:54:51Z

src/test/datascience/interactiveWindow.vscode.test.ts

+
+type DebuggerType = 'VSCodePythonDebugger' | 'JupyterProtocolDebugger';
+const debuggerTypes: DebuggerType[] = ['JupyterProtocolDebugger', 'VSCodePythonDebugger'];
+debuggerTypes.forEach((debuggerType) => {


Running the IW debugger tests against both old and new jupyter protocol debugger.

codecov-commenter · 2022-05-24T18:02:22Z

Codecov Report

Merging #10114 (e3704c2) into main (191122c) will decrease coverage by 5%.
The diff coverage is 68%.

@@          Coverage Diff           @@
##           main   #10114    +/-   ##
======================================
- Coverage    64%      58%    -6%     
======================================
  Files       203      203            
  Lines      9241     9247     +6     
  Branches   1488     1493     +5     
======================================
- Hits       5927     5402   -525     
- Misses     2853     3422   +569     
+ Partials    461      423    -38

Impacted Files	Coverage Δ
src/platform/api/pythonApi.ts	`67% <ø> (-3%)`	⬇️
src/platform/common/types.ts	`100% <ø> (ø)`
src/platform/common/cancellation.ts	`62% <60%> (-6%)`	⬇️
src/platform/errors/errorHandler.ts	`52% <71%> (-12%)`	⬇️
src/platform/common/configSettings.ts	`80% <100%> (-4%)`	⬇️
src/platform/export/exportToPythonPlain.ts	`21% <0%> (-66%)`	⬇️
.../debugger/extension/helpers/protocolParser.node.ts	`15% <0%> (-64%)`	⬇️
src/platform/api/kernelApi.ts	`11% <0%> (-60%)`	⬇️
src/platform/export/exportUtil.node.ts	`26% <0%> (-47%)`	⬇️
src/platform/export/fileConverter.ts	`31% <0%> (-46%)`	⬇️
... and 51 more

src/interactive-window/editor-integration/codeGenerator.ts

src/test/datascience/.vscode/settings.json

Use breakpiont() in the new IW Jupyter Debugger

5722472

DonJayamanne requested a review from a team as a code owner May 24, 2022 17:41

DonJayamanne commented May 24, 2022

View reviewed changes

Misc

e3704c2

DonJayamanne commented May 24, 2022

View reviewed changes

IanMatthewHuff reviewed May 24, 2022

View reviewed changes

src/interactive-window/editor-integration/codeGenerator.ts Show resolved Hide resolved

IanMatthewHuff approved these changes May 24, 2022

View reviewed changes

rchiodo reviewed May 24, 2022

View reviewed changes

src/test/datascience/.vscode/settings.json Outdated Show resolved Hide resolved

rchiodo approved these changes May 24, 2022

View reviewed changes

Misc

5ca5e9c

DonJayamanne merged commit 58c47af into main May 24, 2022

DonJayamanne deleted the issue10037Part3 branch May 24, 2022 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use breakpiont() in the new IW Jupyter Debugger #10114

Use breakpiont() in the new IW Jupyter Debugger #10114

DonJayamanne commented May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

IanMatthewHuff May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

DonJayamanne May 24, 2022

codecov-commenter commented May 24, 2022 •

edited

Loading

		await cellDebugSetup(this.kernel, this.debugAdapter);

		const realPath = this.debugAdapter.getSourcePath(metadata.interactive.uristring);

Use breakpiont() in the new IW Jupyter Debugger #10114

Use breakpiont() in the new IW Jupyter Debugger #10114

Conversation

DonJayamanne commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 24, 2022 • edited Loading

Codecov Report

codecov-commenter commented May 24, 2022 •

edited

Loading