[DebuggerV2] Add basic ng impl of actions, store, data_source & new Alerts view #3075

caisq · 2019-12-24T17:50:01Z

Technical description of changes
- Add HTTP data source implementation in data_soruce/tfdbg2_data_source.ts. Currently the data source doesn't exist in the backend, but will be added soon ([DebuggerV2] Implement /runs route #3051)
- Add actions in actions/. Currently there is only a small number of actions, related to the loading of the debugger v2 plugin, loading of the debugger runs from the data source.
- Add effects in effects/. Currently there is only one effect: dispatch actions on debuggerLoaded.
- Add types, store and redudcer in store/.
- Add a new view in view/alerts/. The new tf-debugger-v2-alerts element (AlertsContainer) is rendered conditionally in lieu of the existing InactiveContainer when there is at least 1 debugger run available via the data source.
- Add unit tests:
  - For reducer: debugger_reducer_test.ts
  - For the overall DebuggerContainer: debugger_container_test.ts
  - To make //tensorboard/webapp:karma_test_chromium pass, we need to decouple the states of the PluginModule and those of the DebuggerModule. This is achieve by creating TestingDebuggerModule in testing/ and using it in /tensorboard/webapp.

The code pattern is largely based on the work of @stephanwlee

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container.ts

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/BUILD

stephanwlee · 2019-12-25T01:30:41Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container.ts

  constructor(private readonly store: Store<State>) {}
+
+  ngOnInit(): void {
+    this.store.dispatch(debuggerLoaded());


Optional but consider this. There are rare cases when you want to use this. We should avoid that certain view is initialized if possible since that can lead to proliferation of bad patterns. Since view is a reflection of state in the store, effects and reducers should be able to update correctly without this.

For instance, whatever you do here can probably be replaced with PluginChanged action, right?

Whatever you do here can probably be replaced with PluginChanged action, right?

I think that's correct. But part of me worry about separation of responsibility as well. It feels a little strange to me that the PluginsChange action, which is at a higher level than the individual plugin (DebuggerV2 specifically), should know something about the internal workings of the plugin.

Also, why do you say these are rare cases? The DebuggerContainer being loaded (and hence its ngOnInit() method being called) when a user switches to the plugin is an essential step is a common workflow for the plugin.

Also, why do you say these are rare cases? The DebuggerContainer being loaded

It is rare because this information is easily attainable from store's perspective and view really should not fire an action with intention of telling the store.

which is at a higher level than the individual plugin (DebuggerV2 specifically), should know something about the internal workings of the plugin.

I am not sure if I agree with this. One of the point of building on the same framework and same instance of the store is better "integration". Being part of the greater system is not a wrong thing.

I added a TODO item here to explore consolidate this effect into the integrated webapp.

stephanwlee · 2019-12-25T01:32:34Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/data_source/tfdbg2_data_source.ts

+
+  fetchRuns() {
+    // TODO(cais): Once the backend uses an DataProvider that unifies tfdbg and
+    // non-tfdbg plugins, switch to using `tf_backend.runStore.refresh()`.


Is it referring to fact that the ngrx store does not contain the runs right now? Would it be possible to add it there if so? Maybe runs here is different than that in general sense?

Yeah, it is not entirely clear what "runs" will mean for DebuggerV2 yet. Currently each logdir may contain up to only 1 DebuggerV2 run (an error will be thrown by the backend Python code is >1 runs is present in a logdir). So the run for DebuggerV2 has a fixed, hardcoded run name right now. This is related to the fact that we have a special route to load the DebuggerV2 run here. It's possible that we'll allow multiple DebuggerV2 runs in the same logdir in the future, at that time, the run fetching mechanism can become more consistent with other plugins.

stephanwlee · 2019-12-25T01:35:55Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/effects/BUILD

+    ],
+)
+
+# TODO(cais): After TestBed.inject() is figured out in OSS, add


Can you explain here in GitHub comments what this would mean? You seem to have written a good test with TestBed.get which is more or less equal to inject

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

stephanwlee · 2019-12-25T01:41:08Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts

+export const DEBUGGER_FEATURE_KEY = 'debugger';
+
+// TODO(cais): Maybe deduplicate this with tensorboard/webapp.
+export enum DataLoadState {


You should certainly dedup ;) also this code, as discussed in offline, should move to webapp so that would be quite natural.

Done. Please note that I made some changes to the types in tensorboard/webapp as well:

The LoadState in tensorboard/webapp/types/api.ts is now renamed as DataLoadState.

The LoadState in tensorboard/core/store/core_types.ts is now moved into tensorboard/webapp/types/api.ts. This is justified by the fact that it is a sufficiently general interface that can be used not only the webapp/core, but also by other components and plugins.

This makes the type system slightly cleaner and more intuitive IMO.

tensorboard/webapp/types/api.ts

I don't think the LoadState belongs in there. I would love it if it gets moved to tensorboard/webapp/types/data.ts or something to that effect.

Done. I created a new file in the same directory (tensorboard/webapp/types/data.ts) and moved the DataLoadState and LoadState interfaces to that file. Changes are made accordingly in tensorboard/webapp and in tensorboard/plugins/debugger_v2.

…gger_v2

caisq

Thanks for the review so far.

Can you explain here in GitHub comments what this would mean? You seem to have written a good test with TestBed.get which is more or less equal to inject

Sorry for the confusion. This now-out-of-date TODO item was about adding debugger_effects_test. Done.

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/BUILD

caisq · 2019-12-29T03:12:50Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/data_source/tfdbg2_data_source.ts

+
+  fetchRuns() {
+    // TODO(cais): Once the backend uses an DataProvider that unifies tfdbg and
+    // non-tfdbg plugins, switch to using `tf_backend.runStore.refresh()`.


Yeah, it is not entirely clear what "runs" will mean for DebuggerV2 yet. Currently each logdir may contain up to only 1 DebuggerV2 run (an error will be thrown by the backend Python code is >1 runs is present in a logdir). So the run for DebuggerV2 has a fixed, hardcoded run name right now. This is related to the fact that we have a special route to load the DebuggerV2 run here. It's possible that we'll allow multiple DebuggerV2 runs in the same logdir in the future, at that time, the run fetching mechanism can become more consistent with other plugins.

caisq · 2019-12-29T03:21:21Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container.ts

  constructor(private readonly store: Store<State>) {}
+
+  ngOnInit(): void {
+    this.store.dispatch(debuggerLoaded());


Whatever you do here can probably be replaced with PluginChanged action, right?

I think that's correct. But part of me worry about separation of responsibility as well. It feels a little strange to me that the PluginsChange action, which is at a higher level than the individual plugin (DebuggerV2 specifically), should know something about the internal workings of the plugin.

Also, why do you say these are rare cases? The DebuggerContainer being loaded (and hence its ngOnInit() method being called) when a user switches to the plugin is an essential step is a common workflow for the plugin.

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

caisq · 2019-12-29T04:19:04Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts

+export const DEBUGGER_FEATURE_KEY = 'debugger';
+
+// TODO(cais): Maybe deduplicate this with tensorboard/webapp.
+export enum DataLoadState {


Done. Please note that I made some changes to the types in tensorboard/webapp as well:

The LoadState in tensorboard/webapp/types/api.ts is now renamed as DataLoadState.

The LoadState in tensorboard/core/store/core_types.ts is now moved into tensorboard/webapp/types/api.ts. This is justified by the fact that it is a sufficiently general interface that can be used not only the webapp/core, but also by other components and plugins.

This makes the type system slightly cleaner and more intuitive IMO.

stephanwlee · 2019-12-30T18:42:14Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/actions/debugger_actions.ts

+/**
+ * Actions for the Alerts Component.
+ */
+export const alertsViewLoaded = createAction('[Alerts] Alerts View Loaded');


[Debugger] seems more apt but it really does not matter what is in this string.

stephanwlee · 2019-12-30T18:43:16Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/data_source/tfdbg2_data_source.ts

+
+/** @typehack */ import * as _typeHackRxjs from 'rxjs';
+
+export const TFDBG2_DATA_SOURCE = new InjectionToken<Tfdbg2DataSource>(


How is this being used? It may be unused since you are providing the Tfdbg2HttpServerDataSource and injecting it in the places it is being used.

Right. It's currently unused. I removed it.

stephanwlee · 2019-12-30T18:45:23Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_component.ts

-export class DebuggerComponent {}
+export class DebuggerComponent {
+  @Input()
+  runs: DebuggerRunListing = {};


optional: if this component does not make sense without runs, make this input a required field by not providing a default value.

I agree. Made it a required field.

stephanwlee · 2019-12-30T18:48:37Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_selectors.ts

+export const getDebuggerRuns = createSelector(
+  selectDebuggerState,
+  (state: DebuggerState): DebuggerRunListing => {
+    return state.runs;


since it is returning a DebuggerRunListing, consider renaming the selector to getDebuggerRunListing (getDebuggerRun is has room for confusion since it can mean list of run IDs).

stephanwlee · 2019-12-30T18:51:32Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

+    const fixture = TestBed.createComponent(DebuggerContainer);
+    fixture.detectChanges();
+
+    store.setState(


Feel free to disagree: when writing views, we try to abstract out what is contained inside the store via semantically meaningful selectors. I think it makes more sense to mock out selectors without re-creating the store structure like this. If you agree with what I said, you can use store.overrideSelector(selector, value).

I see your point. I think this pattern (namely setting store state) is more aligned with the testing principle of "test behavior, not internal implementation". For one thing, it's more resistant against changes in how the details of the selectors change. So I prefer to keep the current pattern. (Also, as you may know, the component tests in tensorboard/webapp are based on this pattern, instead of the overrideSelector() pattern.

stephanwlee · 2019-12-30T19:00:11Z

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

+
+    const initialState = createState(createDebuggerState());
+    await TestBed.configureTestingModule({
+      imports: [HttpClientTestingModule],


since we are using the DataSource here, we may not actually need this especially since we should all stub data source methods out. Also, it may be a good idea to create a testing version of the DataSource.

It is actually needed. Without it, there will be a NullInjectorError, because Tfdbg2HttpServerDataSource requires an HttpClient injection.

I don't feel the urgent need to create a testing version of DataSource yet. Currently I find spyOn to be sufficient. I may add a testing version in the future when the need arises.

stephanwlee · 2019-12-30T19:01:03Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/BUILD

+    deps = [
+        "//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/actions",
+        "@npm//@ngrx/store",
+        "@npm//rxjs",


unused dep (I think)

Right. Removed.

stephanwlee · 2019-12-30T19:33:00Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_reducers_test.ts

+      expect(nextState.runsLoaded.lastLoadedTimeInMs).toBeNull();
+    });
+
+    it('sets runsLoaded and runs on successful runs loading', () => {


write a test for overwriting existing key please.

Done. Thanks for the suggestion.

stephanwlee · 2019-12-30T19:33:36Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_reducers.ts

+    actions.debuggerRunsLoaded,
+    (state: DebuggerState, {runs}): DebuggerState => {
+      return {
+        runs,


you may want to spread state so it is more resistant to changes in the state.

Good point. Done.

stephanwlee · 2019-12-30T19:34:34Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts

+}
+
+export interface State {
+  [DEBUGGER_FEATURE_KEY]?: DebuggerState;


Why is the DebuggerState optional? I don't think it should be optional.

I agree. Done.

caisq

Thanks again.

caisq · 2019-12-31T02:48:55Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/actions/debugger_actions.ts

+/**
+ * Actions for the Alerts Component.
+ */
+export const alertsViewLoaded = createAction('[Alerts] Alerts View Loaded');


caisq · 2019-12-31T03:02:47Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/data_source/tfdbg2_data_source.ts

+
+/** @typehack */ import * as _typeHackRxjs from 'rxjs';
+
+export const TFDBG2_DATA_SOURCE = new InjectionToken<Tfdbg2DataSource>(


Right. It's currently unused. I removed it.

caisq · 2019-12-31T03:04:10Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_component.ts

-export class DebuggerComponent {}
+export class DebuggerComponent {
+  @Input()
+  runs: DebuggerRunListing = {};


I agree. Made it a required field.

caisq · 2019-12-31T03:51:29Z

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

+
+    const initialState = createState(createDebuggerState());
+    await TestBed.configureTestingModule({
+      imports: [HttpClientTestingModule],


It is actually needed. Without it, there will be a NullInjectorError, because Tfdbg2HttpServerDataSource requires an HttpClient injection.

I don't feel the urgent need to create a testing version of DataSource yet. Currently I find spyOn to be sufficient. I may add a testing version in the future when the need arises.

caisq · 2019-12-31T03:54:12Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/BUILD

+    deps = [
+        "//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/actions",
+        "@npm//@ngrx/store",
+        "@npm//rxjs",


Right. Removed.

caisq · 2019-12-31T03:56:16Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_reducers.ts

+    actions.debuggerRunsLoaded,
+    (state: DebuggerState, {runs}): DebuggerState => {
+      return {
+        runs,


Good point. Done.

caisq · 2019-12-31T04:02:12Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_reducers_test.ts

+      expect(nextState.runsLoaded.lastLoadedTimeInMs).toBeNull();
+    });
+
+    it('sets runsLoaded and runs on successful runs loading', () => {


Done. Thanks for the suggestion.

caisq · 2019-12-31T04:02:26Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts

+}
+
+export interface State {
+  [DEBUGGER_FEATURE_KEY]?: DebuggerState;


I agree. Done.

caisq · 2019-12-31T04:04:10Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_selectors.ts

+export const getDebuggerRuns = createSelector(
+  selectDebuggerState,
+  (state: DebuggerState): DebuggerRunListing => {
+    return state.runs;


caisq · 2019-12-31T04:10:03Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container_test.ts

+    const fixture = TestBed.createComponent(DebuggerContainer);
+    fixture.detectChanges();
+
+    store.setState(


I see your point. I think this pattern (namely setting store state) is more aligned with the testing principle of "test behavior, not internal implementation". For one thing, it's more resistant against changes in how the details of the selectors change. So I prefer to keep the current pattern. (Also, as you may know, the component tests in tensorboard/webapp are based on this pattern, instead of the overrideSelector() pattern.

Remove `as Observable<Action>` Remove unused dep

…nto dbg2-alert-component

caisq · 2020-01-03T13:42:15Z

Gentle ping :)

[DebuggerV2] Add basic impl of actions, store, data_source & Alerts

7a411a5

googlebot added the cla: yes label Dec 24, 2019

caisq added 3 commits December 24, 2019 12:52

Remove cruft

caef13f

Remove some unused imports

310c23a

Decouple DebuggerModule states from tensorboard/webapp

42ab932

stephanwlee reviewed Dec 24, 2019

View reviewed changes

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/debugger_container.ts Outdated Show resolved Hide resolved

Simplify selector call

3a62454

caisq requested a review from stephanwlee December 24, 2019 19:01

caisq marked this pull request as ready for review December 24, 2019 19:01

Correct a typo

0dbf34c

stephanwlee reviewed Dec 25, 2019

View reviewed changes

caisq added 3 commits December 28, 2019 23:26

Dedup LoadState and DataLoadState interfaces b/w webapp/core and debu…

6f65c22

…gger_v2

Started working on debugger_effects_test.ts

75a790d

Continue developing debugger_effects_test.ts

7b55cc6

caisq commented Dec 29, 2019

View reviewed changes

caisq requested a review from stephanwlee December 29, 2019 18:23

Move DataLoadState and LoadState intefaces to new file

c7b524a

stephanwlee reviewed Dec 30, 2019

View reviewed changes

Address comments

ddf54ca

caisq commented Dec 31, 2019

View reviewed changes

caisq requested a review from stephanwlee December 31, 2019 04:11

caisq added 3 commits December 31, 2019 08:48

Add @npm/rxjs dependency back to satisfy type inference

7718e80

Add @ngrx/effects/effects type hack

8a68fc3

Remove `as Observable<Action>` Remove unused dep

Merge branch 'dbg2-alert-component' of github.com:caisq/tensorboard i…

6ba8fda

…nto dbg2-alert-component

stephanwlee approved these changes Jan 3, 2020

View reviewed changes

caisq merged commit ffe5e2b into tensorflow:master Jan 3, 2020


		/** @typehack / import as _typeHackRxjs from 'rxjs';

		export const TFDBG2_DATA_SOURCE = new InjectionToken<Tfdbg2DataSource>(

[DebuggerV2] Add basic ng impl of actions, store, data_source & new Alerts view #3075

[DebuggerV2] Add basic ng impl of actions, store, data_source & new Alerts view #3075

Uh oh!

Conversation

caisq commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

caisq left a comment

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!

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

caisq commented Dec 24, 2019 •

edited

Loading