Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/BUILD
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package(default_visibility = ["//tensorboard:internal"])

load("@npm_angular_bazel//:index.bzl", "ng_module")
load("//tensorboard/defs:defs.bzl", "tf_ng_web_test_suite", "tf_ts_library")

licenses(["notice"]) # Apache 2.0

Expand All @@ -19,9 +20,50 @@ ng_module(
"debugger_component.ng.html",
],
deps = [
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/actions",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/data_source",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/effects",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/alerts",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/inactive",
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//@ngrx/effects",
"@npm//@ngrx/store",
"@npm//rxjs",
],
)

tf_ts_library(
name = "test_lib",
testonly = True,
srcs = [
"debugger_container_test.ts",
],
tsconfig = "//:tsconfig-test",
deps = [
":debugger_v2",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/actions",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store:types",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/testing",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/alerts",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/inactive",
"//tensorboard/webapp/angular:expect_angular_core_testing",
"@npm//@angular/common",
"@npm//@angular/compiler",
"@npm//@angular/core",
"@npm//@angular/platform-browser",
"@npm//@ngrx/store",
"@npm//@types/jasmine",
],
)

tf_ng_web_test_suite(
name = "karma_test",
deps = [
":test_lib",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/effects:debugger_effects_test_lib",
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store:debugger_store_test_lib",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package(default_visibility = ["//tensorboard:internal"])

load("//tensorboard/defs:defs.bzl", "tf_ts_library")

licenses(["notice"]) # Apache 2.

tf_ts_library(
name = "actions",
srcs = [
"debugger_actions.ts",
"index.ts",
],
deps = [
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store:types",
"@npm//@ngrx/store",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/* Copyright 2019 The TensorFlow Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/

import {createAction, props} from '@ngrx/store';

import {DebuggerRunListing} from '../store/debugger_types';

// HACK: Below import is for type inference.
// https://github.com/bazelbuild/rules_nodejs/issues/1013
/** @typehack */ import * as _typeHackModels from '@ngrx/store/src/models';

/**
* Actions for Debugger V2.
*/

/**
* Actions for the Debugger Component.
*/
export const debuggerLoaded = createAction('[Debugger] Debugger Loaded');

export const debuggerRunsRequested = createAction(
'[Debugger] Debugger Runs Requested'
);

export const debuggerRunsLoaded = createAction(
'[Debugger] Debugger Runs Loaded',
props<{runs: DebuggerRunListing}>()
);

export const debuggerRunsRequestFailed = createAction(
'[Debugger] Debugger Runs Request Failed'
);

/**
* Actions for the Alerts Component.
*/
export const alertsViewLoaded = createAction('[Debugger] Alerts View Loaded');
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/* Copyright 2019 The TensorFlow Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/

export * from './debugger_actions';
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package(default_visibility = ["//tensorboard:internal"])

load("@npm_angular_bazel//:index.bzl", "ng_module")

ng_module(
name = "data_source",
srcs = [
"tfdbg2_data_source.ts",
"tfdbg2_data_source_module.ts",
],
deps = [
"//tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store:types",
"//tensorboard/webapp/angular:expect_angular_common_http",
"@npm//@angular/common",
"@npm//@angular/core",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/* Copyright 2019 The TensorFlow Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {HttpClient} from '@angular/common/http';
import {Injectable} from '@angular/core';
import {Observable} from 'rxjs';
import {DebuggerRunListing} from '../store/debugger_types';

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

export abstract class Tfdbg2DataSource {
abstract fetchRuns(): Observable<DebuggerRunListing>;
}

@Injectable()
export class Tfdbg2HttpServerDataSource implements Tfdbg2DataSource {
private readonly httpPathPrefix = 'data/plugin/debugger-v2';

constructor(private http: HttpClient) {}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return this.http.get<DebuggerRunListing>(this.httpPathPrefix + '/runs');
}

// TODO(cais): Implement fetchEnvironments().
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/* Copyright 2019 The TensorFlow Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {NgModule} from '@angular/core';
import {HttpClientModule} from '@angular/common/http';
import {Tfdbg2HttpServerDataSource} from './tfdbg2_data_source';

@NgModule({
imports: [HttpClientModule],
providers: [Tfdbg2HttpServerDataSource],
})
export class Tfdbg2ServerDataSourceModule {}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
-->

<div>
<!-- TODO(cais): Use ngSwitch on the active/inactive state. -->
<tf-debugger-v2-inactive></tf-debugger-v2-inactive>
<tf-debugger-v2-inactive
*ngIf="runIds.length === 0; else dataAvailable"
></tf-debugger-v2-inactive>
<ng-template #dataAvailable>
<tf-debugger-v2-alerts></tf-debugger-v2-alerts>
<!-- TODO(cais): Add more elements, such as timeline, graph, source code, etc.-->
</ng-template>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,18 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {Component} from '@angular/core';
import {Component, Input} from '@angular/core';
import {DebuggerRunListing} from './store/debugger_types';

@Component({
selector: 'debugger-component',
templateUrl: './debugger_component.ng.html',
styleUrls: ['./debugger_component.css'],
})
export class DebuggerComponent {}
export class DebuggerComponent {
@Input()
runs: DebuggerRunListing = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Made it a required field.


@Input()
runIds: string[] = [];
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,39 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {Component} from '@angular/core';
import {Store} from '@ngrx/store';
import {Component, OnInit} from '@angular/core';
import {createSelector, select, Store} from '@ngrx/store';
import {DebuggerRunListing, State} from './store/debugger_types';

/** @typehack */ import * as _typeHackRxjs from 'rxjs';
import {debuggerLoaded} from './actions';
import {getDebuggerRunListing} from './store';

// TODO(cais): Move to a separate file.
export interface State {}
/** @typehack */ import * as _typeHackRxjs from 'rxjs';

@Component({
selector: 'tf-debugger-v2',
template: `
<debugger-component></debugger-component>
<debugger-component
[runs]="runs$ | async"
[runIds]="runsIds$ | async"
></debugger-component>
`,
})
export class DebuggerContainer {
export class DebuggerContainer implements OnInit {
readonly runs$ = this.store.pipe(select(getDebuggerRunListing));

readonly runsIds$ = this.store.pipe(
select(
createSelector(
getDebuggerRunListing,
(runs): string[] => Object.keys(runs)
)
)
);

constructor(private readonly store: Store<State>) {}

ngOnInit(): void {
this.store.dispatch(debuggerLoaded());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Loading