Feature Helper: add route and build page #5760

JamesHollyer · 2022-06-15T15:39:06Z

Motivation for features / changes
Adding a page which helps to see which flags are enabled, allow flag changes, and enable more persistent flag settings. This change adds the page and displays which flags are enabled. The check boxes are currently not functional.
Technical description of changes
This PR adds the route in the angular app. The python web server still does not recognize this path so it is only accessible with in app routing. When the angular app is loaded the routing works. The page simply uses the getFeatureFlags selector to get which flags are enabled and loops through each to display their status.
Screenshots of UI changes
Currently the page looks like this:

It is subject to change.

Detailed steps to verify changes work correctly (as executed by you)
I added a link which takes me to the page for testing. This link was not added into the PR.
Alternate designs / implementations considered

qihach64 · 2022-06-16T11:30:27Z

tensorboard/webapp/feature_flag/feature_flag_page/BUILD

+    ],
+    deps = [
+        "//tensorboard/webapp:app_state",
+        "//tensorboard/webapp/angular:expect_angular_material_checkbox",


The visibility rule suggests

New users should instead use the newer MDC-based version. See go/angular-mdc/usage

So we should probably use "material_mdc_checkbox" instead

We have to discuss the "should we use mdc?" question as a team. Up to now we've been trying to ignore MDC and internal requests to migrate to them.

So I am not doing this now.

qihach64 · 2022-06-16T11:30:50Z

tensorboard/webapp/routes/index.ts

+      path: '/flags/',
+      ngComponent: FeatureFlagPageContainer as Type<Component>,
+      defaultRoute: false,
+      deepLinkProvider: new DashboardDeepLinkProvider(),


just curious about what happens if we remove deepLinkProvider here. (I did that and it seems to work the same way as before).

This allows for parameters. For instance it would allow us to type in /flags/?enableLinkTime. However, since we don't have the routing going we can only navigate here from a button so this does not really make a difference. I would like to leave it in the hopes that one day we do get the routing working.

Even if you do find a need for deep links in feature flags, I don't think DashboardDeepLinkProvider will be the one you use (it's specifically for deep links for the dashboard view).

Ok. Removed.

qihach64

Thanks for making the ff page. I also tested the FE routing on both OSS tb and tb.corp by using <a routerLink=['/flags']> on the home page and it works nicely!

However, I'm still having trouble making the FE routing work with initial page load of /flags. I was able to avoid the 404 error and hook up the Angular app to the Python web servers of OSS tb and tb.corp, but for some reason the FE routing didn't kick in after the page gets loaded.

bmd3k · 2022-06-15T15:41:50Z

tensorboard/webapp/routes/index.ts

      deepLinkProvider: new DashboardDeepLinkProvider(),
    },
+    {
+      routeKind: RouteKind.UNKNOWN,


Can you add a new RouteKind?

Oh yea, I meant to go back and do this. Done.

bmd3k · 2022-06-15T15:42:35Z

tensorboard/webapp/routes/index.ts

+      routeKind: RouteKind.UNKNOWN,
+      path: '/flags/',
+      ngComponent: FeatureFlagPageContainer as Type<Component>,
+      defaultRoute: false,


Do you need to specify defaultRoute? (Is false just the default value for defaultRoute if it is unspecified?)

It is the default. Removed.

bmd3k · 2022-06-15T15:44:19Z

tensorboard/webapp/feature_flag/feature_flag_page/BUILD

@@ -0,0 +1,33 @@
+load("//tensorboard/defs:defs.bzl", "tf_ng_module", "tf_sass_binary")


The practice we have been doing is to put components in the "views" subfolder. in this case: tensorboard/webapp/feature_flag/views/BUILD

bmd3k · 2022-06-15T15:47:43Z

tensorboard/webapp/feature_flag/feature_flag_page/feature_flag_page_container.ts

+import {State} from '../../app_state';
+import {getFeatureFlags} from '../store/feature_flag_selectors';
+@Component({
+  selector: 'feature_flag_page',


feature_flag_page => feature-flag-page for element names.

Doh! Didn't even notice since this selector is actually never used.

bmd3k · 2022-06-15T15:47:58Z

tensorboard/webapp/feature_flag/feature_flag_page/feature_flag_page_container.ts

+import {getFeatureFlags} from '../store/feature_flag_selectors';
+@Component({
+  selector: 'feature_flag_page',
+  template: `<feature_flag_page_component


similarly, feature_flag_page_component => feature-flag-page-component for element names.

bmd3k · 2022-06-15T15:54:02Z

tensorboard/webapp/feature_flag/feature_flag_page/feature_flag_page_module.ts

+@NgModule({
+  declarations: [FeatureFlagPageComponent, FeatureFlagPageContainer],
+  imports: [CommonModule, MatCheckboxModule],
+  exports: [FeatureFlagPageComponent],


I'm a little confused why exports and entryComponents are not FeatureFlagPageContainer (since this is the top-level component here)

Good catch. When I first built this for testing routing I only built the component. Changed.

bmd3k · 2022-06-16T13:42:18Z

tensorboard/webapp/feature_flag/feature_flag_page/BUILD

+    ],
+    deps = [
+        "//tensorboard/webapp:app_state",
+        "//tensorboard/webapp/angular:expect_angular_material_checkbox",


We have to discuss the "should we use mdc?" question as a team. Up to now we've been trying to ignore MDC and internal requests to migrate to them.

JamesHollyer

Thanks guys.

JamesHollyer · 2022-06-16T14:17:15Z

tensorboard/webapp/feature_flag/feature_flag_page/BUILD

@@ -0,0 +1,33 @@
+load("//tensorboard/defs:defs.bzl", "tf_ng_module", "tf_sass_binary")


JamesHollyer · 2022-06-16T14:17:28Z

tensorboard/webapp/feature_flag/feature_flag_page/BUILD

+    ],
+    deps = [
+        "//tensorboard/webapp:app_state",
+        "//tensorboard/webapp/angular:expect_angular_material_checkbox",


So I am not doing this now.

JamesHollyer · 2022-06-16T14:18:42Z

tensorboard/webapp/feature_flag/feature_flag_page/feature_flag_page_container.ts

+import {State} from '../../app_state';
+import {getFeatureFlags} from '../store/feature_flag_selectors';
+@Component({
+  selector: 'feature_flag_page',


Doh! Didn't even notice since this selector is actually never used.

JamesHollyer · 2022-06-16T14:19:44Z

tensorboard/webapp/feature_flag/feature_flag_page/feature_flag_page_container.ts

+import {getFeatureFlags} from '../store/feature_flag_selectors';
+@Component({
+  selector: 'feature_flag_page',
+  template: `<feature_flag_page_component


JamesHollyer · 2022-06-16T14:21:19Z

tensorboard/webapp/feature_flag/feature_flag_page/feature_flag_page_module.ts

+@NgModule({
+  declarations: [FeatureFlagPageComponent, FeatureFlagPageContainer],
+  imports: [CommonModule, MatCheckboxModule],
+  exports: [FeatureFlagPageComponent],


Good catch. When I first built this for testing routing I only built the component. Changed.

JamesHollyer · 2022-06-16T14:35:49Z

tensorboard/webapp/routes/index.ts

      deepLinkProvider: new DashboardDeepLinkProvider(),
    },
+    {
+      routeKind: RouteKind.UNKNOWN,


Oh yea, I meant to go back and do this. Done.

JamesHollyer · 2022-06-16T14:36:05Z

tensorboard/webapp/routes/index.ts

+      routeKind: RouteKind.UNKNOWN,
+      path: '/flags/',
+      ngComponent: FeatureFlagPageContainer as Type<Component>,
+      defaultRoute: false,


It is the default. Removed.

JamesHollyer · 2022-06-16T14:39:42Z

tensorboard/webapp/routes/index.ts

+      path: '/flags/',
+      ngComponent: FeatureFlagPageContainer as Type<Component>,
+      defaultRoute: false,
+      deepLinkProvider: new DashboardDeepLinkProvider(),


This allows for parameters. For instance it would allow us to type in /flags/?enableLinkTime. However, since we don't have the routing going we can only navigate here from a button so this does not really make a difference. I would like to leave it in the hopes that one day we do get the routing working.

bmd3k · 2022-06-17T11:41:59Z

tensorboard/webapp/feature_flag/views/BUILD

+    srcs = [
+        "feature_flag_page_component.ts",
+        "feature_flag_page_container.ts",
+        "feature_flag_page_module.ts",


I think it would be ok to just name this feature_flag_module, with the assumption that if other files are added to the view they are likely to be included in the same module.

bmd3k · 2022-06-17T11:44:11Z

tensorboard/webapp/feature_flag/views/BUILD

+)
+
+tf_ng_module(
+    name = "feature_flag_page",


Name it just "views" and therefore make it the default target. So others can refer to it as "//tensorboard/webapp/feature_flag/views"

bmd3k · 2022-06-17T11:46:05Z

tensorboard/webapp/routes/index.ts

+      path: '/flags/',
+      ngComponent: FeatureFlagPageContainer as Type<Component>,
+      defaultRoute: false,
+      deepLinkProvider: new DashboardDeepLinkProvider(),


Even if you do find a need for deep links in feature flags, I don't think DashboardDeepLinkProvider will be the one you use (it's specifically for deep links for the dashboard view).

JamesHollyer

Thanks Brian!

JamesHollyer · 2022-06-17T16:13:35Z

tensorboard/webapp/feature_flag/views/BUILD

+    srcs = [
+        "feature_flag_page_component.ts",
+        "feature_flag_page_container.ts",
+        "feature_flag_page_module.ts",


JamesHollyer · 2022-06-17T16:13:41Z

tensorboard/webapp/routes/index.ts

+      path: '/flags/',
+      ngComponent: FeatureFlagPageContainer as Type<Component>,
+      defaultRoute: false,
+      deepLinkProvider: new DashboardDeepLinkProvider(),


Ok. Removed.

JamesHollyer · 2022-06-17T16:13:45Z

tensorboard/webapp/feature_flag/views/BUILD

+)
+
+tf_ng_module(
+    name = "feature_flag_page",


* Feature Helper: add route and build page * fix lint issues * fix BUILD lint issues * CR * BUILD lint * CR2 * BUILD lint

JamesHollyer added 3 commits June 15, 2022 15:33

Feature Helper: add route and build page

26aec02

fix lint issues

c25db54

fix BUILD lint issues

1a0d6aa

qihach64 reviewed Jun 16, 2022

View reviewed changes

bmd3k reviewed Jun 16, 2022

View reviewed changes

CR

6aec286

JamesHollyer commented Jun 16, 2022

View reviewed changes

BUILD lint

9866049

qihach64 approved these changes Jun 16, 2022

View reviewed changes

JamesHollyer requested a review from bmd3k June 16, 2022 15:43

bmd3k approved these changes Jun 17, 2022

View reviewed changes

CR2

0aa940b

JamesHollyer commented Jun 17, 2022

View reviewed changes

BUILD lint

a11ea93

JamesHollyer merged commit 063aba3 into tensorflow:master Jun 17, 2022

rileyajones mentioned this pull request Jul 11, 2022

Show flags page as modal when showFlags query parameter is set #5800

Merged

yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023

Feature Helper: add route and build page (tensorflow#5760)

a2e062b

* Feature Helper: add route and build page * fix lint issues * fix BUILD lint issues * CR * BUILD lint * CR2 * BUILD lint

dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023

Feature Helper: add route and build page (tensorflow#5760)

1474df0

* Feature Helper: add route and build page * fix lint issues * fix BUILD lint issues * CR * BUILD lint * CR2 * BUILD lint

		@@ -0,0 +1,33 @@
		load("//tensorboard/defs:defs.bzl", "tf_ng_module", "tf_sass_binary")

Feature Helper: add route and build page #5760

Feature Helper: add route and build page #5760

Uh oh!

Conversation

JamesHollyer commented Jun 15, 2022

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!

qihach64 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!

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!

JamesHollyer 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!

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!

JamesHollyer left a comment

Choose a reason for hiding this comment