[DebuggerV2] Add components to show source code #3432

caisq · 2020-03-25T20:15:41Z

Motivation for features / changes
- Continue developing DebuggerV2 plugin: source code rendering part.
Technical description of changes
- In the existing StackTraceComponent, add a click callback for line numbers (e.g., "Line 15" as in the screenshot below). The click callback dispatches a sourceLineFocused action, which triggers downloading of file content via existing effects. (But see [DebuggerV2] Avoid loading source files that are loaded or loading #3430 for a fix in logic)
- Create SourceCodeComponent, which is a wrapper around monaco.
  - Uses ngOnChange() to detect code content change. Renders code in monaco editor whenever the change happens
  - Also uses ngOnChange() to detect focus line number change. Uses Editor.revealLineInCenter() to scroll the code to the line of interest
  - Uses Editor.deltaDecorations() to apply highlighting of the focused line, in both the code itself and in the gutter (see screenshot below).
- Create SourceFilesComponent and SourceFilesContainer, which is composed of a SourceCodeComponent. Additionally, it shows the currently selected file name in a header section. The Container is what communicates with the store.
Screenshots of UI changes
Detailed steps to verify changes work correctly (as executed by you)
- Unit tests added for newly added event emitter, components, and containers.
- Ran the code against a real logdir with tfdbg2 data.

monaco editor appears, but needs more styling

caisq · 2020-03-25T21:00:37Z

stephanwlee · 2020-03-26T22:20:46Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_component.css


 .bottom-section {
+  border-top: 1px solid rgba(0, 0, 0, 0.12);
+  height: 360px;


Note that this has effectively 367px height :)

Acknowledged. I prefer this to be exactly 360px in height. So I changed height to 353px to componensate.

stephanwlee · 2020-03-26T22:21:18Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_component.ts

 ==============================================================================*/
 import {ChangeDetectionStrategy, Component, Input} from '@angular/core';
-import {DebuggerRunListing} from './store/debugger_types';
+import {DebuggerRunListing, SourceFileContent} from './store/debugger_types';


Unused symbol.

Removed it.

stephanwlee · 2020-03-26T22:24:32Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/effects/debugger_effects.ts

          runId !== null &&
          fileContent !== null &&
-          fileContent.loadState !== DataLoadState.LOADING
+          fileContent.loadState === DataLoadState.NOT_LOADED


How about failed? Does network fluke make debugger unusable?

Surprised that there is no test change with this. 🤔

Files for which previous loading efforts have failed will have the state NOT_LOADED. So the code here covers it.

The test change happened in another (merged) PR: #3430. This diff should go away after I merge master into this branch.

stephanwlee · 2020-03-26T22:27:22Z

...rboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.css

+==============================================================================*/
+
+.code-viewer-container {
+  height: 336px;


weird to see height hardcoded like this. where is this magical number from?

Yeah. Good point. I've removed this magic number by making its parent a flex container.

On a related note: I've added a HostListener for window resizing in this commit, so that the monaco editor can re-render properly when the window is resized.

stephanwlee · 2020-03-26T22:31:17Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+  private decorations: string[] = [];
+
+  async ngOnChanges(changes: SimpleChanges): Promise<void> {
+    await loadMonaco();


do this on ngOnInit? Not sure how rendering cycle of Angular's is impacted by this. You may want to use some kind of observable to do this instead.

For example:

container, onInit, sets this.monaco$ = rxjs.from(loadMonaco()).pipe(map(() => windowWithRequireAndMonaco.monaco);.

component takes monaco as input to this component

In my observation, ngOnChange() can be before after ngOnInit(). This makes it hard to call the async loadMonaco() function from ngOnInit(). So I'll keep the code as is.

I think my suggestion still stands: you can pass a reference to monaco from the parent after using async pipe.

Done. Created SourceCodeContainer to wrap around SourceCodeComponent and to provide the monaco input. Unit tests are adjusted accordingly.

stephanwlee · 2020-03-26T23:16:26Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+  let editorSpy: jasmine.SpyObj<any>;
+  let monaco: any;
+  function setUpMonacoFakes() {
+    async function fakeLoadMonaco() {


Hmm, another way to have sane test and what not would be to inject the monaco editor and provide a shim implementation.

I have refactored the fake/spy code into its own module: views/source_code/testing.ts. It is shared between the source_code_component_test.ts and source_files_container_test.ts (see my other reply).

The rationale for using fake/spies like this are:

createSpyObj() allows us to succinctly write a fake implementation.

This approach is hermetic. I.e., it doesn't involve downloading monaco js/cs/ttf files during unit tests, in either the internal or external build/test environments.

stephanwlee · 2020-03-26T23:18:37Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+    store.overrideSelector(getFocusedSourceFileContent, {
+      loadState: DataLoadState.LOADED,
+      lines: ['import tensorflow as tf', '', 'print("hello, world")'],
+    });
+    store.overrideSelector(getFocusedSourceLineSpec, {
+      host_name: 'localhost',
+      file_path: '/home/user/main.py',
+      lineno: 3,
+    });


If you override the selectors before you create the fixture, you don't have to refreshState

I removed the first refreshState() calls in each it. My observation is that the first is unnecessary, but the second (i.e., after overrideSelector() is called again) is necessary. Without it, the test fails as if the selector return value hasn't changed.

I think it is more like, overrideSelector before creating component does not require refreshState but all the subsequent overrides require one.

That doesn't match what's happening herein this test though. In this test, the component is created before overrideSelector() is called, but no refreshState() is necessary for the first set of assertions. It's only after the second call to overrideSelectors() that refreshState() becomes necessary.

stephanwlee · 2020-03-26T23:21:48Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+        provideMockStore({
+          initialState: createState(createDebuggerState()),
+        }),


FYI, if you are going to use overrideSelector pattern, you don't need to pass the initialState (though I would make sure to set the selectors' values to something reasonable in beforeEach)

Good catch. Removed the initialState. The SourceFileComponent uses the store in a relatively simple way (only a few fields like focused source-file content and source line spec), so that it's not necessary to set up any initial selector return values here.

stephanwlee · 2020-03-26T23:28:50Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+    });
+    store.refreshState();
+    fixture.detectChanges();
+    await fixture.whenStable();


A bit surprised that you need to wait for stability all the time. ~~Is this absolutely needed~~? Ah, it must be the async/await in the ngOnChange.

You're right. It's the async ngOnChange().

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

caisq

Thanks for the review!

caisq · 2020-03-27T03:12:31Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/effects/debugger_effects.ts

          runId !== null &&
          fileContent !== null &&
-          fileContent.loadState !== DataLoadState.LOADING
+          fileContent.loadState === DataLoadState.NOT_LOADED


Files for which previous loading efforts have failed will have the state NOT_LOADED. So the code here covers it.

The test change happened in another (merged) PR: #3430. This diff should go away after I merge master into this branch.

caisq · 2020-03-27T13:09:14Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_component.css


 .bottom-section {
+  border-top: 1px solid rgba(0, 0, 0, 0.12);
+  height: 360px;


Acknowledged. I prefer this to be exactly 360px in height. So I changed height to 353px to componensate.

caisq · 2020-03-27T13:12:14Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_component.ts

 ==============================================================================*/
 import {ChangeDetectionStrategy, Component, Input} from '@angular/core';
-import {DebuggerRunListing} from './store/debugger_types';
+import {DebuggerRunListing, SourceFileContent} from './store/debugger_types';


Removed it.

caisq · 2020-03-27T14:04:31Z

...rboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.css

+==============================================================================*/
+
+.code-viewer-container {
+  height: 336px;


Yeah. Good point. I've removed this magic number by making its parent a flex container.

On a related note: I've added a HostListener for window resizing in this commit, so that the monaco editor can re-render properly when the window is resized.

caisq · 2020-03-27T14:40:32Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+  private decorations: string[] = [];
+
+  async ngOnChanges(changes: SimpleChanges): Promise<void> {
+    await loadMonaco();


In my observation, ngOnChange() can be before after ngOnInit(). This makes it hard to call the async loadMonaco() function from ngOnInit(). So I'll keep the code as is.

caisq · 2020-03-27T14:50:12Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+        provideMockStore({
+          initialState: createState(createDebuggerState()),
+        }),


Good catch. Removed the initialState. The SourceFileComponent uses the store in a relatively simple way (only a few fields like focused source-file content and source line spec), so that it's not necessary to set up any initial selector return values here.

caisq · 2020-03-27T16:53:29Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+    store.overrideSelector(getFocusedSourceFileContent, {
+      loadState: DataLoadState.LOADED,
+      lines: ['import tensorflow as tf', '', 'print("hello, world")'],
+    });
+    store.overrideSelector(getFocusedSourceLineSpec, {
+      host_name: 'localhost',
+      file_path: '/home/user/main.py',
+      lineno: 3,
+    });


I removed the first refreshState() calls in each it. My observation is that the first is unnecessary, but the second (i.e., after overrideSelector() is called again) is necessary. Without it, the test fails as if the selector return value hasn't changed.

caisq · 2020-03-27T16:53:44Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+    });
+    store.refreshState();
+    fixture.detectChanges();
+    await fixture.whenStable();


You're right. It's the async ngOnChange().

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

caisq · 2020-03-27T21:50:03Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+  let editorSpy: jasmine.SpyObj<any>;
+  let monaco: any;
+  function setUpMonacoFakes() {
+    async function fakeLoadMonaco() {


I have refactored the fake/spy code into its own module: views/source_code/testing.ts. It is shared between the source_code_component_test.ts and source_files_container_test.ts (see my other reply).

The rationale for using fake/spies like this are:

createSpyObj() allows us to succinctly write a fake implementation.

This approach is hermetic. I.e., it doesn't involve downloading monaco js/cs/ttf files during unit tests, in either the internal or external build/test environments.

stephanwlee · 2020-03-31T15:37:19Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+  private decorations: string[] = [];
+
+  async ngOnChanges(changes: SimpleChanges): Promise<void> {
+    await loadMonaco();


I think my suggestion still stands: you can pass a reference to monaco from the parent after using async pipe.

stephanwlee · 2020-03-31T15:38:18Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+  @HostListener('window:resize', ['$event'])
+  onResize(event: Event) {
+    if (this.editor !== null) {
+      this.editor.layout();


Debounce this if possible. Resize can happen 60hz and we don't need to layout on every frame.

Done. Using rxjs debounce() at a rate of 20 Hz (50-ms interval) now.

stephanwlee · 2020-03-31T15:46:39Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+      lineno: 3,
+    });
+    fixture.detectChanges();
+    await fixture.whenStable();


I believe you no longer need this since you aren't rendering monaco component? (source_code_component)

Feel free to use import {NO_ERRORS_SCHEMA} from '@angular/core';, too.

await fixture.whenStable() and async are still necessary here, because the fake loadMonaco() that serves this test is async, just like the reaal loadMonaco(). It gets called because SourceFileContainer is composed of a SourceCodeComponent. I prefer to let the state of the entire container stabilize before making assertions, even though the assertions made in this test file aren't concerned with monaco per se.

stephanwlee · 2020-03-31T15:48:39Z

.../plugins/debugger_v2/tf_debugger_v2_plugin/views/source_files/source_files_container_test.ts

+    store.overrideSelector(getFocusedSourceFileContent, {
+      loadState: DataLoadState.LOADED,
+      lines: ['import tensorflow as tf', '', 'print("hello, world")'],
+    });
+    store.overrideSelector(getFocusedSourceLineSpec, {
+      host_name: 'localhost',
+      file_path: '/home/user/main.py',
+      lineno: 3,
+    });


I think it is more like, overrideSelector before creating component does not require refreshState but all the subsequent overrides require one.

Unit tests are revised accordingly.

- Use string indexing in `ngOnChanges()`. - Add msissing dependency for views/stack_trace/BUILD - Remove unreachable typing - Use const export from source_code/testing.ts

stephanwlee · 2020-04-01T05:25:07Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+          }
+        })
+      )
+      .subscribe();


Since this component can be created and destroyed a lot, please use takeUntil() to remove the subscription.

Using takeUntil() in conjunction with a Subject that emits in ngOnDestroy() now.

stephanwlee · 2020-04-01T05:25:25Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+      .subscribe();
+  }
+
+  async ngOnChanges(changes: SimpleChanges): Promise<void> {


No need for async anymore! :D

caisq

Thanks for taking another look.

caisq · 2020-04-01T12:55:08Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+      .subscribe();
+  }
+
+  async ngOnChanges(changes: SimpleChanges): Promise<void> {


caisq · 2020-04-01T13:05:32Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/source_code_component.ts

+          }
+        })
+      )
+      .subscribe();


Using takeUntil() in conjunction with a Subject that emits in ngOnDestroy() now.

* Motivation for features / changes * Continue developing DebuggerV2 plugin: source code rendering part. * Technical description of changes * In the existing StackTraceComponent, add a click callback for line numbers (e.g., "Line 15" as in the screenshot below). The click callback dispatches a `sourceLineFocused` action, which triggers downloading of file content via existing effects. (But see tensorflow#3430 for a fix in logic) * Create `SourceCodeComponent`, which is a wrapper around monaco. * Uses `ngOnChange()` to detect code content change. Renders code in monaco editor whenever the change happens * Also uses `ngOnChange()` to detect focus line number change. Uses `Editor.revealLineInCenter()` to scroll the code to the line of interest * Uses `Editor.deltaDecorations()` to apply highlighting of the focused line, in both the code itself and in the gutter (see screenshot below). * Create SourceFilesComponent and SourceFilesContainer, which is composed of a SourceCodeComponent. Additionally, it shows the currently selected file name in a header section. The Container is what communicates with the store. * Screenshots of UI changes * ![image](https://user-images.githubusercontent.com/16824702/77582498-de8add80-6eb5-11ea-89f2-073030e80537.png) * Detailed steps to verify changes work correctly (as executed by you) * Unit tests added for newly added event emitter, components, and containers. * Ran the code against a real logdir with tfdbg2 data.

* Motivation for features / changes * Continue developing DebuggerV2 plugin: source code rendering part. * Technical description of changes * In the existing StackTraceComponent, add a click callback for line numbers (e.g., "Line 15" as in the screenshot below). The click callback dispatches a `sourceLineFocused` action, which triggers downloading of file content via existing effects. (But see #3430 for a fix in logic) * Create `SourceCodeComponent`, which is a wrapper around monaco. * Uses `ngOnChange()` to detect code content change. Renders code in monaco editor whenever the change happens * Also uses `ngOnChange()` to detect focus line number change. Uses `Editor.revealLineInCenter()` to scroll the code to the line of interest * Uses `Editor.deltaDecorations()` to apply highlighting of the focused line, in both the code itself and in the gutter (see screenshot below). * Create SourceFilesComponent and SourceFilesContainer, which is composed of a SourceCodeComponent. Additionally, it shows the currently selected file name in a header section. The Container is what communicates with the store. * Screenshots of UI changes * ![image](https://user-images.githubusercontent.com/16824702/77582498-de8add80-6eb5-11ea-89f2-073030e80537.png) * Detailed steps to verify changes work correctly (as executed by you) * Unit tests added for newly added event emitter, components, and containers. * Ran the code against a real logdir with tfdbg2 data.

caisq added 11 commits March 23, 2020 23:33

[DebuggerV2] Display source code

1a2f7fa

Add SourceCodeComponent and SourceFileComponent

f514b42

monaco editor appears, but needs more styling

Improve styling; add file_path display

77500cc

Line scrolling-to and highlighting is basically working

e13ff39

Working on stying the source files header

6bcb5a6

Fix styling

bfbf80e

Add "no-file-selected" display

c3ffc43

Clean up some ng module dependencies

a41da9c

Add unit test for dispatching sourceLineFocused action

33b07f8

Back out some unwanted changes

d7ffa33

Add source_files_container_test.ts: Unit tests for source_file_container

7d3fcf6

googlebot added the cla: yes label Mar 25, 2020

caisq changed the title ~~Dbg2 source code ng~~ [DebuggerV2] Use monaco-editor to show source code Mar 25, 2020

caisq changed the title ~~[DebuggerV2] Use monaco-editor to show source code~~ [DebuggerV2] Add components to show source code Mar 25, 2020

caisq added 4 commits March 25, 2020 16:26

Fix test; Add missing license headers to .css files

bbf5927

Add missing license header to .css file; Remove obsolete TODO item

0e8421f

Remove cruft; sort imports

1f03da0

Properly tear down test fakes in source_files_container_test.ts

b978312

caisq marked this pull request as ready for review March 25, 2020 20:46

caisq requested a review from stephanwlee March 25, 2020 21:00

stephanwlee reviewed Mar 26, 2020

View reviewed changes

caisq added 2 commits March 27, 2020 13:01

Address Stephan's comments

a36a31f

Refactor-split tests for SourceCodeComponent and SourceFilesContainer

de7fc3b

caisq commented Mar 27, 2020

View reviewed changes

caisq requested a review from stephanwlee March 27, 2020 22:07

stephanwlee approved these changes Mar 31, 2020

View reviewed changes

caisq added 3 commits March 31, 2020 22:31

Add debounce to resize-triggered re-layout; Add SourceCodeContainer

18df880

Unit tests are revised accordingly.

Remove cruft comment

e9d48a1

Changes needed for fixing sync

88eacfd

- Use string indexing in `ngOnChanges()`. - Add msissing dependency for views/stack_trace/BUILD - Remove unreachable typing - Use const export from source_code/testing.ts

Merge branch 'master' into dbg2-source-code-ng

7a0c8f2

stephanwlee reviewed Apr 1, 2020

View reviewed changes

caisq commented Apr 1, 2020

View reviewed changes

Address comments: remove async; use takeUntil() for resize

e70a18a

caisq merged commit 8eedeed into tensorflow:master Apr 1, 2020

[DebuggerV2] Add components to show source code #3432

[DebuggerV2] Add components to show source code #3432

Uh oh!

Conversation

caisq commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caisq commented Mar 25, 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!

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!

Uh oh!

caisq left a comment

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!

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!

caisq commented Mar 25, 2020 •

edited

Loading