diff --git a/tensorboard/webapp/metrics/actions/index.ts b/tensorboard/webapp/metrics/actions/index.ts index 063676205a..6e2df75fd3 100644 --- a/tensorboard/webapp/metrics/actions/index.ts +++ b/tensorboard/webapp/metrics/actions/index.ts @@ -281,5 +281,9 @@ export const metricsClearAllPinnedCards = createAction( '[Metrics] Clear all pinned cards' ); +export const metricsEnableSavingPinsToggled = createAction( + '[Metrics] Enable Saving Pins Toggled' +); + // TODO(jieweiwu): Delete after internal code is updated. export const stepSelectorTimeSelectionChanged = timeSelectionChanged; diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index 4b8061e4e1..730a1514e8 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -268,11 +268,20 @@ export class MetricsEffects implements OnInitEffects { withLatestFrom( this.getVisibleCardFetchInfos(), this.store.select(selectors.getEnableGlobalPins), - this.store.select(selectors.getShouldPersistSettings) + this.store.select(selectors.getShouldPersistSettings), + this.store.select(selectors.getMetricsSavingPinsEnabled) ), filter( - ([, , enableGlobalPins, shouldPersistSettings]) => - enableGlobalPins && shouldPersistSettings + ([ + , + , + enableGlobalPinsFeature, + shouldPersistSettings, + isMetricsSavingPinsEnabled, + ]) => + enableGlobalPinsFeature && + shouldPersistSettings && + isMetricsSavingPinsEnabled ), tap(([{cardId, canCreateNewPins, wasPinned}, fetchInfos]) => { const card = fetchInfos.find((value) => value.id === cardId); @@ -293,11 +302,19 @@ export class MetricsEffects implements OnInitEffects { ofType(initAction), withLatestFrom( this.store.select(selectors.getEnableGlobalPins), - this.store.select(selectors.getShouldPersistSettings) + this.store.select(selectors.getShouldPersistSettings), + this.store.select(selectors.getMetricsSavingPinsEnabled) ), filter( - ([, enableGlobalPins, shouldPersistSettings]) => - enableGlobalPins && shouldPersistSettings + ([ + , + enableGlobalPinsFeature, + shouldPersistSettings, + isMetricsSavingPinsEnabled, + ]) => + enableGlobalPinsFeature && + shouldPersistSettings && + isMetricsSavingPinsEnabled ), tap(() => { const tags = this.savedPinsDataSource.getSavedScalarPins(); @@ -316,15 +333,46 @@ export class MetricsEffects implements OnInitEffects { }) ); - private readonly removeAllPins$ = this.actions$.pipe( + private readonly removeSavedPinsOnDisable$ = this.actions$.pipe( ofType(actions.metricsClearAllPinnedCards), withLatestFrom( this.store.select(selectors.getEnableGlobalPins), - this.store.select(selectors.getShouldPersistSettings) + this.store.select(selectors.getShouldPersistSettings), + this.store.select(selectors.getMetricsSavingPinsEnabled) ), filter( - ([, enableGlobalPins, shouldPersistSettings]) => - enableGlobalPins && shouldPersistSettings + ([ + , + enableGlobalPinsFeature, + shouldPersistSettings, + isMetricsSavingPinsEnabled, + ]) => + enableGlobalPinsFeature && + shouldPersistSettings && + isMetricsSavingPinsEnabled + ), + tap(() => { + this.savedPinsDataSource.removeAllScalarPins(); + }) + ); + + private readonly disableSavingPins$ = this.actions$.pipe( + ofType(actions.metricsEnableSavingPinsToggled), + withLatestFrom( + this.store.select(selectors.getEnableGlobalPins), + this.store.select(selectors.getShouldPersistSettings), + this.store.select(selectors.getMetricsSavingPinsEnabled) + ), + filter( + ([ + , + enableGlobalPins, + getShouldPersistSettings, + getMetricsSavingPinsEnabled, + ]) => + enableGlobalPins && + getShouldPersistSettings && + !getMetricsSavingPinsEnabled ), tap(() => { this.savedPinsDataSource.removeAllScalarPins(); @@ -375,7 +423,7 @@ export class MetricsEffects implements OnInitEffects { /** * Subscribes to: metricsClearAllPinnedCards. */ - this.removeAllPins$ + this.removeSavedPinsOnDisable$ ); }, {dispatch: false} diff --git a/tensorboard/webapp/metrics/metrics_module.ts b/tensorboard/webapp/metrics/metrics_module.ts index 3f3bc7343a..594080dd98 100644 --- a/tensorboard/webapp/metrics/metrics_module.ts +++ b/tensorboard/webapp/metrics/metrics_module.ts @@ -35,6 +35,7 @@ import { getMetricsScalarSmoothing, getMetricsStepSelectorEnabled, getMetricsTooltipSort, + getMetricsSavingPinsEnabled, getRangeSelectionHeaders, getSingleSelectionHeaders, isMetricsSettingsPaneOpen, @@ -125,6 +126,12 @@ export function getMetricsTimeSeriesLinkedTimeEnabled() { }); } +export function getMetricsTimeSeriesSavingPinsEnabled() { + return createSelector(getMetricsSavingPinsEnabled, (isEnabled) => { + return {savingPinsEnabled: isEnabled}; + }); +} + export function getSingleSelectionHeadersFactory() { return createSelector(getSingleSelectionHeaders, (singleSelectionHeaders) => { return {singleSelectionHeaders}; @@ -188,6 +195,9 @@ export function getRangeSelectionHeadersFactory() { PersistentSettingsConfigModule.defineGlobalSetting( getRangeSelectionHeadersFactory ), + PersistentSettingsConfigModule.defineGlobalSetting( + getMetricsTimeSeriesSavingPinsEnabled + ), ], providers: [ { diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index 8a6abda21b..73cbc6453c 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -608,6 +608,9 @@ const reducer = createReducer( if (typeof partialSettings.scalarSmoothing === 'number') { metricsSettings.scalarSmoothing = partialSettings.scalarSmoothing; } + if (typeof partialSettings.savingPinsEnabled === 'boolean') { + metricsSettings.savingPinsEnabled = partialSettings.savingPinsEnabled; + } const isSettingsPaneOpen = partialSettings.timeSeriesSettingsPaneOpened ?? state.isSettingsPaneOpen; @@ -933,6 +936,19 @@ const reducer = createReducer( }, }; }), + on(actions.metricsEnableSavingPinsToggled, (state) => { + const nextSavingPinsEnabled = !( + state.settingOverrides.savingPinsEnabled ?? + state.settings.savingPinsEnabled + ); + return { + ...state, + settingOverrides: { + ...state.settingOverrides, + savingPinsEnabled: nextSavingPinsEnabled, + }, + }; + }), on( actions.multipleTimeSeriesRequested, ( diff --git a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts index 9f72c28a09..4545c5c4d0 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts @@ -1232,6 +1232,25 @@ describe('metrics reducers', () => { expect(thirdState.settings.hideEmptyCards).toBe(false); expect(thirdState.settingOverrides.hideEmptyCards).toBe(false); }); + + it('changes savingPinsEnabled on metricsEnableSavingPinsToggled', () => { + [{value: true}, {value: false}].forEach(({value: initValue}) => { + const prevState = buildMetricsState({ + settings: buildMetricsSettingsState({ + savingPinsEnabled: initValue, + }), + settingOverrides: {}, + }); + + const nextState = reducers( + prevState, + actions.metricsEnableSavingPinsToggled() + ); + + expect(nextState.settings.savingPinsEnabled).toBe(initValue); + expect(nextState.settingOverrides.savingPinsEnabled).toBe(!initValue); + }); + }); }); describe('loading time series data', () => { @@ -3142,6 +3161,7 @@ describe('metrics reducers', () => { scalarSmoothing: 0.3, ignoreOutliers: false, tooltipSort: TooltipSort.ASCENDING, + savingPinsEnabled: true, }), settingOverrides: { scalarSmoothing: 0.5, @@ -3155,6 +3175,7 @@ describe('metrics reducers', () => { partialSettings: { ignoreOutliers: true, tooltipSort: TooltipSort.DESCENDING, + savingPinsEnabled: false, }, }) ); @@ -3162,6 +3183,7 @@ describe('metrics reducers', () => { expect(nextState.settings.scalarSmoothing).toBe(0.3); expect(nextState.settings.ignoreOutliers).toBe(true); expect(nextState.settings.tooltipSort).toBe(TooltipSort.DESCENDING); + expect(nextState.settings.savingPinsEnabled).toBe(false); expect(nextState.settingOverrides.scalarSmoothing).toBe(0.5); expect(nextState.settingOverrides.tooltipSort).toBe( TooltipSort.ALPHABETICAL diff --git a/tensorboard/webapp/metrics/store/metrics_selectors.ts b/tensorboard/webapp/metrics/store/metrics_selectors.ts index 5b7089b769..872950721c 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors.ts @@ -382,6 +382,11 @@ export const getMetricsImageShowActualSize = createSelector( (settings): boolean => settings.imageShowActualSize ); +export const getMetricsSavingPinsEnabled = createSelector( + selectSettings, + (settings): boolean => settings.savingPinsEnabled +); + export const getMetricsTagFilter = createSelector( selectMetricsState, (state): string => state.tagFilter diff --git a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts index ca027d07b8..ec6f2876c2 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts @@ -1298,6 +1298,20 @@ describe('metrics selectors', () => { ); expect(selectors.getMetricsCardMinWidth(state)).toBe(400); }); + + it('returns savingPinsEnabled when called getMetricsSavingPinsEnabled', () => { + [{value: true}, {value: false}].forEach(({value}) => { + selectors.getMetricsSavingPinsEnabled.release(); + const state = appStateFromMetricsState( + buildMetricsState({ + settings: buildMetricsSettingsState({ + savingPinsEnabled: value, + }), + }) + ); + expect(selectors.getMetricsSavingPinsEnabled(state)).toBe(value); + }); + }); }); describe('getMetricsTagFilter', () => { diff --git a/tensorboard/webapp/metrics/store/metrics_types.ts b/tensorboard/webapp/metrics/store/metrics_types.ts index 0cd2957708..0c77c8385f 100644 --- a/tensorboard/webapp/metrics/store/metrics_types.ts +++ b/tensorboard/webapp/metrics/store/metrics_types.ts @@ -246,6 +246,7 @@ export interface MetricsSettings { imageContrastInMilli: number; imageShowActualSize: boolean; histogramMode: HistogramMode; + savingPinsEnabled: boolean; } export interface MetricsNonNamespacedState { @@ -287,4 +288,5 @@ export const METRICS_SETTINGS_DEFAULT: MetricsSettings = { imageContrastInMilli: 1000, imageShowActualSize: false, histogramMode: HistogramMode.OFFSET, + savingPinsEnabled: true, }; diff --git a/tensorboard/webapp/metrics/testing.ts b/tensorboard/webapp/metrics/testing.ts index ea65d86d69..86d846771d 100644 --- a/tensorboard/webapp/metrics/testing.ts +++ b/tensorboard/webapp/metrics/testing.ts @@ -47,6 +47,28 @@ import {DataTableMode} from '../widgets/data_table/types'; export function buildMetricsSettingsState( overrides?: Partial ): MetricsSettings { + return { + cardMinWidth: null, + tooltipSort: TooltipSort.NEAREST, + ignoreOutliers: false, + xAxisType: XAxisType.WALL_TIME, + scalarSmoothing: 0.3, + hideEmptyCards: true, + scalarPartitionNonMonotonicX: false, + imageBrightnessInMilli: 123, + imageContrastInMilli: 123, + imageShowActualSize: true, + histogramMode: HistogramMode.OFFSET, + savingPinsEnabled: true, + ...overrides, + }; +} + +// Since Settings proto has missing fields, we need to build a partial of +// Settings to be used in tests. +export function buildMetricsSettingsOverrides( + overrides?: Partial +): Partial { return { cardMinWidth: null, tooltipSort: TooltipSort.NEAREST, diff --git a/tensorboard/webapp/metrics/views/right_pane/right_pane_test.ts b/tensorboard/webapp/metrics/views/right_pane/right_pane_test.ts index 5501270348..c62a70e836 100644 --- a/tensorboard/webapp/metrics/views/right_pane/right_pane_test.ts +++ b/tensorboard/webapp/metrics/views/right_pane/right_pane_test.ts @@ -548,5 +548,43 @@ describe('metrics right_pane', () => { ).toHaveClass('toggle-opened'); }); }); + + describe('saving pins check box', () => { + beforeEach(() => { + store.overrideSelector(selectors.getEnableGlobalPins, true); + store.overrideSelector(selectors.getMetricsSavingPinsEnabled, false); + }); + + it('does not render if getEnableGlobalPins feature flag is false', () => { + store.overrideSelector(selectors.getEnableGlobalPins, false); + const fixture = TestBed.createComponent(SettingsViewContainer); + fixture.detectChanges(); + + expect(select(fixture, '.saving-pins')).toBeFalsy(); + }); + + it('renders checked saving pins check box if isSavingpinsEnabled is true', () => { + store.overrideSelector(selectors.getMetricsSavingPinsEnabled, true); + + const fixture = TestBed.createComponent(SettingsViewContainer); + fixture.detectChanges(); + + expect( + select(fixture, '.saving-pins input').componentInstance.checked + ).toBeTrue(); + }); + + it('dispatches metricsEnableSavingPinsToggled on toggle', () => { + const fixture = TestBed.createComponent(SettingsViewContainer); + fixture.detectChanges(); + + const checkbox = select(fixture, '.saving-pins input'); + checkbox.nativeElement.click(); + + expect(dispatchSpy).toHaveBeenCalledWith( + actions.metricsEnableSavingPinsToggled() + ); + }); + }); }); }); diff --git a/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ng.html b/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ng.html index 1a9119f073..ec894d0f6e 100644 --- a/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ng.html +++ b/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ng.html @@ -93,6 +93,19 @@

General

+ +
+ Enable saving pins (Scalars only) + +
diff --git a/tensorboard/webapp/metrics/views/right_pane/settings_view_component.scss b/tensorboard/webapp/metrics/views/right_pane/settings_view_component.scss index 5bdf2b2d53..0a798db9b0 100644 --- a/tensorboard/webapp/metrics/views/right_pane/settings_view_component.scss +++ b/tensorboard/webapp/metrics/views/right_pane/settings_view_component.scss @@ -79,7 +79,8 @@ section .control-row:not(:has(+ .control-row > mat-checkbox)):not(:last-child) { width: 5em; } -.scalars-partition-x { +.scalars-partition-x, +.saving-pins { align-items: center; display: flex; diff --git a/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ts b/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ts index 5dcad457eb..f6878cf051 100644 --- a/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ts +++ b/tensorboard/webapp/metrics/views/right_pane/settings_view_component.ts @@ -70,12 +70,15 @@ export class SettingsViewComponent { @Input() linkedTimeSelection!: TimeSelection | null; @Input() stepMinMax!: {min: number; max: number}; @Input() isSlideOutMenuOpen!: boolean; + @Input() isSavingPinsEnabled!: boolean; + @Input() globalPinsFeatureEnabled: boolean = false; @Output() linkedTimeToggled = new EventEmitter(); @Output() stepSelectorToggled = new EventEmitter(); @Output() rangeSelectionToggled = new EventEmitter(); @Output() onSlideOutToggled = new EventEmitter(); + @Output() onEnableSavingPinsToggled = new EventEmitter(); @Input() isImageSupportEnabled!: boolean; diff --git a/tensorboard/webapp/metrics/views/right_pane/settings_view_container.ts b/tensorboard/webapp/metrics/views/right_pane/settings_view_container.ts index 81f3db7b8c..280dc0770c 100644 --- a/tensorboard/webapp/metrics/views/right_pane/settings_view_container.ts +++ b/tensorboard/webapp/metrics/views/right_pane/settings_view_container.ts @@ -28,6 +28,7 @@ import { metricsChangeScalarSmoothing, metricsChangeTooltipSort, metricsChangeXAxisType, + metricsEnableSavingPinsToggled, metricsResetCardWidth, metricsResetImageBrightness, metricsResetImageContrast, @@ -83,6 +84,9 @@ import {HistogramMode, TooltipSort, XAxisType} from '../../types'; (stepSelectorToggled)="onStepSelectorToggled()" (rangeSelectionToggled)="onRangeSelectionToggled()" (onSlideOutToggled)="onSlideOutToggled()" + [isSavingPinsEnabled]="isSavingPinsEnabled$ | async" + (onEnableSavingPinsToggled)="onEnableSavingPinsToggled()" + [globalPinsFeatureEnabled]="globalPinsFeatureEnabled$ | async" > `, @@ -146,6 +150,13 @@ export class SettingsViewContainer { readonly imageShowActualSize$ = this.store.select( selectors.getMetricsImageShowActualSize ); + readonly isSavingPinsEnabled$ = this.store.select( + selectors.getMetricsSavingPinsEnabled + ); + // Feature flag for global pins. + readonly globalPinsFeatureEnabled$ = this.store.select( + selectors.getEnableGlobalPins + ); onTooltipSortChanged(sort: TooltipSort) { this.store.dispatch(metricsChangeTooltipSort({sort})); @@ -222,4 +233,8 @@ export class SettingsViewContainer { onSlideOutToggled() { this.store.dispatch(metricsSlideoutMenuToggled()); } + + onEnableSavingPinsToggled() { + this.store.dispatch(metricsEnableSavingPinsToggled()); + } } diff --git a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts index 432c79c8ff..0ad5862602 100644 --- a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts +++ b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts @@ -125,6 +125,9 @@ export class OSSSettingsConverter extends SettingsConverter< serializableSettings.dashboardDisplayedHparamColumns = settings.dashboardDisplayedHparamColumns; } + if (settings.savingPinsEnabled !== undefined) { + serializableSettings.savingPinsEnabled = settings.savingPinsEnabled; + } return serializableSettings; } @@ -256,6 +259,13 @@ export class OSSSettingsConverter extends SettingsConverter< backendSettings.dashboardDisplayedHparamColumns; } + if ( + backendSettings.hasOwnProperty('savingPinsEnabled') && + typeof backendSettings.savingPinsEnabled === 'boolean' + ) { + settings.savingPinsEnabled = backendSettings.savingPinsEnabled; + } + return settings; } } diff --git a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts index 4c938a169b..d81d4eda16 100644 --- a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts +++ b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts @@ -398,6 +398,20 @@ describe('persistent_settings data_source test', () => { ], }); }); + + it('properly converts savingPinsEnabled', async () => { + getItemSpy.withArgs(TEST_ONLY.GLOBAL_LOCAL_STORAGE_KEY).and.returnValue( + JSON.stringify({ + savingPinsEnabled: false, + }) + ); + + const actual = await firstValueFrom(dataSource.getSettings()); + + expect(actual).toEqual({ + savingPinsEnabled: false, + }); + }); }); describe('#setSettings', () => { @@ -526,6 +540,25 @@ describe('persistent_settings data_source test', () => { }) ); }); + + it('properly converts savingPinsEnabled', async () => { + getItemSpy + .withArgs(TEST_ONLY.GLOBAL_LOCAL_STORAGE_KEY) + .and.returnValue(null); + + await firstValueFrom( + dataSource.setSettings({ + savingPinsEnabled: false, + }) + ); + + expect(setItemSpy).toHaveBeenCalledOnceWith( + TEST_ONLY.GLOBAL_LOCAL_STORAGE_KEY, + JSON.stringify({ + savingPinsEnabled: false, + }) + ); + }); }); describe('settings migration', () => { diff --git a/tensorboard/webapp/persistent_settings/_data_source/types.ts b/tensorboard/webapp/persistent_settings/_data_source/types.ts index 3ec9917a47..ded984fd29 100644 --- a/tensorboard/webapp/persistent_settings/_data_source/types.ts +++ b/tensorboard/webapp/persistent_settings/_data_source/types.ts @@ -47,6 +47,7 @@ export declare interface BackendSettings { singleSelectionHeaders?: ColumnHeader[]; rangeSelectionHeaders?: ColumnHeader[]; dashboardDisplayedHparamColumns?: ColumnHeader[]; + savingPinsEnabled?: boolean; } /** @@ -72,4 +73,5 @@ export interface PersistableSettings { singleSelectionHeaders?: ColumnHeader[]; rangeSelectionHeaders?: ColumnHeader[]; dashboardDisplayedHparamColumns?: ColumnHeader[]; + savingPinsEnabled?: boolean; }