timeseries: match selection using same logic (#5334)

Previously, to allow regex string also match run selection, we have
extend the utility selector to match run selection against the regex.
The logic, however, was massively flawed--instead of matching against
name of a run, experiment, alias, and legacy run name, the
implementation matched against runId which coincidentally contained run
name but was supposed to be an opaque id. This caused an obvious match
like `/^foo/` not to match anything especially when an opaque id is not
created to start with "foo".

In the end, this change basically re-implements the run selection based
on regex more correct.

In the process, we refactored how a run is matched against regex so the
behavior is more congruent in our app.

Author: stephanwlee

tensorboard/webapp/experiments/store/testing.ts

@@ -17,6 +17,7 @@ import {
   EXPERIMENTS_FEATURE_KEY,
   ExperimentsState,
   State,
+  ExperimentsDataState,
 } from './experiments_types';
 
 /**
@@ -34,6 +35,17 @@ export function buildExperiment(override?: Partial<Experiment>) {
   };
 }
 
+export function buildExperimentState(
+  override?: Partial<ExperimentsDataState>
+): ExperimentsState {
+  return {
+    data: {
+      experimentMap: {},
+      ...override,
+    },
+  };
+}
+
 /**
  * Get application state from an experiment state.
  */

tensorboard/webapp/runs/views/runs_table/BUILD

@@ -67,6 +67,7 @@ tf_ng_module(
         "//tensorboard/webapp/types",
         "//tensorboard/webapp/types:ui",
         "//tensorboard/webapp/util:colors",
+        "//tensorboard/webapp/util:matcher",
         "//tensorboard/webapp/widgets/filter_input",
         "//tensorboard/webapp/widgets/range_input",
         "@npm//@angular/common",

tensorboard/webapp/runs/views/runs_table/runs_table_container.ts

@@ -16,8 +16,8 @@ import {
   ChangeDetectionStrategy,
   Component,
   Input,
-  OnInit,
   OnDestroy,
+  OnInit,
 } from '@angular/core';
 import {createSelector, Store} from '@ngrx/store';
 import {combineLatest, Observable, of, Subject} from 'rxjs';
@@ -32,9 +32,19 @@ import {
   takeUntil,
 } from 'rxjs/operators';
 
-import {DataLoadState, LoadState} from '../../../types/data';
 import * as alertActions from '../../../alert/actions';
 import {State} from '../../../app_state';
+import {
+  actions as hparamsActions,
+  selectors as hparamsSelectors,
+} from '../../../hparams';
+import {
+  DiscreteFilter,
+  DiscreteHparamValue,
+  DiscreteHparamValues,
+  DomainType,
+  IntervalFilter,
+} from '../../../hparams/types';
 import {
   getCurrentRouteRunSelection,
   getEnabledColorGroup,
@@ -48,18 +58,9 @@ import {
   getRunSelectorSort,
   getRunsLoadState,
 } from '../../../selectors';
-import {
-  actions as hparamsActions,
-  selectors as hparamsSelectors,
-} from '../../../hparams';
-import {
-  DiscreteFilter,
-  IntervalFilter,
-  DiscreteHparamValue,
-  DiscreteHparamValues,
-  DomainType,
-} from '../../../hparams/types';
+import {DataLoadState, LoadState} from '../../../types/data';
 import {SortDirection} from '../../../types/ui';
+import {matchRunToRegex} from '../../../util/matcher';
 import {
   runColorChanged,
   runPageSelectionToggled,
@@ -69,9 +70,8 @@ import {
   runSelectorSortChanged,
   runTableShown,
 } from '../../actions';
-import {SortKey, SortType} from '../../types';
 import {MAX_NUM_RUNS_TO_ENABLE_BY_DEFAULT} from '../../store/runs_types';
-
+import {SortKey, SortType} from '../../types';
 import {
   HparamColumn,
   IntervalFilterChange,
@@ -440,36 +440,18 @@ export class RunsTableContainer implements OnInit, OnDestroy {
           return items;
         }
 
-        let regex: RegExp | null = null;
-
-        // Do not break all the future updates because of malformed
-        // regexString. User can be still modifying it.
-        try {
-          regex = regexString ? new RegExp(regexString) : null;
-        } catch (e) {}
-
-        if (!regex) {
-          return [];
-        }
-
         const shouldIncludeExperimentName = this.columns.includes(
           RunsTableColumn.EXPERIMENT_NAME
         );
         return items.filter((item) => {
-          // For some reason, TypeScript is faulty and cannot coerce the type to
-          // non-null inspite the `if (!regex)` above.
-          regex = regex!;
-
-          if (!shouldIncludeExperimentName) {
-            return regex.test(item.run.name);
-          }
-
-          const legacyRunName = `${item.experimentName}/${item.run.name}`;
-          return (
-            regex.test(item.run.name) ||
-            regex.test(item.experimentAlias) ||
-            regex.test(item.experimentName) ||
-            regex.test(legacyRunName)
+          return matchRunToRegex(
+            {
+              runName: item.run.name,
+              experimentAlias: item.experimentAlias,
+              experimentName: item.experimentName,
+            },
+            regexString,
+            shouldIncludeExperimentName
           );
         });
       }),

tensorboard/webapp/runs/views/runs_table/runs_table_test.ts

@@ -1472,7 +1472,7 @@ describe('runs_table', () => {
       });
     });
 
-    it('filters by original experiment name and legacy run name', () => {
+    it('filters by legacy experiment alias and run name put together', () => {
       selectSpy
         .withArgs(getExperiment, {experimentId: 'rowling'})
         .and.returnValue(
@@ -1530,18 +1530,15 @@ describe('runs_table', () => {
         ['LotR', 'The Silmarillion'],
       ]);
 
-      // Alias for Harry Potter contains "z". If we were matching against
-      // alias + run name, Harry Potter should not match below.
-      // This regex matches:
-      // - (Harry P)*o*tter/The Chamber of S(ecrets)
-      // - (The L)ord of the rings/The S(ilmarillion)
-      // and does not match
-      // - HPz/The Chamber of S(ecrets) because of "[^z]+".
+      // Alias for Harry Potter contains "z". Since legacy Polymer-based
+      // tf-run-selector and the new Angular run selector match regex
+      // against '<alias>/<run name>' instead of '<experiment>/<run name>',
+      // below regex matches:
+      // - LotR/The S(ilmarillion)
       store.overrideSelector(getRunSelectorRegexFilter, 'o[^z]+/.+S[ei]');
       store.refreshState();
       fixture.detectChanges();
       expect(getTableRowTextContent(fixture)).toEqual([
-        ['HPz', 'The Chamber Of Secrets'],
         ['LotR', 'The Silmarillion'],
       ]);
     });

tensorboard/webapp/util/BUILD

@@ -54,15 +54,29 @@ tf_ts_library(
     ],
 )
 
+tf_ts_library(
+    name = "matcher",
+    srcs = [
+        "matcher.ts",
+    ],
+    deps = [
+        "//tensorboard/webapp/runs:types",
+    ],
+)
+
 tf_ts_library(
     name = "ui_selectors",
     srcs = [
         "ui_selectors.ts",
     ],
     deps = [
         ":colors",
+        ":matcher",
         "//tensorboard/webapp:app_state",
+        "//tensorboard/webapp/app_routing:types",
         "//tensorboard/webapp/app_routing/store",
+        "//tensorboard/webapp/experiments/store:selectors",
+        "//tensorboard/webapp/runs:types",
         "//tensorboard/webapp/runs/store:selectors",
         "@npm//@ngrx/store",
     ],
@@ -91,6 +105,7 @@ tf_ts_library(
     testonly = True,
     srcs = [
         "lang_test.ts",
+        "matcher_test.ts",
         "ngrx_test.ts",
         "string_test.ts",
         "ui_selectors_test.ts",
@@ -99,6 +114,7 @@ tf_ts_library(
     deps = [
         ":colors",
         ":lang",
+        ":matcher",
         ":ngrx",
         ":string",
         ":ui_selectors",
@@ -107,6 +123,7 @@ tf_ts_library(
         "//tensorboard/webapp/app_routing:testing",
         "//tensorboard/webapp/app_routing:types",
         "//tensorboard/webapp/app_routing/store:testing",
+        "//tensorboard/webapp/experiments/store:testing",
         "//tensorboard/webapp/runs/store:testing",
         "@npm//@ngrx/store",
         "@npm//@types/jasmine",

tensorboard/webapp/util/matcher.ts

@@ -0,0 +1,60 @@
+/* Copyright 2021 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.
+==============================================================================*/
+/**
+ * @fileoverview Helps matching runs.
+ */
+
+export interface RunMatchable {
+  runName: string;
+  experimentName: string;
+  experimentAlias: string;
+}
+
+/**
+ * Matches an entry based on regex and business logic.
+ *
+ * - Regex matches name of a run.
+ * - When `shouldMatchExperiment` is specified, it matches regex against one of
+ *   experiment name, experiment alias, and legacy run name which is generated
+ *   with "<exp alias>/<run name>".
+ * - An empty regex string always returns true.
+ * - Invalid regex always return false.
+ * - Regex matches are case insensitive.
+ * - Regex matches are not anchored.
+ */
+export function matchRunToRegex(
+  runMatchable: RunMatchable,
+  regexString: string,
+  shouldMatchExperiment: boolean
+): boolean {
+  if (!regexString) return true;
+
+  let regex: RegExp;
+  try {
+    regex = new RegExp(regexString, 'i');
+  } catch {
+    return false;
+  }
+
+  const matchables = [runMatchable.runName];
+  if (shouldMatchExperiment) {
+    matchables.push(
+      runMatchable.experimentName,
+      runMatchable.experimentAlias,
+      `${runMatchable.experimentAlias}/${runMatchable.runName}`
+    );
+  }
+  return matchables.some((matchable) => regex!.test(matchable));
+}