-
Notifications
You must be signed in to change notification settings - Fork 290
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
Fixes to IW debugging with breakpoints #10263
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,12 +10,20 @@ import { DebugProtocol } from 'vscode-debugprotocol'; | |
import { IJupyterSession, IKernel } from '../../../kernels/types'; | ||
import { IPlatformService } from '../../../platform/common/platform/types'; | ||
import { IDumpCellResponse, IDebugLocationTrackerFactory } from '../../../kernels/debugger/types'; | ||
import { traceError, traceInfoIfCI } from '../../../platform/logging'; | ||
import { traceError, traceInfo, traceInfoIfCI } from '../../../platform/logging'; | ||
import { getInteractiveCellMetadata } from '../../../interactive-window/helpers'; | ||
import { KernelDebugAdapterBase } from '../../../kernels/debugger/kernelDebugAdapterBase'; | ||
import { InteractiveCellMetadata } from '../../editor-integration/types'; | ||
|
||
export class KernelDebugAdapter extends KernelDebugAdapterBase { | ||
private readonly debugLocationTracker?: DebugAdapterTracker; | ||
private readonly cellToDebugFileSortedInReverseOrderByLineNumber: { | ||
debugFilePath: string; | ||
interactiveWindow: Uri; | ||
lineOffset: number; | ||
metadata: InteractiveCellMetadata; | ||
}[] = []; | ||
|
||
constructor( | ||
session: DebugSession, | ||
notebookDocument: NotebookDocument, | ||
|
@@ -72,24 +80,143 @@ export class KernelDebugAdapter extends KernelDebugAdapterBase { | |
throw new Error('Not an interactive window cell'); | ||
} | ||
try { | ||
const response = await this.session.customRequest('dumpCell', { | ||
code: (metadata.generatedCode?.code || cell.document.getText()).replace(/\r\n/g, '\n') | ||
}); | ||
const code = (metadata.generatedCode?.code || cell.document.getText()).replace(/\r\n/g, '\n'); | ||
const response = await this.session.customRequest('dumpCell', { code }); | ||
|
||
// We know jupyter will strip out leading white spaces, hence take that into account. | ||
const lines = metadata.generatedCode!.realCode.splitLines({ trim: false, removeEmptyEntries: false }); | ||
const indexOfFirstNoneEmptyLine = lines.findIndex((line) => line.trim().length); | ||
console.error(indexOfFirstNoneEmptyLine); | ||
const norm = path.normalize((response as IDumpCellResponse).sourcePath); | ||
this.fileToCell.set(norm, { | ||
uri: Uri.parse(metadata.interactive.uristring), | ||
lineOffset: | ||
metadata.interactive.lineIndex + | ||
(metadata.generatedCode?.lineOffsetRelativeToIndexOfFirstLineInCell || 0) | ||
}); | ||
this.cellToFile.set(Uri.parse(metadata.interactive.uristring), { | ||
path: norm, | ||
this.fileToCell.set(norm, Uri.parse(metadata.interactive.uristring)); | ||
|
||
// If this cell doesn't have a cell marker, then | ||
// Jupyter will strip out any leading whitespace. | ||
// Take that into account. | ||
let numberOfStrippedLines = 0; | ||
if (metadata.generatedCode && !metadata.generatedCode.hasCellMarker) { | ||
numberOfStrippedLines = metadata.generatedCode.firstNonBlankLineIndex; | ||
} | ||
this.cellToDebugFileSortedInReverseOrderByLineNumber.push({ | ||
debugFilePath: norm, | ||
interactiveWindow: Uri.parse(metadata.interactive.uristring), | ||
metadata, | ||
lineOffset: | ||
numberOfStrippedLines + | ||
metadata.interactive.lineIndex + | ||
(metadata.generatedCode?.lineOffsetRelativeToIndexOfFirstLineInCell || 0) | ||
}); | ||
// Order cells in reverse order. | ||
this.cellToDebugFileSortedInReverseOrderByLineNumber.sort( | ||
(a, b) => b.metadata.interactive.lineIndex - a.metadata.interactive.lineIndex | ||
); | ||
} catch (err) { | ||
traceError(`Failed to dump cell for ${cell.index} with code ${metadata.interactive.originalSource}`, err); | ||
} | ||
} | ||
protected override translateDebuggerFileToRealFile( | ||
source: DebugProtocol.Source | undefined, | ||
lines?: { line?: number; endLine?: number; lines?: number[] } | ||
) { | ||
if (!source || !source.path || !lines || (typeof lines.line !== 'number' && !Array.isArray(lines.lines))) { | ||
return; | ||
} | ||
// Find the cell that matches this line in the IW file by mapping the debugFilePath to the IW file. | ||
const cell = this.cellToDebugFileSortedInReverseOrderByLineNumber.find( | ||
(item) => item.debugFilePath === source.path | ||
); | ||
if (!cell) { | ||
return; | ||
} | ||
source.name = path.basename(cell.interactiveWindow.path); | ||
source.path = cell.interactiveWindow.toString(); | ||
if (typeof lines?.endLine === 'number') { | ||
lines.endLine = lines.endLine + (cell.lineOffset || 0); | ||
} | ||
if (typeof lines?.line === 'number') { | ||
lines.line = lines.line + (cell.lineOffset || 0); | ||
} | ||
if (lines?.lines && Array.isArray(lines?.lines)) { | ||
lines.lines = lines?.lines.map((line) => line + (cell.lineOffset || 0)); | ||
} | ||
} | ||
protected override translateRealFileToDebuggerFile( | ||
source: DebugProtocol.Source | undefined, | ||
lines?: { line?: number; endLine?: number; lines?: number[] } | ||
) { | ||
if (!source || !source.path || !lines || (typeof lines.line !== 'number' && !Array.isArray(lines.lines))) { | ||
return; | ||
} | ||
const startLine = lines.line || lines.lines![0]; | ||
// Find the cell that matches this line in the IW file by mapping the debugFilePath to the IW file. | ||
const cell = this.cellToDebugFileSortedInReverseOrderByLineNumber.find( | ||
(item) => startLine >= item.metadata.interactive.lineIndex + 1 | ||
); | ||
if (!cell) { | ||
return; | ||
} | ||
source.path = cell.debugFilePath; | ||
if (typeof lines?.endLine === 'number') { | ||
lines.endLine = lines.endLine - (cell.lineOffset || 0); | ||
} | ||
if (typeof lines?.line === 'number') { | ||
lines.line = lines.line - (cell.lineOffset || 0); | ||
} | ||
if (lines?.lines && Array.isArray(lines?.lines)) { | ||
lines.lines = lines?.lines.map((line) => line - (cell.lineOffset || 0)); | ||
} | ||
} | ||
|
||
protected override async sendRequestToJupyterSession(message: DebugProtocol.ProtocolMessage) { | ||
if (this.jupyterSession.disposed || this.jupyterSession.status === 'dead') { | ||
traceInfo(`Skipping sending message ${message.type} because session is disposed`); | ||
return; | ||
} | ||
|
||
const request = message as unknown as DebugProtocol.SetBreakpointsRequest; | ||
if (request.type === 'request' && request.command === 'setBreakpoints') { | ||
const sortedLines = (request.arguments.lines || []).concat( | ||
(request.arguments.breakpoints || []).map((bp) => bp.line) | ||
); | ||
const startLine = sortedLines.length ? sortedLines[0] : undefined; | ||
// Find the cell that matches this line in the IW file by mapping the debugFilePath to the IW file. | ||
const cell = startLine | ||
? this.cellToDebugFileSortedInReverseOrderByLineNumber.find( | ||
(item) => startLine >= item.metadata.interactive.lineIndex + 1 | ||
) | ||
: undefined; | ||
if (cell) { | ||
const clonedRequest: typeof request = JSON.parse(JSON.stringify(request)); | ||
if (request.arguments.lines) { | ||
request.arguments.lines = request.arguments.lines.filter( | ||
(line) => line <= cell.metadata.generatedCode!.endLine | ||
); | ||
} | ||
if (request.arguments.breakpoints) { | ||
request.arguments.breakpoints = request.arguments.breakpoints.filter( | ||
(bp) => bp.line <= cell.metadata.generatedCode!.endLine | ||
); | ||
} | ||
if (sortedLines.filter((line) => line > cell.metadata.generatedCode!.endLine).length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rchiodo we add breakpoints for each cell by splitting the breakpoints by cells. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is breakpoints the only thing that needs this special translation? What about stack frames? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's fine |
||
// Find all the lines that don't belong to this cell & add breakpoints for those as well | ||
// However do that separately as they belong to different files. | ||
await this.setBreakpoints({ | ||
source: clonedRequest.arguments.source, | ||
breakpoints: clonedRequest.arguments.breakpoints?.filter( | ||
(bp) => bp.line > cell.metadata.generatedCode!.endLine | ||
), | ||
lines: clonedRequest.arguments.lines?.filter( | ||
(line) => line > cell.metadata.generatedCode!.endLine | ||
) | ||
}); | ||
} | ||
} | ||
} | ||
|
||
return super.sendRequestToJupyterSession(message); | ||
} | ||
|
||
protected getDumpFilesForDeletion() { | ||
return this.cellToDebugFileSortedInReverseOrderByLineNumber.map((item) => item.debugFilePath); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment seems wrong here. It's not searching by line but rather by the translated debugFilePath. That does map to a specific cell entry.