Run by line partial implementation #11608
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| sonar.sources=src/client,src/datascience-ui | ||
| sonar.exclusions=src/datascience-ui/**/codicon*.* | ||
| sonar.tests=src/test | ||
| sonar.cfamily.build-wrapper-output.bypass=true | ||
| sonar.cpd.exclusions=src/datascience-ui/**/redux/actions.ts,src/client/**/raw-kernel/rawKernel.ts,src/client/datascience/jupyter/*ariable*.ts |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Run by line for notebook cells minimal implementation. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { | |
| ICellHashListener, | ||
| ICellHashProvider, | ||
| IFileHashes, | ||
| INotebook, | ||
| INotebookExecutionLogger | ||
| } from '../types'; | ||
|
|
||
|
|
@@ -148,25 +149,25 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo | |
| return lines; | ||
| } | ||
|
|
||
| public generateHashFileName(cell: ICell, expectedCount: number): string { | ||
|
There was a problem hiding this comment.
Not really using this now, but theoretically once the debugpy problems are fixed, this file name will be used to specify what code is 'just my code' |
||
| // First get the true lines from the cell | ||
| const { stripped } = this.extractStrippedLines(cell); | ||
|
|
||
| // Then use that to make a hash value | ||
| const hashedCode = stripped.join(''); | ||
| const hash = hashjs.sha1().update(hashedCode).digest('hex').substr(0, 12); | ||
|
There was a problem hiding this comment. That's what jupyter uses to generate the file name internally. |
||
| return `<ipython-input-${expectedCount}-${hash}>`; | ||
| } | ||
|
|
||
| // tslint:disable-next-line: cyclomatic-complexity | ||
| public async addCellHash(cell: ICell, expectedCount: number): Promise<void> { | ||
| // Find the text document that matches. We need more information than | ||
| // the add code gives us | ||
| const doc = this.documentManager.textDocuments.find((d) => this.fileSystem.arePathsSame(d.fileName, cell.file)); | ||
| if (doc) { | ||
| // Compute the code that will really be sent to jupyter | ||
| const { stripped, trueStartLine } = this.extractStrippedLines(cell); | ||
|
|
||
| const line = doc.lineAt(trueStartLine); | ||
| const endLine = doc.lineAt(Math.min(trueStartLine + stripped.length - 1, doc.lineCount - 1)); | ||
|
|
||
|
|
@@ -181,28 +182,6 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo | |
| const startOffset = doc.offsetAt(new Position(cell.line, 0)); | ||
| const endOffset = doc.offsetAt(endLine.rangeIncludingLineBreak.end); | ||
|
|
||
| // Compute the runtime line and adjust our cell/stripped source for debugging | ||
| const runtimeLine = this.adjustRuntimeForDebugging(cell, stripped, startOffset, endOffset); | ||
| const hashedCode = stripped.join(''); | ||
|
|
@@ -291,6 +270,52 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo | |
| } | ||
| } | ||
|
|
||
| private extractStrippedLines(cell: ICell): { stripped: string[]; trueStartLine: number } { | ||
| // Compute the code that will really be sent to jupyter | ||
| const lines = splitMultilineString(cell.data.source); | ||
| const stripped = this.extractExecutableLines(cell); | ||
|
|
||
| // Figure out our true 'start' line. This is what we need to tell the debugger is | ||
| // actually the start of the code as that's what Jupyter will be getting. | ||
| let trueStartLine = cell.line; | ||
| for (let i = 0; i < stripped.length; i += 1) { | ||
| if (stripped[i] !== lines[i]) { | ||
| trueStartLine += i + 1; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Find the first non blank line | ||
| let firstNonBlankIndex = 0; | ||
| while (firstNonBlankIndex < stripped.length && stripped[firstNonBlankIndex].trim().length === 0) { | ||
| firstNonBlankIndex += 1; | ||
| } | ||
|
|
||
| // Jupyter also removes blank lines at the end. Make sure only one | ||
| let lastLinePos = stripped.length - 1; | ||
| let nextToLastLinePos = stripped.length - 2; | ||
| while (nextToLastLinePos > 0) { | ||
| const lastLine = stripped[lastLinePos]; | ||
| const nextToLastLine = stripped[nextToLastLinePos]; | ||
| if ( | ||
| (lastLine.length === 0 || lastLine === '\n') && | ||
| (nextToLastLine.length === 0 || nextToLastLine === '\n') | ||
| ) { | ||
| stripped.splice(lastLinePos, 1); | ||
| lastLinePos -= 1; | ||
| nextToLastLinePos -= 1; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| // Make sure the last line with actual content ends with a linefeed | ||
| if (!stripped[lastLinePos].endsWith('\n') && stripped[lastLinePos].length > 0) { | ||
| stripped[lastLinePos] = `${stripped[lastLinePos]}\n`; | ||
| } | ||
|
|
||
| return { stripped, trueStartLine }; | ||
| } | ||
|
|
||
| private handleContentChange(docText: string, c: TextDocumentContentChangeEvent, hashes: IRangedCellHash[]) { | ||
| // First compute the number of lines that changed | ||
| const lineDiff = c.range.start.line - c.range.end.line + c.text.split('\n').length - 1; | ||
|
|
@@ -406,3 +431,11 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo | |
| return traceFrame; | ||
| } | ||
| } | ||
|
|
||
| export function getCellHashProvider(notebook: INotebook): ICellHashProvider | undefined { | ||
|
There was a problem hiding this comment.
Figured it made sense to centralize this here. There was a problem hiding this comment. If you just need the loggers looks like this should just take INotebookExecutionLogger[] instead of the entire notebook. So it doesn't have to expose the whole interface. There was a problem hiding this comment. Then the caller has to know that it comes from an array of loggers. Two pieces of information isntead of one. They have to have a notebook, why tell them the cell hash provider is on the loggers? In reply to: 420474295 [](ancestors = 420474295) There was a problem hiding this comment. As in the CellHashProvider knows it's in the logger list for a notebook, nobody else needs to know this. They just know it's associated with a notebook. We might change how the cell hash provider is associated with a notebook in the future, but we'd likely still have it associated with a notebook itself. In reply to: 420482187 [](ancestors = 420482187,420474295) There was a problem hiding this comment. My objection here is more how much is on the INotebook interface. The CellHashProvider could just call notebook.Dispose() here. Or notebook.restartKernel(). I feel like that's something we do a lot, especially with INotebook and IJupyterServer. There was a problem hiding this comment. But then the caller would have to understand about loggers. I don't think that scopes it down at all. I think it does the opposite. CellHashProvider knows it's a logger on a notebook. Nothing else needs to know that. However other things need some way to get the provider. I suppose other ways we could have done this would be to add a function to the notebook to retrieve it (that seems worse), add some sort of helper class that all you pass in is the notebook identity (that doesn't sound too bad), or this way. In reply to: 420884909 [](ancestors = 420884909) There was a problem hiding this comment. Maybe we should split the notebook interface then. Something that's smaller (or readonly). In reply to: 420881550 [](ancestors = 420881550) There was a problem hiding this comment. LIke IExecutableNotebook and INotebook where all of the state modifying items are on IExecutableNotebook. In reply to: 420915749 [](ancestors = 420915749,420881550) There was a problem hiding this comment. I don't know if we need to do it now, but if you remember the question that I was asking at the end of the SOLID talk about oversized interfaces, INotebook was totally one of the interfaces that I was thinking about. I think part of the advice there was that breaking up the interface might be a good first step to at least make it hard in code to mess with something that should not be messed with. Splitting that might be part of a bigger overall code-health fix, so that might be better to not mess with in this PR. There was a problem hiding this comment. With Ian on this, we're passing something that's too broad. |
||
| const logger = notebook.getLoggers().find((f) => f instanceof CellHashProvider); | ||
| if (logger) { | ||
| // tslint:disable-next-line: no-any | ||
| return (logger as any) as ICellHashProvider; | ||
| } | ||
| } | ||
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.
Continue will say stop.
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.
Yep that's on purpose.
In reply to: 420490258 [](ancestors = 420490258)