Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hparams: Allow users to load more Hparams if they are not all included in default. #6780

Merged
merged 2 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions tensorboard/webapp/hparams/_redux/hparams_actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,7 @@ export const dashboardHparamColumnOrderChanged = createAction(
'[Hparams] Dashboard Hparam Column Order Changed',
props<ReorderColumnEvent>()
);

export const loadAllDashboardHparams = createAction(
'[Hparams] Load all Hparams'
);
6 changes: 5 additions & 1 deletion tensorboard/webapp/hparams/_redux/hparams_effects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ export class HparamsEffects {

private readonly loadHparamsOnReload$: Observable<string[]> =
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[])
Expand Down
65 changes: 30 additions & 35 deletions tensorboard/webapp/hparams/_redux/hparams_effects_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ describe('hparams effects', () => {
dataSource.fetchSessionGroups.and.returnValue(of(mockSessionGroups));
});

afterEach(() => {
store?.resetSelectors();
});

describe('loadHparamsData$', () => {
beforeEach(() => {
effects.loadHparamsData$.subscribe((action) => {
Expand Down Expand Up @@ -162,41 +166,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);
Expand Down
8 changes: 7 additions & 1 deletion tensorboard/webapp/hparams/_redux/hparams_reducers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,13 @@ const reducer: ActionReducer<HparamsState, Action> = createReducer(
dashboardDisplayedHparamColumns: newColumns,
};
}
)
),
on(actions.loadAllDashboardHparams, (state) => {
return {
...state,
numDashboardHparamsToLoad: 0, // All.
};
})
);

export function reducers(state: HparamsState | undefined, action: Action) {
Expand Down
12 changes: 12 additions & 0 deletions tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@
(removeColumn)="removeColumn.emit($event)"
(hideColumn)="hideColumn.emit($event)"
(addFilter)="addFilter.emit($event)"
(loadAllColumns)="loadAllColumns.emit()"
>
</scalar-card-data-table>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ export class ScalarCardComponent<Downloader> {
@Output() addColumn = new EventEmitter<AddColumnEvent>();
@Output() removeColumn = new EventEmitter<HeaderToggleInfo>();
@Output() addFilter = new EventEmitter<FilterAddedEvent>();
@Output() loadAllColumns = new EventEmitter<null>();

@Output() onLineChartZoom = new EventEmitter<Extent | null>();
@Output() onCardStateChanged = new EventEmitter<Partial<CardState>>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ function areSeriesEqual(
(addColumn)="onAddColumn($event)"
(removeColumn)="onRemoveColumn($event)"
(addFilter)="addHparamFilter($event)"
(loadAllColumns)="loadAllColumns()"
></scalar-card-component>
`,
styles: [
Expand Down Expand Up @@ -763,4 +764,8 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
})
);
}

loadAllColumns() {
this.store.dispatch(hparamsActions.loadAllDashboardHparams());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
(addColumn)="addColumn.emit($event)"
(removeColumn)="onRemoveColumn($event)"
(addFilter)="addFilter.emit($event)"
(loadAllColumns)="loadAllColumns.emit()"
>
<ng-container header>
<ng-container *ngFor="let header of getHeaders()">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export class ScalarCardDataTable {
@Output() addColumn = new EventEmitter<AddColumnEvent>();
@Output() removeColumn = new EventEmitter<HeaderToggleInfo>();
@Output() addFilter = new EventEmitter<FilterAddedEvent>();
@Output() loadAllColumns = new EventEmitter<null>();

ColumnHeaderType = ColumnHeaderType;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
(addColumn)="addColumn.emit($event)"
(removeColumn)="removeColumn.emit($event)"
(addFilter)="addFilter.emit($event)"
(loadAllColumns)="loadAllColumns.emit()"
>
<ng-container header>
<ng-container *ngFor="let header of getHeaders()">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export class RunsDataTable {
@Output() removeColumn = new EventEmitter<ColumnHeader>();
@Output() onSelectionDblClick = new EventEmitter<string>();
@Output() addFilter = new EventEmitter<FilterAddedEvent>();
@Output() loadAllColumns = new EventEmitter<null>();

// 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ const getRunsLoading = createSelector<
(addColumn)="addColumn($event)"
(removeColumn)="removeColumn($event)"
(addFilter)="addHparamFilter($event)"
(loadAllColumns)="loadAllColumns()"
></runs-data-table>
`,
styles: [
Expand Down Expand Up @@ -361,6 +362,10 @@ export class RunsTableContainer implements OnInit, OnDestroy {
})
);
}

loadAllColumns() {
this.store.dispatch(hparamsActions.loadAllDashboardHparams());
}
}

export const TEST_ONLY = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,14 @@

<div class="column-load-info">
<label>{{ numColumnsLoaded }} columns loaded.</label>
<label *ngIf="hasMoreColumnsToLoad">
Warning: There were too many columns to load all of them.
</label>
<div *ngIf="hasMoreColumnsToLoad" class="load-more-columns">
<label>
Warning: There were too many columns to load all of them efficiently.
</label>
<button mat-stroked-button (click)="loadAllColumnsClicked()">
Load all anyway
</button>
</div>
</div>

<div #columnList class="column-list">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to wrap the list of columns in a virtual scroll viewbox prior to launching the data project
https://material.angular.io/cdk/scrolling/overview#cdk-virtual-scroll-overview

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added that suggestion to our list of tasks. We should at least play with it.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export class ColumnSelectorComponent implements OnInit, AfterViewInit {
@Input() numColumnsLoaded!: number;
@Input() hasMoreColumnsToLoad!: boolean;
@Output() columnSelected = new EventEmitter<ColumnHeader>();
@Output() loadAllColumns = new EventEmitter<null>();

@ViewChild('search')
private readonly searchField!: ElementRef;
Expand Down Expand Up @@ -124,6 +125,10 @@ export class ColumnSelectorComponent implements OnInit, AfterViewInit {
this.columnSelected.emit(header);
}

loadAllColumnsClicked() {
this.loadAllColumns.emit();
}

activate() {
this.isActive = true;
}
Expand Down
33 changes: 29 additions & 4 deletions tensorboard/webapp/widgets/data_table/column_selector_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -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();
}));
});
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
[numColumnsLoaded]="numColumnsLoaded"
[hasMoreColumnsToLoad]="hasMoreColumnsToLoad"
(columnSelected)="onColumnAdded($event)"
(loadAllColumns)="loadAllColumns.emit()"
></tb-data-table-column-selector-component>
</custom-modal>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit {
@Output() removeColumn = new EventEmitter<ColumnHeader>();
@Output() addColumn = new EventEmitter<AddColumnEvent>();
@Output() addFilter = new EventEmitter<FilterAddedEvent>();
@Output() loadAllColumns = new EventEmitter<null>();

@ViewChild('columnSelectorModal', {static: false})
private readonly columnSelectorModal!: CustomModalComponent;
Expand Down
Loading