From eb5689150f83ce0f772ca672834680d2d0af542f Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 17 Aug 2021 16:39:00 -0700 Subject: [PATCH 1/2] time series: run selection based on regex filter Previously, regex filter was mainly to influence the list yousee on the runs table. This is where it differs from the traditional runs-selector (Polymer) which not only filtered runs but also changed the runs selection that you see on the chart. This change brings the Polymer behavior to Angular as users have preferred that. --- tensorboard/webapp/util/ui_selectors.ts | 24 +++++++++++-- tensorboard/webapp/util/ui_selectors_test.ts | 37 ++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/tensorboard/webapp/util/ui_selectors.ts b/tensorboard/webapp/util/ui_selectors.ts index b3476f5741..dedaf60a1e 100644 --- a/tensorboard/webapp/util/ui_selectors.ts +++ b/tensorboard/webapp/util/ui_selectors.ts @@ -29,7 +29,10 @@ import {createSelector} from '@ngrx/store'; import {getExperimentIdsFromRoute} from '../app_routing/store/app_routing_selectors'; import {State} from '../app_state'; -import {getRunSelectionMap} from '../runs/store/runs_selectors'; +import { + getRunSelectionMap, + getRunSelectorRegexFilter, +} from '../runs/store/runs_selectors'; /** @typehack */ import * as _typeHackStore from '@ngrx/store'; @@ -46,5 +49,22 @@ export const getCurrentRouteRunSelection = createSelector( } return getRunSelectionMap(state, {experimentIds}); }, - (runSelection) => runSelection + getRunSelectorRegexFilter, + (runSelection, regexFilter) => { + if (!runSelection) return null; + + let regexExp: RegExp; + + try { + regexExp = new RegExp(regexFilter, 'i'); + } catch { + regexExp = new RegExp(''); + } + + const filteredSelection = new Map(); + for (const [key, value] of runSelection.entries()) { + filteredSelection.set(key, regexExp.test(key) && value); + } + return filteredSelection; + } ); diff --git a/tensorboard/webapp/util/ui_selectors_test.ts b/tensorboard/webapp/util/ui_selectors_test.ts index 194651489d..92d45d5120 100644 --- a/tensorboard/webapp/util/ui_selectors_test.ts +++ b/tensorboard/webapp/util/ui_selectors_test.ts @@ -99,5 +99,42 @@ describe('ui_selectors test', () => { 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', + }) + ), + }; + + expect(getCurrentRouteRunSelection(state)).toEqual( + new Map([ + ['run1', true], + ['run2', false], + ['run3', false], + ]) + ); + }); }); }); From c1283203424c7e7d52dc04c7fe6920516467038b Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 17 Aug 2021 18:20:39 -0700 Subject: [PATCH 2/2] remove select all which is no longer relevant --- .../webapp/runs/actions/runs_actions.ts | 5 - .../webapp/runs/store/runs_reducers.ts | 18 -- .../webapp/runs/store/runs_reducers_test.ts | 32 --- .../runs_table/runs_table_component.ng.html | 16 -- .../views/runs_table/runs_table_component.ts | 1 - .../views/runs_table/runs_table_container.ts | 12 -- .../runs/views/runs_table/runs_table_test.ts | 202 ------------------ 7 files changed, 286 deletions(-) diff --git a/tensorboard/webapp/runs/actions/runs_actions.ts b/tensorboard/webapp/runs/actions/runs_actions.ts index 91358b8bf4..f9938bfdfc 100644 --- a/tensorboard/webapp/runs/actions/runs_actions.ts +++ b/tensorboard/webapp/runs/actions/runs_actions.ts @@ -62,11 +62,6 @@ export const runPageSelectionToggled = createAction( props<{experimentIds: string[]; runIds: string[]}>() ); -export const runsSelectAll = createAction( - '[Runs] Runs Select All', - props<{experimentIds: string[]}>() -); - export const runSelectorPaginationOptionChanged = createAction( '[Runs] Run Selector Pagination Option Changed', props<{pageSize: number; pageIndex: number}>() diff --git a/tensorboard/webapp/runs/store/runs_reducers.ts b/tensorboard/webapp/runs/store/runs_reducers.ts index cf79e1ead0..63b1401964 100644 --- a/tensorboard/webapp/runs/store/runs_reducers.ts +++ b/tensorboard/webapp/runs/store/runs_reducers.ts @@ -218,24 +218,6 @@ const dataReducer: ActionReducer = createReducer( selectionState: nextSelectionState, }; }), - on(runsActions.runsSelectAll, (state, {experimentIds}) => { - const stateKey = serializeExperimentIds(experimentIds); - const nextSelectionState = new Map(state.selectionState); - const subSelectionState = new Map(nextSelectionState.get(stateKey) ?? []); - - for (const experimentId of experimentIds) { - for (const runId of state.runIds[experimentId]) { - subSelectionState.set(runId, true); - } - } - - nextSelectionState.set(stateKey, subSelectionState); - - return { - ...state, - selectionState: nextSelectionState, - }; - }), on(runsActions.fetchRunsSucceeded, (state, {runsForAllExperiments}) => { const groupKeyToColorString = new Map(state.groupKeyToColorString); const defaultRunColorForGroupBy = new Map(state.defaultRunColorForGroupBy); diff --git a/tensorboard/webapp/runs/store/runs_reducers_test.ts b/tensorboard/webapp/runs/store/runs_reducers_test.ts index fba3c99c13..8571c67fd8 100644 --- a/tensorboard/webapp/runs/store/runs_reducers_test.ts +++ b/tensorboard/webapp/runs/store/runs_reducers_test.ts @@ -587,38 +587,6 @@ describe('runs_reducers', () => { }); }); - describe('runsSelectAll', () => { - it('selects all runs', () => { - const state = buildRunsState({ - runIds: { - e1: ['r1', 'r2'], - e2: ['r3'], - }, - selectionState: new Map([['["e1","e2"]', new Map([['r1', false]])]]), - }); - - const nextState = runsReducers.reducers( - state, - actions.runsSelectAll({ - experimentIds: ['e1', 'e2'], - }) - ); - - expect(nextState.data.selectionState).toEqual( - new Map([ - [ - '["e1","e2"]', - new Map([ - ['r1', true], - ['r2', true], - ['r3', true], - ]), - ], - ]) - ); - }); - }); - describe('runSelectorPaginationOptionChanged', () => { it('updates the pagination option', () => { const state = buildRunsState(undefined, { diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html b/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html index fc5e54fca2..498a322a33 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html @@ -24,7 +24,6 @@
-
- -
- All runs in this page are selected but not all runs ({{ numSelectedItems - }} of {{ allItemsLength }}) are selected. -
- -
(); @Output() onSelectionToggle = new EventEmitter(); @Output() onPageSelectionToggle = new EventEmitter<{items: RunTableItem[]}>(); - @Output() onSelectAllPages = new EventEmitter(); @Output() onPaginationChange = new EventEmitter<{ pageIndex: number; 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 a4a509ac58..09849ec6bd 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts @@ -67,7 +67,6 @@ import { runSelectorPaginationOptionChanged, runSelectorRegexFilterChanged, runSelectorSortChanged, - runsSelectAll, runTableShown, } from '../../actions'; import {SortKey, SortType} from '../../types'; @@ -208,7 +207,6 @@ function matchFilter( [usePagination]="usePagination" (onSelectionToggle)="onRunSelectionToggle($event)" (onPageSelectionToggle)="onPageSelectionToggle($event)" - (onSelectAllPages)="onSelectAllPages()" (onPaginationChange)="onPaginationChange($event)" (onRegexFilterChange)="onRegexFilterChange($event)" (onSortChange)="onSortChange($event)" @@ -592,16 +590,6 @@ export class RunsTableContainer implements OnInit, OnDestroy { ); } - onSelectAllPages() { - if (!this.usePagination) { - throw new Error( - 'Select all events cannot be dispatched when pagination is disabled' - ); - } - - this.store.dispatch(runsSelectAll({experimentIds: this.experimentIds})); - } - onPaginationChange(event: {pageIndex: number; pageSize: number}) { if (!this.usePagination) { throw new Error( 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 e3b0ba2d46..1cd22030bc 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts @@ -84,7 +84,6 @@ import { runSelectorPaginationOptionChanged, runSelectorRegexFilterChanged, runSelectorSortChanged, - runsSelectAll, runTableShown, } from '../../actions'; import {DomainType} from '../../data_source/runs_data_source_types'; @@ -1823,207 +1822,6 @@ describe('runs_table', () => { ); } ); - - it('does not render select all button when pagination is disabled', () => { - store.overrideSelector(getRunSelectorPaginationOption, { - pageIndex: 0, - pageSize: 2, - }); - store.overrideSelector(getRuns, [ - buildRun({id: 'book1', name: "The Philosopher's Stone"}), - buildRun({id: 'book2', name: 'The Chamber Of Secrets'}), - buildRun({id: 'book3', name: 'The Prisoner of Azkaban'}), - ]); - store.overrideSelector( - getCurrentRouteRunSelection, - new Map([ - ['book1', true], - ['book2', true], - ['book3', false], - ]) - ); - - const fixture = createComponent( - ['tolkien'], - [RunsTableColumn.CHECKBOX, RunsTableColumn.RUN_NAME], - false /* usePagination */ - ); - fixture.detectChanges(); - - const showAll = fixture.nativeElement.querySelector( - Selector.SELECT_ALL_ROW - ); - expect(showAll).not.toBeTruthy(); - }); - - it('renders select all button when page is selected but not all items', () => { - store.overrideSelector(getRunSelectorPaginationOption, { - pageIndex: 0, - pageSize: 2, - }); - store.overrideSelector(getRuns, [ - buildRun({id: 'book1', name: "The Philosopher's Stone"}), - buildRun({id: 'book2', name: 'The Chamber Of Secrets'}), - buildRun({id: 'book3', name: 'The Prisoner of Azkaban'}), - ]); - store.overrideSelector( - getCurrentRouteRunSelection, - new Map([ - ['book1', true], - ['book2', true], - ['book3', false], - ]) - ); - - const fixture = createComponent( - ['tolkien'], - [RunsTableColumn.CHECKBOX, RunsTableColumn.RUN_NAME], - true /* usePagination */ - ); - fixture.detectChanges(); - - const showAll = fixture.nativeElement.querySelector( - Selector.SELECT_ALL_ROW - ); - expect(showAll.textContent).toContain( - 'All runs in this page are selected but not all runs (2 of 3)' - ); - }); - - it('does not render select if everything is selected', () => { - store.overrideSelector(getRunSelectorPaginationOption, { - pageIndex: 0, - pageSize: 2, - }); - store.overrideSelector(getRuns, [ - buildRun({id: 'book1', name: "The Philosopher's Stone"}), - buildRun({id: 'book2', name: 'The Chamber Of Secrets'}), - buildRun({id: 'book3', name: 'The Prisoner of Azkaban'}), - ]); - store.overrideSelector( - getCurrentRouteRunSelection, - new Map([ - ['book1', true], - ['book2', true], - ['book3', true], - ]) - ); - - const fixture = createComponent( - ['rowling'], - [RunsTableColumn.CHECKBOX, RunsTableColumn.RUN_NAME], - true /* usePagination */ - ); - fixture.detectChanges(); - - const showAll = fixture.nativeElement.querySelector( - Selector.SELECT_ALL_ROW - ); - expect(showAll).toBeNull(); - }); - - it('does not render select all if page is not all selected', () => { - store.overrideSelector(getRunSelectorPaginationOption, { - pageIndex: 0, - pageSize: 2, - }); - store.overrideSelector(getRuns, [ - buildRun({id: 'book1', name: "The Philosopher's Stone"}), - buildRun({id: 'book2', name: 'The Chamber Of Secrets'}), - buildRun({id: 'book3', name: 'The Prisoner of Azkaban'}), - ]); - store.overrideSelector( - getCurrentRouteRunSelection, - new Map([ - ['book1', true], - ['book2', false], - ['book3', true], - ]) - ); - - const fixture = createComponent( - ['rowling'], - [RunsTableColumn.CHECKBOX, RunsTableColumn.RUN_NAME], - true /* usePagination */ - ); - fixture.detectChanges(); - - const showAll = fixture.nativeElement.querySelector( - Selector.SELECT_ALL_ROW - ); - expect(showAll).toBeNull(); - }); - - it('renders select all even when all filtered items are selected', () => { - store.overrideSelector(getRunSelectorRegexFilter, '[oO]f'); - store.overrideSelector(getRunSelectorPaginationOption, { - pageIndex: 0, - pageSize: 2, - }); - store.overrideSelector(getRuns, [ - buildRun({id: 'book1', name: "The Philosopher's Stone"}), - buildRun({id: 'book2', name: 'The Chamber Of Secrets'}), - buildRun({id: 'book3', name: 'The Prisoner of Azkaban'}), - ]); - store.overrideSelector( - getCurrentRouteRunSelection, - new Map([ - ['book1', false], - ['book2', true], - ['book3', true], - ]) - ); - - const fixture = createComponent( - ['tolkien'], - [RunsTableColumn.CHECKBOX, RunsTableColumn.RUN_NAME], - true /* usePagination */ - ); - fixture.detectChanges(); - - const showAll = fixture.nativeElement.querySelector( - Selector.SELECT_ALL_ROW - ); - expect(showAll.textContent).toContain( - 'All runs in this page are selected but not all runs (2 of 3)' - ); - }); - - it('dispatches runsSelectAll when click on select', () => { - store.overrideSelector(getRunSelectorPaginationOption, { - pageIndex: 0, - pageSize: 2, - }); - store.overrideSelector(getRuns, [ - buildRun({id: 'book1', name: "The Philosopher's Stone"}), - buildRun({id: 'book2', name: 'The Chamber Of Secrets'}), - buildRun({id: 'book3', name: 'The Prisoner of Azkaban'}), - ]); - store.overrideSelector( - getCurrentRouteRunSelection, - new Map([ - ['book1', true], - ['book2', true], - ['book3', false], - ]) - ); - - const fixture = createComponent( - ['rowling'], - [RunsTableColumn.CHECKBOX, RunsTableColumn.RUN_NAME], - true /* usePagination */ - ); - fixture.detectChanges(); - - const button = fixture.nativeElement.querySelector('.select-all button'); - button.click(); - - expect(dispatchSpy).toHaveBeenCalledWith( - runsSelectAll({ - experimentIds: ['rowling'], - }) - ); - }); }); describe('"too many runs" alert', () => {