From f96864385e33eeef2f5960f0377d48a0824cef6f Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Thu, 7 Mar 2024 14:15:01 -0500 Subject: [PATCH 1/2] Allow user to load all hparams if they weren't all loaded by default. --- .../webapp/hparams/_redux/hparams_actions.ts | 4 ++ .../webapp/hparams/_redux/hparams_effects.ts | 6 +- .../hparams/_redux/hparams_effects_test.ts | 61 ++++++++----------- .../webapp/hparams/_redux/hparams_reducers.ts | 8 ++- .../hparams/_redux/hparams_reducers_test.ts | 12 ++++ .../hparams/_redux/hparams_selectors_test.ts | 19 ++++-- .../scalar_card_component.ng.html | 1 + .../card_renderer/scalar_card_component.ts | 1 + .../card_renderer/scalar_card_container.ts | 5 ++ .../scalar_card_data_table.ng.html | 1 + .../card_renderer/scalar_card_data_table.ts | 1 + .../views/runs_table/runs_data_table.ng.html | 1 + .../runs/views/runs_table/runs_data_table.ts | 1 + .../views/runs_table/runs_table_container.ts | 5 ++ .../column_selector_component.ng.html | 11 +++- .../data_table/column_selector_component.scss | 14 +++++ .../data_table/column_selector_component.ts | 5 ++ .../data_table/column_selector_test.ts | 33 ++++++++-- .../data_table/data_table_component.ng.html | 1 + .../data_table/data_table_component.ts | 1 + 20 files changed, 143 insertions(+), 48 deletions(-) diff --git a/tensorboard/webapp/hparams/_redux/hparams_actions.ts b/tensorboard/webapp/hparams/_redux/hparams_actions.ts index db4d4abdc4..063f5ef7cb 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_actions.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_actions.ts @@ -71,3 +71,7 @@ export const dashboardHparamColumnOrderChanged = createAction( '[Hparams] Dashboard Hparam Column Order Changed', props() ); + +export const loadAllDashboardHparams = createAction( + '[Hparams] Load all Hparams' +); diff --git a/tensorboard/webapp/hparams/_redux/hparams_effects.ts b/tensorboard/webapp/hparams/_redux/hparams_effects.ts index 49be1492d7..3c374ffc74 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_effects.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_effects.ts @@ -62,7 +62,11 @@ export class HparamsEffects { private readonly loadHparamsOnReload$: Observable = this.actions$.pipe( - ofType(coreActions.reload, coreActions.manualReload), + ofType( + coreActions.reload, + coreActions.manualReload, + hparamsActions.loadAllDashboardHparams + ), withLatestFrom(this.store.select(getExperimentIdsFromRoute)), filter(([, experimentIds]) => Boolean(experimentIds)), map(([, experimentIds]) => experimentIds as string[]) diff --git a/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts b/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts index cb04d305e7..336f7dac03 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts @@ -162,41 +162,32 @@ describe('hparams effects', () => { ]); }); - it('fetches data on reload', () => { - action.next(coreActions.reload()); - expect(dataSource.fetchExperimentInfo).toHaveBeenCalledWith( - ['expFromRoute'], - 1111 - ); - expect(dataSource.fetchSessionGroups).toHaveBeenCalledWith( - ['expFromRoute'], - [buildHparamSpec({name: 'h1'})] - ); - expect(actualActions).toEqual([ - hparamsActions.hparamsFetchSessionGroupsSucceeded({ - hparamSpecs: mockHparamSpecs, - sessionGroups: mockSessionGroups, - }), - ]); - }); - - it('fetches data on manualReload', () => { - action.next(coreActions.manualReload()); - expect(dataSource.fetchExperimentInfo).toHaveBeenCalledWith( - ['expFromRoute'], - 1111 - ); - expect(dataSource.fetchSessionGroups).toHaveBeenCalledWith( - ['expFromRoute'], - [buildHparamSpec({name: 'h1'})] - ); - expect(actualActions).toEqual([ - hparamsActions.hparamsFetchSessionGroupsSucceeded({ - hparamSpecs: mockHparamSpecs, - sessionGroups: mockSessionGroups, - }), - ]); - }); + for (const {actionName, actionInstance} of [ + {actionName: 'reload', actionInstance: coreActions.reload()}, + {actionName: 'manualReload', actionInstance: coreActions.manualReload()}, + { + actionName: 'loadAllDashboardHparams', + actionInstance: hparamsActions.loadAllDashboardHparams(), + }, + ]) { + it(`fetches data on ${actionName}`, () => { + action.next(actionInstance); + expect(dataSource.fetchExperimentInfo).toHaveBeenCalledWith( + ['expFromRoute'], + 1111 + ); + expect(dataSource.fetchSessionGroups).toHaveBeenCalledWith( + ['expFromRoute'], + [buildHparamSpec({name: 'h1'})] + ); + expect(actualActions).toEqual([ + hparamsActions.hparamsFetchSessionGroupsSucceeded({ + hparamSpecs: mockHparamSpecs, + sessionGroups: mockSessionGroups, + }), + ]); + }); + } it('does not attempt to load hparams when experiment ids are null', () => { store.overrideSelector(selectors.getExperimentIdsFromRoute, null); diff --git a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts index f71060a050..16fac47605 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts @@ -166,7 +166,13 @@ const reducer: ActionReducer = createReducer( dashboardDisplayedHparamColumns: newColumns, }; } - ) + ), + on(actions.loadAllDashboardHparams, (state) => { + return { + ...state, + numDashboardHparamsToLoad: 0, // All. + }; + }) ); export function reducers(state: HparamsState | undefined, action: Action) { diff --git a/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts b/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts index e559f7eb85..d8df44244e 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts @@ -712,4 +712,16 @@ describe('hparams/_redux/hparams_reducers_test', () => { ]); }); }); + + describe('loadAllDashboardHparams', () => { + it('sets numDashboardHparamsToLoad to 0', () => { + const state = buildHparamsState({ + numDashboardHparamsToLoad: 1000, + }); + + const state2 = reducers(state, actions.loadAllDashboardHparams()); + + expect(state2.numDashboardHparamsToLoad).toEqual(0); + }); + }); }); diff --git a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts index 70f041baf2..ff4836dc2c 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts @@ -15,7 +15,6 @@ limitations under the License. import {ColumnHeaderType} from '../../widgets/data_table/types'; import {DomainType} from '../types'; -import {State} from './types'; import * as selectors from './hparams_selectors'; import { buildHparamSpec, @@ -165,7 +164,13 @@ describe('hparams/_redux/hparams_selectors_test', () => { }); describe('#getNumDashboardHparamsToLoad', () => { - it('returns dashboard specs', () => { + beforeEach(() => { + // Clear the memoization. + selectors.getNumDashboardHparamsToLoad.clearResult(); + selectors.getNumDashboardHparamsToLoad.release(); + }); + + it('returns value', () => { const state = buildStateFromHparamsState( buildHparamsState({ numDashboardHparamsToLoad: 5, @@ -176,8 +181,14 @@ describe('hparams/_redux/hparams_selectors_test', () => { }); }); - describe('#getNumDashboardHparamsToLoad', () => { - it('returns dashboard specs', () => { + describe('#getNumDashboardHparamsLoaded', () => { + beforeEach(() => { + // Clear the memoization. + selectors.getNumDashboardHparamsLoaded.clearResult(); + selectors.getNumDashboardHparamsLoaded.release(); + }); + + it('returns value', () => { const state = buildStateFromHparamsState( buildHparamsState({ numDashboardHparamsLoaded: 22, diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html index 5c9fea5b85..79d4264b1e 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html @@ -211,6 +211,7 @@ (removeColumn)="removeColumn.emit($event)" (hideColumn)="hideColumn.emit($event)" (addFilter)="addFilter.emit($event)" + (loadAllColumns)="loadAllColumns.emit()" > diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 264cc5128b..989a76f616 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -132,6 +132,7 @@ export class ScalarCardComponent { @Output() addColumn = new EventEmitter(); @Output() removeColumn = new EventEmitter(); @Output() addFilter = new EventEmitter(); + @Output() loadAllColumns = new EventEmitter(); @Output() onLineChartZoom = new EventEmitter(); @Output() onCardStateChanged = new EventEmitter>(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 89664123af..2a94e9fa31 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -211,6 +211,7 @@ function areSeriesEqual( (addColumn)="onAddColumn($event)" (removeColumn)="onRemoveColumn($event)" (addFilter)="addHparamFilter($event)" + (loadAllColumns)="loadAllColumns()" > `, styles: [ @@ -763,4 +764,8 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { }) ); } + + loadAllColumns() { + this.store.dispatch(hparamsActions.loadAllDashboardHparams()); + } } diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html index 00c9f58c22..880ce02d9c 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html @@ -27,6 +27,7 @@ (addColumn)="addColumn.emit($event)" (removeColumn)="onRemoveColumn($event)" (addFilter)="addFilter.emit($event)" + (loadAllColumns)="loadAllColumns.emit()" > diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index e3d8c66829..f8d9b0a36b 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -70,6 +70,7 @@ export class ScalarCardDataTable { @Output() addColumn = new EventEmitter(); @Output() removeColumn = new EventEmitter(); @Output() addFilter = new EventEmitter(); + @Output() loadAllColumns = new EventEmitter(); ColumnHeaderType = ColumnHeaderType; diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html index 1feaaf0a7d..83688127bd 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html @@ -38,6 +38,7 @@ (addColumn)="addColumn.emit($event)" (removeColumn)="removeColumn.emit($event)" (addFilter)="addFilter.emit($event)" + (loadAllColumns)="loadAllColumns.emit()" > diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts index 9a7779ef25..d4cf74814f 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts @@ -65,6 +65,7 @@ export class RunsDataTable { @Output() removeColumn = new EventEmitter(); @Output() onSelectionDblClick = new EventEmitter(); @Output() addFilter = new EventEmitter(); + @Output() loadAllColumns = new EventEmitter(); // These columns must be stored and reused to stop needless re-rendering of // the content and headers in these columns. This has been known to cause 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 b38c62b67c..375b9c9391 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts @@ -115,6 +115,7 @@ const getRunsLoading = createSelector< (addColumn)="addColumn($event)" (removeColumn)="removeColumn($event)" (addFilter)="addHparamFilter($event)" + (loadAllColumns)="loadAllColumns()" > `, styles: [ @@ -361,6 +362,10 @@ export class RunsTableContainer implements OnInit, OnDestroy { }) ); } + + loadAllColumns() { + this.store.dispatch(hparamsActions.loadAllDashboardHparams()); + } } export const TEST_ONLY = { diff --git a/tensorboard/webapp/widgets/data_table/column_selector_component.ng.html b/tensorboard/webapp/widgets/data_table/column_selector_component.ng.html index c64463fffe..dacca276ea 100644 --- a/tensorboard/webapp/widgets/data_table/column_selector_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/column_selector_component.ng.html @@ -25,9 +25,14 @@
- +
+ + +
diff --git a/tensorboard/webapp/widgets/data_table/column_selector_component.scss b/tensorboard/webapp/widgets/data_table/column_selector_component.scss index dcb90aa68c..ca2c457874 100644 --- a/tensorboard/webapp/widgets/data_table/column_selector_component.scss +++ b/tensorboard/webapp/widgets/data_table/column_selector_component.scss @@ -48,6 +48,20 @@ limitations under the License. margin-bottom: 12px; } + .load-more-columns { + color: mat.get-color-from-palette($tb-warn, 600); + display: flex; + flex-direction: column; + margin-top: 6px; + + button { + color: inherit; + font-size: inherit; + height: 24px; + margin-top: 8px; + } + } + .column-list { display: flex; flex-direction: column; diff --git a/tensorboard/webapp/widgets/data_table/column_selector_component.ts b/tensorboard/webapp/widgets/data_table/column_selector_component.ts index 464ecc7088..1b55a25e63 100644 --- a/tensorboard/webapp/widgets/data_table/column_selector_component.ts +++ b/tensorboard/webapp/widgets/data_table/column_selector_component.ts @@ -38,6 +38,7 @@ export class ColumnSelectorComponent implements OnInit, AfterViewInit { @Input() numColumnsLoaded!: number; @Input() hasMoreColumnsToLoad!: boolean; @Output() columnSelected = new EventEmitter(); + @Output() loadAllColumns = new EventEmitter(); @ViewChild('search') private readonly searchField!: ElementRef; @@ -124,6 +125,10 @@ export class ColumnSelectorComponent implements OnInit, AfterViewInit { this.columnSelected.emit(header); } + loadAllColumnsClicked() { + this.loadAllColumns.emit(); + } + activate() { this.isActive = true; } diff --git a/tensorboard/webapp/widgets/data_table/column_selector_test.ts b/tensorboard/webapp/widgets/data_table/column_selector_test.ts index 848c66e133..096cb04405 100644 --- a/tensorboard/webapp/widgets/data_table/column_selector_test.ts +++ b/tensorboard/webapp/widgets/data_table/column_selector_test.ts @@ -41,6 +41,10 @@ describe('column selector', () => { return fixture.debugElement.query(By.css('button.selected')); } + function getLoadAllColumnsButton() { + return fixture.debugElement.query(By.css('.column-load-info button')); + } + beforeEach(async () => { await TestBed.configureTestingModule({ declarations: [ColumnSelectorComponent], @@ -217,13 +221,34 @@ describe('column selector', () => { })); it('renders too many columns warning', fakeAsync(() => { - fixture.componentInstance.numColumnsLoaded = 100; fixture.componentInstance.hasMoreColumnsToLoad = true; fixture.detectChanges(); - const numColumns = fixture.debugElement.query(By.css('.column-load-info')); - expect(numColumns.nativeElement.textContent).toContain( - 'Warning: There were too many columns to load all of them.' + const numColumnsEl = fixture.debugElement.query( + By.css('.column-load-info') ); + expect(numColumnsEl.nativeElement.textContent).toContain( + 'Warning: There were too many columns to load all of them efficiently.' + ); + })); + + it('does not render "load all" button by default', fakeAsync(() => { + const loadAllButton = getLoadAllColumnsButton(); + expect(loadAllButton).toBeFalsy(); + })); + + it('renders "load all" button and responds to click', fakeAsync(() => { + let loadAllColumnsClicked = false; + fixture.componentInstance.loadAllColumns.subscribe(() => { + loadAllColumnsClicked = true; + }); + fixture.componentInstance.hasMoreColumnsToLoad = true; + fixture.detectChanges(); + + const loadAllButton = getLoadAllColumnsButton(); + loadAllButton.nativeElement.click(); + flush(); + + expect(loadAllColumnsClicked).toBeTrue(); })); }); diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index 3d134c5ed1..e008410885 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -83,6 +83,7 @@ [numColumnsLoaded]="numColumnsLoaded" [hasMoreColumnsToLoad]="hasMoreColumnsToLoad" (columnSelected)="onColumnAdded($event)" + (loadAllColumns)="loadAllColumns.emit()" > diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index 4c720a1c25..bf56af98e7 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -83,6 +83,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { @Output() removeColumn = new EventEmitter(); @Output() addColumn = new EventEmitter(); @Output() addFilter = new EventEmitter(); + @Output() loadAllColumns = new EventEmitter(); @ViewChild('columnSelectorModal', {static: false}) private readonly columnSelectorModal!: CustomModalComponent; From c4c732ae47ef4f43bceedd6e84a6a1d6a0834f8b Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Fri, 8 Mar 2024 10:41:58 -0500 Subject: [PATCH 2/2] Use resetSelectors() to make tests non-flaky. Instead of the way I was previously removing the flakiness. --- .../hparams/_redux/hparams_effects_test.ts | 4 ++++ .../hparams/_redux/hparams_selectors_test.ts | 18 +++--------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts b/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts index 336f7dac03..e8b8e1e1d1 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_effects_test.ts @@ -101,6 +101,10 @@ describe('hparams effects', () => { dataSource.fetchSessionGroups.and.returnValue(of(mockSessionGroups)); }); + afterEach(() => { + store?.resetSelectors(); + }); + describe('loadHparamsData$', () => { beforeEach(() => { effects.loadHparamsData$.subscribe((action) => { diff --git a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts index ff4836dc2c..9bf3a9755a 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts @@ -164,13 +164,7 @@ describe('hparams/_redux/hparams_selectors_test', () => { }); describe('#getNumDashboardHparamsToLoad', () => { - beforeEach(() => { - // Clear the memoization. - selectors.getNumDashboardHparamsToLoad.clearResult(); - selectors.getNumDashboardHparamsToLoad.release(); - }); - - it('returns value', () => { + it('returns dashboard specs', () => { const state = buildStateFromHparamsState( buildHparamsState({ numDashboardHparamsToLoad: 5, @@ -181,14 +175,8 @@ describe('hparams/_redux/hparams_selectors_test', () => { }); }); - describe('#getNumDashboardHparamsLoaded', () => { - beforeEach(() => { - // Clear the memoization. - selectors.getNumDashboardHparamsLoaded.clearResult(); - selectors.getNumDashboardHparamsLoaded.release(); - }); - - it('returns value', () => { + describe('#getNumDashboardHparamsToLoad', () => { + it('returns dashboard specs', () => { const state = buildStateFromHparamsState( buildHparamsState({ numDashboardHparamsLoaded: 22,