Skip to content

Commit 143508b

Browse files
rileyajonesrileyajones
authored
Hparams: Fix repainting bug (#6642)
## Motivation for features / changes Previously any changes to the metrics state would result in the runs table being redrawn. This was happening for two reasons both related to selectors with dependencies on `state`. Whenever any value on state changed these selectors would reflow. While testing this I discovered that entering a value into the tag filter triggered a `navigated` event which would in turn re-fetch the session_groups data. I have chosen to fix this by deduplicating events at the effect layer. ## Screenshots of UI changes (or N/A) Before ![12fa591b-6846-4f9d-a12f-8b3ce21c2e18](https://github.com/tensorflow/tensorboard/assets/78179109/e2c02b63-b353-44a2-8189-2bf6acab3de0) After ![f18b30d5-0156-4b24-85f9-8c3ba930919c](https://github.com/tensorflow/tensorboard/assets/78179109/9bfb6c0e-4bc3-40a5-87d6-998bcb6ccee6)
1 parent 717dec8 commit 143508b

File tree

9 files changed

+127
-73
lines changed

9 files changed

+127
-73
lines changed

‎tensorboard/webapp/hparams/_redux/hparams_effects.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {
2323
switchMap,
2424
withLatestFrom,
2525
throttleTime,
26-
combineLatestWith,
26+
distinctUntilChanged,
2727
} from 'rxjs/operators';
2828

2929
import {navigated} from '../../app_routing/actions';
@@ -55,12 +55,21 @@ export class HparamsEffects {
5555

5656
private readonly runTableShown$: Observable<string[]> = this.actions$.pipe(
5757
ofType(runsActions.runTableShown),
58-
map(({experimentIds}) => experimentIds)
58+
map(({experimentIds}) => experimentIds),
59+
distinctUntilChanged((prev, cur) => prev.join('') === cur.join(''))
5960
);
6061

61-
private readonly loadHparamsOnNavigationOrReload$: Observable<string[]> =
62+
private readonly navigated$: Observable<string[]> = this.actions$.pipe(
63+
ofType(navigated),
64+
withLatestFrom(this.store.select(getExperimentIdsFromRoute)),
65+
filter(([, experimentIds]) => Boolean(experimentIds)),
66+
map(([, experimentIds]) => experimentIds as string[]),
67+
distinctUntilChanged((prev, cur) => prev.join('') === cur.join(''))
68+
);
69+
70+
private readonly loadHparamsOnReload$: Observable<string[]> =
6271
this.actions$.pipe(
63-
ofType(navigated, coreActions.reload, coreActions.manualReload),
72+
ofType(coreActions.reload, coreActions.manualReload),
6473
withLatestFrom(this.store.select(getExperimentIdsFromRoute)),
6574
filter(([, experimentIds]) => Boolean(experimentIds)),
6675
map(([, experimentIds]) => experimentIds as string[])
@@ -69,10 +78,11 @@ export class HparamsEffects {
6978
/** @export */
7079
loadHparamsData$ = createEffect(() => {
7180
return merge(
81+
this.navigated$,
7282
this.runTableShown$,
73-
this.loadHparamsOnNavigationOrReload$
83+
this.loadHparamsOnReload$
7484
).pipe(
75-
combineLatestWith(
85+
withLatestFrom(
7686
this.store.select(getEnableHparamsInTimeSeries),
7787
this.store.select(getActiveRoute)
7888
),

‎tensorboard/webapp/hparams/_redux/hparams_effects_test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,17 @@ describe('hparams effects', () => {
160160
]);
161161
});
162162

163+
it('does not refetch data if experiments have not changed', () => {
164+
action.next(runsActions.runTableShown({experimentIds: ['exp1']}));
165+
action.next(runsActions.runTableShown({experimentIds: ['exp1']}));
166+
expect(actualActions).toEqual([
167+
hparamsActions.hparamsFetchSessionGroupsSucceeded({
168+
hparamsAndMetricsSpecs: mockHparamsAndMetricsSpecs,
169+
sessionGroups: mockSessionGroups,
170+
}),
171+
]);
172+
});
173+
163174
it('fetches data after navigation', () => {
164175
action.next(appRoutingActions.navigated({} as any));
165176
expect(dataSource.fetchExperimentInfo).toHaveBeenCalledWith([
@@ -180,6 +191,17 @@ describe('hparams effects', () => {
180191
]);
181192
});
182193

194+
it('does not fetch data when navigating to the same experiment', () => {
195+
action.next(appRoutingActions.navigated({} as any));
196+
action.next(appRoutingActions.navigated({} as any));
197+
expect(actualActions).toEqual([
198+
hparamsActions.hparamsFetchSessionGroupsSucceeded({
199+
hparamsAndMetricsSpecs: mockHparamsAndMetricsSpecs,
200+
sessionGroups: mockSessionGroups,
201+
}),
202+
]);
203+
});
204+
183205
it('fetches data on reload', () => {
184206
action.next(coreActions.reload());
185207
expect(dataSource.fetchExperimentInfo).toHaveBeenCalledWith([

‎tensorboard/webapp/metrics/store/metrics_selectors.ts

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -486,22 +486,23 @@ export const getTableEditorSelectedTab = createSelector(
486486
(state): DataTableMode => state.tableEditorSelectedTab
487487
);
488488

489-
export const getMetricsCardRangeSelectionEnabled = createSelector(
490-
getCardStateMap,
491-
getMetricsRangeSelectionEnabled,
492-
getMetricsLinkedTimeEnabled,
493-
(
494-
cardStateMap: CardStateMap,
495-
globalRangeSelectionEnabled: boolean,
496-
linkedTimeEnabled: boolean,
497-
cardId: CardId
498-
) =>
499-
cardRangeSelectionEnabled(
500-
cardStateMap,
501-
globalRangeSelectionEnabled,
502-
linkedTimeEnabled,
503-
cardId
504-
)
489+
export const getMetricsCardRangeSelectionEnabled = memoize((cardId) =>
490+
createSelector(
491+
getCardStateMap,
492+
getMetricsRangeSelectionEnabled,
493+
getMetricsLinkedTimeEnabled,
494+
(
495+
cardStateMap: CardStateMap,
496+
globalRangeSelectionEnabled: boolean,
497+
linkedTimeEnabled: boolean
498+
) =>
499+
cardRangeSelectionEnabled(
500+
cardStateMap,
501+
globalRangeSelectionEnabled,
502+
linkedTimeEnabled,
503+
cardId
504+
)
505+
)
505506
);
506507

507508
/**
@@ -639,11 +640,15 @@ export const getRangeSelectionHeaders = createSelector(
639640

640641
export const getColumnHeadersForCard = memoize((cardId: string) => {
641642
return createSelector(
642-
(state) => state,
643+
getMetricsCardRangeSelectionEnabled(cardId),
643644
getSingleSelectionHeaders,
644645
getRangeSelectionHeaders,
645-
(state, singleSelectionHeaders, rangeSelectionHeaders) => {
646-
return getMetricsCardRangeSelectionEnabled(state, cardId)
646+
(
647+
cardRangeSelectionEnabled,
648+
singleSelectionHeaders,
649+
rangeSelectionHeaders
650+
) => {
651+
return cardRangeSelectionEnabled
647652
? rangeSelectionHeaders
648653
: singleSelectionHeaders;
649654
}

‎tensorboard/webapp/metrics/store/metrics_selectors_test.ts

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1344,7 +1344,7 @@ describe('metrics selectors', () => {
13441344
describe('getMetricsCardRangeSelectionEnabled', () => {
13451345
it('returns card specific value when defined', () => {
13461346
expect(
1347-
selectors.getMetricsCardRangeSelectionEnabled(
1347+
selectors.getMetricsCardRangeSelectionEnabled('card1')(
13481348
appStateFromMetricsState(
13491349
buildMetricsState({
13501350
rangeSelectionEnabled: false,
@@ -1355,12 +1355,11 @@ describe('metrics selectors', () => {
13551355
},
13561356
},
13571357
})
1358-
),
1359-
'card1'
1358+
)
13601359
)
13611360
).toBeTrue();
13621361
expect(
1363-
selectors.getMetricsCardRangeSelectionEnabled(
1362+
selectors.getMetricsCardRangeSelectionEnabled('card1')(
13641363
appStateFromMetricsState(
13651364
buildMetricsState({
13661365
rangeSelectionEnabled: true,
@@ -1371,41 +1370,38 @@ describe('metrics selectors', () => {
13711370
},
13721371
},
13731372
})
1374-
),
1375-
'card1'
1373+
)
13761374
)
13771375
).toBeFalse();
13781376
});
13791377

13801378
it('returns global value when card specific value is not defined', () => {
13811379
expect(
1382-
selectors.getMetricsCardRangeSelectionEnabled(
1380+
selectors.getMetricsCardRangeSelectionEnabled('card1')(
13831381
appStateFromMetricsState(
13841382
buildMetricsState({
13851383
rangeSelectionEnabled: true,
13861384
cardStateMap: {
13871385
card1: {},
13881386
},
13891387
})
1390-
),
1391-
'card1'
1388+
)
13921389
)
13931390
).toBeTrue();
13941391
expect(
1395-
selectors.getMetricsCardRangeSelectionEnabled(
1392+
selectors.getMetricsCardRangeSelectionEnabled('card1')(
13961393
appStateFromMetricsState(
13971394
buildMetricsState({
13981395
rangeSelectionEnabled: false,
13991396
})
1400-
),
1401-
'card1'
1397+
)
14021398
)
14031399
).toBeFalse();
14041400
});
14051401

14061402
it('returns global value when linked time is enabled', () => {
14071403
expect(
1408-
selectors.getMetricsCardRangeSelectionEnabled(
1404+
selectors.getMetricsCardRangeSelectionEnabled('card1')(
14091405
appStateFromMetricsState(
14101406
buildMetricsState({
14111407
rangeSelectionEnabled: true,
@@ -1417,13 +1413,12 @@ describe('metrics selectors', () => {
14171413
},
14181414
},
14191415
})
1420-
),
1421-
'card1'
1416+
)
14221417
)
14231418
).toBeTrue();
14241419

14251420
expect(
1426-
selectors.getMetricsCardRangeSelectionEnabled(
1421+
selectors.getMetricsCardRangeSelectionEnabled('card1')(
14271422
appStateFromMetricsState(
14281423
buildMetricsState({
14291424
rangeSelectionEnabled: false,
@@ -1435,8 +1430,7 @@ describe('metrics selectors', () => {
14351430
},
14361431
},
14371432
})
1438-
),
1439-
'card1'
1433+
)
14401434
)
14411435
).toBeFalse();
14421436
});

‎tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -596,8 +596,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
596596
this.isPinned$ = this.store.select(getCardPinnedState, this.cardId);
597597

598598
this.rangeEnabled$ = this.store.select(
599-
getMetricsCardRangeSelectionEnabled,
600-
this.cardId
599+
getMetricsCardRangeSelectionEnabled(this.cardId)
601600
);
602601

603602
this.hparamsEnabled$ = this.store.select(getEnableHparamsInTimeSeries);

‎tensorboard/webapp/metrics/views/card_renderer/scalar_card_line_chart_container.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,7 @@ export class ScalarCardLineChartContainer
160160
this.loadState$ = this.store.select(getCardLoadState, this.cardId);
161161

162162
this.rangeEnabled$ = this.store.select(
163-
getMetricsCardRangeSelectionEnabled,
164-
this.cardId
163+
getMetricsCardRangeSelectionEnabled(this.cardId)
165164
);
166165
}
167166

‎tensorboard/webapp/metrics/views/card_renderer/scalar_card_line_chart_test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ describe('scalar card line chart', () => {
453453
);
454454
store.overrideSelector(selectors.getMetricsStepSelectorEnabled, false);
455455
store.overrideSelector(
456-
selectors.getMetricsCardRangeSelectionEnabled,
456+
selectors.getMetricsCardRangeSelectionEnabled('card1'),
457457
false
458458
);
459459
store.overrideSelector(selectors.getMetricsCardUserViewBox, null);
@@ -1794,7 +1794,10 @@ describe('scalar card line chart', () => {
17941794
start: {step: 10},
17951795
end: {step: 25},
17961796
};
1797-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
1797+
store.overrideSelector(
1798+
getMetricsCardRangeSelectionEnabled('card1'),
1799+
true
1800+
);
17981801
store.refreshState();
17991802
fixture.detectChanges();
18001803

‎tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ describe('scalar card', () => {
386386
);
387387
store.overrideSelector(selectors.getMetricsStepSelectorEnabled, false);
388388
store.overrideSelector(
389-
selectors.getMetricsCardRangeSelectionEnabled,
389+
selectors.getMetricsCardRangeSelectionEnabled('card1'),
390390
false
391391
);
392392
store.overrideSelector(selectors.getMetricsCardUserViewBox, null);
@@ -864,7 +864,10 @@ describe('scalar card', () => {
864864
selectors.getIsScalarColumnCustomizationEnabled,
865865
true
866866
);
867-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, false);
867+
store.overrideSelector(
868+
getMetricsCardRangeSelectionEnabled('card1'),
869+
false
870+
);
868871
const fixture = createComponent('card1');
869872

870873
openOverflowMenu(fixture);
@@ -882,7 +885,10 @@ describe('scalar card', () => {
882885
selectors.getIsScalarColumnCustomizationEnabled,
883886
true
884887
);
885-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
888+
store.overrideSelector(
889+
getMetricsCardRangeSelectionEnabled('card1'),
890+
true
891+
);
886892
const fixture = createComponent('card1');
887893

888894
openOverflowMenu(fixture);
@@ -3054,7 +3060,10 @@ describe('scalar card', () => {
30543060
runToSeries
30553061
);
30563062
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
3057-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
3063+
store.overrideSelector(
3064+
getMetricsCardRangeSelectionEnabled('card1'),
3065+
true
3066+
);
30583067
store.overrideSelector(
30593068
selectors.getCurrentRouteRunSelection,
30603069
new Map([
@@ -3136,7 +3145,10 @@ describe('scalar card', () => {
31363145
runToSeries
31373146
);
31383147
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
3139-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
3148+
store.overrideSelector(
3149+
getMetricsCardRangeSelectionEnabled('card1'),
3150+
true
3151+
);
31403152
store.overrideSelector(
31413153
selectors.getCurrentRouteRunSelection,
31423154
new Map([['run1', true]])
@@ -3202,7 +3214,10 @@ describe('scalar card', () => {
32023214
runToSeries
32033215
);
32043216
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
3205-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
3217+
store.overrideSelector(
3218+
getMetricsCardRangeSelectionEnabled('card1'),
3219+
true
3220+
);
32063221
store.overrideSelector(
32073222
selectors.getCurrentRouteRunSelection,
32083223
new Map([['run1', true]])
@@ -3318,7 +3333,10 @@ describe('scalar card', () => {
33183333
runToSeries
33193334
);
33203335
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
3321-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
3336+
store.overrideSelector(
3337+
getMetricsCardRangeSelectionEnabled('card1'),
3338+
true
3339+
);
33223340
store.overrideSelector(
33233341
selectors.getCurrentRouteRunSelection,
33243342
new Map([
@@ -4156,7 +4174,10 @@ describe('scalar card', () => {
41564174
start: {step: 10},
41574175
end: {step: 25},
41584176
});
4159-
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
4177+
store.overrideSelector(
4178+
getMetricsCardRangeSelectionEnabled('card1'),
4179+
true
4180+
);
41604181
store.refreshState();
41614182
fixture.detectChanges();
41624183

0 commit comments

Comments
 (0)