Run by line partial implementation #11608

rchiodo · 2020-05-05T23:30:18Z

For #11607

Partial implementation of run by line behind a flag. Should be enough for a demo.

List of stuff to be done after this:

Debug variable info not complete
DataFrame open during debugging if not same cell
Step into other cells doesn't work (waiting on debugpy bug)
Hover on python files?
Tests
Run by line on install debugpy
Focus/Scroll in cell after step

…riable

…ient

Remove exclusions as they mess up subsequent runs

rchiodo · 2020-05-05T23:34:40Z

src/client/datascience/editor-integration/cellhashprovider.ts

@@ -406,3 +430,11 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo
        return traceFrame;
    }
 }
+
+export function getCellHashProvider(notebook: INotebook): ICellHashProvider | undefined {


getCellHashProvider [](start = 16, length = 19)

Figured it made sense to centralize this here.

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.

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)

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)

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.

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)

Maybe we should split the notebook interface then. Something that's smaller (or readonly).

In reply to: 420881550 [](ancestors = 420881550)

LIke IExecutableNotebook and INotebook where all of the state modifying items are on IExecutableNotebook.

In reply to: 420915749 [](ancestors = 420915749,420881550)

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.

With Ian on this, we're passing something that's too broad.

rchiodo · 2020-05-05T23:35:15Z

src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts

+            this.getDocument(),
+            this.getVariableHover(wordAtPosition)
+        ]);
+        if (!variableHover && languageServer && document) {


If there's a word under the cursor and the debugger (or kernel) knows about it, it takes precedence over normal hover.

rchiodo · 2020-05-05T23:36:30Z

src/client/datascience/interactive-ipynb/nativeEditorDebuggerListener.ts

+
+// tslint:disable: no-any
+@injectable()
+export class NativeEditorDebuggerListener


I should put a comment on this saying what this class is for.

rchiodo · 2020-05-05T23:48:30Z

src/datascience-ui/interactive-common/editor.tsx

+    glyphMarginClassName: 'codicon codicon-debug-stackframe',
+    stickiness
+};
+const TOP_STACK_FRAME_DECORATION: monacoEditor.editor.IModelDecorationOptions = {


Copied these from the VS code decorators they setup when debugging.

codecov-io · 2020-05-05T23:49:54Z

Codecov Report

Merging #11608 into master will decrease coverage by 0.11%.
The diff coverage is 39.79%.

@@            Coverage Diff             @@
##           master   #11608      +/-   ##
==========================================
- Coverage   60.70%   60.58%   -0.12%     
==========================================
  Files         626      627       +1     
  Lines       33767    33865      +98     
  Branches     4748     4763      +15     
==========================================
+ Hits        20498    20517      +19     
- Misses      12289    12359      +70     
- Partials      980      989       +9

Impacted Files	Coverage Δ
.../datascience/interactive-common/interactiveBase.ts	`5.55% <0.00%> (-0.04%)`	⬇️
...atascience/interactive-window/interactiveWindow.ts	`15.62% <0.00%> (ø)`
src/client/datascience/jupyter/jupyterVariables.ts	`25.00% <0.00%> (ø)`
src/datascience-ui/interactive-common/mainState.ts	`54.76% <ø> (ø)`
...ient/datascience/interactive-ipynb/nativeEditor.ts	`11.03% <8.00%> (-0.58%)`	⬇️
...active-common/intellisense/intellisenseProvider.ts	`37.65% <26.66%> (-0.89%)`	⬇️
...datascience/editor-integration/cellhashprovider.ts	`75.36% <64.70%> (-2.30%)`	⬇️
src/client/common/utils/localize.ts	`95.53% <100.00%> (+0.02%)`	⬆️
src/client/datascience/constants.ts	`99.73% <100.00%> (+<0.01%)`	⬆️
...ience/interactive-common/interactiveWindowTypes.ts	`100.00% <100.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e93fbe4...e93fbe4. Read the comment docs.

rchiodo · 2020-05-05T23:49:54Z

src/datascience-ui/native-editor/nativeCell.tsx

+                            hidden={this.isMarkdownCell()}
+                            disabled={this.props.busy}
+                        >
+                            <div className="codicon codicon-button">{'\uead7'}</div>


I should probably make these unicode values constants.

IanMatthewHuff · 2020-05-06T00:03:47Z

src/client/datascience/interactive-common/interactiveBase.ts

@@ -567,7 +567,7 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
                line,
                id,
                originator: this.id,
-                debug: debug !== undefined ? debug : false
+                debug: debug !== undefined ? true : false


This almost looked like an error skimming down due the change from a bool to an object. This is just an up to you thing. But if the function took a debugInfo? it might make a bit more sense. Debug just sounds like a bool property to me and we use it as a bool property right here in a different call.

Yeah I'll rename it.

In reply to: 420477853 [](ancestors = 420477853)

IanMatthewHuff · 2020-05-06T00:05:22Z

src/client/datascience/interactive-common/interactiveBase.ts

+                    if (debug.runByLine && debug.hashFileName) {
+                        await this.jupyterDebugger.startRunByLine(this._notebook, debug.hashFileName);
+                    } else {
+                        await this.jupyterDebugger.startDebugging(this._notebook);


runByLine = true without a debug.hasFileName just goes down the normal debugging path, which seems a bit funny. Is that intended?

No it should probably throw an exception.

In reply to: 420478342 [](ancestors = 420478342)

IanMatthewHuff · 2020-05-06T00:07:25Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

@@ -352,12 +358,12 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
        line: number,
        id?: string,
        data?: nbformat.ICodeCell | nbformat.IRawCell | nbformat.IMarkdownCell,
-        debug?: boolean,
+        debugOptions?: { runByLine: boolean; hashFileName?: string },


Didn't even see this when I made my comment on the debug options above. I do like this name better than the debug name for the options object above.

rchiodo · 2020-05-06T00:12:12Z

src/datascience-ui/react-common/image.tsx

@@ -196,6 +199,18 @@ const images: { [key: string]: { light: string; dark: string } } = {
    JupyterServerDisconnected: {
        light: require('./images/JupyterServerDisconnected/disconnected-light.svg'),
        dark: require('./images/JupyterServerDisconnected/disconnected-dark.svg')
+    },
+    RunByLine: {


Actually I don't really need this. I'll delete them.

IanMatthewHuff · 2020-05-06T00:20:53Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

+                    }
+                }
+            }
+        } catch (exc) {


The try here is around a bunch of code. If hashjs in getCellHashProvider throws an error I'm not sure if we want it showing up as cell output.

Or maybe we do? Guess we have to see it somewhere.

Yeah we do this elsewhere when running cells in a notebook.

In reply to: 420482775 [](ancestors = 420482775)

DavidKutu · 2020-05-06T00:49:46Z

package.nls.json

-    "DataScience.libraryRequiredToLaunchJupyterKernelNotInstalledInterpreter": "Data Science library {1} is not installed in interpreter {0}. Install?"
+    "DataScience.libraryRequiredToLaunchJupyterKernelNotInstalledInterpreter": "Data Science library {1} is not installed in interpreter {0}. Install?",
+    "DataScience.runByLine": "Run by line",
+    "DataScience.continueRunByLine": "Stop"


Continue will say stop.

Yep that's on purpose.

In reply to: 420490258 [](ancestors = 420490258)

DonJayamanne · 2020-05-06T16:57:04Z

src/client/datascience/editor-integration/cellhashprovider.ts

+
+        // Then use that to make a hash value
+        const hashedCode = stripped.join('');
+        const hash = hashjs.sha1().update(hashedCode).digest('hex').substr(0, 12);


Why only first 12 characters?

That's what jupyter uses to generate the file name internally.

DonJayamanne · 2020-05-06T16:57:46Z

src/client/datascience/editor-integration/cellhashprovider.ts

@@ -406,3 +430,11 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo
        return traceFrame;
    }
 }
+
+export function getCellHashProvider(notebook: INotebook): ICellHashProvider | undefined {


With Ian on this, we're passing something that's too broad.

DonJayamanne · 2020-05-06T17:01:54Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

+    private async handleRunByLine(runByLine: IRunByLine) {
+        try {
+            // If there's any payload, it has the code and the id
+            if (runByLine.cell.id && runByLine.cell.data.cell_type !== 'messages') {


What if cell_type=markdown or raw.
Shouldn't we handle other cell types.

This should be impossible so it should just throw an exception unless it's a code cell

…ient

sonarqubecloud · 2020-05-06T20:52:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 1 Security Hotspot to review)
1 Code Smell

No Coverage information
0.0% Duplication

rchiodo added 27 commits April 30, 2020 10:59

Preliminary getting variables from the debugger

750f977

Implement updating as stepping

49c411d

Get debugger and regular kernel to use same scripts

94f9ac9

Put back old code and create experiment to enable/disable this

b205ea5

Implement experiment and begin on test for debugger

2737798

Merge remote-tracking branch 'origin/master' into rchiodo/debugger_va…

16d83ae

…riable

Merge remote-tracking branch 'origin/master' into rchiodo/debugger_va…

0e0dcbd

…riable

Finish fixing tests

0215046

Add news entry

3466c81

Create multiplexer

f584b10

Basic idea of glyph updating and control

f01dc46

Forget the svgs

7e357ea

Rework to go through interactive path

5d69a7a

Highlight line kinda showing up

2b80529

Fix variables to come out.

e8f1c3e

Fix ip line sticking around too long

f5e3864

Colors and icon working for ip

584b59b

Implement F10 and F5

9e1be55

Hover and step into icons

bde0c96

Remove unnecessary variables and support step into

2006f9b

Update frame id for variable

e51a870

Merge remote-tracking branch 'origin/master' into rchiodo/debugger_cl…

c184ef5

…ient

Fixup after merge

71dafc8

Exclude everything glob for just my code

f6bfb8f

Try fixing order for messages to debugger

d8283b6

Remove exclusions as they mess up subsequent runs

Fix request problem and dark theme

5403b83

Add news entry

b84b85f

rchiodo requested review from DavidKutu, DonJayamanne and greazer May 5, 2020 23:30

rchiodo commented May 5, 2020

View reviewed changes

Some extra comments and fix sonar problems

00a958f

rchiodo commented May 5, 2020

View reviewed changes

IanMatthewHuff reviewed May 6, 2020

View reviewed changes

Add loc strings and make constants for code icons

973bc15

rchiodo commented May 6, 2020

View reviewed changes

Sonar problems and removing unused code

94bdf9d

IanMatthewHuff reviewed May 6, 2020

View reviewed changes

rchiodo added 3 commits May 5, 2020 17:23

Change name to debugInfo

e0a3470

Try again to exclude the codicon files

be6722c

Wrong base directory

d0af006

DavidKutu reviewed May 6, 2020

View reviewed changes

DonJayamanne approved these changes May 6, 2020

View reviewed changes

IanMatthewHuff approved these changes May 6, 2020

View reviewed changes

rchiodo added 6 commits May 6, 2020 12:03

Fixes from code review and run by line new icon

d089d70

Merge remote-tracking branch 'origin/master' into rchiodo/debugger_cl…

c0c88db

…ient

Fix functional test failures

a6b5591

Fix unit test failure (yay, caught a bug)

7c4084f

Fix stupid sonar warnings

ac3c342

Fix some more failing tests

e93fbe4

rchiodo merged commit c1d8619 into master May 6, 2020

rchiodo deleted the rchiodo/debugger_client branch May 6, 2020 21:22

lock bot locked as resolved and limited conversation to collaborators May 20, 2020

Run by line partial implementation #11608

Run by line partial implementation #11608

Uh oh!

Conversation

rchiodo commented May 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 6, 2020

Uh oh!

Uh oh!

codecov-io commented May 5, 2020 •

edited

Loading