-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Global pins] Saving pins to localStorage #6819
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
Changes from 3 commits
4ed2c0b
586e503
c29f40d
5602eb9
6fdd93e
4806818
0bc3a45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
==============================================================================*/ | ||
import {Injectable} from '@angular/core'; | ||
import {Tag} from './types'; | ||
|
||
const SAVED_SCALAR_PINS_KEY = 'tb-saved-scalar-pins'; | ||
|
||
@Injectable() | ||
export class SavedPinsDataSource { | ||
saveScalarPin(tag: Tag): void { | ||
const existingPins = this.getSavedScalarPins(); | ||
if (!existingPins.includes(tag)) { | ||
existingPins.push(tag); | ||
} | ||
localStorage.setItem(SAVED_SCALAR_PINS_KEY, JSON.stringify(existingPins)); | ||
} | ||
|
||
removeScalarPin(tag: Tag): void { | ||
const existingPins = this.getSavedScalarPins(); | ||
localStorage.setItem( | ||
SAVED_SCALAR_PINS_KEY, | ||
JSON.stringify(existingPins.filter((pin) => pin !== tag)) | ||
); | ||
} | ||
|
||
getSavedScalarPins(): Tag[] { | ||
const savedPins = localStorage.getItem(SAVED_SCALAR_PINS_KEY); | ||
if (savedPins) { | ||
return JSON.parse(savedPins) as Tag[]; | ||
} | ||
return []; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
==============================================================================*/ | ||
import {NgModule} from '@angular/core'; | ||
import {SavedPinsDataSource} from './saved_pins_data_source'; | ||
|
||
@NgModule({ | ||
providers: [SavedPinsDataSource], | ||
}) | ||
export class SavedPinsDataSourceModule {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
==============================================================================*/ | ||
import {TestBed} from '@angular/core/testing'; | ||
import {SavedPinsDataSource} from './saved_pins_data_source'; | ||
|
||
const SAVED_SCALAR_PINS_KEY = 'tb-saved-scalar-pins'; | ||
|
||
describe('SavedPinsDataSource Test', () => { | ||
let mockStorage: Record<string, string>; | ||
let dataSource: SavedPinsDataSource; | ||
|
||
beforeEach(async () => { | ||
await TestBed.configureTestingModule({ | ||
providers: [SavedPinsDataSource], | ||
}); | ||
|
||
dataSource = TestBed.inject(SavedPinsDataSource); | ||
|
||
mockStorage = {}; | ||
spyOn(window.localStorage, 'setItem').and.callFake( | ||
(key: string, value: string) => { | ||
if (key !== SAVED_SCALAR_PINS_KEY) { | ||
throw new Error('incorrect key used'); | ||
} | ||
|
||
mockStorage[key] = value; | ||
} | ||
); | ||
|
||
spyOn(window.localStorage, 'getItem').and.callFake((key: string) => { | ||
if (key !== SAVED_SCALAR_PINS_KEY) { | ||
throw new Error('incorrect key used'); | ||
} | ||
|
||
return mockStorage[key]; | ||
}); | ||
}); | ||
|
||
describe('getSavedScalarPins', () => { | ||
it('gets the saved scalar pins', () => { | ||
window.localStorage.setItem( | ||
SAVED_SCALAR_PINS_KEY, | ||
JSON.stringify(['new_tag']) | ||
); | ||
|
||
const result = dataSource.getSavedScalarPins(); | ||
|
||
expect(result).toEqual(['new_tag']); | ||
}); | ||
|
||
it('returns empty list if there is no saved pins', () => { | ||
const result = dataSource.getSavedScalarPins(); | ||
|
||
expect(result).toEqual([]); | ||
}); | ||
}); | ||
|
||
describe('saveScalarPin', () => { | ||
it('stores the provided tag in the local storage', () => { | ||
dataSource.saveScalarPin('tag1'); | ||
|
||
expect(dataSource.getSavedScalarPins().length).toEqual(1); | ||
roseayeon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
expect(dataSource.getSavedScalarPins()).toEqual(['tag1']); | ||
}); | ||
|
||
it('adds the provided tag to the existing list', () => { | ||
localStorage.setItem(SAVED_SCALAR_PINS_KEY, JSON.stringify(['tag1'])); | ||
|
||
dataSource.saveScalarPin('tag2'); | ||
|
||
expect(dataSource.getSavedScalarPins().length).toEqual(2); | ||
expect(dataSource.getSavedScalarPins()).toEqual(['tag1', 'tag2']); | ||
}); | ||
|
||
it('does not addd the provided tag if it already exists', () => { | ||
localStorage.setItem( | ||
SAVED_SCALAR_PINS_KEY, | ||
JSON.stringify(['tag1', 'tag2']) | ||
); | ||
|
||
dataSource.saveScalarPin('tag2'); | ||
|
||
expect(dataSource.getSavedScalarPins().length).toEqual(2); | ||
expect(dataSource.getSavedScalarPins()).toEqual(['tag1', 'tag2']); | ||
}); | ||
}); | ||
|
||
describe('removeScalarPin', () => { | ||
it('removes the given tag if it exists', () => { | ||
dataSource.saveScalarPin('tag3'); | ||
|
||
dataSource.removeScalarPin('tag3'); | ||
|
||
expect(dataSource.getSavedScalarPins().length).toEqual(0); | ||
}); | ||
|
||
it('does not remove anything if the given tag does not exist', () => { | ||
dataSource.saveScalarPin('tag1'); | ||
|
||
dataSource.removeScalarPin('tag3'); | ||
|
||
expect(dataSource.getSavedScalarPins().length).toEqual(1); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -42,13 +42,14 @@ import { | |||
TagMetadata, | ||||
TimeSeriesRequest, | ||||
TimeSeriesResponse, | ||||
SavedPinsDataSource, | ||||
} from '../data_source/index'; | ||||
import { | ||||
getCardLoadState, | ||||
getCardMetadata, | ||||
getMetricsTagMetadataLoadState, | ||||
} from '../store'; | ||||
import {CardId, CardMetadata} from '../types'; | ||||
import {CardId, CardMetadata, PluginType} from '../types'; | ||||
|
||||
export type CardFetchInfo = CardMetadata & { | ||||
id: CardId; | ||||
|
@@ -73,7 +74,8 @@ export class MetricsEffects implements OnInitEffects { | |||
constructor( | ||||
private readonly actions$: Actions, | ||||
private readonly store: Store<State>, | ||||
private readonly dataSource: MetricsDataSource | ||||
private readonly metricsDataSource: MetricsDataSource, | ||||
private readonly savedPinsDataSource: SavedPinsDataSource | ||||
) {} | ||||
|
||||
/** @export */ | ||||
|
@@ -141,7 +143,7 @@ export class MetricsEffects implements OnInitEffects { | |||
this.store.dispatch(actions.metricsTagMetadataRequested()); | ||||
}), | ||||
switchMap(([, , experimentIds]) => { | ||||
return this.dataSource.fetchTagMetadata(experimentIds!).pipe( | ||||
return this.metricsDataSource.fetchTagMetadata(experimentIds!).pipe( | ||||
tap((tagMetadata: TagMetadata) => { | ||||
this.store.dispatch(actions.metricsTagMetadataLoaded({tagMetadata})); | ||||
}), | ||||
|
@@ -174,7 +176,7 @@ export class MetricsEffects implements OnInitEffects { | |||
} | ||||
|
||||
private fetchTimeSeries(request: TimeSeriesRequest) { | ||||
return this.dataSource.fetchTimeSeries([request]).pipe( | ||||
return this.metricsDataSource.fetchTimeSeries([request]).pipe( | ||||
tap((responses: TimeSeriesResponse[]) => { | ||||
const errors = responses.filter(isFailedTimeSeriesResponse); | ||||
if (errors.length) { | ||||
|
@@ -261,6 +263,23 @@ export class MetricsEffects implements OnInitEffects { | |||
}) | ||||
); | ||||
|
||||
private readonly loadSavedPins$ = this.actions$.pipe( | ||||
roseayeon marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
ofType(actions.cardPinStateToggled), | ||||
withLatestFrom(this.getVisibleCardFetchInfos()), | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (For a followup PR): Can we check getShouldPersistSettings() before deciding whether to write state? tensorboard/tensorboard/webapp/persistent_settings/_redux/persistent_settings_selectors.ts Line 26 in 04a064f
The idea is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, if a shared TensorBoard is used, we should not store user activities in local storage. To determine this, we need to check if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is exactly what I would recommend. |
||||
map(([{cardId, canCreateNewPins, wasPinned}, fetchInfos]) => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we protect this with a feature flag until you have the entire feature built and tested end-to-end? I'm worried you will build out the rest of the feature and test it and find some reason to make changes to the storage layer. However, people will already have started writing their pins to local storage (even if they don't see the rest of this feature) and perhaps what they store will be incompatible with the final version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM! Added a new feature flag |
||||
const card = fetchInfos.find((value) => value.id === cardId); | ||||
// Saving only scalar pinned cards. | ||||
if (!card || card.plugin !== PluginType.SCALARS) { | ||||
return; | ||||
} | ||||
if (wasPinned) { | ||||
this.savedPinsDataSource.removeScalarPin(card.tag); | ||||
} else if (canCreateNewPins) { | ||||
this.savedPinsDataSource.saveScalarPin(card.tag); | ||||
} | ||||
}) | ||||
); | ||||
|
||||
/** | ||||
* In general, this effect dispatch the following actions: | ||||
* | ||||
|
@@ -292,7 +311,12 @@ export class MetricsEffects implements OnInitEffects { | |||
/** | ||||
* Subscribes to: card visibility, reloads. | ||||
*/ | ||||
this.loadTimeSeries$ | ||||
this.loadTimeSeries$, | ||||
|
||||
/** | ||||
* Subscribes to: cardPinStateToggled. | ||||
*/ | ||||
this.loadSavedPins$ | ||||
); | ||||
}, | ||||
{dispatch: false} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you haven't already, please don't forget to use copybara_presubmit to import the changes into a local google3 repo and run TAP on it.
(I have no reason to believe that the import will fail but I just want to make sure you've done that step).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I almost forgot to use
copybara_presubmit
. I checked that TAP passed.