From 844f8b9d994216efc3c60eac0fc002ee8bde7ecd Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Thu, 23 Sep 2021 17:48:48 -0700 Subject: [PATCH 1/3] timeseries: match selection using same logic 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 oapque 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. --- .../webapp/experiments/store/testing.ts | 12 ++ .../webapp/runs/views/runs_table/BUILD | 1 + .../views/runs_table/runs_table_container.ts | 60 ++---- .../runs/views/runs_table/runs_table_test.ts | 12 +- tensorboard/webapp/util/BUILD | 17 ++ tensorboard/webapp/util/matcher.ts | 49 +++++ tensorboard/webapp/util/matcher_test.ts | 121 ++++++++++++ tensorboard/webapp/util/ui_selectors.ts | 64 +++++-- tensorboard/webapp/util/ui_selectors_test.ts | 177 ++++++++++++++---- 9 files changed, 411 insertions(+), 102 deletions(-) create mode 100644 tensorboard/webapp/util/matcher.ts create mode 100644 tensorboard/webapp/util/matcher_test.ts diff --git a/tensorboard/webapp/experiments/store/testing.ts b/tensorboard/webapp/experiments/store/testing.ts index d761316c70..37138b882e 100644 --- a/tensorboard/webapp/experiments/store/testing.ts +++ b/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) { }; } +export function buildExperimentState( + override?: Partial +): ExperimentsState { + return { + data: { + experimentMap: {}, + ...override, + }, + }; +} + /** * Get application state from an experiment state. */ diff --git a/tensorboard/webapp/runs/views/runs_table/BUILD b/tensorboard/webapp/runs/views/runs_table/BUILD index 093c2343a2..7dd3a88613 100644 --- a/tensorboard/webapp/runs/views/runs_table/BUILD +++ b/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", diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts index 2a2cf5bc9a..66c80447d5 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts +++ b/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,14 @@ 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( + {...item.run, ...item}, + regexString, + shouldIncludeExperimentName ); }); }), diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts index d9ec72a48e..5ba29015bd 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts @@ -1530,18 +1530,16 @@ 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. + // Alias for Harry Potter contains "z". Since tf-run-selector matches + // regex against '/' instead of '/' + // Harry Potter should 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]+". + // - HPz/The Chamber of S(ecrets) + // - 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'], ]); }); diff --git a/tensorboard/webapp/util/BUILD b/tensorboard/webapp/util/BUILD index 73c44c1fec..fcffc1e5e0 100644 --- a/tensorboard/webapp/util/BUILD +++ b/tensorboard/webapp/util/BUILD @@ -54,6 +54,16 @@ tf_ts_library( ], ) +tf_ts_library( + name = "matcher", + srcs = [ + "matcher.ts", + ], + deps = [ + "//tensorboard/webapp/runs:types", + ], +) + tf_ts_library( name = "ui_selectors", srcs = [ @@ -61,8 +71,12 @@ tf_ts_library( ], 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", diff --git a/tensorboard/webapp/util/matcher.ts b/tensorboard/webapp/util/matcher.ts new file mode 100644 index 0000000000..7b5f838938 --- /dev/null +++ b/tensorboard/webapp/util/matcher.ts @@ -0,0 +1,49 @@ +/* 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. + */ + +import {Run} from '../runs/types'; + +export interface RunWithExperimentName extends Run { + experimentName: string; + experimentAlias: string; +} + +export function matchRunToRegex( + runWithName: RunWithExperimentName, + regexString: string, + shouldMatchExperiment: boolean +): boolean { + if (!regexString) return true; + + let regex: RegExp; + try { + regex = new RegExp(regexString, 'i'); + } catch { + return false; + } + + const matchables = [runWithName.name]; + if (shouldMatchExperiment) { + matchables.push( + runWithName.experimentName, + runWithName.experimentAlias, + `${runWithName.experimentAlias}/${runWithName.name}` + ); + } + return matchables.some((matchable) => regex!.test(matchable)); +} diff --git a/tensorboard/webapp/util/matcher_test.ts b/tensorboard/webapp/util/matcher_test.ts new file mode 100644 index 0000000000..0ed0ddedfb --- /dev/null +++ b/tensorboard/webapp/util/matcher_test.ts @@ -0,0 +1,121 @@ +/* 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. +==============================================================================*/ + +import {buildRun} from '../runs/store/testing'; +import {matchRunToRegex, RunWithExperimentName} from './matcher'; + +function buildRunWithName( + override: Partial = {} +): RunWithExperimentName { + return { + ...buildRun(), + experimentAlias: 'alias', + experimentName: 'Experiment name', + ...override, + }; +} + +describe('matcher test', () => { + describe('#matchRunToRegex', () => { + it('matches against name of a run against regex', () => { + const actual = matchRunToRegex( + buildRunWithName({name: 'faaaaro'}), + '^f[a]+r', + false + ); + expect(actual).toBe(true); + }); + + it('matches name with any casing', () => { + const actual = matchRunToRegex( + buildRunWithName({name: 'faaaar'}), + 'faAA+r', + false + ); + expect(actual).toBe(true); + }); + + describe('shouldMatchExperiment flag', () => { + it('matches against experimentName when flag is on', () => { + expect( + matchRunToRegex( + buildRunWithName({name: 'faaaaro', experimentName: 'hello^^'}), + '\\^\\^$', + true + ) + ).toBe(true); + expect( + matchRunToRegex( + buildRunWithName({name: 'faaaaro', experimentName: 'hello^^'}), + '\\^\\^$', + false + ) + ).toBe(false); + }); + + it('matches against experimentAlias when flag is on', () => { + expect( + matchRunToRegex( + buildRunWithName({name: 'faaaaro', experimentAlias: 'aliasName'}), + '^aliasName$', + true + ) + ).toBe(true); + expect( + matchRunToRegex( + buildRunWithName({name: 'faaaaro', experimentAlias: 'aliasName'}), + '^aliasName$', + false + ) + ).toBe(false); + }); + + it('matches against composite name for backwards compat', () => { + expect( + matchRunToRegex( + buildRunWithName({ + name: 'world', + experimentAlias: 'hello', + experimentName: 'goodbye', + }), + '^hello/world$', + true + ) + ).toBe(true); + expect( + matchRunToRegex( + buildRunWithName({ + name: 'world', + experimentAlias: 'hello', + experimentName: 'goodbye', + }), + 'hello', + true + ) + ).toBe(true); + }); + }); + + it('returns true when regex string is empty', () => { + expect(matchRunToRegex(buildRunWithName(), '', false)).toBe(true); + }); + + it('returns false when a regex string is invaid regex', () => { + expect(matchRunToRegex(buildRunWithName({name: '*'}), '*', false)).toBe( + false + ); + }); + }); +}); diff --git a/tensorboard/webapp/util/ui_selectors.ts b/tensorboard/webapp/util/ui_selectors.ts index f7b9dce4af..6bad6194f1 100644 --- a/tensorboard/webapp/util/ui_selectors.ts +++ b/tensorboard/webapp/util/ui_selectors.ts @@ -27,17 +27,29 @@ limitations under the License. import {createSelector} from '@ngrx/store'; -import {getExperimentIdsFromRoute} from '../app_routing/store/app_routing_selectors'; -import {State} from '../app_state'; -import {CHART_COLOR_PALLETE, NON_MATCHED_COLOR} from './colors'; import { getDefaultRunColorIdMap, + getExperimentIdsFromRoute, + getExperimentIdToAliasMap, + getRouteKind, getRunColorOverride, +} from '../app_routing/store/app_routing_selectors'; +import {RouteKind} from '../app_routing/types'; +import {State} from '../app_state'; +import {getExperiment} from '../experiments/store/experiments_selectors'; +import { + getRuns, getRunSelectionMap, getRunSelectorRegexFilter, } from '../runs/store/runs_selectors'; +import {Run} from '../runs/types'; +import {CHART_COLOR_PALLETE, NON_MATCHED_COLOR} from './colors'; +import {matchRunToRegex} from './matcher'; -/** @typehack */ import * as _typeHackStore from '@ngrx/store'; +interface RunAndNames extends Run { + experimentAlias: string; + experimentName: string; +} /** * Selects the run selection (runId to boolean) of current routeId. @@ -47,26 +59,40 @@ import { export const getCurrentRouteRunSelection = createSelector( (state: State): Map | null => { const experimentIds = getExperimentIdsFromRoute(state); - if (experimentIds === null) { - return null; - } - return getRunSelectionMap(state, {experimentIds}); + return experimentIds ? getRunSelectionMap(state, {experimentIds}) : null; }, getRunSelectorRegexFilter, - (runSelection, regexFilter) => { - if (!runSelection) return null; + (state: State): Map => { + const experimentIds = getExperimentIdsFromRoute(state) ?? []; + const aliasMap = getExperimentIdToAliasMap(state); - let regexExp: RegExp; - - try { - regexExp = new RegExp(regexFilter, 'i'); - } catch { - regexExp = new RegExp(''); + const runAndNameMap = new Map(); + for (const experimentId of experimentIds) { + const experiment = getExperiment(state, {experimentId}); + if (!experiment) continue; + const runs = getRuns(state, {experimentId}); + for (const run of runs) { + runAndNameMap.set(run.id, { + ...run, + experimentName: experiment.name, + experimentAlias: aliasMap[experimentId], + }); + } } - + return runAndNameMap; + }, + getRouteKind, + (runSelection, regexFilter, runAndNameMap, routeKind) => { + if (!runSelection) return null; + const includeExperimentInfo = routeKind === RouteKind.COMPARE_EXPERIMENT; const filteredSelection = new Map(); - for (const [key, value] of runSelection.entries()) { - filteredSelection.set(key, regexExp.test(key) && value); + + for (const [runId, value] of runSelection.entries()) { + const runAndName = runAndNameMap.get(runId)!; + filteredSelection.set( + runId, + matchRunToRegex(runAndName, regexFilter, includeExperimentInfo) && value + ); } return filteredSelection; } diff --git a/tensorboard/webapp/util/ui_selectors_test.ts b/tensorboard/webapp/util/ui_selectors_test.ts index ca096bb699..5340d905b9 100644 --- a/tensorboard/webapp/util/ui_selectors_test.ts +++ b/tensorboard/webapp/util/ui_selectors_test.ts @@ -18,10 +18,22 @@ import { } from '../app_routing/store/testing'; import {buildRoute} from '../app_routing/testing'; import {RouteKind} from '../app_routing/types'; -import {buildRunsState, buildStateFromRunsState} from '../runs/store/testing'; import { + buildExperiment, + buildExperimentState, + buildStateFromExperimentsState, +} from '../experiments/store/testing'; +import { + buildRun, + buildRunsState, + buildStateFromRunsState, +} from '../runs/store/testing'; +import { + getExperiment, getExperimentIdsFromRoute, + getExperimentIdToAliasMap, getRouteId, + getRouteKind, getRunSelectionMap, } from '../selectors'; import {CHART_COLOR_PALLETE, NON_MATCHED_COLOR} from './colors'; @@ -34,6 +46,9 @@ describe('ui_selectors test', () => { getRouteId.release(); getRunSelectionMap.release(); getCurrentRouteRunSelection.release(); + getExperiment.release(); + getExperimentIdToAliasMap.release(); + getRouteKind.release(); }); describe('#getCurrentRouteRunSelection', () => { @@ -61,6 +76,14 @@ describe('ui_selectors test', () => { ]), }) ), + ...buildStateFromExperimentsState( + buildExperimentState({ + experimentMap: { + '123': buildExperiment({id: '123', name: 'Experiment 123'}), + '234': buildExperiment({id: '234', name: 'Experiment 234'}), + }, + }) + ), }; expect(getCurrentRouteRunSelection(state)).toEqual( @@ -95,46 +118,130 @@ describe('ui_selectors test', () => { ]), }) ), + ...buildStateFromExperimentsState( + buildExperimentState({ + experimentMap: { + '234': buildExperiment({id: '234', name: 'Experiment 234'}), + }, + }) + ), }; expect(getCurrentRouteRunSelection(state)).toBeNull(); }); - it('filters runs based on regex', () => { - const state = { - ...buildStateFromAppRoutingState( - buildAppRoutingState({ - activeRoute: buildRoute({ - routeKind: RouteKind.EXPERIMENT, - pathname: '/experiment/234/', - params: {experimentId: '234'}, - }), - }) - ), - ...buildStateFromRunsState( - buildRunsState({ - selectionState: new Map([ - [ - '["234"]', - new Map([ - ['run1', true], - ['run2', true], - ['run3', false], - ]), - ], - ]), - regexFilter: 'n1', - }) - ), - }; + describe('regex filter', () => { + it('filters runs based on regex and run name', () => { + const state = { + ...buildStateFromAppRoutingState( + buildAppRoutingState({ + activeRoute: buildRoute({ + routeKind: RouteKind.EXPERIMENT, + pathname: '/experiment/234/', + params: {experimentId: '234'}, + }), + }) + ), + ...buildStateFromRunsState( + buildRunsState({ + selectionState: new Map([ + [ + '["234"]', + new Map([ + ['234/run1', true], + ['234/run2', true], + ['234/run3', false], + ]), + ], + ]), + runIds: { + '234': ['234/run1', '234/run2', '234/run3'], + }, + runMetadata: { + '234/run1': buildRun({id: '234/run1', name: 'run1'}), + '234/run2': buildRun({id: '234/run2', name: 'run2'}), + '234/run3': buildRun({id: '234/run3', name: 'run3'}), + }, + regexFilter: '^r.n[1]', + }) + ), + ...buildStateFromExperimentsState( + buildExperimentState({ + experimentMap: { + '234': buildExperiment({id: '234', name: 'Experiment 234'}), + }, + }) + ), + }; - expect(getCurrentRouteRunSelection(state)).toEqual( - new Map([ - ['run1', true], - ['run2', false], - ['run3', false], - ]) - ); + expect(getCurrentRouteRunSelection(state)).toEqual( + new Map([ + ['234/run1', true], + ['234/run2', false], + ['234/run3', false], + ]) + ); + }); + + it('filters run name, experiment name, and alias in compare mode', () => { + const state = { + ...buildStateFromAppRoutingState( + buildAppRoutingState({ + activeRoute: buildRoute({ + routeKind: RouteKind.COMPARE_EXPERIMENT, + pathname: '/compare/apple:123,banana:234/', + params: {experimentIds: 'apple:123,banana:234'}, + }), + }) + ), + ...buildStateFromRunsState( + buildRunsState({ + selectionState: new Map([ + [ + '["123","234"]', + new Map([ + ['123/run1', false], + ['123/run2', true], + ['234/run1', true], + ['234/run2', true], + ['234/run3', false], + ]), + ], + ]), + runIds: { + '123': ['123/run1', '123/run2'], + '234': ['234/run1', '234/run2', '234/run3'], + }, + runMetadata: { + '123/run1': buildRun({id: '123/run1', name: 'run1'}), + '123/run2': buildRun({id: '123/run2', name: 'run2'}), + '234/run1': buildRun({id: '234/run1', name: 'run1'}), + '234/run2': buildRun({id: '234/run2', name: 'run2'}), + '234/run3': buildRun({id: '234/run3', name: 'run3'}), + }, + regexFilter: '^(apple/..3|r.n[1])', + }) + ), + ...buildStateFromExperimentsState( + buildExperimentState({ + experimentMap: { + '123': buildExperiment({id: '123', name: 'Experiment 123'}), + '234': buildExperiment({id: '234', name: 'Experiment 234'}), + }, + }) + ), + }; + + expect(getCurrentRouteRunSelection(state)).toEqual( + new Map([ + ['123/run1', false], + ['123/run2', false], + ['234/run1', true], + ['234/run2', false], + ['234/run3', false], + ]) + ); + }); }); }); From e1c2b897b6de9d624f41cc718a347c3cc2b757be Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Fri, 24 Sep 2021 12:03:53 -0700 Subject: [PATCH 2/3] cr --- .../views/runs_table/runs_table_container.ts | 6 ++- .../runs/views/runs_table/runs_table_test.ts | 10 ++--- tensorboard/webapp/util/matcher.ts | 27 ++++++++---- tensorboard/webapp/util/matcher_test.ts | 43 ++++++++++--------- tensorboard/webapp/util/ui_selectors.ts | 19 ++++---- tensorboard/webapp/util/ui_selectors_test.ts | 9 +++- 6 files changed, 70 insertions(+), 44 deletions(-) diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts index 66c80447d5..bf11965da8 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts @@ -445,7 +445,11 @@ export class RunsTableContainer implements OnInit, OnDestroy { ); return items.filter((item) => { return matchRunToRegex( - {...item.run, ...item}, + { + runName: item.run.name, + experimentAlias: item.experimentAlias, + experimentName: item.experimentName, + }, regexString, shouldIncludeExperimentName ); diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts index 5ba29015bd..c1c7d2bd20 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts +++ b/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,10 +1530,10 @@ describe('runs_table', () => { ['LotR', 'The Silmarillion'], ]); - // Alias for Harry Potter contains "z". Since tf-run-selector matches - // regex against '/' instead of '/' - // Harry Potter should match below. - // This regex matches: + // Alias for Harry Potter contains "z". Since legacy Polymer based + // tf-run-selector matches, and the new Angular run selector, regex + // against '/' instead of '/', + // below regex matches: // - HPz/The Chamber of S(ecrets) // - LotR/The S(ilmarillion) store.overrideSelector(getRunSelectorRegexFilter, 'o[^z]+/.+S[ei]'); diff --git a/tensorboard/webapp/util/matcher.ts b/tensorboard/webapp/util/matcher.ts index 7b5f838938..163e9e74b3 100644 --- a/tensorboard/webapp/util/matcher.ts +++ b/tensorboard/webapp/util/matcher.ts @@ -16,15 +16,26 @@ limitations under the License. * @fileoverview Helps matching runs. */ -import {Run} from '../runs/types'; - -export interface RunWithExperimentName extends Run { +export interface RunMatchable { + runName: string; experimentName: string; experimentAlias: string; } +/** + * Matches an entry based on regex and business logic. + * + * - An empty regex string always returns true. + * - Invalid regex always return false. + * - Regex matches are case insensitive. + * - Regex matches are not anchored. + * - Matches name of a run. + * - When `shouldMatchExperiment` is specified, it matches against experiment + * name, experiment alias, and legacy run name which is generated with + * "/". + */ export function matchRunToRegex( - runWithName: RunWithExperimentName, + runMatchable: RunMatchable, regexString: string, shouldMatchExperiment: boolean ): boolean { @@ -37,12 +48,12 @@ export function matchRunToRegex( return false; } - const matchables = [runWithName.name]; + const matchables = [runMatchable.runName]; if (shouldMatchExperiment) { matchables.push( - runWithName.experimentName, - runWithName.experimentAlias, - `${runWithName.experimentAlias}/${runWithName.name}` + runMatchable.experimentName, + runMatchable.experimentAlias, + `${runMatchable.experimentAlias}/${runMatchable.runName}` ); } return matchables.some((matchable) => regex!.test(matchable)); diff --git a/tensorboard/webapp/util/matcher_test.ts b/tensorboard/webapp/util/matcher_test.ts index 0ed0ddedfb..dd25edda88 100644 --- a/tensorboard/webapp/util/matcher_test.ts +++ b/tensorboard/webapp/util/matcher_test.ts @@ -13,14 +13,11 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {buildRun} from '../runs/store/testing'; -import {matchRunToRegex, RunWithExperimentName} from './matcher'; +import {matchRunToRegex, RunMatchable} from './matcher'; -function buildRunWithName( - override: Partial = {} -): RunWithExperimentName { +function buildRunWithName(override: Partial = {}): RunMatchable { return { - ...buildRun(), + runName: 'run name', experimentAlias: 'alias', experimentName: 'Experiment name', ...override, @@ -31,7 +28,7 @@ describe('matcher test', () => { describe('#matchRunToRegex', () => { it('matches against name of a run against regex', () => { const actual = matchRunToRegex( - buildRunWithName({name: 'faaaaro'}), + buildRunWithName({runName: 'faaaaro'}), '^f[a]+r', false ); @@ -40,7 +37,7 @@ describe('matcher test', () => { it('matches name with any casing', () => { const actual = matchRunToRegex( - buildRunWithName({name: 'faaaar'}), + buildRunWithName({runName: 'faaaar'}), 'faAA+r', false ); @@ -51,15 +48,15 @@ describe('matcher test', () => { it('matches against experimentName when flag is on', () => { expect( matchRunToRegex( - buildRunWithName({name: 'faaaaro', experimentName: 'hello^^'}), - '\\^\\^$', + buildRunWithName({runName: 'faaaaro', experimentName: 'hello'}), + 'lo$', true ) ).toBe(true); expect( matchRunToRegex( - buildRunWithName({name: 'faaaaro', experimentName: 'hello^^'}), - '\\^\\^$', + buildRunWithName({runName: 'faaaaro', experimentName: 'hello'}), + 'lo$', false ) ).toBe(false); @@ -68,14 +65,20 @@ describe('matcher test', () => { it('matches against experimentAlias when flag is on', () => { expect( matchRunToRegex( - buildRunWithName({name: 'faaaaro', experimentAlias: 'aliasName'}), + buildRunWithName({ + runName: 'faaaaro', + experimentAlias: 'aliasName', + }), '^aliasName$', true ) ).toBe(true); expect( matchRunToRegex( - buildRunWithName({name: 'faaaaro', experimentAlias: 'aliasName'}), + buildRunWithName({ + runName: 'faaaaro', + experimentAlias: 'aliasName', + }), '^aliasName$', false ) @@ -86,7 +89,7 @@ describe('matcher test', () => { expect( matchRunToRegex( buildRunWithName({ - name: 'world', + runName: 'world', experimentAlias: 'hello', experimentName: 'goodbye', }), @@ -97,7 +100,7 @@ describe('matcher test', () => { expect( matchRunToRegex( buildRunWithName({ - name: 'world', + runName: 'world', experimentAlias: 'hello', experimentName: 'goodbye', }), @@ -112,10 +115,10 @@ describe('matcher test', () => { expect(matchRunToRegex(buildRunWithName(), '', false)).toBe(true); }); - it('returns false when a regex string is invaid regex', () => { - expect(matchRunToRegex(buildRunWithName({name: '*'}), '*', false)).toBe( - false - ); + it('returns false when a regex string is invalid regex', () => { + expect( + matchRunToRegex(buildRunWithName({runName: '*'}), '*', false) + ).toBe(false); }); }); }); diff --git a/tensorboard/webapp/util/ui_selectors.ts b/tensorboard/webapp/util/ui_selectors.ts index 6bad6194f1..2f3754d63e 100644 --- a/tensorboard/webapp/util/ui_selectors.ts +++ b/tensorboard/webapp/util/ui_selectors.ts @@ -44,7 +44,7 @@ import { } from '../runs/store/runs_selectors'; import {Run} from '../runs/types'; import {CHART_COLOR_PALLETE, NON_MATCHED_COLOR} from './colors'; -import {matchRunToRegex} from './matcher'; +import {matchRunToRegex, RunMatchable} from './matcher'; interface RunAndNames extends Run { experimentAlias: string; @@ -62,36 +62,37 @@ export const getCurrentRouteRunSelection = createSelector( return experimentIds ? getRunSelectionMap(state, {experimentIds}) : null; }, getRunSelectorRegexFilter, - (state: State): Map => { + (state: State): Map => { const experimentIds = getExperimentIdsFromRoute(state) ?? []; const aliasMap = getExperimentIdToAliasMap(state); - const runAndNameMap = new Map(); + const runMatchableMap = new Map(); for (const experimentId of experimentIds) { const experiment = getExperiment(state, {experimentId}); if (!experiment) continue; const runs = getRuns(state, {experimentId}); for (const run of runs) { - runAndNameMap.set(run.id, { - ...run, + runMatchableMap.set(run.id, { + runName: run.name, experimentName: experiment.name, experimentAlias: aliasMap[experimentId], }); } } - return runAndNameMap; + return runMatchableMap; }, getRouteKind, - (runSelection, regexFilter, runAndNameMap, routeKind) => { + (runSelection, regexFilter, runMatchableMap, routeKind) => { if (!runSelection) return null; const includeExperimentInfo = routeKind === RouteKind.COMPARE_EXPERIMENT; const filteredSelection = new Map(); for (const [runId, value] of runSelection.entries()) { - const runAndName = runAndNameMap.get(runId)!; + const runMatchable = runMatchableMap.get(runId)!; filteredSelection.set( runId, - matchRunToRegex(runAndName, regexFilter, includeExperimentInfo) && value + matchRunToRegex(runMatchable, regexFilter, includeExperimentInfo) && + value ); } return filteredSelection; diff --git a/tensorboard/webapp/util/ui_selectors_test.ts b/tensorboard/webapp/util/ui_selectors_test.ts index 5340d905b9..cb0e4e2db4 100644 --- a/tensorboard/webapp/util/ui_selectors_test.ts +++ b/tensorboard/webapp/util/ui_selectors_test.ts @@ -219,7 +219,7 @@ describe('ui_selectors test', () => { '234/run2': buildRun({id: '234/run2', name: 'run2'}), '234/run3': buildRun({id: '234/run3', name: 'run3'}), }, - regexFilter: '^(apple/..3|r.n[1])', + regexFilter: '^(apple/r..3|r.n[1])', }) ), ...buildStateFromExperimentsState( @@ -234,10 +234,17 @@ describe('ui_selectors test', () => { expect(getCurrentRouteRunSelection(state)).toEqual( new Map([ + // Inherits false from `selectionState`. ['123/run1', false], + // legacy name = "apple/run2" and does not match `r.n[1]` and + // `apple/r..3`. ['123/run2', false], + // legacy name = "banana/run1". Inherits true + matches `r.n[1]`. ['234/run1', true], + // legacy name = "banana/run2". Does not match `r.n[1]` and + // `apple/r..3`. ['234/run2', false], + // Inherits false from `selectionState`. ['234/run3', false], ]) ); From bf6463cd7e5d8265cea91c381a529c1d085c9d46 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Mon, 27 Sep 2021 14:59:29 -0700 Subject: [PATCH 3/3] cr 2 --- .../webapp/runs/views/runs_table/runs_table_test.ts | 5 ++--- tensorboard/webapp/util/matcher.ts | 8 ++++---- tensorboard/webapp/util/ui_selectors.ts | 4 ++-- tensorboard/webapp/util/ui_selectors_test.ts | 6 +++++- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts index c1c7d2bd20..44862a5708 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts @@ -1530,11 +1530,10 @@ describe('runs_table', () => { ['LotR', 'The Silmarillion'], ]); - // Alias for Harry Potter contains "z". Since legacy Polymer based - // tf-run-selector matches, and the new Angular run selector, regex + // Alias for Harry Potter contains "z". Since legacy Polymer-based + // tf-run-selector and the new Angular run selector match regex // against '/' instead of '/', // below regex matches: - // - HPz/The Chamber of S(ecrets) // - LotR/The S(ilmarillion) store.overrideSelector(getRunSelectorRegexFilter, 'o[^z]+/.+S[ei]'); store.refreshState(); diff --git a/tensorboard/webapp/util/matcher.ts b/tensorboard/webapp/util/matcher.ts index 163e9e74b3..c6f37157c1 100644 --- a/tensorboard/webapp/util/matcher.ts +++ b/tensorboard/webapp/util/matcher.ts @@ -25,14 +25,14 @@ export interface RunMatchable { /** * 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 "/". * - An empty regex string always returns true. * - Invalid regex always return false. * - Regex matches are case insensitive. * - Regex matches are not anchored. - * - Matches name of a run. - * - When `shouldMatchExperiment` is specified, it matches against experiment - * name, experiment alias, and legacy run name which is generated with - * "/". */ export function matchRunToRegex( runMatchable: RunMatchable, diff --git a/tensorboard/webapp/util/ui_selectors.ts b/tensorboard/webapp/util/ui_selectors.ts index 2f3754d63e..3af5ae1118 100644 --- a/tensorboard/webapp/util/ui_selectors.ts +++ b/tensorboard/webapp/util/ui_selectors.ts @@ -28,16 +28,16 @@ limitations under the License. import {createSelector} from '@ngrx/store'; import { - getDefaultRunColorIdMap, getExperimentIdsFromRoute, getExperimentIdToAliasMap, getRouteKind, - getRunColorOverride, } from '../app_routing/store/app_routing_selectors'; import {RouteKind} from '../app_routing/types'; import {State} from '../app_state'; import {getExperiment} from '../experiments/store/experiments_selectors'; import { + getDefaultRunColorIdMap, + getRunColorOverride, getRuns, getRunSelectionMap, getRunSelectorRegexFilter, diff --git a/tensorboard/webapp/util/ui_selectors_test.ts b/tensorboard/webapp/util/ui_selectors_test.ts index cb0e4e2db4..f9b04f35b4 100644 --- a/tensorboard/webapp/util/ui_selectors_test.ts +++ b/tensorboard/webapp/util/ui_selectors_test.ts @@ -202,6 +202,7 @@ describe('ui_selectors test', () => { new Map([ ['123/run1', false], ['123/run2', true], + ['123/run3', true], ['234/run1', true], ['234/run2', true], ['234/run3', false], @@ -209,12 +210,13 @@ describe('ui_selectors test', () => { ], ]), runIds: { - '123': ['123/run1', '123/run2'], + '123': ['123/run1', '123/run2', '123/run3'], '234': ['234/run1', '234/run2', '234/run3'], }, runMetadata: { '123/run1': buildRun({id: '123/run1', name: 'run1'}), '123/run2': buildRun({id: '123/run2', name: 'run2'}), + '123/run3': buildRun({id: '123/run3', name: 'run3'}), '234/run1': buildRun({id: '234/run1', name: 'run1'}), '234/run2': buildRun({id: '234/run2', name: 'run2'}), '234/run3': buildRun({id: '234/run3', name: 'run3'}), @@ -239,6 +241,8 @@ describe('ui_selectors test', () => { // legacy name = "apple/run2" and does not match `r.n[1]` and // `apple/r..3`. ['123/run2', false], + // legacy name = "appple/run3" matches "apple/r..3". + ['123/run3', true], // legacy name = "banana/run1". Inherits true + matches `r.n[1]`. ['234/run1', true], // legacy name = "banana/run2". Does not match `r.n[1]` and