Skip to content

Commit 7daa225

Browse files
authored
Linked Time: Fix bug with pinned cards and simplify logic around emitting minMax (#6241)
## Motivation for features / changes See #6240 for a more detailed write up on the backstory surrounding this effort. I managed to find two bugs while testing #6240. * Pinning a card then reloading the page will not result in a `minMax` being generated for the pinned copy * There is currently some bounding logic in place ensuring `minMax` values are not generated outside of the of the min and max steps in the card. Adding a new selector to retrieve the pre-derived `dataMinMax` prevents us from needing a dependency on the cards data when emitting a `cardMinMaxChanged` event. This dramatically simplifies the logic around surrounding the event and removes the need for the observable altogether. ## Screenshots of UI changes None ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Navigate to localhost:6006 3) Pin a scalar card 4) Refresh the page 5) Use the developer console to find the final redux state once the page is finished loading 6) Inspect the states metrics.cardStateMap and ensure the pinned has an entry there. **Note**: Pinned card ids always start with `"baseCardId"` ## Alternate designs / implementations considered
1 parent 4fe4dfa commit 7daa225

File tree

4 files changed

+85
-1
lines changed

4 files changed

+85
-1
lines changed

tensorboard/webapp/metrics/store/metrics_reducers.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -890,6 +890,13 @@ const reducer = createReducer(
890890
...nextCardStateMap[cardId],
891891
dataMinMax: nextMinMax,
892892
};
893+
const pinnedId = state.cardToPinnedCopy.get(cardId);
894+
if (pinnedId) {
895+
nextCardStateMap[pinnedId] = {
896+
...nextCardStateMap[pinnedId],
897+
dataMinMax: nextMinMax,
898+
};
899+
}
893900
}
894901

895902
const nextState: MetricsState = {

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1374,6 +1374,12 @@ describe('metrics reducers', () => {
13741374
},
13751375
},
13761376
},
1377+
cardToPinnedCopy: new Map([
1378+
[
1379+
'{"plugin":"scalars","tag":"tagA","runId":null}',
1380+
'somePinnedCardId',
1381+
],
1382+
]),
13771383
});
13781384

13791385
const sample = 9;
@@ -1440,6 +1446,12 @@ describe('metrics reducers', () => {
14401446
maxStep: 99,
14411447
},
14421448
},
1449+
somePinnedCardId: {
1450+
dataMinMax: {
1451+
minStep: 0,
1452+
maxStep: 99,
1453+
},
1454+
},
14431455
};
14441456
expect(nextState.cardStateMap).toEqual(expectedCardStateMap);
14451457
});

tensorboard/webapp/metrics/store/metrics_selectors.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,11 @@ export const isMetricsSlideoutMenuOpen = createSelector(
500500
);
501501

502502
/**
503-
* Gets the min and max step of a metrics card.
503+
* Gets the min and max step visible in a metrics card.
504+
* This value can either be the data min max or be overridden
505+
* by a user min max.
506+
*
507+
* Note: userMinMax is not necessarily a subset of dataMinMax.
504508
*/
505509
export const getMetricsCardMinMax = createSelector(
506510
getCardStateMap,
@@ -510,6 +514,16 @@ export const getMetricsCardMinMax = createSelector(
510514
}
511515
);
512516

517+
/**
518+
* Returns the min and max step found in the cards data.
519+
*/
520+
export const getMetricsCardDataMinMax = createSelector(
521+
getCardStateMap,
522+
(cardStateMap: CardStateMap, cardId: CardId): MinMaxStep | undefined => {
523+
return cardStateMap[cardId]?.dataMinMax;
524+
}
525+
);
526+
513527
/**
514528
* Gets the time selection of a metrics card.
515529
*/

tensorboard/webapp/metrics/store/metrics_selectors_test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -660,6 +660,57 @@ describe('metrics selectors', () => {
660660
});
661661
});
662662

663+
describe('getMetricsCardDataMinMax', () => {
664+
it('returns undefined when cardStateMap is undefined', () => {
665+
const state = appStateFromMetricsState(buildMetricsState({}));
666+
expect(
667+
selectors.getMetricsCardDataMinMax(state, 'card1')
668+
).toBeUndefined();
669+
});
670+
671+
it('returns undefined when card has no cardState', () => {
672+
const state1 = appStateFromMetricsState(
673+
buildMetricsState({
674+
cardStateMap: {},
675+
})
676+
);
677+
678+
const state2 = appStateFromMetricsState(
679+
buildMetricsState({
680+
cardStateMap: {
681+
card1: {},
682+
},
683+
})
684+
);
685+
686+
expect(
687+
selectors.getMetricsCardDataMinMax(state1, 'card1')
688+
).toBeUndefined();
689+
expect(
690+
selectors.getMetricsCardDataMinMax(state2, 'card1')
691+
).toBeUndefined();
692+
});
693+
694+
it('returns data cards minMax when defined', () => {
695+
const state = appStateFromMetricsState(
696+
buildMetricsState({
697+
cardStateMap: {
698+
card1: {
699+
dataMinMax: {
700+
minStep: 0,
701+
maxStep: 100,
702+
},
703+
},
704+
},
705+
})
706+
);
707+
expect(selectors.getMetricsCardDataMinMax(state, 'card1')).toEqual({
708+
minStep: 0,
709+
maxStep: 100,
710+
});
711+
});
712+
});
713+
663714
describe('getPinnedCardsWithMetadata', () => {
664715
beforeEach(() => {
665716
selectors.getPinnedCardsWithMetadata.release();

0 commit comments

Comments
 (0)